Skip to content

Commit

Permalink
fix: decimals and fiat conversion crashes in simulation (#24422)
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.
-->

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.
  • Loading branch information
dbrans committed May 8, 2024
1 parent e21c67f commit 8488d63
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ const ETH_TO_FIAT_RATE = 3;

const ERC20_TOKEN_ADDRESS_1_MOCK: Hex = '0x0erc20_1';
const ERC20_TOKEN_ADDRESS_2_MOCK: Hex = '0x0erc20_2';
const ERC20_TOKEN_ADDRESS_3_MOCK: Hex = '0x0erc20_3';
const ERC20_DECIMALS_1_MOCK = 3;
const ERC20_DECIMALS_2_MOCK = 4;
const ERC20_DECIMALS_INVALID_MOCK = 'xyz';
const ERC20_TO_FIAT_RATE_1_MOCK = 1.5;
const ERC20_TO_FIAT_RATE_2_MOCK = 6;

Expand All @@ -68,9 +70,10 @@ describe('useBalanceChanges', () => {
beforeEach(() => {
jest.clearAllMocks();
mockGetTokenStandardAndDetails.mockImplementation((address: Hex) => {
const decimalMap: Record<Hex, number> = {
const decimalMap: Record<Hex, number | string> = {
[ERC20_TOKEN_ADDRESS_1_MOCK]: ERC20_DECIMALS_1_MOCK,
[ERC20_TOKEN_ADDRESS_2_MOCK]: ERC20_DECIMALS_2_MOCK,
[ERC20_TOKEN_ADDRESS_3_MOCK]: ERC20_DECIMALS_INVALID_MOCK,
};
if (decimalMap[address]) {
return Promise.resolve({
Expand Down Expand Up @@ -253,6 +256,41 @@ describe('useBalanceChanges', () => {

expect(result.current.value[0].amount.decimalPlaces()).toBe(18);
});

it('uses default decimals when token details are not valid numbers', async () => {
const { result, waitForNextUpdate } = setupHook([
{
...dummyBalanceChange,
difference: DIFFERENCE_1_MOCK,
isDecrease: true,
address: ERC20_TOKEN_ADDRESS_3_MOCK,
standard: SimulationTokenStandard.erc20,
},
]);

await waitForNextUpdate();

expect(result.current.value[0].amount.decimalPlaces()).toBe(18);
});

it('handles token fiat rate with more than 15 significant digits', async () => {
mockFetchTokenExchangeRates.mockResolvedValue({
[ERC20_TOKEN_ADDRESS_1_MOCK]: 0.1234567890123456,
});
const { result, waitForNextUpdate } = setupHook([
{
...dummyBalanceChange,
difference: DIFFERENCE_1_MOCK,
isDecrease: true,
address: ERC20_TOKEN_ADDRESS_1_MOCK,
standard: SimulationTokenStandard.erc20,
},
]);

await waitForNextUpdate();

expect(result.current.value[0].fiatAmount).toBe(-0.002098765413209875);
});
});

describe('with native balance change', () => {
Expand Down Expand Up @@ -287,6 +325,19 @@ describe('useBalanceChanges', () => {
]);
});

it('handles native fiat rate with more than 15 significant digits', async () => {
mockGetConversionRate.mockReturnValue(0.1234567890123456);
const { result, waitForNextUpdate } = setupHook({
...dummyBalanceChange,
difference: DIFFERENCE_ETH_MOCK,
isDecrease: true,
});

await waitForNextUpdate();

expect(result.current.value[0].fiatAmount).toBe(-663.3337769927953);
});

it('handles no native balance change', async () => {
const { result, waitForNextUpdate } = setupHook(undefined);
await waitForNextUpdate();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ const NATIVE_DECIMALS = 18;

const ERC20_DEFAULT_DECIMALS = 18;

// See https://github.com/MikeMcl/bignumber.js/issues/11#issuecomment-23053776
function convertNumberToStringWithPrecisionWarning(value: number): string {
return String(value);
}

// Converts a SimulationTokenStandard to a TokenStandard
function convertStandard(standard: SimulationTokenStandard) {
switch (standard) {
Expand Down Expand Up @@ -55,8 +60,17 @@ function getAssetAmount(
// Fetches the decimals for the given token address.
async function fetchErc20Decimals(address: Hex): Promise<number> {
try {
const { decimals } = await getTokenStandardAndDetails(address);
return decimals ? parseInt(decimals, 10) : ERC20_DEFAULT_DECIMALS;
const { decimals: decStr } = await getTokenStandardAndDetails(address);
if (!decStr) {
return ERC20_DEFAULT_DECIMALS;
}
for (const radix of [10, 16]) {
const parsedDec = parseInt(decStr, radix);
if (isFinite(parsedDec)) {
return parsedDec;
}
}
return ERC20_DEFAULT_DECIMALS;
} catch {
return ERC20_DEFAULT_DECIMALS;
}
Expand Down Expand Up @@ -106,7 +120,9 @@ function getNativeBalanceChange(
}
const asset = NATIVE_ASSET_IDENTIFIER;
const amount = getAssetAmount(nativeBalanceChange, NATIVE_DECIMALS);
const fiatAmount = amount.times(nativeFiatRate).toNumber();
const fiatAmount = amount
.times(convertNumberToStringWithPrecisionWarning(nativeFiatRate))
.toNumber();
return { asset, amount, fiatAmount };
}

Expand All @@ -129,7 +145,9 @@ function getTokenBalanceChanges(

const fiatRate = erc20FiatRates[tokenBc.address];
const fiatAmount = fiatRate
? amount.times(fiatRate).toNumber()
? amount
.times(convertNumberToStringWithPrecisionWarning(fiatRate))
.toNumber()
: FIAT_UNAVAILABLE;

return { asset, amount, fiatAmount };
Expand Down

0 comments on commit 8488d63

Please sign in to comment.