Offload bn.js

[ahsan-javaid]

Description

This PR removes bn.js dependency from stacks.js packages.
bn.js minified size is around 43kb. This will help to reduce the bundle size. In the subsequent pr, further dependencies can be removed to reduce the bundle size further.

For details refer to issue #1120

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 1 of @yknl or @zone117x for review

[vercel]

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/8ymHvhN88uJ8HHh5WMX4rteUD65G
✅ Preview: https://stacks-js-git-fix-bundle-sizeremove-bnjs-blockstack.vercel.app

[codecov]

Codecov Report

Merging #1150 (c3ccc1e) into master (af65b91) will increase coverage by 0.10%.
The diff coverage is 75.64%.

@@            Coverage Diff             @@
##           master    #1150      +/-   ##
==========================================
+ Coverage   62.03%   62.13%   +0.10%     
==========================================
  Files         122      122              
  Lines        8315     8377      +62     
  Branches     1534     1553      +19     
==========================================
+ Hits         5158     5205      +47     
- Misses       3052     3065      +13     
- Partials      105      107       +2     

Impacted Files	Coverage Δ
packages/common/src/utils.ts	42.20% <71.21%> (+14.75%)	⬆️
packages/bns/src/utils.ts	73.33% <100.00%> (ø)
packages/cli/src/cli.ts	12.42% <100.00%> (-0.12%)	⬇️
packages/cli/src/network.ts	30.00% <100.00%> (ø)
packages/encryption/src/ec.ts	75.62% <100.00%> (ø)
packages/keychain/src/wallet/signer.ts	40.00% <100.00%> (ø)
packages/stacking/src/index.ts	88.13% <100.00%> (-0.07%)	⬇️
packages/stacking/src/utils.ts	55.17% <100.00%> (ø)
packages/transactions/src/clarity/serialize.ts	97.97% <100.00%> (-0.03%)	⬇️

---

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 b9962ea...c3ccc1e. Read the comment docs.

[pradel]

@ahsan-javaid amazing to see some work to improve the bundle size, much needed 🙏

[saralab]

@kyranjamie @zone117x

[ahsan-javaid]

@zone117x Share your thoughts on this pr as well. Thanks 🙏

[zone117x]

Is a goal of this PR to avoid breaking changes? If so, this new class is a breaking change. It prevents other libraries from passing a bn.js lib instance of BN because the class types are different, and the functionality is different.

If we don't care about breaking changes, then I'm not sure why we'd try to replicate the bn.js structure when everything can use a bigint instance, and call any of these helper functions when needed.

[ahsan-javaid]

Done!

[codecov-commenter]

Codecov Report

Merging #1150 (99de1b5) into master (8b41449) will increase coverage by 0.15%.
The diff coverage is 68.67%.

❗ Current head 99de1b5 differs from pull request most recent head 93b17d3. Consider uploading reports for the commit 93b17d3 to get more accurate results

@@            Coverage Diff             @@
##           master    #1150      +/-   ##
==========================================
+ Coverage   64.39%   64.54%   +0.15%     
==========================================
  Files         124      124              
  Lines        8603     8629      +26     
  Branches     1834     1849      +15     
==========================================
+ Hits         5540     5570      +30     
- Misses       2818     2822       +4     
+ Partials      245      237       -8     

Impacted Files	Coverage Δ
packages/cli/src/network.ts	29.16% <0.00%> (-0.91%)	⬇️
packages/keychain/src/wallet/signer.ts	37.50% <ø> (-2.50%)	⬇️
packages/stacking/src/utils.ts	54.31% <0.00%> (-0.40%)	⬇️
packages/cli/src/cli.ts	15.13% <11.76%> (+0.06%)	⬆️
packages/encryption/src/ec.ts	74.69% <83.33%> (+1.23%)	⬆️
packages/common/src/utils.ts	44.55% <85.10%> (+17.56%)	⬆️
packages/bns/src/utils.ts	75.00% <100.00%> (ø)
packages/stacking/src/index.ts	87.77% <100.00%> (-0.07%)	⬇️
packages/transactions/src/clarity/serialize.ts	98.00% <100.00%> (-0.02%)	⬇️
packages/storage/src/hub.ts	72.11% <0.00%> (-11.22%)	⬇️
... and 3 more

---

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 8b41449...93b17d3. Read the comment docs.

[ahsan-javaid]

@zone117x Requesting the review on this pr 🙏.

[ahsan-javaid]

@zone117x Any further thoughts on this pr 🙏

[zone117x]

LGTM

[janniks]

Looks good so far — but I'll need some more time to examine in detail 😊

[zone117x]

Note that this specific PR is a relatively minor move towards native bigint compared to many changes that have already been pushed. For example, the new and much improved cryptography dependencies are heavily dependent on native bigint, see https://github.com/paulmillr/noble-secp256k1/blob/main/index.ts (totally dependent on bigint: exponentiation operator **, bigint literals 123n, etc).

We should not hold back from merging this PR due to react native support, as that's already been thrown out the window.

Facebook's react native js runtimes are embarrassingly outdated, but looks like they are making progress towards bigint support at least facebook/hermes#510.

Alternatively, @aulneau and I have had discussions around this topic, and found offloading certain operations to native webviews as a potential viable solution, e.g. https://github.com/webview-crypto/react-native-webview-crypto.

@aulneau do you have any updates around this issue?

[aulneau]

If packages are moving towards more native bigint usage -- they simply cannot be used in react-native as it stands currently. I've explored ways to create a messaging bridge between the native webview and react native which then makes it possible to use something like micro-stacks or noble-secp256k1 in react native. This however has a large overhead and burden of complexity.

If stacks.js is already using the noble packages, this change in this PR will not make it so react native users can't use stacks.js, it would have already been the case because of the heavy usage of bigint in the noble packages.

I propose switching back to bn.js for all packages in stacks.js if the only concern is bundle size.

An alternative would be to use a different big int package that has a smaller footprint than bn.js.

I've thought a lot about how this would be possible in the context of micro-stacks. What I've landed on currently is to take an approach where consumers of the lib could pass in their own versions of things like a secp256k1 package or other encoding libs, which have the same signature. That way you can pass a more performant secp256k1 package that is built for react native, rather than using what is best for the web. Additionally, I think using native Bigint is fine as long as there is no math being done. That is easy enough to polyfill in react native. It's much more tricky when it uses ** and the like.

Hope that helps. I'm planning on having an example implementation for react native and micro-stacks in the coming months.

[zone117x]

Thanks for the insight and research @aulneau!

We should consider a similar approach in stacks.js if we prioritize react-native support. Keep in mind one of their maintainers mentioned upcoming support for bigints just a week or so ago.