Skip to content

Commit

Permalink
fix: calcTokenAmount BigNumber more than 15 digits error (#25799)
Browse files Browse the repository at this point in the history
<!--
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 a BigNumber Error on calcTokenAmount 
- Fixes Swaps destinationAmount type. '0' should take place of null
occurrences
- Adds calcTokenAmount tests

#### Explanation:
When certain large params are passed, e.g. when the decimal passed is
36, the calculation of `Math.pow(10, Number(decimals || 0))` is expected
→ 100000000. Then, when we pass this to BigNumber div() we get the
error:
```
BigNumber Error:
div() number type has more than 15 significant digits: 9.999999999999999e+35
```
however, if we pass the same value as a BigNumber, we do not get the
error. That is, instead of passing the result of `Math.pow(10,
Number(decimals || 0))`, we pass the result of `new
BigNumber(10).pow(decimals)`

honestly, I don't quite understand why this is. We can see the
[bignumber.js#div code
here](https://github.com/MikeMcl/bignumber.js/blob/v4.1.0/bignumber.js#L750)

#### Other notes:
- It turns out that updating the bignumber.js package from 4.1.0 → 9.1.2
could also fix this issue
- There is another, less-used BigNumber class that exists in our code
([this BigNumber div
code](https://github.com/ethers-io/ethers.js/blob/f97b92bbb1bde22fcc44100af78d7f31602863ab/packages/bignumber/src.ts/bignumber.ts#L81))

---

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

## **Related issues**

Fixes: #13738
Related:
#25741 (comment)

## **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**
(divisor was previously called multiplier)
![CleanShot 2024-07-16 at 01 07
09@2x](https://github.com/user-attachments/assets/f1444f58-ca02-404a-aba9-153455bb177c)

### **After**

![CleanShot 2024-07-16 at 01 19
06@2x](https://github.com/user-attachments/assets/c5251516-f8b1-459a-be5f-06f99d66b3f7)

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension 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.
  • Loading branch information
digiwand authored Jul 17, 2024
1 parent 2585dcf commit 5f0949b
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 6 deletions.
4 changes: 2 additions & 2 deletions app/scripts/controllers/swaps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ export default class SwapsController extends BaseController<
aggregator,
approvalNeeded,
averageGas,
destinationAmount = 0,
destinationAmount,
destinationToken,
destinationTokenInfo,
gasEstimateWithRefund,
Expand Down Expand Up @@ -595,7 +595,7 @@ export default class SwapsController extends BaseController<
: totalEthCost;

const decimalAdjustedDestinationAmount = calcTokenAmount(
destinationAmount,
destinationAmount ?? '0',
destinationTokenInfo.decimals,
);

Expand Down
49 changes: 48 additions & 1 deletion shared/lib/transaction-controller-utils.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { TransactionEnvelopeType } from '@metamask/transaction-controller';

import BigNumber from 'bignumber.js';
import { EtherDenomination } from '../constants/common';
import { CHAIN_IDS } from '../constants/network';
import { Numeric } from '../modules/Numeric';
Expand All @@ -18,6 +18,53 @@ describe('transaction controller utils', () => {
});
});

describe('calcTokenAmount()', () => {
// @ts-expect-error This is missing from the Mocha type definitions
it.each([
// number values
[0, 5, '0'],
[123456, undefined, '123456'],
[123456, 5, '1.23456'],
[123456, 6, '0.123456'],
// Do not delete the following test. Testing decimal = 36 is important because it has broken
// BigNumber#div in the past when the value that was passed into it was not a BigNumber.
[123456, 36, '1.23456e-31'],
[3000123456789678, 6, '3000123456.789678'],
// eslint-disable-next-line no-loss-of-precision
[3000123456789123456789123456789, 3, '3.0001234567891233e+27'], // expected precision lost
// eslint-disable-next-line no-loss-of-precision
[3000123456789123456789123456789, 6, '3.0001234567891233e+24'], // expected precision lost
// string values
['0', 5, '0'],
['123456', undefined, '123456'],
['123456', 5, '1.23456'],
['123456', 6, '0.123456'],
['3000123456789678', 6, '3000123456.789678'],
[
'3000123456789123456789123456789',
3,
'3.000123456789123456789123456789e+27',
],
[
'3000123456789123456789123456789',
6,
'3.000123456789123456789123456789e+24',
],
// BigNumber values
[new BigNumber('3000123456789678'), 6, '3000123456.789678'],
[
new BigNumber('3000123456789123456789123456789'),
6,
'3.000123456789123456789123456789e+24',
],
])(
'returns the value %s divided by 10^%s = %s',
(value, decimals, expected) => {
expect(calcTokenAmount(value, decimals).toString()).toBe(expected);
},
);
});

describe('getSwapsTokensReceivedFromTxMeta', () => {
it('returns null if txMeta is not well formed', () => {
expect(getSwapsTokensReceivedFromTxMeta('ETH', {}, '0x00')).toBe(null);
Expand Down
11 changes: 8 additions & 3 deletions shared/lib/transactions-controller-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,14 @@ export function toPrecisionWithoutTrailingZeros(n, precision) {
.replace(/(\.[0-9]*[1-9])0*|(\.0*)/u, '$1');
}

export function calcTokenAmount(value, decimals) {
const multiplier = Math.pow(10, Number(decimals || 0));
return new BigNumber(String(value)).div(multiplier);
/**
* @param {number|string|BigNumber} value
* @param {number} decimals
* @returns {BigNumber}
*/
export function calcTokenAmount(value, decimals = 0) {
const divisor = new BigNumber(10).pow(decimals);
return new BigNumber(String(value)).div(divisor);
}

export function getSwapsTokensReceivedFromTxMeta(
Expand Down

0 comments on commit 5f0949b

Please sign in to comment.