Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: public did mediator routing keys as did keys #1977

Merged

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Oct 15, 2022

This PR fixes an issue @reflectivedevelopment found while using the recent changes made to support routing keys associated with public DID endpoints.

Without this fix, when creating a public DID OOB invitation, creation will fail due to no DIDComm service being found. This stemmed from PyDID expecting that routing keys be expressed strictly as DID URLs. Previously, we were resolving the routing keys as raw base58 encoded public keys. This PR makes it so that routing keys are resolved as did:key url values (if already did:key, simply pass through the values but if not, transform into did:key urls).

Credit to @cjhowland for doing most of the digging on this issue 🙂

Squash of multiple commits:
fix: revert changes to manager
fix: don't use dereference_as
fix: correct typo in method
fix: remove stray print statement

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
Copy link
Contributor Author

dbluhm commented Oct 15, 2022

@cjhowland and I need to add unit tests to this PR ensuring we've got coverage of the added lines.

We also need to ensure that if the resolved routing key value is a did:key but not a did:key URL that we perform an appropriate transformation.

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2022

Codecov Report

Merging #1977 (99f17de) into main (de04b9d) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1977      +/-   ##
==========================================
- Coverage   93.51%   93.51%   -0.01%     
==========================================
  Files         539      539              
  Lines       34600    34615      +15     
==========================================
+ Hits        32355    32369      +14     
- Misses       2245     2246       +1     

@dbluhm dbluhm marked this pull request as ready for review October 21, 2022 16:37
ianco
ianco previously approved these changes Oct 25, 2022
@andrewwhitehead
Copy link
Contributor

Is this finished testing?

@dbluhm
Copy link
Contributor Author

dbluhm commented Dec 7, 2022

@andrewwhitehead yes, this is ready for review

@dbluhm dbluhm enabled auto-merge January 11, 2023 01:03
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dbluhm dbluhm merged commit 4c20dcd into openwallet-foundation:main Jan 11, 2023
@dbluhm dbluhm deleted the fix/public-did-mediator-routing-keys branch July 7, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants