-
Notifications
You must be signed in to change notification settings - Fork 135
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
Integrate the SD-JWT #1358
base: next
Are you sure you want to change the base?
Integrate the SD-JWT #1358
Conversation
At this point I don't feel well opening the PR to the veramo repo yet. Linting is based on biome and not prettier, testing on vitest and not jest. |
Okay, I'll make it draft and keep working on here. :) |
Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
This reverts commit 4d0dc75. Signed-off-by: Lukas <[email protected]>
This reverts commit 63d76ff7c15cbfc6abe13e64eaf0ee3802813481. Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
This reverts commit 4d0dc75. Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
This reverts commit 376924f. Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
Signed-off-by: Lukas <[email protected]>
@cre8 I added test and fixed the schema generation :) |
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.
mostly very good! thanks again for this!
in addition to the other comments, could you add a test inside the test-react-app
package that exercises this functionality?
if (!issuer) { | ||
throw new Error('credential.issuer must not be empty') | ||
} | ||
if (issuer.split('#').length === 1) { |
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 function should work even if just an issuer DID is provided and then it can check for managed keys for the DID and see if any appropriate ones can be used. of course, a specific key can also be specified
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 removed the constraint
packages/sd-jwt/package.json
Outdated
"url": "https://github.com/decentralized-identity/veramo.git", | ||
"directory": "packages/selective-disclosure" | ||
}, | ||
"author": "Consensys Mesh R&D <[email protected]>", |
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.
feel free to change author/contributors to yourself
@@ -0,0 +1,346 @@ | |||
import { subtle } from 'node:crypto' |
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.
these first 2 dependencies imported in this file could present problems. I know this is only a test, but we try to only use dependencies that work across all supported platforms. Is it not possible to use one of the other crypto libraries already used elsewhere for this?
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, It's just the verify function
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.
Have you ever written a verify function that is platform-independent? I tried to use the @noble package, but it doesn’t work.
packages/sd-jwt/package.json
Outdated
}, | ||
"dependencies": { | ||
"@sphereon/ssi-sdk-ext.did-utils": "^0.16.0", | ||
"@sd-jwt/core": "0.3.2-next.107", |
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.
do all of these sd-jwt packages support browser and react native (except for crypto-nodejs
)?
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'll check on them and let you know :)
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.
Yeah, It works regardless of platform except crypto-nodejs
Good I'll definitely add this module |
Hi @lukasjhan and @cre8 :) maybe this is not the thread but i want to try to ask you a question maybe you can help me . |
Yes, it's possible, I implemented it here: |
Also be aware that sd-jwt is supported by our lower level libraries, including our credential mappers. If you include the sd-jwt veramo plugin into the idunion didcomm project using our SDK it should work including presentation exchange etc |
Thanks a lot @nklomp : I think the problem will bee that the IDunion Project use the // Creating a creadential we have this snippet :
i think i will get some kind of error passing a sd-jwt type; |
Since that is using our low-level libs, which have sd-jwt support it should work at that point. Having said that, we are actually integrating sd-jwt into our SDKs, wallets and agents as we speak. So we should have this working probably next week. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@lukasjhan I'm just wondering if there's been any progress on this recently? Is there anything in particular keeping us from moving forward? I know it will need to be refactored to support the new |
Sphereon has published an SD jwt package in the meantime: https://github.com/Sphereon-Opensource/SSI-SDK/tree/develop/packages%2Fsd-jwt I haven't tested it yet, but when it works it makes no sense to continue the work on a second package from my point of view. |
Please be aware that our one, especially in a feature branch depends on very specific modules from our SDK. We have modules for uniform identifier/key resolution, x.509 support, jwt/jws signatures that the sd-jwt module depends on. |
All of that to say, that our module is more versatile as it does not have some of the errors and assumptions in this plugin (encountering a jwk, doesn't mean a did:jwk perse of course). At the same time since our sdk can work with regular jwks, x5cs etc next to DIDs and since we have our own jose signature service, our plugin depends on multiple of our other modules. So you cannot simply include our module as a single dep into a vanilla Veramo environment |
Hi @nickreynolds, sry I've been busy for this summer. I think I can do |
@lukasjhan great to hear that you can pick this back up! The best way to understand the new architecture is probably to review this PR: #1395 Basically, credential plugins now implement a new shared interface and each of these is passed as a |
Sry for delay. I'll finish this pr by ends of this year. |
Hi I'm currently developing sd-jwt typescript library in Open Wallet Foundation with @cre8. I want to continuously contribute to Veramo by working on sd-jwt-related integration.
Related Issue: #1276
This is my first time contributing to Veramo. If there is anything you would like to change, please let me know. I'm open to any changes :)
What issue is this PR fixing
Example:
closes #123
fixes #456
Linking to an issue provides some context and a reason for the PR to be reviewed, as well as simplifying the release
notes and changelogs that get generated automatically. If an issue is linked like this it will be automatically closed
when the PR is merged.
What is being changed
Add SD-JWT features in Veramo
Quality
Check all that apply:
pnpm i
,pnpm build
,pnpm test
,pnpm test:browser
locally.Details
sd-jwt package has 4 main features
About Testing
This is the sample of tests, but
I failed setup the agent in test file in this way
If there is a guide on how to set up this context, please let me know and I will add it.
Or, this is the original test file in cre8's repo. https://github.com/cre8/sd-jwt-veramo/blob/main/src/agent-plugin/sd-jwt-plugin.spec.ts
If it's okay to add it like this, then I'll add it like this.
Please take a look and let me know about the test.
Thank you.