fix: cherry-pick: Prevent coercing small spending caps to zero (#28179) #28183

pedronfigueiredo · 2024-10-30T15:01:33Z

Cherry-pick: #28179

Description

Previously there was a bug that affected the approve screen. When users had a small spending cap (between 0.001 and 0.0001 or smaller), it was coerced to 0.

This was caused by the method new Intl.NumberFormat(locale).format(spendingCap) that applied the 1,000 large number formatting, so the fix is to bypass it entirely for values smaller than 1.

Additionally, these unformatted small numbers are presented in scientific notation, so we leverage toNonScientificString(spendingCap) to prevent that.

Related issues

Fixes:
#28117

Manual testing steps

See original bug report.

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.

Description

Related issues

Fixes:

Manual testing steps

Go to this page...

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-10-30T15:01: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.

metamaskbot · 2024-10-30T16:17:33Z

Builds ready [ad45e56]

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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2042 ± 71 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	567	2365	1961	352	169
		domContentLoaded	1720	2356	2017	153	73
		load	1779	2366	2042	148	71
		domInteractive	28	94	62	16	8
		backgroundConnect	10	64	24	19	9
		firstReactRender	50	130	100	18	8
		getState	5	62	14	15	7
		initialActions	0	1	0	0	0
		loadScripts	1251	1870	1491	131	63
		setupStore	12	77	36	25	12
		uiStartup	1980	2559	2263	161	77

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

background: 3.1 KiB (0.06%)
ui: 1.13 KiB (0.01%)
common: 284 Bytes (0.00%)

Gudahtt · 2024-10-31T19:52:31Z

ui/pages/confirmations/components/confirm/info/approve/hooks/use-approve-token-simulation.ts

@@ -15,6 +15,15 @@ function isSpendingCapUnlimited(decodedSpendingCap: number) {
  return decodedSpendingCap >= UNLIMITED_THRESHOLD;
 }

+function toNonScientificString(num: number): string {


Note for other reviewers, this function was ported here from this other recent PR: #27992

This will create a conflict when we merge this back into develop; we'll need to update this file to import this function instead if using it inlined here (i.e. we'll need to accept the develop version when it conflicts, as we typically would).

Gudahtt

LGTM!

hjetpoluru · 2024-10-31T19:53:50Z

Is this needed as it was not present in the original PR merged to develop.

Gudahtt · 2024-10-31T19:59:17Z

Thanks for highlighting @hjetpoluru; the extra changes are to bring in a function that the cherry-picked code depends upon. I left a comment on this here: #28183 (comment)

## **Description** Previously there was a bug that affected the approve screen. When users had a small spending cap (between 0.001 and 0.0001 or smaller), it was coerced to 0. This was caused by the method `new Intl.NumberFormat(locale).format(spendingCap)` that applied the `1,000` large number formatting, so the fix is to bypass it entirely for values smaller than 1. Additionally, these unformatted small numbers are presented in scientific notation, so we leverage `toNonScientificString(spendingCap)` to prevent that.  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28179?quickstart=1) ## **Related issues** Fixes: [#28117](#28117) ## **Manual testing steps** See original bug report. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] 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. ## **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.

metamaskbot · 2024-10-31T20:39:23Z

Builds ready [3e3e10f]

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, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2233 ± 124 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	310	3253	2142	521	250
		domContentLoaded	1875	2755	2181	227	109
		load	1891	2934	2233	259	124
		domInteractive	33	131	60	21	10
		backgroundConnect	10	204	46	56	27
		firstReactRender	61	301	123	50	24
		getState	6	486	50	103	49
		initialActions	0	0	0	0	0
		loadScripts	1355	2105	1632	194	93
		setupStore	13	331	57	70	34
		uiStartup	2099	4106	2557	460	221

Bundle size diffs [🚀 Bundle size reduced!]

background: -168.37 KiB (-3.37%)
ui: 4.42 KiB (0.06%)
common: 107.58 KiB (1.36%)

metamaskbot · 2024-10-31T22:12:31Z

No release label on PR. Adding release label release-12.6.0 on PR, as PR was cherry-picked in branch 12.6.0.

pedronfigueiredo added the team-confirmations Push issues to confirmations team label Oct 30, 2024

pedronfigueiredo self-assigned this Oct 30, 2024

pedronfigueiredo requested a review from a team as a code owner October 30, 2024 15:01

DDDDDanica previously approved these changes Oct 30, 2024

View reviewed changes

pedronfigueiredo dismissed DDDDDanica’s stale review via ad45e56 October 30, 2024 15:17

Gudahtt reviewed Oct 31, 2024

View reviewed changes

Gudahtt approved these changes Oct 31, 2024

View reviewed changes

pedronfigueiredo added 2 commits October 31, 2024 17:37

Add toNonScientificString

3e3e10f

Gudahtt force-pushed the pnf/cherry-pick-28179 branch from ad45e56 to 3e3e10f Compare October 31, 2024 20:07

danjm merged commit f644102 into Version-v12.6.0 Oct 31, 2024
72 checks passed

danjm deleted the pnf/cherry-pick-28179 branch October 31, 2024 21:49

github-actions bot locked and limited conversation to collaborators Oct 31, 2024

metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cherry-pick: Prevent coercing small spending caps to zero (#28179) #28183

fix: cherry-pick: Prevent coercing small spending caps to zero (#28179) #28183

pedronfigueiredo commented Oct 30, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 30, 2024

metamaskbot commented Oct 30, 2024

Gudahtt Oct 31, 2024

Gudahtt left a comment

hjetpoluru commented Oct 31, 2024

Gudahtt commented Oct 31, 2024

metamaskbot commented Oct 31, 2024

metamaskbot commented Oct 31, 2024

fix: cherry-pick: Prevent coercing small spending caps to zero (#28179) #28183

fix: cherry-pick: Prevent coercing small spending caps to zero (#28179) #28183

Conversation

pedronfigueiredo commented Oct 30, 2024 • edited by github-actions bot Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 30, 2024

metamaskbot commented Oct 30, 2024

Gudahtt Oct 31, 2024

Choose a reason for hiding this comment

Gudahtt left a comment

Choose a reason for hiding this comment

hjetpoluru commented Oct 31, 2024

Gudahtt commented Oct 31, 2024

metamaskbot commented Oct 31, 2024

metamaskbot commented Oct 31, 2024

pedronfigueiredo commented Oct 30, 2024 •

edited by github-actions bot

Loading