Thanks @shahthepro, looks like a lot of test failures that are probably related to these changes. I'll update the branch to run them again but would you mind checking them out if it still fails?

@tomlinton Thanks. PR is not yet ready for review. Yeah, will be fixing the tests

Sorry @shahthepro, someone (cough @micahalcorn) requested review 👺 😂

Sorry, it's early and I may have missed something but I don't get why we're doing string operations here instead of using BN.js? Here's a conversion from "human units" to an 8-decimal token value:

const ONE_TOKEN = new BN('100000000') // 1e8
function toReal(v) {
  return v.mul(ONE_TOKEN)
}

Seems a bit more accurate and straight forward than trying to do math this way?

EDIT: Converting a decimal to a real token value:

> (new BN(1.032435245*1e8)).toString()
'103243524'

Ethersjs has great utility functions for this kind of thing. Maybe we should add it as a dependency and slowly try to move away from web3js. Tough job though 😞

@mikeshultz

Seems a bit more accurate and straight forward than trying to do math this way?

Agreed. This seems simple. Will update it. Thanks

@franckc

Agreed with @sparrowDom that adding some simple unit tests would be nice.

Thanks. Will do it

@tomlinton

Ethersjs has great utility functions for this kind of thing. Maybe we should add it as a dependency and slowly try to move away from web3js. Tough job though 😞

We could but Mike's solution should reduce the complexity of the code right now. I'm not sure if we should add a dependency right now. But thanks, let me check this

@mikeshultz
I tried that but it doesn't work well. new web3.utils.BN('123.12345').toString() results in 122812345 which is way off.

However, I did some other change that will simply it more (more like treating it as a string).

@sparrowDom @franckc The issue with decimal places (that are not multiples of 3) is fixed now and have also added a couple of simple unit tests

@tomlinton Tried ether.js, but the potential that Domen reported was still reproducible (i.e the function not working with decimals which are not multiples of 3). I managed to fix this without ether.js or even using web3. So skipping adding ether.js for now

@shahthepro you can't feed BN a decimal. In fact, I'm surprised that didn't error. Here's a slightly more comprehensive example using your given numbers starting with a string:

> x = new BN(parseInt(parseFloat('123.12345') * 1e6)) // for 6-decimal, use 1e8 for 8-decimal, etc
<BN: 756b6fa>
> x.toString()
'123123450'

EDIT: This also assumes relatively small numbers as user input that won't overflow in JS. Might want to add some checks if you expect extremely large numbers(N > Number.MAX_SAFE_INTEGER / 1e8).

@shahthepro you can't feed BN a decimal. In fact, I'm surprised that didn't error. Here's a slightly more comprehensive example using your given numbers starting with a string:

EDIT: This also assumes relatively small numbers as user input that won't overflow in JS. Might want to add some checks if you expect extremely large numbers(N > Number.MAX_SAFE_INTEGER / 1e8).

Yep, this is what I'm concerned about. We had code to truncate anything longer than 18 decimals (before passing that to toWei). There is a possibility, albeit rare, that it can fail. Also, even with 18 decimals, JS seems to round off digits.

I'm probably gonna merge this change in and will fix forward, if we ever any issue with the present implementation

Thanks @mikeshultz