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

fix(credential-eip712): add support for all did methods that use secp256k #1011

Merged
merged 6 commits into from
Oct 3, 2022

Conversation

andyv09
Copy link
Contributor

@andyv09 andyv09 commented Sep 29, 2022

What issue is this PR fixing

This PR fixes the issue (Fixes #991 ) with credential-eip712 library, where only did:ethr was supported, while every did method that uses secp256k should've been.

Closes #991

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

credential-eip712

  • chainID is set to 1 for methods other than did:ethr

utils

  • function getEthereumAddress now generates an address for compressed and uncompressed publicKeyHex, publicKeyBase58 and publicKeyBase64.
  • added utility functions that extract bytes from public keys. Some bits from these functions are duplicated from here and can be later replaced with exported functions from this lib.
  • adds support for 'EcdsaSecp256k1RecoveryMethod2019'

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran yarn, yarn build, yarn test 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 there arent any new features, and I am aware that a PR without tests will likely get rejected.

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #1011 (fd4181f) into next (125bf42) will decrease coverage by 0.09%.
The diff coverage is 75.54%.

❗ Current head fd4181f differs from pull request most recent head c37dbaf. Consider uploading reports for the commit c37dbaf to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1011      +/-   ##
==========================================
- Coverage   80.25%   80.15%   -0.10%     
==========================================
  Files         118      118              
  Lines        4056     4173     +117     
  Branches      875      897      +22     
==========================================
+ Hits         3255     3345      +90     
- Misses        798      825      +27     
  Partials        3        3              

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.

wow, that was fast :)
Thanks for contributing this!

There are some methods added that were already present either directly or with functionality already imported from ethersjs. Please check my comments and let me know what you think.

packages/utils/src/did-utils.ts Outdated Show resolved Hide resolved
packages/utils/src/did-utils.ts Outdated Show resolved Hide resolved
packages/utils/src/did-utils.ts Outdated Show resolved Hide resolved
packages/utils/src/did-utils.ts Outdated Show resolved Hide 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.

This looks great!

There is a small conflict that needs to be resolved before being able to merge.
Can you please take care of it?

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.

Please remove the secp256k1 dependency and other unused imports as they are not needed.

More dependencies === more trouble; we try to be very strict with this :)

packages/utils/src/did-utils.ts Outdated Show resolved Hide resolved
packages/utils/package.json Outdated Show resolved Hide resolved
packages/utils/package.json Outdated Show resolved Hide 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.

fantastic, thank you for the contribution!

@mirceanis mirceanis merged commit 9940068 into decentralized-identity:next Oct 3, 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.

credential-eip712 is not compatible with every DID method
2 participants