fuels-ts
fuels-ts copied to clipboard
FuelLabs
Migrate `bn.js` to `bignumber`
Suggestions were made to replace one library with the other.
Ideally, we should have a pros and cons list before deciding.
[!NOTE]
- Incoming from https://github.com/FuelLabs/fuels-ts/discussions/1537
Based on this discussion and ethers v6 using native bigint in their maths "package" (see here), it seems like ethers got rid of big-number libraries altogether and went completely in favor of native bigint.
just to add more context: #280 #468
it's interesting to take a look at those to understand why bn was chosen at first place and what problems it solves
Related to the JSON.stringify, we could fix it by defining our own BigInt.prototype.toJSON (based on this comment):
It would solve the issue across the board. Although messing with the prototype is risky as it can cause collisions ... what's the chance that two libraries would be adding this same method? Also, as the comment I linked to above mentioned, MDN also recommends it, although in the image I'm serializing into hex, not a stringified number.
BN doesn't add the 0x prefix when stringifying so we could get rid of it here as well.
Related to the
JSON.stringify, we could fix it by defining our ownBigInt.prototype.toJSON(based on this comment):It would solve the issue across the board. Although messing with the prototype is risky as it can cause collisions ... what's the chance that two libraries would be adding this same method? Also, as the comment I linked to above mentioned, MDN also recommends it, although in the image I'm serializing into hex, not a stringified number.
when you stringify again, it's not a BigInt anymore
when you stringify again, it's not a BigInt anymore
Stringifying BN has the same behavior and we lose context that it was a BN before.
I was not there when BN was chosen, however from my understanding it solved problems for developers trying to build for environments where a native BigInt was not yet supported. As the ethers discussion reads (who migrated from BN to BigInt in v6), Node v12 was EOL in April 2022 (did not support BigInt) and all later versions of Node do, and all modern browsers do. Therefore the case for not using it for that reason is now pretty void.
For our own purposes, I believe this has come from reported inaccuracies when using BN for calculations (can't find anything to link but I believe @Torres-ssf reported this?). Additionally, there is an opportunity to reduce the SDK size through the removal of BN to a natively supported type, as @nedsalk is investigating in #1592.
I support @nedsalk implementation of BigInt.Prototype.toJson as also being the recommended implementation from MDN. I think using stringify and still expecting to always have maintained types is dangerous.
After taking a look at it with @danielbate, replacing BN with native bigint seems like a ton of relatively easy work, but it'll be a breaking change with all our consumers and we have to make that known.
The benefits, as mentioned by @danielbate above, are:
- Fixing reported inaccuracies when using BN? (@Torres-ssf)
- Reducing the non-treeshakeable SDK size by > 22kB (#1592),
- Using a native primitive
Besides that, we've already got a precedent for it with ethers so it wouldn't come as a big shock to anybody.
The cons at this moment are:
- A breaking change for all ts-sdk consumers, most notably the Fuel's frontend team
@FuelLabs/frontend @FuelLabs/sdk-ts It would be great to get additional input from everyone.
As of the time of writing, there is no consensus in the team for moving forward with this issue so I'm removing my assignment from it. I won't be closing it as it's a valid issue and something that should probably be done in the future for package size benefits.
Related to package size reduction, by doing an in-depth analysis of bn.js we could maybe exclude its unnecessary dependencies from our build via rollup and squeeze out some kilobytes that way. bn.js itself isn't tree-shakeable.
Closing this for now. Contextually tree-shaking is being discussed in #1592 so that can be resolved there. The proposal that this issue makes we have not got a consensus on so therefore doesn't seem right for us at the moment.