Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EPIC] Exchange amounts between UI and backend using strings with base units instead of js numbers #11376

Open
5 of 7 tasks
micieslak opened this issue Jul 3, 2023 · 1 comment
Assignees
Labels
bug Something isn't working core-team Epic
Milestone

Comments

@micieslak
Copy link
Member

micieslak commented Jul 3, 2023

Description

Currently in various places we pass amounts from UI to the backend as a JS number.
In most cases it works seamlessly but there are corner cases we should take into account to provide reliable solution.

The problem is that not all decimal fractions have a finite decimal representation (simply explained e.g. here: https://indepth.dev/posts/1019/the-simple-math-behind-decimal-binary-conversion-algorithms#why-not-all-fractions-can-be-finitely-represented-in-binary)

E.g. decimal number 0.1 in binary system is 0.0(00110011), there is no finite representation.

In practice the problem can be observed on that example: https://www.mycompiler.io/view/DLtkitu1nHN

Even simpler example is just:

console.log(0.07 * 10**18) // 70000000000000010

It means that when user provides 0.07 as an input, the value passed to the backend as float is in fact a different number. If backend multiples that value by 10**18 to operate on basic wei units, it leads to distortion, the amount used is bigger then expected.

As a consequence, when the remaining supply is 0.07 (70000000000000000), the UI will ofc present it as 0.07, but making airdrop via passing 0.07 float to the backend will fail because of insufficient funds.

There is also an opposite problem. When the remaining supply is is 999999999999999999 (for eth-like amounts with dp 10^18), when sent from backend to the UI as a float it would be just 1. So defining airdrop for 1 (effectively 1000000000000000000 wei) will fail as well.

The proposed solution is to exchange amounts (UI <-> backend, both directions) in textual form representing amount in the basic units (like weis, no floating points) instead of converting it to numbers.

As a consequence:

  1. UI takes user input in a localized from and needs to convert it to the basic units before passing to the backend. (e.g. 0.07 or 0,07 depending on locale to 70000000000000000 when the multiplier is 10^18). UI needs to properly perform decimal arithmetic here (not direct multiplication on binary floats). For that, the proper multiplier obtained from the backend needs to be used, 0 (10^0) for collectibles and 18 (10^18) for assets in most cases, but it may be different for some currencies like USDC.
  2. UI obtains amounts (like wallet balance, total supply, remaining supply) in the basic units as well. Before presenting to the user the value must be converted appropriately (using provided multiplier and number of decimals for displaying) and localized. In that approach, UI side can properly handle the situation when the amount presented to the user is slightly different than the actual one. E.g. 69999999999999999 can be presented as <0.07 or 0.07 - ε to inform user that the presented amount is not strictly the value kept under the hood. It may be useful when user wants to transfer all funds - there should be possibility to specify directly "all funds" instead of taking numeric value. It also gives possibility to reliably compare amounts on UI side.

To handle properly user input and values provided by the backend as strings, UI needs to perform decimal arithmetic properly.

Library https://github.com/MikeMcl/big.js/ seems to be a good choice:

const dec = new Big("0.07")
const multiplier = 10**18

console.log(dec.times(multiplier).toString()) // 70000000000000000

An alternative is https://github.com/MikeMcl/decimal.js but it's bigger, containing a lot of functionality that is not needed for our purpose.

The UI uses localized representation of numbers. Because of that, user-provided input cannot be directly used for constructing Big object. First it needs to be de-localized to form produced by Number.toString(). Raw JS Number can be used as an intermediary form, assuming that conversion for user input to number is fully reversible (user input -> number -> string produces the same value), what is true when the input is limited to appropriate number of decimal digits.

console.log(parseFloat("40.00000000000001").toString()) // 40.00000000000001 - still ok
console.log(parseFloat("40.000000000000001").toString()) // 40 - not ok

It's worth noting that big.js relies on Number.toString() conversion instead of actual binary value when constructed from raw js number:

const dec1 = 0.07
const dec2 = new Big("0.07")
const dec3 = new Big("0.07")
const multiplier = 10**18

console.log((dec1 * multiplier).toString()) // 70000000000000010 - not ok
console.log(dec2.times(multiplier).toString()) // 70000000000000000 - ok
console.log(dec3.times(multiplier).toString()) // 70000000000000000 - ok

Sub-tasks

@micieslak micieslak added bug Something isn't working ui-team labels Jul 3, 2023
@micieslak micieslak added this to the 0.14 milestone Jul 3, 2023
@micieslak
Copy link
Member Author

micieslak commented Jul 3, 2023

The alternative approach is to perform conversion to base units already on UI side (that approach is already used in wallet, but it is also vulnerable to the distortions described in the task).

To do it correctly, decimal arithmetic library could be used, e.g. https://mikemcl.github.io/decimal.js/
Then we can perform multiplications sefely:

const dec = new Decimal("0.07")
const multiplier = 10**18

console.log(dec.times(multiplier).toString()) // 70000000000000000

Update:
As agreed with @alaibe and @endulab this approach is the most suitable. The description has been updated, and big.js lib proposed as a better match than decimal.js.

@alexandraB99 alexandraB99 added C3 and removed C3 labels Jul 3, 2023
@micieslak micieslak self-assigned this Jul 3, 2023
@micieslak micieslak changed the title Pass amounts from UI to backend as string instead of js number Exchange amounts between UI and backend using strings with base units instead of js numbers Jul 6, 2023
@alexandraB99 alexandraB99 changed the title Exchange amounts between UI and backend using strings with base units instead of js numbers [EPIC] Exchange amounts between UI and backend using strings with base units instead of js numbers Aug 9, 2023
@alexandraB99 alexandraB99 modified the milestones: 0.14, 0.15 Aug 9, 2023
@alexandraB99 alexandraB99 modified the milestones: 0.15, 0.16 Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core-team Epic
Projects
Status: No status
Development

No branches or pull requests

5 participants