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

[Bundle Size][Encryption] Use noble-secp256k1 to replace elliptic dependency #1200

Merged
merged 3 commits into from
Mar 28, 2022

Conversation

ahsan-javaid
Copy link
Contributor

Description

This PR replaces the elliptic dependency with @noble/secp256k1 in enryption package.

Most of the bundle size is comming from elliptic library around 130kb.
Reference: https://bundlephobia.com/package/elliptic@6.5.4

@noble/secp256k1 is fast and small in size: https://bundlephobia.com/package/@noble/secp256k1@1.5.5

For details refer to issue #1192

Type of Change

  • New feature
  • Bug fix
  • API reference/documentation update
  • Other

Does this introduce a breaking change?

No

Are documentation updates required?

No

Testing information

  1. npm run test

Checklist

  • Code is commented where needed
  • Unit test coverage for new or modified code paths
  • npm run test passes
  • Changelog is updated
  • Tag @janniks and @zone117x for review

@vercel
Copy link

vercel bot commented Feb 28, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/blockstack/stacks-js/HBxwbMz2Wn6KyTeDrr9NGzhsV1i7
✅ Preview: https://stacks-js-git-fix-encryptionoffload-elliptic-lib-blockstack.vercel.app

@ahsan-javaid ahsan-javaid changed the title fix: use noble-secp256k1 in encryption to replace elliptic dependency [Bundle Size][Encryption] Use noble-secp256k1 to replace elliptic dependency Feb 28, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2022

Codecov Report

Merging #1200 (2fbeb2a) into master (ac3858c) will increase coverage by 0.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1200      +/-   ##
==========================================
+ Coverage   64.83%   65.07%   +0.24%     
==========================================
  Files         122      122              
  Lines        8385     8442      +57     
  Branches     1542     1548       +6     
==========================================
+ Hits         5436     5494      +58     
+ Misses       2841     2840       -1     
  Partials      108      108              
Impacted Files Coverage Δ
packages/encryption/src/cryptoRandom.ts 100.00% <100.00%> (ø)
packages/encryption/src/ec.ts 75.79% <100.00%> (+0.17%) ⬆️
packages/transactions/src/utils.ts 95.60% <0.00%> (+2.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac3858c...2fbeb2a. Read the comment docs.

@ahsan-javaid
Copy link
Contributor Author

@janniks Any further thought's on this PR. Can this be merged or need any further changes.🙏

Copy link
Collaborator

@janniks janniks left a comment

Choose a reason for hiding this comment

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

Looks good in general, I will go over the existing tests once I have a moment. The added tests compare equality of elliptic to noble (which we might want to keep just to be sure they are compatible) but is a bit different to testing equality of previous to new implementation. But maybe the remaining tests were already robust enough. I will just double-check...

packages/encryption/src/ec.ts Show resolved Hide resolved
@janniks
Copy link
Collaborator

janniks commented Mar 16, 2022

@kantai and @zone117x could you maybe give your feedback on these changes as well, as they comprise cryptography based changes/replacements...?

Copy link
Collaborator

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This LGTM!

@janniks janniks merged commit fe3f30e into master Mar 28, 2022
@janniks janniks deleted the fix/encryption/offload-elliptic-lib branch March 28, 2022 21:07
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.

Offload elliptic from encryption
5 participants