Concern over bn.js dependency in 1.0

[frangio]

While porting OpenZeppelin's test suite to bn.js, we found a lot of pain points, some of which could have security implications.

For example, scientific notation is not supported, but instead of erroring it produces completely unrelated numbers.

> new BN('5e+18').toString()
'63518'

This appears to have been fixed in the repository in 2017, but the current latest release on npm is from before that, so doesn't include those fixes. This makes me think perhaps we should consider the library to be unmaintained...

The error messages produced by the library are also very unhelpful.

> new BN(5000000000000000000000000000)
Error: Assertion failed
> a = new BN(0)
> a.add(3)
TypeError: Cannot create property 'negative' on number '3'

This will make for a terrible development experience for web3.js users.

Is bn.js really a good choice for web3.js? Have you considered alternatives?

[nivida]

Hay @frangio,

I heard that from many sides and had some experience with it by my self. Yes, I'm currently looking for possible alternatives.
I think it will end up with the solution from GoogleChromeLabs:
https://github.com/GoogleChromeLabs/jsbi

[frangio]

Nice! jsbi looks interesting, and ECMAScript BigInts is what we should all be using once that lands. I'm a bit concerned by the following though (taken from the README):

They don’t perform type checks in the current implementation, because such checks are a waste of time when we assume that you know what you’re doing. Don’t try to call them with other inputs, or you’ll get “interesting” failures!

I think the BigInt library used in the web3 ecosystem should do type checking. What do you think?

[alcuadrado]

I've been wondering for some time if there were any plans around native BigInt support in web3.js, mainly because of its potential to avoid/decrease fragmentation between libraries. I've never considered @frangio's point of the security implications of using bn.js, and I'm happy that initiated this discussion.

