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: enable creation of DIDs for all registered methods #2067

Merged

Conversation

chumbert
Copy link
Contributor

@chumbert chumbert commented Jan 5, 2023

Here're a couple of changes leveraging the recent addition of the DIDMethods registry.
The aim is to allow agents using DID methods defined in plugin to create DIDs without implementing method-specific endpoints if not necessary.

Changes:

  • Relax condition on did format for the POST /wallet/did endpoint
  • Validate new DIDs parameters using DIDMethods in Askar and InMemory profiles
    validate new DIDs parameters
  • Factorize some validation code

Notes:

  • A lot of tests are impacted by the change, if anybody has a suggestion for a quick solution to avoid that...
  • It would be nice to implement the DID derivation in each DIDMethod but this would require quite some work to be able to remove the did:sov and did:key initialization hardcoded in DIDMethods.

Signed-off-by: Clément Humbert [email protected]

@chumbert chumbert force-pushed the feature/create-all-registered-dids branch from f6f89f8 to 9cb75be Compare January 5, 2023 08:02
To leverage the recent addition of the `DIDMethods` registry:
    * Relax condition on did format for the `POST /wallet/did` endpoint
    * Validate new DIDs parameters using `DIDMethods` in Askar and InMemory profiles
      validate new DIDs parameters

Signed-off-by: Clément Humbert <[email protected]>
@chumbert chumbert force-pushed the feature/create-all-registered-dids branch from 9cb75be to 224b93e Compare January 5, 2023 08:08
@swcurran
Copy link
Contributor

swcurran commented Jan 5, 2023

FYI - @andrewwhitehead @ianco @dbluhm

@ianco
Copy link
Contributor

ianco commented Jan 6, 2023

I notice there are changes to the askar and in_memory wallets but not indy?

@chumbert
Copy link
Contributor Author

chumbert commented Jan 9, 2023

I notice there are changes to the askar and in_memory wallets but not indy?

@ianco

Correct. As far as I've seen, indy wallet implementation does not support the injection context yet. And it does not appear to be a trivial change (with a 15min investigation).

Even if we want it to be added, I would argue we should defer this to another piece of work and PR. What do you think ?

@swcurran
Copy link
Contributor

I would agree that we add that this is needed for the Indy-SDK as an issue and whereever else a notification should be added. However, I think we can proceed with that limitation understood.

chumbert and others added 2 commits January 13, 2023 07:46
Get holder-defined DID from options instead of requests' root.

Signed-off-by: Clément Humbert <[email protected]>
@chumbert chumbert force-pushed the feature/create-all-registered-dids branch from 44746f9 to aae6d83 Compare January 13, 2023 07:02
@ianco
Copy link
Contributor

ianco commented Jan 13, 2023

I would agree that we add that this is needed for the Indy-SDK as an issue and whereever else a notification should be added. However, I think we can proceed with that limitation understood.

OK I think on-going support for indy-sdk in aca-py is an open question right now anyways, so adding a separate issue to add this functionality to the "indy" modules in aca-py is a good approach ...

Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

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

Overall it looks good. Two things - (1) we need some documentation somewhere (minimum would be something in the swagger docs, nice would be updates to DIDResolution.md, @swcurran thoughts?), and (2) we need to update the alice/faber demo to include this feature somewhere (I can help with this one)

* Augment existing swagger documentation for the `POST
  /wallet/did/create` route
* Add `DIDMethods.md` documentation to introduce the concept of the
  `DIDMethods` registry and its use to register new methods.

Signed-off-by: Clément Humbert <[email protected]>
@chumbert
Copy link
Contributor Author

(1) we need some documentation somewhere (minimum would be something in the swagger docs, nice would be updates to DIDResolution.md

I added a DIDMethods.md to avoid mixing all the concepts. It refers to DIDResolution.md as a follow-up read.

(2) we need to update the alice/faber demo to include this feature somewhere (I can help with this one)

I'll gladly take pointers on how we should include the feature in the demo.

@ianco ianco enabled auto-merge January 16, 2023 15:03
@ianco
Copy link
Contributor

ianco commented Jan 16, 2023

@chumbert the docs look good, thanks! I need to think about how to add this to the demo ...

The branch is out of date though, if you can sync it up with the main branch then we can merge

@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

@ianco ianco merged commit ea31bd7 into openwallet-foundation:main Jan 16, 2023
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.

3 participants