-
Notifications
You must be signed in to change notification settings - Fork 512
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: use noble libs for cryptography #2273
feat: use noble libs for cryptography #2273
Conversation
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
@intelliot |
What is the impact on bundle size? I know the existing libs are a substantial source of the size. |
Will have a look tomorrow |
One improvement is that we'll be able to take out some of the custom dedupe code for |
Unfortunately, crypto-browserify seems to include elliptic (and in turn bn.js) regardless (I looked in the webpack output)
With change:
As before:
It seems it will require some further investigations, perhaps updating some of the other packages? |
If this file is updated would browserify crypto shim still be required?
|
Yep, can remove the crypto resolve.fallback entry after replacing that with @noble/hashes:
|
@ckniffen I generally open the monorepo root, but I noticed the eslint config files have root: true soIntelliJ seems to only be applying the root configuration and would only find out about issues when the CI complained. So I opened up the leaves, but then found the typing doesn't seem to work very well. |
Awesome. A 7.5% decrease is really good and removing an extra shim will be really nice. I'm inclined to count this as a major release change. |
Seems to be some issues with the browser tests. |
I usually open the monorepo with all the packages and then just search within it if I need something specific. Lerna makes it ok to install, build, and run the tests from the top, so I usually do that. (Although if you want fewer tests you can cd into the individual package and run npm test there) |
Open whole repo ftw! :)
Takes some work, but if you get all the packages using the same
eslint/typescript config (tricky refactoring financial code such as this),
you can also set up tsconfig paths options so the IDE can understand
everything without building, and refactors work across package boundaries
|
Are these tests a bit flakey at times ? |
// By default, set zip215 to false for compatibility reasons. | ||
// ZIP 215 is a stricter Ed25519 signature verification scheme. | ||
// However, setting it to false adheres to the more commonly used | ||
// RFC8032 / NIST186-5 standards, making it compatible with systems | ||
// like the XRP Ledger. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this comment
- Remove tests from `ripple-keypairs` that exclusively tested `rippled-address-codec`. The tests in `codec.test.ts` and `xrp-codec.test.ts` are all present and accounted for in `packages/ripple-address-codec/test/xrp-codec.test.ts`. - Update `package-lock.json` after the rebase with main - Remove references to `decimal.js` in the documentation This will clean up the diff for #2273
- Remove tests from `ripple-keypairs` that exclusively tested `rippled-address-codec`. The tests in `codec.test.ts` and `xrp-codec.test.ts` are all present and accounted for in `packages/ripple-address-codec/test/xrp-codec.test.ts`. - Update `package-lock.json` after the rebase with main - Remove references to `decimal.js` in the documentation * add missing entry for `bignumber.js` to `ripple-binary-codec` This will clean up the diff for #2273
# Conflicts: # package-lock.json # packages/ripple-binary-codec/package.json
Back in the day a hash was passed as an array of numbers iirc, so it's
probably left over from that. I will tidy that up in the followup PR we
discussed to address the TODO re: SigningScheme interfaces.
(on mobile)
|
Suggested commit message
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one nit that I'd like fixed before merge but ✅
Switch to using `@noble/hashes`, `@noble/curves`, `@scure/base`, `@scure/bip32`, and `@scure/bip39`. This replaces `crypto` polyfills (such as `crypto-browserify`), `create-hash`, `elliptic`, `hash.js`, `bn.js` (both versions), and their many dependencies. This also means there are 33 less dependencies downloaded when running a fresh `npm install` and will make the project much easier to maintain. This reduces the bundle size by 44% (82kb minified and gzipped) over the current 3.0 branch as well as reducing the amount of configuration required to bundle. Closes #1814, #1817, #2272, and #2306 Co-authored-by: Caleb Kniffen <[email protected]>
wow! we did it! thanks @ckniffen |
- Remove tests from `ripple-keypairs` that exclusively tested `rippled-address-codec`. The tests in `codec.test.ts` and `xrp-codec.test.ts` are all present and accounted for in `packages/ripple-address-codec/test/xrp-codec.test.ts`. - Update `package-lock.json` after the rebase with main - Remove references to `decimal.js` in the documentation * add missing entry for `bignumber.js` to `ripple-binary-codec` This will clean up the diff for #2273
Switch to using `@noble/hashes`, `@noble/curves`, `@scure/base`, `@scure/bip32`, and `@scure/bip39`. This replaces `crypto` polyfills (such as `crypto-browserify`), `create-hash`, `elliptic`, `hash.js`, `bn.js` (both versions), and their many dependencies. This also means there are 33 less dependencies downloaded when running a fresh `npm install` and will make the project much easier to maintain. This reduces the bundle size by 44% (82kb minified and gzipped) over the current 3.0 branch as well as reducing the amount of configuration required to bundle. Closes #1814, #1817, #2272, and #2306 Co-authored-by: Caleb Kniffen <[email protected]>
- Remove tests from `ripple-keypairs` that exclusively tested `rippled-address-codec`. The tests in `codec.test.ts` and `xrp-codec.test.ts` are all present and accounted for in `packages/ripple-address-codec/test/xrp-codec.test.ts`. - Update `package-lock.json` after the rebase with main - Remove references to `decimal.js` in the documentation * add missing entry for `bignumber.js` to `ripple-binary-codec` This will clean up the diff for #2273
Switch to using `@noble/hashes`, `@noble/curves`, `@scure/base`, `@scure/bip32`, and `@scure/bip39`. This replaces `crypto` polyfills (such as `crypto-browserify`), `create-hash`, `elliptic`, `hash.js`, `bn.js` (both versions), and their many dependencies. This also means there are 33 less dependencies downloaded when running a fresh `npm install` and will make the project much easier to maintain. This reduces the bundle size by 44% (82kb minified and gzipped) over the current 3.0 branch as well as reducing the amount of configuration required to bundle. Closes #1814, #1817, #2272, and #2306 Co-authored-by: Caleb Kniffen <[email protected]>
- Remove tests from `ripple-keypairs` that exclusively tested `rippled-address-codec`. The tests in `codec.test.ts` and `xrp-codec.test.ts` are all present and accounted for in `packages/ripple-address-codec/test/xrp-codec.test.ts`. - Update `package-lock.json` after the rebase with main - Remove references to `decimal.js` in the documentation * add missing entry for `bignumber.js` to `ripple-binary-codec` This will clean up the diff for #2273
Switch to using `@noble/hashes`, `@noble/curves`, `@scure/base`, `@scure/bip32`, and `@scure/bip39`. This replaces `crypto` polyfills (such as `crypto-browserify`), `create-hash`, `elliptic`, `hash.js`, `bn.js` (both versions), and their many dependencies. This also means there are 33 less dependencies downloaded when running a fresh `npm install` and will make the project much easier to maintain. This reduces the bundle size by 44% (82kb minified and gzipped) over the current 3.0 branch as well as reducing the amount of configuration required to bundle. Closes #1814, #1817, #2272, and #2306 Co-authored-by: Caleb Kniffen <[email protected]>
- Remove tests from `ripple-keypairs` that exclusively tested `rippled-address-codec`. The tests in `codec.test.ts` and `xrp-codec.test.ts` are all present and accounted for in `packages/ripple-address-codec/test/xrp-codec.test.ts`. - Update `package-lock.json` after the rebase with main - Remove references to `decimal.js` in the documentation * add missing entry for `bignumber.js` to `ripple-binary-codec` This will clean up the diff for #2273
Switch to using `@noble/hashes`, `@noble/curves`, `@scure/base`, `@scure/bip32`, and `@scure/bip39`. This replaces `crypto` polyfills (such as `crypto-browserify`), `create-hash`, `elliptic`, `hash.js`, `bn.js` (both versions), and their many dependencies. This also means there are 33 less dependencies downloaded when running a fresh `npm install` and will make the project much easier to maintain. This reduces the bundle size by 44% (82kb minified and gzipped) over the current 3.0 branch as well as reducing the amount of configuration required to bundle. Closes #1814, #1817, #2272, and #2306 Co-authored-by: Caleb Kniffen <[email protected]>
- Remove tests from `ripple-keypairs` that exclusively tested `rippled-address-codec`. The tests in `codec.test.ts` and `xrp-codec.test.ts` are all present and accounted for in `packages/ripple-address-codec/test/xrp-codec.test.ts`. - Update `package-lock.json` after the rebase with main - Remove references to `decimal.js` in the documentation * add missing entry for `bignumber.js` to `ripple-binary-codec` This will clean up the diff for #2273
Switch to using `@noble/hashes`, `@noble/curves`, `@scure/base`, `@scure/bip32`, and `@scure/bip39`. This replaces `crypto` polyfills (such as `crypto-browserify`), `create-hash`, `elliptic`, `hash.js`, `bn.js` (both versions), and their many dependencies. This also means there are 33 less dependencies downloaded when running a fresh `npm install` and will make the project much easier to maintain. This reduces the bundle size by 44% (82kb minified and gzipped) over the current 3.0 branch as well as reducing the amount of configuration required to bundle. Closes #1814, #1817, #2272, and #2306 Co-authored-by: Caleb Kniffen <[email protected]>
Authors
This PR is a collab with @ckniffen / @sublimator
Use @noble/{curbes,hashes}
Replace elliptic/hash.js/bn.js with noble suite (incl native bigint implementations of ecc)
See: repo
See: most recent audit
TODO