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

Add ecdsa-rdfc-2019, eddsa-rdfc-2022 and ecdsa-sd-2023 to supported cryptosuites. #55

Merged
merged 13 commits into from
Nov 14, 2023

Conversation

JSAssassin
Copy link
Contributor

No description provided.

@JSAssassin
Copy link
Contributor Author

JSAssassin commented Nov 10, 2023

@dlongley The verification tests for VC signed with ecdsa-sd-2023 is currently failing, this is the error I am getting https://gist.github.com/JSAssassin/91efb24233792093b7a459893cac4c8f. Could you please guide me on how I can generate a derived proof? Does bedrock-vc-issuer digitalbazaar/bedrock-vc-issuer#124 need to be updated to create vcs with derived proofs?

I also want to point out that the older credentials using ecdsa-2019 and eddsa-2022 with data-integrity/v1 context are no longer passing verification tests. This is the error I am getting https://gist.github.com/JSAssassin/41a6a551af7a130c5492e8030a75a5ac. I had to update them to use data-integrity/v2 context to make the tests pass, is that expected behavior?

@JSAssassin JSAssassin requested a review from dlongley November 10, 2023 16:50
@dlongley
Copy link
Member

@JSAssassin, I'll respond out of band on some of the questions above for quicker discussion.

legacy `ecdsa-2019` and `eddsa-2022` cryptosuites.
@JSAssassin JSAssassin marked this pull request as ready for review November 13, 2023 16:48
@JSAssassin
Copy link
Contributor Author

NOTE: All tests are passing locally.

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Approving -- but some peer deps that were removed as unnecessary need to be added back. I'm not sure what tool / command is being used to determine whether those are needed or not -- but it's probably failing us in some way. Those packages are required peer dependencies.

"@bedrock/meter": "^5.0.0",
"@bedrock/meter-http": "^12.0.0",
"@bedrock/meter-usage-reporter": "^9.0.0",
"@bedrock/mongodb": "^10.0.0",
"@bedrock/multikey-context": "^2.0.0",
"@bedrock/oauth2-verifier": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

This is a required peer dependency of @bedrock/service-core so it should not be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted back the test deps removal here fd3bbbe. Not sure why but when I used npx depcheck it flagged @bedrock/service-core and @bedrock/ledger-context as unused packages and removal of those packages didn't cause any issues during installation or when running test.

test/package.json Outdated Show resolved Hide resolved
@JSAssassin JSAssassin merged commit c91572f into main Nov 14, 2023
@JSAssassin JSAssassin deleted the use-new-cryptosuites branch November 14, 2023 17:16
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.

2 participants