Hateful Dispare: Don't Change the BN .toString method please

[SilentCicero]

https://github.com/ethers-io/ethers.js/blob/master/utils/bignumber.js#L126

Ethers.js BigNumber converts the toString method to always return base10. This was NOT known to me even after a decent read of the docs and confused the hell out of me and my tests. As in every other BN library .toString allows the base specifier input. I think I and other devs would appreciate you don't change such a critical method away from the BN library spec.

I see you commented out the override and added an explicit toHexString method. I would flag this hard and several times in the docs if you choose to continue to override toString in this non-conventional way.

Sincerely, your dear friend and code admirror.

[ricmoo]

Hey Nick!

So, this was something I'd put a lot of consideration into back in the day; three-ish years ago, and did a lot of research on. :)

Keep in mind that toString() is part of the Object.prototype, and while there are some notable exception (e.g. Buffer.toString(encoding)), overriding these is generally frowned upon and can cause weird behaviours in some libraries and VMs, especially when native binds may be present, which may discard arguments, causing your to be really confused, since it would work other places. ;)

You can create a function to be called in place of the default toString() method. The toString() method takes no arguments and should return a string. See here

Here are some various ECMAScript specifications too:

• https://tc39.github.io/ecma262/#sec-object.prototype.tostring
• https://www.ecma-international.org/ecma-262/6.0/#sec-object.prototype.tostring
• https://www.ecma-international.org/ecma-262/5.1/#sec-15.2.4.2

Other reasons I avoided using toString(radix) is to constrain the scope of the library; it currently uses BN.js, but that may change in the future. Currently I only support base-10 and base-16. If I supported arbitrary radices, I may have to make sure weird things like base-58 and base-7 work in the future, which I would rather do on an as-needed basis, than as a blanket "support all the bases". The interface is a minimum set of things that are commonly required, but provide a reasonable lowest-common-denomiator of Big Number libraries.

The ethers documentation also reinforces what it means to be a "hexstring", as opposed to a hexadecimal number. A hexstring always has a "0x" prefix. So, toHexString is a much stronger statement than even toHex to toString(16).

A lot of the other libraries also take in weird things, such as toString(16) or toString('hex'), toString(radix, length), toString(encoding), toString(endian, encoding), et cetera, which then becomes confusing as to which to expect, and then people want whichever they are used to to be supported, which then just adds to the confusion and support that needs to be carried forward.

The documentation does point out that it returns a decimal value, but I'll make sure to highlight that more in the v4 docs, which should be coming out very soon.

Libraries that do support toString(radix):

• https://mikemcl.github.io/bignumber.js/#toS
• https://github.com/indutny/bn.js/ (actually supports toString(radix, length).

Libraries that do not support it:

• https://github.com/MikeMcl/big.js/blob/master/big.js (always returns exponential notation)
• https://www.npmjs.com/package/decimal.js
• https://www.npmjs.com/package/big-number

These were literally just the first 5 random libraries I found on google and NPM, so it's not /that/ wide-spread. Just sayin'. :)

[SilentCicero]

@ricmoo as always, great reasoning and thinking. Sorry for my verboseness but this caused me a ton of problems (2 days) -- I thought I was going crazy.

I would say flag it more in the docs though. And also, BN and BigNumber are the common ones use in Ethereum, which is why I flag the toString(radix) as common, even though others might support different notation.

None-the-less, I think this conversation is good record for the decision, you could even include it in the docs as further reference.

Anyway, good day!