Change all internal value handling to wei (not eth)

[wanderingstan]

Currently, we store listing prices in ETH throughout the DApp (and in origin-js). This is bad practice for monetary values, as rounding problems can occur when dealing with real values instead of ints. (And it's my fault for making this bad design choice last year. 🤦‍♀️)

We should refactor to always representing prices in Wei (or fundamental unit of tokens, when that comes) and only convert to ETH at render time.

This will also prepare us for handling ERC20 tokens with arbitrary number of decimals.

[DanielVF]

I'm thinking we'll be passing and returning these numbers as strings from the origin.js API. This lets us use a json compatible type. It's easy to create a BigNumber from a string, which you would be doing anyway to use the value.

[wanderingstan]

@DanielVF is that because BigNumbers are so hard to deal with, and always display strangely?

I'm with you in spirit, but worry about introducing a third representation format. (js Number, BigNumber, string of number)

It seems the "right" way would be to use BigNumber in the DApp, but I agree it's a pain. Anyone else have opinions on this?

[nick]

Agree we should represent everything in wei and pass strings back from origin-js. web3.utils.fromWei doesn't work with numbers, it will throw Please pass numbers as strings or BigNumber objects to avoid precision errors.

[wanderingstan]

If web3.utils accepts strings (along with BigNumbers), then thats's good enough for me! I retract any hesitation about using them for origin-js/DApp.

[gaboesquivel]

I can help on this one. Yes, Math with floating point in JS is totally broken, you want to pass the value in the smallest unit and the use UI filters to display the value in floating point to end user. You want to use number instead of string to avoid parsing to make math operations.

Stripe API is a good reference https://stripe.com/docs/currencies#zero-decimal

Dinero.js is a great lib for the UI https://sarahdayan.github.io/dinero.js

To be more accurate :), in JSON/JS there's no BigNumber only number primitive type http://json-schema.org/latest/json-schema-core.html#rfc.section.4.2.1

BigNumber could be enforced with Flow. This could be handy too https://github.com/MikeMcl/bignumber.js/

[gustavoguimaraes]

Hi all,

Is this issue still applicable? I see some commits that changes the prices from wei to eth for rendering purposes only in the codebase. This issue shows as a good first issue but I cannot tell if it is still relevant given that it is from May.

[wanderingstan]

Hi @gustavoguimaraes thanks for bringing this up. I believe things have migrated a bit in the codebase and now we actually are storing things in raw eth in many places. This is a discussion we need to re-visit.

So yes, don't do any work on this until we figure out what path we want to take.

[franckc]

I don't think this would require any change to the JSON schemas (since there is no validation at the schema level of the monetary unit being used). But logic code in origin-js would be impacted. Since with the GraphQL rewrite we plan to replace origin-js with new code, this could be a good opportunity to clean this up ?

I could imagine having a "Money" class at the business logic layer that gets constructed off data from IPFS. That constructor should be smart enough to handle legacy IPFS data for old listings/offers that were not using natural units. Then the Money object would be what gets used throughout the DApp. And it would provide helper methods like basic math operations, getting token symbol, unit / natural unit conversion and representation, etc...

Also, we had discussed storing the ERC20 address rather than its symbol (and using a special address like 0x0 for ETH) in the IPFS data. The address could go in the existing "currency" string field of the JSON schema or it could be defined as a new field "address".

[micahalcorn]

Just leaving a note here that @crazybuster and I are having trouble dealing with BigNumbers 😕

According to the docs...

toBN Will safely convert any given value (including BigNumber.js instances) into a BN.js instance, for handling big numbers in JavaScript.

isBN Checks if a given value is a BN.js instance.

[DanielVF]

@micahalcorn Ever find out what was the deal with isBN?

[micahalcorn]

Nope 😕

[DanielVF]

Well, this is fun. It works fine in dapp2.

The web3 code for utils.isBN hasn't changed in a year, nor has the code for web3 code toBN. The BN dependencies used by web.utils have not have their version numbers changed in two years.

It looks like the difference is in what is returned by "toBN". I wonder if there's some kind of webpack weirdness going on with DAPP1.

Sadly, I've got to run for now before chasing this all the way down to the ground.

[micahalcorn]

Still relevant per @nick 🙂