If the decision of using bn.js is to be revised, I strongly recommend jsbi, as it's the only library with a clear and software-assisted migration plan into native BigInt. Without such a plan the ecosystem will probably have to manually migrate everything again in 1.5-2 years. Native BigInts are a specially hard to adopt after-the-fact because of semantic differences between userland implementations, and because they can't be polyfilled (they overload operators, which can't be done in userland).

I think the BigInt library used in the web3 ecosystem should do type checking. What do you think?

I got really disappointed when you showed me that, and I agree with your concern, but I think they are just making explicit something that's already pervasive in the js ecosystem.

[sarthakumrao]

bignumber.js module can be a better alternative for `bn.js
It will give correct output for @frangio's example

new BN('5e+18').toString()
'5000000000000000000'

[Gerdinand]

The bn.js dependency in 1.0 has many issues, including not supporting decimals, scientific notation, giving unclear errors when calculating using numbers (not strings, even when numbers are small enough to not be a problem for Javascript). This is making our process of upgrading to truffle v5/ web3 1.0 for our test suites very inconvenient. I don't think the BigNumber.js should have been removed in favour of BN.js, when this is a big breaking change without noticeable advantages (Is speed in BigNumber calculations really an advantage with the kind of calculations we are running? We are limited more by the blockchain speed than some arithmetics).

I hope a more stable BigNumber solution can be chosen soon.

[nivida]

@Gerdinand It's on my checklist for beta.41 :-)

[alcuadrado]

including not supporting decimals

This point is important, native BigInt doesn't support decimals either.

I'm not sure if the bigint/bignumber library used to send and receive values to an ethereum node should support them, it sounds error prone to me.

[nivida]

@alcuadrado I have to dig in deeper and test it. I will drop a comment here if I started to fix this.

[interfect]

Is there some rationale in an issue somewhere for why the decision was made to switch over to BN in the first place? I assume there's a good reason, but it would be nice to know what it is. I dug through some of the old issues, and all I could find was the idea being rejected in #99

[glesaint]

@Gerdinand It's on my checklist for beta.41 :-)

As bn.js is still in use in beta.50, with its last release 595 days ago, can you share the current plans for bignumbers?

[nivida]

@glesaint Thanks for asking this!

BigInt is already supported in chrome and exists as an experimental feature for Firefox. I've done some research about the coming BigInt of JS and found the following polyfill which also has a babel plugin. It would be possible to extend from the JSBI and to create a BigDecimals as explained in the referenced blog post below.

JSBI: https://github.com/GoogleChromeLabs/jsbi
Google blog post: https://developers.google.com/web/updates/2018/05/bigint

[sshelton76]

Personally, I just use strings for everything until it comes time to do math operations, then I run it through math.js it is actively maintained by people who know more about math than I do and it does everything I need it to with any fuss. When I absolutely HAVE to convert the result to BN or whatever bignumber implementation dujor, I can always use the string version as the input source instead of trying to cast about here and there.

[haltman-at]

So are there any plans for how decimals will be supported, since Solidity is eventually going to include fixed-point decimals?

[nivida]

I will tomorrow start with the testing of BigInt/JSBI, bignumber.js, fixed-bn.js, BN.js, and math.js.

@haltman-at A contributor in the deno issue created a POC of a BigDecimals object.

[nivida]

I've tested today several BigNumber libraries competitors and came to the following result:

The JSBI bundle size is the un-minified bundle size

• BigInt and the polyfill are the fastest but do not provide all the features we need.
• The BigNumber.js library is the only lib which is supporting all the required features for the usage with Ethereum but is really slow in comparison to the other.
• BN.js does not provide all of the required features we need.

Possible Solutions:

BigInt

We could create a fixed BigInt and a BigDecimal with the BigInteger package as the base. This solution is probably way more performant than the bignumber.js package. The big pro for using the BigInteger package is the fallback to the native BigInt.

Bignumber.js

We can use the bignumber.js package directly but we will lose performance.

Conclusion

From a technical standpoint do we have to implement a BigDecimal and FixedBigInt with the BigInteger package but from a project planning standpoint do we have to use the Bignumber.js package first and we can work later on the required wrappers.

Edit:
I've also thought to add the config properties of the BigNumber config method to the Web3 module options and to use them on the usage of a BigNumber object.

[fare]

BigInt is standard, is coming to Firefox, and if we don't want to wait for it, shouldn't we pick a compatibility layer for it that will only be a trivial wrapper when BigInt is present, and an implementation of the same interface (modulo slightly more awkward syntax) when it's absent?

PS: IIUC, that's what BigInteger is and does these days.

[alcuadrado]

I just wanted to raise the attention that in less than a month all supported node.js versions will have BigInt.

I don't know what's the situation with safari, but considering that metamask doesn't support it, I don't think it's much relevant. Firefox is the only supported browser lagging behind with their BigInt implementation.

UPDATE: This was wrong. I mixed node versions. node 6 has EOL'ed

[sshelton76]

I still think math.js is the superior solution here. I notice you said you'd benchmark it. But I don't see it in the list.

[nivida]

@sshelton76 Thanks for your feedback! I didn't add math.js on the list because it's using similar dependencies as bignumber.js for decimals.

But I've checked the GitHub repository of math.js closer and found this issue.

Edit: We could fork the referenced bignum-native repository and add the JSBI fallback.

[nivida]

Firefox will enable BigInt by default on the next release: https://hg.mozilla.org/mozilla-central/rev/a21eae6d674f

[haltman-at]

I'm just worried about potentially having to change my code twice. I mean, if the type for numbers is changed now, it'll likely have to change again later to support fixed-point decimals, right?

[nivida]

I'm just worried about potentially having to change my code twice. I mean, if the type for numbers is changed now, it'll likely have to change again later to support fixed-point decimals, right?

Thanks for asking this! The idea would be to create a fixed-point decimal number based on the used lib for the handling of big numbers.

---

I've done some tests with BigInt and BigInt based floating point numbers and couldn't find a well maintained and ready for production package. I also tested JSBI and ported existing BigInt floating point numbers implementation and after the first steps were it clear to see that it would have been ended up in an own standalone package called jsbi-decimal.

Because the BigInt and JSBI solutions aren't stable or do not provide the required features is it better to use a well maintained and widely used package as bignumber.js who is also compatible with math.js.

I will create a PR soon with the proposed changes and the bignumber.js lib as a dependency.

[cliffhall]

I also ran into hassles with BN after I upgraded web3.js recently. Aside from having to change all the add/sub/mul functions in my unit tests, the worst of it was that you can pass a JavaScript number to the constructor, but if it is too large, BN internally asserts it is unsafe and throws an unhelpful assertion error.

But if you convert the JavaScript number to a string and pass that to the constructor, it works.

I banged my head against it for awhile before realizing that and posting this issue (which was rejected), suggesting a fromNumber method that would just convert the JavaScript number to a string and then return a BN constructed from that.

[cliffhall]

Hey folks, a better alternative to BN.js might be JSBI - a pure JS implementation of the ECMAScript BigInt proposal, which is slated to become a native part of JS in the near future.

In JS, you'll be able to use normal arithmetic and equality operators with BigInt types, but the BigInt class (and JSBI) have named methods as well.

If refactoring away from BN.js, I think it might be best to go with JSBI rather than reverting to BigNumber.js (which even in 9.0.0, I'm still unable to use the ES6 module import with). This suggestion is mainly because BN.js and BigNumber.js both have their own differing operator and equality method names and both differ with what will soon become JavaScript's native BigInt class.

Chrome and Firefox already offer their own BigInt native implementations. By refactoring to JSBI, projects can get ahead of the game, and use what will soon be the standard way of handling big integers.

[frangio]

What is the status of this issue in the current roadmap?

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions