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

fix: (cherry-pick)(Version v12.1.0) Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741 #25804

Conversation

digiwand
Copy link
Contributor

Description

Cherry-pick #25741 for Version-v12.1.0

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2730 (ellipsis)
Fixes: #25751 (Error)

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

…25741)

<!--
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.
-->

Plan is to cherry-pick this PR for V12.1
- Adds ellipsis to Permit value
- Fixes error:
```
BigNumber Error: new BigNumber() number type has more than 15 significant digits
```

Note that the precision is lost through formatAmount. Fix will be added
#25755

Most file updates are snapshot updates

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

Fixes: MetaMask/MetaMask-planning#2730
(ellipsis)
Fixes: #25751
(Error)

- `JSON.parse` coerces number values into scientific notation if they
are greater than Number.MAX_SAFE_INTEGER, which is the max safe integer
in JavaScript.
e.g. 30001231231212312138768 → 3.0001231231212312e+22
- `JSON.parse` reviver cannot help since the value will be coerced by
the time it
 reaches the reviver function.
- We preserve the value by extracting the value with a regex and
overwriting the `JSON.parse` value. The preserved value should never be
converted to a JavaScript number since it will lose its precision.
Instead, we can preserve the value as a string, BigNumber, or Numeric
type. BigInt also has rounding limitations.
- BigNumber errors out when constructed with numbers with 15+ digits. It
can accept strings to preserve digits.
- formatAmount in
`ui/pages/confirmations/components/simulation-details/formatAmount.ts`
utilizes Intl.NumberFormat, which requires a BigInt or number passed to
it causing a loss in precision when computing large numbers. It does not
accept strings. Currently, it will display 0s for the digits after 15
digits.
e.g. 30001231231212312138768 → 30,001,231,231,212,312,000,000
Issue ticket to address this
#25755

1. run `yarn storybook`
2. go to
http://localhost:6006/?path=/story/pages-confirmations-confirm-signatures-signedtypeddatav3orv4--permit-story&args=msgParams.data:3.0001231231212312e+22
3. copy and paste the following into data
```
"{\"types\":{\"EIP712Domain\":[{\"name\":\"name\",\"type\":\"string\"},{\"name\":\"version\",\"type\":\"string\"},{\"name\":\"chainId\",\"type\":\"uint256\"},{\"name\":\"verifyingContract\",\"type\":\"address\"}],\"Permit\":[{\"name\":\"owner\",\"type\":\"address\"},{\"name\":\"spender\",\"type\":\"address\"},{\"name\":\"value\",\"type\":\"uint256\"},{\"name\":\"nonce\",\"type\":\"uint256\"},{\"name\":\"deadline\",\"type\":\"uint256\"}]},\"primaryType\":\"Permit\",\"domain\":{\"name\":\"MyToken\",\"version\":\"1\",\"verifyingContract\":\"0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC\",\"chainId\":1},\"message\":{\"owner\":\"0x935e73edb9ff52e23bac7f7e043a1ecd06d05477\",\"spender\":\"0x5B38Da6a701c568545dCfcB03FcB875f56beddC4\",\"value\":30001231231212312138768,\"nonce\":0,\"deadline\":50000000000}}"
```

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

![CleanShot 2024-07-10 at 01 14
52](https://github.com/MetaMask/metamask-extension/assets/20778143/7a7ff9e3-07a9-4a3b-b8f4-7f672998cdab)

![CleanShot 2024-07-11 at 00 37
25@2x](https://github.com/MetaMask/metamask-extension/assets/20778143/df6b1f0e-fa12-4b6e-9ae1-0486feb7571a)
![CleanShot 2024-07-11 at 00 37
37@2x](https://github.com/MetaMask/metamask-extension/assets/20778143/faffb8e0-6eae-4ef9-8f3b-c399b7f9f7ae)

- [ ] 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.

- [ ] 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.
@digiwand digiwand added the team-confirmations Push issues to confirmations team label Jul 12, 2024
@digiwand digiwand requested review from a team as code owners July 12, 2024 14:25
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Collaborator

Builds ready [c9dbb7d]
Page Load Metrics (149 ± 168 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint641571002110
domContentLoaded95626136
load391667149349168
domInteractive95626136
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.5 KiB (0.05%)
  • common: 276 Bytes (0.00%)

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.58%. Comparing base (ce982e3) to head (c9dbb7d).

Files Patch % Lines
shared/modules/transaction.utils.ts 90.91% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##           Version-v12.1.0   #25804      +/-   ##
===================================================
+ Coverage            69.57%   69.58%   +0.01%     
===================================================
  Files                 1360     1360              
  Lines                48200    48215      +15     
  Branches             13306    13311       +5     
===================================================
+ Hits                 33535    33550      +15     
  Misses               14665    14665              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@digiwand digiwand added type-bug release-12.1.0 Issue or pull request that will be included in release 12.1.0 release-blocker This bug is blocking the next release and removed type-bug labels Jul 12, 2024
@cryptotavares cryptotavares merged commit 50f347c into Version-v12.1.0 Jul 17, 2024
74 checks passed
@cryptotavares cryptotavares deleted the Version-v12.1.0-feat-permit-simulation-ellipsis-format-2 branch July 17, 2024 11:12
@github-actions github-actions bot locked and limited conversation to collaborators Jul 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression-RC-12.1.0 release-12.1.0 Issue or pull request that will be included in release 12.1.0 release-blocker This bug is blocking the next release team-confirmations Push issues to confirmations team type-bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants