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

feat: jsonld-credentials #718

Conversation

karimStekelenburg
Copy link
Contributor

@karimStekelenburg karimStekelenburg commented Apr 22, 2022

Adds the low-level functionality that is required for the issuance and verification of JSON-LD based credentials (W3C).

The key features this PR includes are:

  • The W3cCredentialService that can be used for signing and verifying JSON-LD based credentials and presentations.
  • The W3cCredentialRepository that handles storage for JSON-LD based credentials.
  • The Ed25519Signature2018 signature suite that adds support for signing and verifying Ed255192018 signatures.
  • The BbsBlsSignature2020 signature suite that adds support for signing and verifying BbsBls2020 signatures.
  • The BbsBlsSignatureProof2020 signature suite that adds support for deriving, signing and verifying proofs of BbsBls2020 signed credentials.

@karimStekelenburg karimStekelenburg changed the title feat/jsonld-credentials feat: jsonld-credentials Apr 28, 2022
@karimStekelenburg karimStekelenburg marked this pull request as ready for review April 29, 2022 00:13
@karimStekelenburg karimStekelenburg requested a review from a team as a code owner April 29, 2022 00:13
@karimStekelenburg
Copy link
Contributor Author

@TimoGlastra I missed this during development, but apparently one of our external dependencies (@digitalbazaar/ed25519-verification-key-2020) requires Node version >=14. Can we bump AFJ's required version, or will this cause too much trouble for existing projects?

I'm not why this requirement is there yet. However, the dependency isn't that big, so we might be able to fix it by pulling it into AFJ and make some compatibility modifications.

@TimoGlastra
Copy link
Contributor

Node 12 will reach EOL on 2022-04-30 (tomorrow) so I'm fine with dropping support for node12.

However looking at the code the usage of the verification key library is so minimal I'd like to remove it either way. The number of dependencies this PR adds is non-trivial and If possible it would be great to remove a few before merging (I think by removing the verification key and context dependencies we can drop 4 dependencies)

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Nice work @karimStekelenburg! Left quite some comments, but most are cosmetic / cleanup.

DEVREADME.md Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
packages/core/src/crypto/BbsService.ts Show resolved Hide resolved
packages/core/src/modules/vc/constants.ts Show resolved Hide resolved
packages/core/types/base64url-universal.d.ts Outdated Show resolved Hide resolved
@TimoGlastra TimoGlastra changed the base branch from main to next May 2, 2022 11:58
@TimoGlastra
Copy link
Contributor

I merged #684 and set the target of this branch to next. Good luck with the merge conflicts @karimStekelenburg 😄

@karimStekelenburg
Copy link
Contributor Author

@TimoGlastra not sure what you mean 'just the type'. You mean we shouldn't update the context? Didn't understand what you meant by sending the link either.

@karimStekelenburg
Copy link
Contributor Author

Instead of modifying the input object I'd use destructuring: (or is this an instance of VerificationMethod instead of a plain object?)

verificationMethod = {
   ...verificationMethod,
   publicKeyMultibase: undefined
   publicKeyBase58: pubKeyBase58
}

Jup, much better indeed 🚀

@TimoGlastra
Copy link
Contributor

@TimoGlastra not sure what you mean 'just the type'. You mean we shouldn't update the context? Didn't understand what you meant by sending the link either.

It's type not @type

@karimStekelenburg
Copy link
Contributor Author

Fixed latest comments in beeaf68.

@karimStekelenburg
Copy link
Contributor Author

@TimoGlastra done fixing the issues, we should be ready to go.

@TimoGlastra
Copy link
Contributor

DCO is failing

@karimStekelenburg
Copy link
Contributor Author

Isn't that automatically fixed when squashing?

@TimoGlastra
Copy link
Contributor

Well yes, but we can't merge without. The squash would only be valid if all commits that went into the squash are signed off

@TimoGlastra TimoGlastra added this to the v0.3.0 milestone May 19, 2022
Signed-off-by: Karim <karim@animo.id>
@karimStekelenburg karimStekelenburg merged commit fd293e9 into openwallet-foundation:0.3.0-pre May 19, 2022
@karimStekelenburg
Copy link
Contributor Author

Had to manually merge this into 0.3.0-pre due to some fun conflicts, but I think we're good!

TimoGlastra pushed a commit that referenced this pull request May 19, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
karimStekelenburg added a commit that referenced this pull request May 24, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request May 26, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request May 31, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jun 3, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jun 10, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jun 15, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jun 15, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jun 17, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jun 30, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jul 3, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Jul 4, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Aug 11, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Aug 26, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
TimoGlastra pushed a commit that referenced this pull request Aug 26, 2022
Signed-off-by: Karim Stekelenburg <karim@animo.id>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants