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

Integrate the SD-JWT #1358

Draft
wants to merge 22 commits into
base: next
Choose a base branch
from

Conversation

lukasjhan
Copy link

@lukasjhan lukasjhan commented Mar 3, 2024

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:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.
  • I did not add automated tests because I had difficulty setting up the test agent, and I am aware that a PR without tests will likely get rejected.

Details

sd-jwt package has 4 main features

  • create sd-jwt-vc
  • create sd-jwt-vc presentation
  • verify sd-jwt-vc
  • verify sd-jwt-vc presentation

About Testing

This is the sample of tests, but

I failed setup the agent in test file in this way

const context = {
  agent: {
    execute: jest.fn(),
    availableMethods: jest.fn(),
    getSchema: jest.fn(),
    emit: jest.fn(),
  },
} as IAgentContext<any>

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.

@cre8
Copy link
Contributor

cre8 commented Mar 3, 2024

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.

@lukasjhan lukasjhan marked this pull request as draft March 4, 2024 00:43
@lukasjhan
Copy link
Author

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. :)

lukasjhan and others added 12 commits March 6, 2024 14:35
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
This reverts commit 4d0dc75.

Signed-off-by: Lukas <Lukas@hopae.io>
This reverts commit 63d76ff7c15cbfc6abe13e64eaf0ee3802813481.

Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
This reverts commit 4d0dc75.

Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
This reverts commit 376924f.

Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
Signed-off-by: Lukas <Lukas@hopae.io>
@lukasjhan
Copy link
Author

@cre8 I added test and fixed the schema generation :)

Copy link
Contributor

@nickreynolds nickreynolds left a 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) {
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

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

I removed the constraint

"url": "https://github.com/decentralized-identity/veramo.git",
"directory": "packages/selective-disclosure"
},
"author": "Consensys Mesh R&D <hello@veramo.io>",
Copy link
Contributor

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'
Copy link
Contributor

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?

Copy link
Author

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

Copy link
Author

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.

},
"dependencies": {
"@sphereon/ssi-sdk-ext.did-utils": "^0.16.0",
"@sd-jwt/core": "0.3.2-next.107",
Copy link
Contributor

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)?

Copy link
Author

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 :)

Copy link
Author

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

@lukasjhan
Copy link
Author

Good I'll definitely add this module test-react-app :)
I'll do it after I resolved all comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants