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(credential-ld): Added Ed25519Signature2020 & JsonWebSignature2020 experimental support #1030

Merged
merged 13 commits into from
Nov 18, 2022

Conversation

Eengineer1
Copy link
Contributor

Linked issue & detailed approach #1003 .

DaevMithran and others added 2 commits October 11, 2022 20:15
…[DEV-1305] (#4)

* feat: Add ed25519Signature2020 support

Signed-off-by: DaevMithran <daevmithran1999@gmail.com>

* feat(json-ld): Added `Ed25519Signature2020` experimental support & key pre-mods

* Added `JsonWebSignature2020` experimental support

Signed-off-by: DaevMithran <daevmithran1999@gmail.com>
Co-authored-by: Ankur Banerjee <ankurdotb@users.noreply.github.com>
Co-authored-by: Tasos <50984242+Eengineer1@users.noreply.github.com>
@Eengineer1 Eengineer1 changed the title feat(json-ld): Added Ed25519Signature2020 & JsonWebSignature2020 experimental support feat(credential-ld): Added Ed25519Signature2020 & JsonWebSignature2020 experimental support Oct 12, 2022
@mirceanis
Copy link
Member

mirceanis commented Oct 14, 2022

Thanks for adding this.
It looks like these new packages or rather their dependencies are causing conflicts in some environments.

We ran into lots of such issues with @transmute and @digitalbazaar dependencies while trying to add JSON-LD support, and I don't know of a solution that works in all cases.

@Eengineer1
Copy link
Contributor Author

Eengineer1 commented Oct 14, 2022

@mirceanis Thanks for mentioning.
I can recall from our early chat about dependency issues.

What's included as well this PR:

  • Bump @types/eslint, 8.4.3 -> 8.4.6.
  • Bump @types/eslint-scope, 3.7.3 -> 3.7.4.
  • Added as direct dependency in test-react-app to not clash with lower expected versions.
  • Added node version compatible < v16.x, multiformats@9.7.1 for multibase keys.

I'm pushing a fix on @digitalcredentials/vc <> @transmute/json-web-signature compatibility later today.

Questions:

  • Are we open to exploring bumps, as long as the tests work as existing?
  • To what extent are we open on refactoring tests to comply to bumps?

@mirceanis
Copy link
Member

Questions:

  • Are we open to exploring bumps, as long as the tests work as existing?

What do you mean by bumps? version bumps?
Then yes. We try to keep up-to-date with all dependencies. We use renovate-bot to automatically bump dependencies that are safe.

  • To what extent are we open on refactoring tests to comply to bumps?

Tests should reflect real-world scenarios as close as possible. If we can't get our tests to run it's very likely that some user will face the same issues, so we are not going to include changes that we can't handle ourselves.
Refactoring tests is acceptable, as long as it doesn't hide any issues coming from dependencies.

We have a number of constraints that we impose on the packages in this repository:

  • they should all work in all 3 environments: nodejs, web, react-native
  • it's ok to also need workarounds for some environments as long as we know what the workarounds are
  • Veramo plugin method arguments and results should be serializable to JSON (this allows us to automatically compute the OpenAPI schemas)
  • plugins must not conflict with each-other; a user should be able to create a Veramo instance with all available plugins.

If we can't enforce these constraints on a new plugin then it should not be in this repository.

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Merging #1030 (f2aa85a) into next (125bf42) will decrease coverage by 0.57%.
The diff coverage is 76.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1030      +/-   ##
==========================================
- Coverage   80.25%   79.67%   -0.58%     
==========================================
  Files         118      127       +9     
  Lines        4056     4610     +554     
  Branches      875      993     +118     
==========================================
+ Hits         3255     3673     +418     
- Misses        798      933     +135     
- Partials        3        4       +1     

@Eengineer1
Copy link
Contributor Author

@mirceanis Hey, this is ready for your review.

Kudos to the Transmute team on helping out untangle the non-polyfilled dependency knot upstream.

@mirceanis
Copy link
Member

@mirceanis Hey, this is ready for your review.

This looks good. Can you try to rebase to fix the conflicts?

@Eengineer1
Copy link
Contributor Author

@mirceanis Hey, this is ready for your review.

This looks good. Can you try to rebase to fix the conflicts?

Resolved.

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

looks great, thank you for the contribution!

@mirceanis mirceanis merged commit fbf7d48 into decentralized-identity:next Nov 18, 2022
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.

3 participants