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

15 significant digits for input number type is not enough #11

Closed
Olathe opened this issue Aug 20, 2013 · 11 comments
Closed

15 significant digits for input number type is not enough #11

Olathe opened this issue Aug 20, 2013 · 11 comments

Comments

@Olathe
Copy link

Olathe commented Aug 20, 2013

The readme says "It accepts a value of type Number, String or BigNumber Object", which would be nice, but which is currently incorrect. BigNumber(Math.random()) and BigNumber(Math.PI) and several other higher-precision numbers fail with the following message:

BigNumber Error: new BigNumber() number type has more than 15 significant digits: [digits]

This has several problems:

  • Math.random() and Math.PI are valid numbers, which should be handled properly.
  • If you're worried about imprecision, allowing and encouraging literal numbers is exactly the wrong thing to allow. Strings literals should be used instead in that case, as you've apparently noted. The current design makes the one decent use of accepting numbers (for example, numbers from a database where you have no control over the precision anyway) more bug-prone (because people can have code that seems to work for a while on some inputs they try and then breaks in production) while making the improper use easier.
  • If you're going to allow conversion of a base two floating point number into a base ten floating point number, you're almost surely introducing error anyway. For instance, you can't tell whether the number that gives the string '0.1' is exactly 1/10 or the result of a calculation that's merely fairly close to 1/10 because of error, yet you accept that number as if it's exactly 1/10. If that possibility of error when the number is merely near 1/10 is acceptable, why not Math.PI?
  • If you're going to keep the error message, the digits in the error message should include the decimal point. There's no point in showing only the digits (are people supposed to count them or something?) rather than the number itself.
@MikeMcl
Copy link
Owner

MikeMcl commented Aug 21, 2013

Yes, the README doesn't mention, as the API document states, that "values of type number with more than 15 significant digits are considered invalid as calling toString or valueOf on such numbers may not result in the intended value".

There is a good reason for this limitation. Notice, for example, that precision is lost when the following 16 significant digit numbers are converted from decimal to binary floating point and back to decimal again

console.log( 88259496234518.57 );    // 88259496234518.56
console.log( -707422234799.9383 );   // -707422234799.9384

This does not happen with any numbers which have 15 s.d. or less.

Also, but less importantly, most number literals that have 'too many' significant digits to be accurately stored will be represented as numbers with 16 digits or more,

console.log( 12345678.123456789 );      // 12345678.12345679        16 s.d.
console.log( 99999999999999999999 );    // 100000000000000000000    21 s.d.

although not all, particularly those that result in exponential notation and that have numerous intervening zeros

console.log( 1.0000000000000001 );                 // 1
console.log( 10000000000000001 );                  // 1e+16
console.log( 0.10000000000000001 );                // 0.1
console.log( 0.000000000000010000000000000001 );   // 1e-14

The constructor accepts literal numbers as a convenience and the error message acts as a warning that a number may not be as intended. In production it is expected that the error messaging would be turned off and the constructor will then accept any numbers

BigNumber.config({ ERRORS: false });
new BigNumber( Math.PI );          // '3.141592653589793'
new BigNumber( Math.random() );    // '0.9544870883598676'

Alternatively, if a user doesn't want to turn off error messages, toString can be called on the values

new BigNumber( Math.PI.toString() );          // '3.141592653589793'
new BigNumber( Math.random().toString() );    // '0.9544870883598676'

I am not sure what you are referring to when you state that the current design makes "the improper use [of literal numbers] easier". As shown above, their improper use would certainly be easier if the 15 s.d. limit was removed, as you seem to want.

I do not agree that: "If you're going to allow conversion of a base two floating point number into a base ten floating point number, you're almost surely introducing error anyway." There is no reason to assume a literal number passed to the constructor is in error, and a string argument could equally be formed from the result of inaccurate floating-point calculations. The 15 s.d. limit helps prevent such errors

console.log( 0.7 + 0.1 );    // 0.7999999999999999      16 s.d. so rejected
console.log( 0.2 + 0.1 );    // 0.30000000000000004     17 s.d. so rejected

As long as only numbers with 15 s.d. or less are involved, any errors introduced by the imprecision of floating-point arithmetic will result in a number with more than 15 s.d.

While you seem to consider the 15 s.d. limit a pointless cause of irritation, confusion and bugs, I think on balance it is better to have it then not. It is a reminder about the potential loss of precision in decimal-binary-decimal conversions and is easily turned off.

