-
Notifications
You must be signed in to change notification settings - Fork 208
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: add did:webvh as a DID resolver #2186
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 4b35c3d The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: Brian Richter <[email protected]>
Signed-off-by: Brian Richter <[email protected]>
4757a8c
to
4b35c3d
Compare
// private resolver = didWeb.getResolver() | ||
|
||
public async resolve(agentContext: AgentContext, did: string): Promise<DidResolutionResult> { | ||
const result = await resolveDID(did) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably have a try catch, as it seems the resolveDID
can throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we generally return a specific notFound result as defined in https://www.w3.org/TR/did-resolution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, will update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @brianorwhatever, thanks so much for this PR!
I have some doubts with adding the didwebvh-ts library as is:
- It seems to depend on some dependencies that may not work well in React Native. Have you already tested the library to work in React Native? Libraries that may cause issues are nanoid (fixed recently, but not released yet), elysia (web framework?), multiformats (we had to remove this dependency and re-implement some stuff, not sure what parts of multiformats it uses). It's important that the core package works in all environments.
- crypto is handled by the didwebvh-ts libary. Generally we want all crypto to be handled by the wallet / crypto implementation of Credo and thus it'd be great if the crypto is pluggable . This is important as it's often required for organizations to have all crypto handled in a single place. Generally all our lower-level libs we create nowadays (sd-jwt vc, mdoc, oid4vc-ts) all don't come with any crypto support, and it needs to be provided dyncamically.
I understand this might not be so easy to fix, but unless we can meet these requirements I think it's better to create this as an extension module ,and we can clearly mention in the docs that this module depends on outside crypto /may not work in React Native.
Curious to hear your thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Brian! I think did:webvh resolving capabilities is a great addition to Credo. The problem I see is that, currently, didwebvh-ts
seems to be too backend-oriented, including the elysia
dependency (used to expose the resolver as a web server).
Do you think it would be possible to split this library into a pure JS-client package and a separate app that manages the REST API to resolve DIDs in a backend? I think this would be useful not only for us but also for other client-side apps.
We could also reimplement the resolver logic within Credo Core. I'd like to avoid that because it can be harder to maintain afterwards as did:webvh spec evolves, but can be of course more efficient in terms of dependency tree.
@TimoGlastra @genaris ok, great feedback thank you! I have not yet tested in React Native as I am having trouble getting bifold to use my local credo package.. so definitely could have some errors there. Goal is definitely to have Here is the list of issues I will tackle based on the above comments:
Please let me know if you think of anything else I should address |
I'll admit I don't know how to use
pnpm
so I worry thepackage.json
changes aren't required it seems to have sorted and upgraded some unrelated packages. Any help is appreciated!