refactor(dids): use class instances in module config #1175
Conversation
Signed-off-by: Timo Glastra <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1175 +/- ##
==========================================
- Coverage 88.80% 88.80% -0.01%
==========================================
Files 707 705 -2
Lines 16567 16554 -13
Branches 2806 2798 -8
==========================================
- Hits 14713 14700 -13
Misses 1744 1744
Partials 110 110
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
I really like this update: it will allow a lot more flexibility to add custom registrars and resolvers.
I left a question regarding a potential custom PeerDidRegistrar or PeerDidResolver.
And maybe for another PR, but just something I'm wondering now I'm reviewing this: is there a way of registering a registrar or resolver after the agent is instantiated? I know it's not super necessary but I don't like so much to be forced to explicitly define all the registrars and resolvers (even the default ones) when I just want to use an additional one, especially because the framework can include some new ones in the future and I could be missing something without knowing. Unless we expose some constant arrays indicating the default registrars and resolvers (hard to maintain tough)
| ] | ||
|
|
||
| // Add peer did registrar if it is not included yet | ||
| if (!registrars.find((registrar) => registrar instanceof PeerDidRegistrar)) { |
There was a problem hiding this comment.
What about making findRegistrarForMethod public and using it instead, allowing the usage of a custom Peer Did Registrar that is not a sub-class of PeerDidRegistrar? Do you think it could be possible or it's needed to enforce the default implementation?
There was a problem hiding this comment.
Yes good point. I'll look at a way to do it using the supportedMethods array. Reason I specifically required this one is that the interface for each registrar is different and so a different did:peer registrat may not work. Resolver should be less of an issue. The framework is currently written to work with this specific resolver/registrat
There was a problem hiding this comment.
Looked at it a bit. Don't think we can easily do this now as we depend on this specific did peer implementation. Any thoughts?
There was a problem hiding this comment.
I think it's fine if we depend on it right now, as I don't see yet any specific need where we would like to override it.
We just need make sure that framework will always use it even if user attempts to register another one for did:peer. I think it is the case, because we use Array.find method, which will return the first one added. From my POV there is no need to do a specific check or throwing an specific error as long as we document this constraint somewhere (another TODO for aries-javascript-docs 😅 )
There was a problem hiding this comment.
Ah I get what you're saying. Yes good point. We should maybe do an extra check in that case... OR we should only allow a single registrar/resolver to be registered for each method maybe?
There was a problem hiding this comment.
Well I think it would be better, as you say, to allow a single registrar/resolver per method and throw an error if user attempts to register a second one. But that could be part of a separate PR after 0.3.0 release
| ] | ||
|
|
||
| // Add peer did resolver if it is not included yet | ||
| if (!resolvers.find((resolver) => resolver instanceof PeerDidResolver)) { |
There was a problem hiding this comment.
Same question as for the registrar, but I think we don't have a findResolverForMethod yet
There was a problem hiding this comment.
This one could be updated to use the supportedMethods array, as the resolving interface is consistent
We van add a method to the did config to add new ones. We should make sure to always get them from the config, but should be easy to add |
|
@genaris it would look something like this. Is that what you're thinking of? const didsModule = new DidsModule()
didsModule.config.addRegistrar(new CustomDidRegistrar()) |
It looks great! |
Signed-off-by: Timo Glastra <[email protected]>
|
Any other things you want resolved as part of this PR @genaris? Otherwise I'd like it to be merged as part of the 0.3.0 release (which is really close now!) |
Makes the registration of did resolvers and registrars consistent with how we do it in the credentials module by requiring class instances to be passed.
Not really a breaking change as 0.2.0 doesn't allow custom modules yet