I agree that it would be better to keep the decimal point in the error messages so that the offending value can be identified more easily. I will update the constructor presently, and add a note about the 15 s.d. limit in the README.

Thanks for the feedback.

I would be interested to hear if anybody else would prefer the 15 s.d. limit to be removed.

@igorlino
Copy link

igorlino commented Feb 9, 2016

@MikeMcl

nice library!

I would be interested to hear if anybody else would prefer the 15 s.d. limit to be removed.

I think it would be practical if it could be configurable, since maybe I want to work with 20 digits, without having to disable the errors.

BigNumber.config({ ERRORS: false });

@MikeMcl
Copy link
Owner

MikeMcl commented Feb 10, 2016

Thanks.

Can't you just convert the numbers to strings?

n = 12345123451234512345;
x = new BigNumber(String(n));

And most 20 digit numbers can't be stored exactly as JavaScript numbers anyway

console.log(n);    // 12345123451234513000

I have removed the 15 digit limit for numbers in decimal.js and I may do it here, but not at present.

@Slowlearneruk
Copy link

Shouldn't the cap be Number.MAX_SAFE_INTEGER rather than 15 digits?

@MikeMcl
Copy link
Owner

MikeMcl commented Mar 9, 2016

No, it is a matter of significant digits rather than whether a number is an integer or float.

Many values with sixteen significant digits or more cannot be stored precisely using JavaScript numbers (double-precision floating-point). For example,

console.log(9.543245345454127)    // 9.543245345454126

@Slowlearneruk
Copy link

I wonder if it could be a little more selective - as was working with integers it was frustrating to have to drop from my ceiling from MAX_SAVE_INTEGER to 999999999999999. It is an excellent library BTW which this extremely minor criticism does nothing to change :)

@MikeMcl
Copy link
Owner

MikeMcl commented Mar 10, 2016

Thanks for the feedback.

See #91

From v2.2.0, all integers up to Number.MAX_SAFE_INTEGER are now accepted.

@mqklin
Copy link

mqklin commented Jan 9, 2019

@MikeMcl is it possible to automatically convert arguments to String? Now we have to wrap every argument, very inconvenient: toBigNumber(String(10 ** -decimals)).mul(String(quantity)).mul(String(priceItem.value.usd)));

@MikeMcl
Copy link
Owner

MikeMcl commented Jan 9, 2019

@mqklin

There is no need to convert arguments to string. You are commenting on a very old issue here which probably contains discussion that is irrelevant to later versions of this library.

Since v7.0.0 there is no error thrown on numbers with more than 15 significant digits unless a DEBUG property is added to BigNumber and set to true.

BigNumber.DEBUG = true;

@mqklin
Copy link

mqklin commented Jan 10, 2019

@MikeMcl We use v7.2.1
That's strange, because we didn't set DEBUG = true

@mqklin
Copy link

mqklin commented Jan 10, 2019

Oh, we use bignumber.js inside web3 lib, which uses old version: https://github.com/frozeman/bignumber.js-nolookahead - we should use the new one.

Thank you!

dbrans added a commit to MetaMask/metamask-extension that referenced this issue May 8, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**
This PR fixes 2 bugs in `useBalanceChanges`

### Fix 1: decimals parsing in `fetchErc20Decimals` sometimes returns
`NaN`
`fetchErc20Decimals` uses `parseInt` to parse the return from
getTokenStandardAndDetails. `parseInt` will return `NaN` if it cannot
parse the string:
```
> parseInt('0x')
NaN
```
This PR patches `fetchErc20Decimals` to try a couple methods of parsing
and fall back to the default if none of them work.

