fix: change in number format to fix loss of precision for very big values by jpuri · Pull Request #25968 · MetaMask/metamask-extension

jpuri · 2024-07-19T11:32:37Z

Description

fix loss of precision for very big values in signature simulations

Related issues

Fixes: #25755

Manual testing steps

Create a permit request of arbitrary high value like 115792089237316195423570985008687907853269984665640564039457584007913129639935
As you submit the request ensure that precision is not lost

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.

github-actions · 2024-07-19T11:32:51Z

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.

ui/pages/confirmations/components/simulation-details/formatAmount.ts

metamaskbot · 2024-07-22T07:50:35Z

Builds ready [d5bcd33]

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, 10, 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 (145 ± 159 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	76	163	103	22	11
		domContentLoaded	8	101	31	22	11
		load	38	1585	145	331	159
		domInteractive	8	101	31	22	11

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 55 Bytes (0.00%)
common: 0 Bytes (0.00%)

codecov · 2024-07-22T07:50:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.94%. Comparing base (51410b2) to head (115a1b2).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #25968   +/-   ##
========================================
  Coverage    69.94%   69.94%           
========================================
  Files         1409     1409           
  Lines        49795    49802    +7     
  Branches     13773    13775    +2     
========================================
+ Hits         34826    34833    +7     
  Misses       14969    14969

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

metamaskbot · 2024-07-25T13:22:27Z

Builds ready [3f21bca]

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, 10, 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 (218 ± 223 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	75	369	110	62	30
		domContentLoaded	9	67	28	16	8
		load	42	1713	218	464	223
		domInteractive	9	67	28	16	8

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 88 Bytes (0.00%)
common: 0 Bytes (0.00%)

dbrans · 2024-07-25T13:36:16Z

ui/pages/confirmations/components/simulation-details/formatAmount.ts

+      // string is valid parameter for format function
+      // for some reason it gives TS issue
+      // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/format#number
+      amount.toFixed(0) as unknown as number,


Awesome find!

In that case, we don't need the if (maximumFractionDigits === 0) { above... we can have the same code path for all amounts here.

sonarqubecloud · 2024-07-25T15:48:45Z

Quality Gate passed

Issues
0 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-25T16:14:55Z

Builds ready [115a1b2]

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, 10, 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 (506 ± 358 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	79	277	119	42	20
		domContentLoaded	11	61	35	13	6
		load	54	1889	506	745	358
		domInteractive	11	61	35	13	6

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 31 Bytes (0.00%)
common: 0 Bytes (0.00%)

digiwand

Great fix!

digiwand · 2024-08-19T13:10:27Z

Cherry-pick to V12.1.0 #26496

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

dbrans reviewed Jul 19, 2024

View reviewed changes

ui/pages/confirmations/components/simulation-details/formatAmount.ts Outdated Show resolved Hide resolved

jpuri force-pushed the numberformat_fix branch from 694ecf8 to d5bcd33 Compare July 22, 2024 07:19

jpuri changed the title ~~Change in number format to fix loss of precision for very big values~~ fix: change in number format to fix loss of precision for very big values Jul 22, 2024

jpuri requested a review from dbrans July 22, 2024 07:20

jpuri added 2 commits July 25, 2024 18:19

Change in number format to fix loss of precision for very big values

e106cd3

update

23fa295

jpuri force-pushed the numberformat_fix branch from 38b1c82 to 23fa295 Compare July 25, 2024 12:49

update

3f21bca

dbrans reviewed Jul 25, 2024

View reviewed changes

update

19be278

jpuri requested a review from dbrans July 25, 2024 15:42

update

407f96d

jpuri marked this pull request as ready for review July 25, 2024 15:45

jpuri requested a review from a team as a code owner July 25, 2024 15:45

Merge branch 'develop' into numberformat_fix

115a1b2

digiwand approved these changes Jul 25, 2024

View reviewed changes

digiwand mentioned this pull request Jul 25, 2024

feat: Support Permit variants: PermitSingle, PermitBatch, PermitTransferFrom, PermitBatchTransferFrom, TradeOrder, Seaport #26107

Merged

7 tasks

matthewwalsh0 approved these changes Jul 26, 2024

View reviewed changes

jpuri merged commit 30dce33 into develop Jul 26, 2024

jpuri deleted the numberformat_fix branch July 26, 2024 13:11

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

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

digiwand added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Aug 19, 2024

digiwand removed the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 19, 2024

digiwand added the rc-cherry-picked label Aug 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change in number format to fix loss of precision for very big values#25968

fix: change in number format to fix loss of precision for very big values#25968
jpuri merged 6 commits intodevelopfrom
numberformat_fix

jpuri commented Jul 19, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jul 19, 2024

Uh oh!

Uh oh!

metamaskbot commented Jul 22, 2024

Uh oh!

codecov bot commented Jul 22, 2024 •

edited

Loading

Uh oh!

metamaskbot commented Jul 25, 2024

Uh oh!

dbrans Jul 25, 2024

Uh oh!

sonarqubecloud bot commented Jul 25, 2024

Uh oh!

metamaskbot commented Jul 25, 2024

Uh oh!

digiwand left a comment

Uh oh!

digiwand commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jpuri commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Jul 19, 2024

Uh oh!

Uh oh!

metamaskbot commented Jul 22, 2024

Uh oh!

codecov bot commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Jul 25, 2024

Uh oh!

dbrans Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 25, 2024

Quality Gate passed

Uh oh!

metamaskbot commented Jul 25, 2024

Uh oh!

digiwand left a comment

Choose a reason for hiding this comment

Uh oh!

digiwand commented Aug 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jpuri commented Jul 19, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading