-
Notifications
You must be signed in to change notification settings - Fork 219
feat(webvh): Added did:webvh resolver #2238
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
base: main
Are you sure you want to change the base?
feat(webvh): Added did:webvh resolver #2238
Conversation
Signed-off-by: Brian Richter <brian@aviary.tech>
🦋 Changeset detectedLatest commit: facc66b The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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 |
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.
The new approach is awesome, thanks @brianorwhatever. I left some comments. It seems some parts are todo, do you want to address these as part of this PR, or in another one?
|
||
/** See {@link WebvhModuleConfigOptions.baseUrl} */ | ||
public get baseUrl() { | ||
return this.options.baseUrl ?? 'https://webvh.io' |
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.
What does this default domain do? Is the baseUrl for when you create DIDs? I think having no default value and allowing it to be passed when creating one makes sense.
} | ||
} | ||
const privateKey = Buffer.from(MultiBaseEncoder.decode(privateKeyMultibase).data) | ||
if (privateKeyMultibase && !isValidPrivateKey(privateKey, KeyType.Ed25519)) { |
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.
Is Ed25519 required? Just curious, we'd like to use it with P256 as well
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.
Yes unfortunately ED25519 is required for did:webvh for update keys. We did it that way to standardize it on one interoperable crypto suite you can of course use any keys in a DID document you desire.
}, | ||
} | ||
} | ||
const privateKey = Buffer.from(MultiBaseEncoder.decode(privateKeyMultibase).data) |
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.
Is it possible to pass an existing public key, but not the private key? We usually create a key first, then use that to create a diddoc
(This is fine to add later, just trying to understand how it works)
} | ||
|
||
// Register the DID using didwebvh-ts | ||
const response = await createDID() |
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.
Shouldn't we somehow pass the did document here?
} | ||
|
||
// Update the DID using didwebvh-ts | ||
const response = await updateDID({doc: options.didDocument.toJSON()}) |
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.
I think we should first fetch the record to see if it exists
} | ||
|
||
public async deactivate(agentContext: AgentContext, options: WebvhDidDeactivateOptions): Promise<DidDeactivateResult> { | ||
// const didRepository = agentContext.dependencyManager.resolve(DidRepository) |
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.
If this is not supported yet we can return an error for now and remove this code
} | ||
|
||
interface IVerificationMethod { | ||
type: string |
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.
Type must be MultiKey right?
throw new Error('Not implemented') | ||
} | ||
|
||
public async verify(signature: Uint8Array, message: Uint8Array, publicKey: Uint8Array): Promise<boolean> { |
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.
It's probably good if the interface from did web vh passes the keytype as well, so it's ready to support additional key types as well
signature: Buffer.from(signature), | ||
}) | ||
} catch (error) { | ||
// Log error in a non-production environment |
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.
You can use agentContext.config.logger.error for this, and can be disabled by the user of the framework
export class DIDWebvhCrypto extends AbstractCrypto { | ||
private agentContext?: AgentContext | ||
|
||
public constructor(agentContext?: AgentContext) { |
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.
I think we can make agent context required
@TimoGlastra I forgot to mention I was focussing solely on resolver in this PR. I added some of the base stuff for the registrar, which will be coming soon. I'm happy to either remove that stuff or fix it up later. Higher priority is resolving DID documents and resources. |
Ah that makes sense @brianorwhatever. Let's focus this pr on the resolving in that case and we can add the registration parts next 👍 |
Curious is the implementation backwards compatible with older versions? The swiss eid public beta that was announced yesterday uses version 0.3. I'm trying to understand if we could use this resolver / didwebvh-ts for integrating with this? |
OK, great I will remove the stuff about the registrar. Unfortunately, I don't think that this is 0.3 compatible I can compare the differences and see what would be required to make it work. |
Hey @TimoGlastra, quick question about verifying Data Integrity proofs in a custom setup. We're building an Inside our registry logic (in a helper
The last piece is actually verifying the We also considered using the signature suites directly (like Is there a recommended/simpler way in Credo to verify a DI proof on a custom JSON-LD object like this, making sure key resolution via the Currently have it stubbed out, but want to make sure we implement the crypto verification correctly using Credo's infrastructure. Thanks! |
- Removed WebvhDidRegistrar class and its related exports from the dids module. - Updated index.ts and WebvhModule.ts to reflect the removal of the registrar. - Adjusted DIDWebvhCrypto class to ensure agentContext is always defined. - Cleaned up unused functions and imports in didWebvhUtil.ts. Signed-off-by: Brian Richter <brian@aviary.tech>
…ial definition support - Added WebVhAnonCredsRegistry class to handle anon credentials with did:webvh identifiers. - Implemented methods for retrieving schemas, credential definitions, and revocation registry definitions. - Introduced validation and error handling for resource resolution and proof verification. - Created utility functions for multihash encoding and base58 conversion. - Added tests for WebVhAnonCredsRegistry and resource transformation. - Updated package.json to include new dependencies for class-transformer and class-validator. Signed-off-by: Brian Richter <brian@aviary.tech>
8d90812
to
facc66b
Compare
@genaris @TimoGlastra as discussed I have pulled the crypto out of didwebvh-ts while also removing all of the dependencies. Please take a look at this new PR