### Fix 2: fiat rates for ETH or tokens may contain more than 15
decimals
The constructor and many methods on `BigNumber` will throw an error if
you attempt to call them with a javascript number that has more than 15
decimal places. An [explanation can be found here, along with a
workaround](MikeMcl/bignumber.js#11 (comment)):
Converting these numbers to a string may loose some precision, but is a
safe.

**New tests added and fixed** 
```
● useBalanceChanges › with token balance changes › uses default decimals when token details are not valid numbers

    expect(received).toBe(expected) // Object.is equality

    Expected: 18
    Received: 0

      271 |       await waitForNextUpdate();
      272 |
    > 273 |       expect(result.current.value[0].amount.decimalPlaces()).toBe(18);
          |                                                              ^
      274 |     });
      275 |   });
      276 |

● useBalanceChanges › with token balance changes › handles token fiat rate with more than 15 significant digits

    BigNumber Error: times() number type has more than 15 significant digits: 0.1234567890123456

      141 |     const fiatRate = erc20FiatRates[tokenBc.address];
      142 |     const fiatAmount = fiatRate
    > 143 |       ? amount.times(fiatRate).toNumber()
          |                ^
      144 |       : FIAT_UNAVAILABLE;
      145 |
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24422?quickstart=1)

## **Related issues**

Fixes: #24336

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
dbrans added a commit to MetaMask/metamask-extension that referenced this issue May 8, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

This PR fixes 2 bugs in `useBalanceChanges`

`NaN`
`fetchErc20Decimals` uses `parseInt` to parse the return from
getTokenStandardAndDetails. `parseInt` will return `NaN` if it cannot
parse the string:
```
> parseInt('0x')
NaN
```
This PR patches `fetchErc20Decimals` to try a couple methods of parsing
and fall back to the default if none of them work.

decimals
The constructor and many methods on `BigNumber` will throw an error if
you attempt to call them with a javascript number that has more than 15
decimal places. An [explanation can be found here, along with a
workaround](MikeMcl/bignumber.js#11 (comment)):
Converting these numbers to a string may loose some precision, but is a
safe.

**New tests added and fixed**
```
● useBalanceChanges › with token balance changes › uses default decimals when token details are not valid numbers

    expect(received).toBe(expected) // Object.is equality

    Expected: 18
    Received: 0

      271 |       await waitForNextUpdate();
      272 |
    > 273 |       expect(result.current.value[0].amount.decimalPlaces()).toBe(18);
          |                                                              ^
      274 |     });
      275 |   });
      276 |

● useBalanceChanges › with token balance changes › handles token fiat rate with more than 15 significant digits

    BigNumber Error: times() number type has more than 15 significant digits: 0.1234567890123456

      141 |     const fiatRate = erc20FiatRates[tokenBc.address];
      142 |     const fiatAmount = fiatRate
    > 143 |       ? amount.times(fiatRate).toNumber()
          |                ^
      144 |       : FIAT_UNAVAILABLE;
      145 |
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24422?quickstart=1)

Fixes: #24336

1. Go to this page...
2.
3.

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<!-- [screenshots/recordings] -->

<!-- [screenshots/recordings] -->

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
danjm pushed a commit to MetaMask/metamask-extension that referenced this issue May 20, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

This PR fixes 2 bugs in `useBalanceChanges`

`NaN`
`fetchErc20Decimals` uses `parseInt` to parse the return from
getTokenStandardAndDetails. `parseInt` will return `NaN` if it cannot
parse the string:
```
> parseInt('0x')
NaN
```
This PR patches `fetchErc20Decimals` to try a couple methods of parsing
and fall back to the default if none of them work.

decimals
The constructor and many methods on `BigNumber` will throw an error if
you attempt to call them with a javascript number that has more than 15
decimal places. An [explanation can be found here, along with a
workaround](MikeMcl/bignumber.js#11 (comment)):
Converting these numbers to a string may loose some precision, but is a
safe.

**New tests added and fixed**
```
● useBalanceChanges › with token balance changes › uses default decimals when token details are not valid numbers

    expect(received).toBe(expected) // Object.is equality

    Expected: 18
    Received: 0

      271 |       await waitForNextUpdate();
      272 |
    > 273 |       expect(result.current.value[0].amount.decimalPlaces()).toBe(18);
          |                                                              ^
      274 |     });
      275 |   });
      276 |

● useBalanceChanges › with token balance changes › handles token fiat rate with more than 15 significant digits

    BigNumber Error: times() number type has more than 15 significant digits: 0.1234567890123456

      141 |     const fiatRate = erc20FiatRates[tokenBc.address];
      142 |     const fiatAmount = fiatRate
    > 143 |       ? amount.times(fiatRate).toNumber()
          |                ^
      144 |       : FIAT_UNAVAILABLE;
      145 |
```

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24422?quickstart=1)

Fixes: #24336

1. Go to this page...
2.
3.

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

<!-- [screenshots/recordings] -->

<!-- [screenshots/recordings] -->

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants