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(dids): add did registrar #953

Conversation

TimoGlastra
Copy link
Contributor

This has been on my local clone for months, and finally got to finishing it.

Adds a generic did registrar based on the DID Registration spec from DIF. It adds support for registering did:key, did:sov and did:peer dids. For it to be useful with did:sov dids there's probably a bit more work needed as you need to provide an submitterDid which needs to have the endorser role. For now this can still be done using the publicDidSeed property, but the idea is to remove the publicDidSeed in the near future. We should add a way to store a did, without registering it on the ledger, but as the PR is already quite large I'll do that in a separate PR.

It also makes it possible to register did registrars and resolvers on the dids module. No way to configure them yet, but once that's possible it'll work out of the box:

const didsModule = new DidsModule({
  resolvers: [MyCustomerResolver]
})

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
…trar-module-030-full

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra requested a review from a team as a code owner July 19, 2022 14:48
@TimoGlastra TimoGlastra added the 0.3.0 Changes for the 0.3.0 release label Jul 19, 2022
Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

nice @TimoGlastra!

2 things:

  1. I am really not a big fan of the error handling that is done here with the didState and the result message. It is not an uncommon pattern but its not consistent with the rest of the framework. Is there any specific reason why you chose for this?
  2. Is there any specific reason why the registrar and resolver are separated? Like, do we see a case where you would like to only have a resolver or registrar without the other? I am not opposed to splitting it up, but more just curious for the reasoning behind it.

@TimoGlastra
Copy link
Contributor Author

I am really not a big fan of the error handling that is done here with the didState and the result message. It is not an uncommon pattern but its not consistent with the rest of the framework. Is there any specific reason why you chose for this?

This is based on https://identity.foundation/did-registration/. It's a standard interface for did registration. We also support the counterpart for resolution: https://w3c-ccg.github.io/did-resolution/. When adding the resolver functionality I discussed with Jakub and he was also in favour of this approach, even though not in line with the rest of AFJ's interface.

I kinda like it because it allows for a bit more detailed responses. E.g. the didState has more than just success and failed.

But the main reason I went with this appraoch is because there was an interface defined that we'll probably never do better ourselves. There's a lot of difference between did methods and getting it right is hard. We could choose to only adopt the spec for the input parameters, but then we still need some way of didState, etc...

How would you approach it differently?

Is there any specific reason why the registrar and resolver are separated? Like, do we see a case where you would like to only have a resolver or registrar without the other? I am not opposed to splitting it up, but more just curious for the reasoning behind it.

Registrars are often more complicated than resolvers. I see e.g. mobile devices supporting a lot of resolves, while only supporting a few registrars. This is also what is done with e.g. the universal resolver and universal registrar. There's just a lot less complexity in a resolver. We could of course combine them and add not implemented errors, but a bit worried about possible extra needed dependencies for registrars over resolvers.

What do you think?

@berendsliedrecht
Copy link
Contributor

I am really not a big fan of the error handling that is done here with the didState and the result message. It is not an uncommon pattern but its not consistent with the rest of the framework. Is there any specific reason why you chose for this?

This is based on https://identity.foundation/did-registration/. It's a standard interface for did registration. We also support the counterpart for resolution: https://w3c-ccg.github.io/did-resolution/. When adding the resolver functionality I discussed with Jakub and he was also in favour of this approach, even though not in line with the rest of AFJ's interface.

I kinda like it because it allows for a bit more detailed responses. E.g. the didState has more than just success and failed.

But the main reason I went with this appraoch is because there was an interface defined that we'll probably never do better ourselves. There's a lot of difference between did methods and getting it right is hard. We could choose to only adopt the spec for the input parameters, but then we still need some way of didState, etc...

How would you approach it differently?

Okay yeah I see your point. I think its okay to keep it like this. Since these can also be dynamically added by any user, keeping 100% consistent with the interfaces provided by the specs is the way to go.

Is there any specific reason why the registrar and resolver are separated? Like, do we see a case where you would like to only have a resolver or registrar without the other? I am not opposed to splitting it up, but more just curious for the reasoning behind it.

Registrars are often more complicated than resolvers. I see e.g. mobile devices supporting a lot of resolves, while only supporting a few registrars. This is also what is done with e.g. the universal resolver and universal registrar. There's just a lot less complexity in a resolver. We could of course combine them and add not implemented errors, but a bit worried about possible extra needed dependencies for registrars over resolvers.

What do you think?

The mobile case is indeed a good point. I was not specifically in favor of combining them, but was looking more for a use case to justify the seperation.

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
…lastra/aries-framework-javascript into feat/did-registrar-module-030-full
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra merged commit d2fe29e into openwallet-foundation:0.3.0-pre Jul 21, 2022
@TimoGlastra TimoGlastra deleted the feat/did-registrar-module-030-full branch July 21, 2022 15:05
TimoGlastra added a commit that referenced this pull request Aug 11, 2022
Signed-off-by: Timo Glastra <timo@animo.id>
TimoGlastra added a commit that referenced this pull request Aug 26, 2022
Signed-off-by: Timo Glastra <timo@animo.id>
TimoGlastra added a commit that referenced this pull request Aug 26, 2022
Signed-off-by: Timo Glastra <timo@animo.id>
genaris pushed a commit to 2060-io/aries-framework-javascript that referenced this pull request Sep 16, 2022
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.3.0 Changes for the 0.3.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants