-
Notifications
You must be signed in to change notification settings - Fork 403
migrate off libsodium #1722
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
migrate off libsodium #1722
Conversation
db7bc41 to
28c2583
Compare
|
This conflicts with #1720 but let's get that one merged first. |
033cdf5 to
c4da581
Compare
|
This seems like really low-hanging fruit for a massive reduction in bundle size, is it not gonna make it into 0.35? |
|
Looks straight forward at first glance. However, I would be very surprised if the pure-JS implementation of Argon2 is as fast as the WebAssembly one. So this PR has the potential to hit existing users hard. Which then leads to the questions: who uses CosmJS pro private key encryption at all? Should we remove that functionality alltogether? |
|
I thought if it was going to be removed it already would've been. Releasing a slower version could make for a good scream test. Maybe mark all the types with https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#deprecated Or just switch to |
Agreed. I would like to release a 0.36.0 tomorrow with ONLY this change in it. Then users can go back to 0.35.0 without a problem. Could you update this PR to latest main? |
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.
Nice!
86d382d to
aabdc79
Compare
|
cosmjs/packages/amino/src/secp256k1hdwallet.ts Lines 37 to 48 in 1264c2c
The basic parameters run so slowly the unit tests fail to finish in 15s. Or 60s. At least when running on Node.js 20. https://github.com/cosmos/cosmjs/actions/runs/16947888017/job/48033364456 But 22 seems to have new optimizations. The result for Node 22 says: https://github.com/cosmos/cosmjs/actions/runs/16947888017/job/48033364449 So just to keep CI happy - until Node.js 20 is dropped - maybe The absolutely huge performance improvement for this low-level math code in 22 might be related to https://v8.dev/blog/maglev https://nodejs.org/en/blog/announcements/v22-release-announce If you merge this, just make sure and do a branch-merge and not a squashed commit, so it's easy to revert the last few commits separately later. |
9be002c to
c97b703
Compare
This should make the deserialize 'can restore multiple accounts' unit test take about 12s, approximating pure JS performance on node 22.
a0e779d to
7b65103
Compare
|
|
||
| // Emulate a slower implementation. The fast WASM code may get removed. | ||
| // This approximates the speed of using a pure JS implementation (@noble/hashes) in Node 22. | ||
| const screamTest = new Promise((resolve) => setTimeout(resolve, options.opsLimit * 250)); |
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.
💅 we can use sleep from @cosmjs/utils which does exactly that but is a bit more readable
This removes a WebAssembly dependency and a huge chunk of bundle size https://bundlephobia.com/package/@cosmjs/[email protected]
(The other, even more huge chunk is cosmos/cosmjs-types#98)
Closes #584
Closes #907
Closes #964
Closes #1031
Closes #1478
Closes #1585
Related: #1479 #1796
Technically, now that there's no longer a quirky requirement to
await sodium.readyat the start of every function, asynchronous methods could be made synchronous now after this PR.Considered but rejected a faster alternative toAt least as long as Node 20 is supported, looks like@noble/hashesArgon2idhash-wasmis needed https://bundlephobia.com/package/[email protected]But hopefully Argon2 just gets standardized in Web Crypto someday and then implemented by Node.js. https://twiss.github.io/webcrypto-modern-algos/#argon2