number is not a suitable data type to represent amounts

[liamaharon]

I noticed that some functions in the SDK accept the parameter amount as a number.

JS does funky stuff with numbers that contain 7 or more decimal places.

For example, with 6 decimal places everything is fine

const x = 0.000001; // 6 decimal places
console.log(x); // 0.000001

Adding 1 more 0, the number may be represented in exponential notation, which can blow up

const x = 1.0000001; // 7 decimal places
console.log(x); // 1.0000001

const y = 0.0000001; // 7 decimal places
console.log(y); // 1e-7

ethers.parseEther(x); // runs sucesfully
ethers.parseEther(y); // blows up

There are also many more nasty bugs that could slip by due to the nature of floating point precision in JS. These kinds of bug don't result in an exception, and are often undetected until it's too late (which in nahmii's case means money lost). Due to the micropayment nature of nahmii, it's doubly important that we consider this. Here's a quick article on JS floating point precision https://hackernoon.com/a-note-on-numbers-in-ethereum-and-javascript-3e6ac3b2fad9

Numbers in JS are weird. Allowing users to use numbers in place of strings is tempting fate, especially when considering a lot of developers using this SDK may be coming from other, louder failing languages.

I suggest that it's made a rule to never to allow amounts to be represented as a number, instead always using a string or BN. Please let me know your thoughts, @morfj

[morfj]

Agreed, the support for numbers are usually just a convenience and all (?) places also handles string input AFAIK. So its up to the caller how "risky" they want to play it. The CLI always sends strings since a few releases back.

[hubiierik]

Amounts are mostly represented by MonetaryAmount by now. There may be a few places that still uses Number, but that should be regarded as bugs. I suggest we close this issue, and register any discovered Number representations as bugs from now on.