[Bug]: MetaMask encounters errors when processing transactions initiated from dapp #24336
Assignees
Labels
release-11.16.0
Issue or pull request that will be included in release 11.16.0
release-11.17.0
Issue or pull request that will be included in release 11.17.0
release-11.18.0
Issue or pull request that will be included in release 11.18.0
release-blocker
This bug is blocking the next release
Sev1-high
High severity; partial loss of service with severe impact upon users, with no workaround.
team-confirmations
Push issues to confirmations team
type-bug
Comments
sleepytanya
added
type-bug
release-11.16.0
metamaskbot
added
the
regression-prod-11.16.0
Merged
Merged
|
MM error with short number: Screen.Recording.2024-05-01.at.19.34.39.mov |
sleepytanya
changed the title
sleepytanya
added
team-confirmations-system
gauthierpetetin
added
the
Sev1-high
bschorchit
added
release-blocker
|
The bug doesn't happen on swaps initiated from within the wallet: |
metamaskbot
added
the
release-11.18.0
7 tasks
dbrans
added a commit
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 |
```
[](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
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 |
```
[](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
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 |
```
[](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.
metamaskbot
added
the
release-11.17.0
|
Missing release label release-11.17.0 on issue. Adding release label release-11.17.0 on issue, as issue is linked to PR #24382 which has this release label. |
](https://codespaces.new/MetaMask/metamask-extension/pull/24422?quickstart=1){kind=link}
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
MetaMask encounters errors when processing transactions initiated from dapps.
Expected behavior
Smooth and accurate transaction handling in scenarios involving extensive numerical data.
Screenshots/Recordings
Ethereum mainnet, Chrome:
bigNumber.mov
Polygon, Chrome:
Screen.Recording.2024-05-01.at.18.30.40.mov
Polygon, Firefox:
firefox.mov
Different error message for shorter number:
diffErr.mov
Steps to reproduce
Error messages or log output
No response
Version
11.16.0
Build type
None
Browser
Chrome, Firefox
Operating system
MacOS
Hardware wallet
No response
Additional context
No response
Severity
No response
The text was updated successfully, but these errors were encountered: