Ethers very confusingly treats BN values as zero.

[blakewest]

Hi, I've been users Ethers for a little bit. Generally very good, but I've run into this issue a few times now, and I had to create an issue for it. Essentially, it seems that the Ethers BigNumber instances will gladly accept a BN (bn.js) instance, but will silently treat it as zero. This has led to some •very• confusing and potentially dangerous bugs which take me a while to debug. As an example...

const currentTermInDays = await creditLine.termInDays()
console.log("Original term:", String(currentTermInDays), "plus 1000:", String(currentTermInDays.add(new BN(1000))))

// Output:
// Original term: 30 plus 1000: 30

If you pass in a string, it will work as expected, and if you pass in BigNumber (this version, not the Ethers version), it will fail loudly (good!).
However, I've ended up running into this bug because we use BN for other parts of the project (largely because web3 uses BN). So I end up doing something like the above where I get a value from Ethers, and then want to manipulate it, passing in a local value, which by default will be BN.js value. The Ethers documentation is clear that a BN instance wouldn't work, so it's totally fine that it doesn't work, but it really seems like you should fail on this type, just like you do with BigNumber.js. I only just realized today after re-reading documentation that Ether's BigNumber is not BigNumber.js. So I'd also been confused by the fact that multipliedBy and minus and such don't work. I know all the BigNumber stuff is confusing, and there's like 100 libraries for it, so it seems extra important for the error messages here to be loud and clear.

As an aside, when you do fail for something like BigNumber.js, it could be nice to add a little sentence like, Ether's BigNumber does not accept BN.js, BigNumber.js, etc. See docs here on accepted types.

Thanks!

[blakewest]

Also, I took a little look through your from function here. Some brief poking around makes me think that none of those branches should hit, except maybe the bytes_1 branch which I'm not sure how to easily test that one locally.
It looks like BN unfortunately doesn't provide any any simple "isBN" function except on the BN module itself. They do have a toJSON and a toString though, which could be helpful.

[ricmoo]

That is odd. I would have thought it would crash hard on BN.js instances too. I’ll look into this today.

There are wayyyy to many big number libraries out there and everyone has their favourite and always gets annoyed when one doesn’t have the one feature they like their library doesn’t have... So I try to document it thoroughly. And throw when I can detect they are using it wrong. :)

[ricmoo]

I found the issue and will have a fix pushed soon. I'll publish once the CI passes (usually takes about 1 hour).

[blakewest]

Awesome, thanks! Super fast turnaround!

[ricmoo]

This should be available in 5.0.23. Try it out and let me know if you have any problems.

(sorry for the delay; new laptop and I had to migrate OpenGPG and handful of other weird credentials)

Thanks! :)

[ricmoo]

Closing this now. If you have any further issues, please re-open. :)