fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741

digiwand · 2024-07-10T14:05:36Z

Description

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

Related issues

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

Technical Details

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 [Bug]: Precision is lost displaying Permit value in simulation and data tree #25755

Manual testing steps

run yarn storybook
go to http://localhost:6006/?path=/story/pages-confirmations-confirm-signatures-signedtypeddatav3orv4--permit-story&args=msgParams.data:3.0001231231212312e+22
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}}"

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

- calculation passed to BigNumber may result in over 15 digits which errors - preserve the precision up to formatAmount - currently formatAmount takes away precision after it transforms the value using toNumber to pass to Intl.Number

github-actions · 2024-07-10T14:05:49Z

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.

…cision

digiwand · 2024-07-11T00:08:21Z

...ges/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.tsx

@@ -52,10 +53,15 @@ const PermitSimulation: React.FC<{
  }, [exchangeRate, value]);

  const { tokenValue, tokenValueMaxPrecision } = useMemo(() => {
-    const valueBN = new BigNumber(value / Math.pow(10, tokenDecimals));


needed to split the BigNumber constants to use built in pow method since the result of value / Math.pow(10, tokenDecimals) could lead to an unexpected lengthy number. If it is over 15 digits it would cause the error.

We are also not applying to string on the result of the equation to pass to BigNumber directly because this could also result in precision loss.

metamaskbot · 2024-07-11T00:30:59Z

Builds ready [1182bb5]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (235 ± 236 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	73	376	113	62	30
		domContentLoaded	11	83	31	18	9
		load	50	1924	235	492	236
		domInteractive	11	83	31	18	9

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 202 Bytes (0.00%)
common: 276 Bytes (0.00%)

jpuri

👍 changes look good

...ges/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.tsx

shared/modules/transaction.utils.ts

ui/pages/confirmations/components/confirm/row/dataTree.tsx

metamaskbot · 2024-07-11T15:24:46Z

Builds ready [d82f208]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (153 ± 146 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	71	422	118	71	34
		domContentLoaded	10	125	33	24	11
		load	45	1458	153	305	146
		domInteractive	10	125	33	24	11

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 202 Bytes (0.00%)
common: 276 Bytes (0.00%)

sonarqubecloud · 2024-07-11T15:29:17Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-07-11T16:05:30Z

Builds ready [fceba54]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (71 ± 10 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	61	153	101	20	10
		domContentLoaded	9	71	30	16	7
		load	39	127	71	20	10
		domInteractive	9	71	30	16	7

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 0 Bytes (0.00%)
ui: 202 Bytes (0.00%)
common: 276 Bytes (0.00%)

codecov · 2024-07-11T16:05:59Z

Codecov Report

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

Project coverage is 69.97%. Comparing base (5ee57a6) to head (fceba54).
Report is 32 commits behind head on develop.

Files	Patch %	Lines
shared/modules/transaction.utils.ts	90.91%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #25741      +/-   ##
===========================================
+ Coverage    69.77%   69.97%   +0.19%     
===========================================
  Files         1376     1390      +14     
  Lines        48403    48910     +507     
  Branches     13348    13456     +108     
===========================================
+ Hits         33773    34221     +448     
- Misses       14630    14689      +59

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

legobeat

LGTM!

digiwand · 2024-07-12T13:08:21Z

applying a similar fix for 15 digits [issue] PR:(#13738) #25799

later, we can reuse the calcTokenAmount that is being updated in the PR

digiwand added 4 commits July 10, 2024 14:52

feat: support simulations in stories

3afe15a

feat: update parseTypedDataMessage to preserve value precision

eff04cc

fix: number type has more than 15 error

9d82e3e

- calculation passed to BigNumber may result in over 15 digits which errors - preserve the precision up to formatAmount - currently formatAmount takes away precision after it transforms the value using toNumber to pass to Intl.Number

feat: apply ellipsis to simulation value

ed262d5

digiwand requested review from a team as code owners July 10, 2024 14:05

metamaskbot added the team-confirmations Push issues to confirmations team label Jul 10, 2024

digiwand added 4 commits July 10, 2024 17:19

fix: loss of precision is misleading. do not use formatAmount

89f75ac

fix: normalize parseTypedDataMessage value to string for consistency

a48d310

fix: update parseTypedDataMessage to handle spaces

626a5db

test: parseTypedDataMessage message.value

050222f

digiwand changed the title ~~fix: Error: new BigNumber() more than 15 digits feat: apply~~ fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis Jul 10, 2024

digiwand added 4 commits July 10, 2024 23:06

fix: invalid jsdoc @fixme -> FIXME

847b690

feat: add ellipsis to Permit value text

516efbe

feat: revert scientific notation in favor of values that may lose pre…

2d8b145

…cision

fix: fixme jsdoc lint

1182bb5

digiwand mentioned this pull request Jul 11, 2024

[Bug]: Precision is lost displaying Permit value in simulation and data tree #25755

Closed

digiwand commented Jul 11, 2024

View reviewed changes

jpuri previously approved these changes Jul 11, 2024

View reviewed changes

pedronfigueiredo previously approved these changes Jul 11, 2024

View reviewed changes

matthewwalsh0 requested changes Jul 11, 2024

View reviewed changes

matthewwalsh0 previously approved these changes Jul 11, 2024

View reviewed changes

test:fix: update snapshots to support Permit updates

ab747cf

digiwand dismissed stale reviews from matthewwalsh0, pedronfigueiredo, and jpuri via ab747cf July 11, 2024 14:20

digiwand added 2 commits July 11, 2024 15:23

test:fix: update TypedSignInfo snapshot

6ffabd0

test:fix: update typedSignDataV1 info snapshot

939dbf6

digiwand added 2 commits July 11, 2024 15:36

test:fix: snapshots min-width: 0

741cf8c

Merge branch 'develop' into feat-permit-simulation-ellipsis-format

d82f208

test:fix: snapshot transaction-data min-width 0

fceba54

digiwand requested review from matthewwalsh0, pedronfigueiredo and jpuri July 11, 2024 16:18

pedronfigueiredo approved these changes Jul 12, 2024

View reviewed changes

legobeat approved these changes Jul 12, 2024

View reviewed changes

This was referenced Jul 12, 2024

[Sentry] [Bug] Bug/Error: div() number type has more than 15 significant digits #13738

Closed

fix: calcTokenAmount BigNumber more than 15 digits error #25799

Merged

digiwand merged commit 528c653 into develop Jul 12, 2024
76 checks passed

digiwand deleted the feat-permit-simulation-ellipsis-format branch July 12, 2024 13:09

github-actions bot locked and limited conversation to collaborators Jul 12, 2024

metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 12, 2024

digiwand added release-12.1.0 Issue or pull request that will be included in release 12.1.0 and removed release-12.3.0 Issue or pull request that will be included in release 12.3.0 labels Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741

fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741

digiwand commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 10, 2024

digiwand Jul 11, 2024

metamaskbot commented Jul 11, 2024

jpuri left a comment

metamaskbot commented Jul 11, 2024

sonarqubecloud bot commented Jul 11, 2024

metamaskbot commented Jul 11, 2024

codecov bot commented Jul 11, 2024

legobeat left a comment

digiwand commented Jul 12, 2024

fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741

fix: Error: new BigNumber() more than 15 digits feat: apply ellipsis #25741

Conversation

digiwand commented Jul 10, 2024 • edited Loading

Description

Related issues

Technical Details

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jul 10, 2024

digiwand Jul 11, 2024

Choose a reason for hiding this comment

metamaskbot commented Jul 11, 2024

jpuri left a comment

Choose a reason for hiding this comment

metamaskbot commented Jul 11, 2024

sonarqubecloud bot commented Jul 11, 2024

Quality Gate passed

metamaskbot commented Jul 11, 2024

codecov bot commented Jul 11, 2024

Codecov Report

legobeat left a comment

Choose a reason for hiding this comment

digiwand commented Jul 12, 2024

digiwand commented Jul 10, 2024 •

edited

Loading