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

feat: support resolving did:peer:1 received in did exchange #2611

Merged

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Nov 17, 2023

This is an experimental implementation of supporting did:peer:1 resolution. This enables ACA-Py to correctly handle DID Exchange requests from AFJ (AFJ now fails to process our responses since we're not passing back a did:peer, still the unqualified values).

Notably, this does not support creating or sending did:peer:1, just storing and resolving.

Detailed changes:

  • Storing a DID Document (using base connection manager) no longer first requires parsing as the (soon to be deprecated) DIDDoc class (not PyDID DIDDocument).
    • This alone removes a whole slue of issues that the LegacyCorrections class was written to deal with.
  • Storing the key -> did -> connection mapping is now always delegated to BaseConnectionManager.record_did; this helps with consistently handling DIDs
  • Fetching a DID Document with BaseConnectionManager.fetch_did_document no longer parses the response from the wallet using DIDDoc. This helps this call to be a primitive used by DID resolvers that rely on stored documents to resolve DIDs without the DIDDoc parsing logic getting in the way.
  • Update the connection record to not require "Indy" shaped DIDs

Note to self, this resolver should have a clean up routine run when the associated connection record is deleted, similar to did:peer:2/3

@dbluhm dbluhm force-pushed the feature/did-peer-1-resolve branch from 06a293b to d7bb9c8 Compare November 28, 2023 15:56
@dbluhm dbluhm marked this pull request as ready for review December 6, 2023 22:09
@dbluhm dbluhm force-pushed the feature/did-peer-1-resolve branch from b863fbd to 94c8117 Compare December 6, 2023 22:09
@dbluhm
Copy link
Contributor Author

dbluhm commented Dec 6, 2023

This is now ready for review! I would like to continue building did:peer:2 support on top of these changes. @swcurran @usingtechnology

@swcurran swcurran requested a review from Jsyro December 6, 2023 22:18
@swcurran
Copy link
Contributor

swcurran commented Dec 6, 2023

Adding @Jsyro to reviewer. Can’t give a Maintainer approval, but I can translate… :-)

Resolves did:peer:1 received in did exchange

Signed-off-by: Daniel Bluhm <[email protected]>
Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm dbluhm force-pushed the feature/did-peer-1-resolve branch from 94c8117 to 783895b Compare December 6, 2023 23:11
Signed-off-by: Daniel Bluhm <[email protected]>
Copy link

sonarqubecloud bot commented Dec 6, 2023

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

Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great explanation of your work - simple enough even I could follow!

@dbluhm
Copy link
Contributor Author

dbluhm commented Dec 7, 2023

Note to self, this resolver should have a clean up routine run when the associated connection record is deleted, similar to did:peer:2/3

Took a closer look at this. This is a problem that actually just was never solved in the legacy peer case as well. I think fixing this belongs in a future PR. I'll log an issue.

@dbluhm
Copy link
Contributor Author

dbluhm commented Dec 7, 2023

Issue created: #2654

@Jsyro
Copy link
Contributor

Jsyro commented Dec 7, 2023

Looks great, one question though.

This maintains existing storage and fetching behaviour of the legacy unqualified DIDDocs, those documents are not going to be converted to did:peer:1?

@swcurran
Copy link
Contributor

swcurran commented Dec 7, 2023

This maintains existing storage and fetching behaviour of the legacy unqualified DIDDocs, those documents are not going to be converted to did:peer:1?

No — existing documents need not be converted, since they are only ever used internally, never with external parties. At some point we might update them, although almost certainly not to did:peer:1.

@swcurran swcurran merged commit 2857a2a into openwallet-foundation:main Dec 7, 2023
9 checks passed
@dbluhm dbluhm deleted the feature/did-peer-1-resolve branch December 7, 2023 18:13
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.

4 participants