feat: Add redesigned ERC20 Approve confirmation and SpendingCap section by pedronfigueiredo · Pull Request #26606 · MetaMask/metamask-extension

pedronfigueiredo · 2024-08-22T11:20:24Z

Description

Introduces support for ERC20 token transactions with the approve type. This includes a new spending cap section and bespoke logic in the "static simulation" section. Above a certain threshold, spending caps are represented as "Unlimited".

Related issues

Fixes: #2924

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-08-22T11:20:37Z

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/confirm/info/approve/hooks/use-approve-token-simulation.ts

ui/pages/confirmations/components/confirm/info/approve/hooks/use-is-nft.ts

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

ui/pages/confirmations/components/confirm/info/approve/approve.tsx

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

ui/pages/confirmations/components/confirm/info/approve/approve.tsx

test/e2e/tests/confirmations/transactions/erc20-increase-allowance-redesign.spec.ts

bschorchit · 2024-08-26T21:36:02Z

@pedronfigueiredo for the erc20 approve and increaseAllowance, can we update the subtitle copy and the copy within the estimated changes to match the one here? Ty 💛

Subtitle
Update to: This site wants permission to spend your tokens.

Estimated changes
Update to: You’re giving someone else permission to spend this amount from your account.

Key row within Estimated changes
Update to: Spending cap

metamaskbot · 2024-08-27T09:55:44Z

Builds ready [f86909d]

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, 11, 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 (81 ± 12 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	76	203	111	30	14
		domContentLoaded	51	164	77	25	12
		load	58	165	81	24	12
		domInteractive	19	101	33	20	9

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

background: 0 Bytes (0.00%)
ui: 6.47 KiB (0.09%)
common: 261 Bytes (0.00%)

codecov · 2024-08-27T09:56:35Z

Codecov Report

Attention: Patch coverage is 56.81818% with 38 lines in your changes missing coverage. Please review.

Project coverage is 70.06%. Comparing base (f354888) to head (50a7441).
Report is 77 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ations/components/confirm/info/approve/approve.tsx	17.95%	32 Missing ⚠️
...s/confirmations/components/confirm/title/title.tsx	53.85%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #26606      +/-   ##
===========================================
- Coverage    70.08%   70.06%   -0.02%     
===========================================
  Files         1414     1417       +3     
  Lines        49328    49407      +79     
  Branches     13781    13811      +30     
===========================================
+ Hits         34568    34615      +47     
- Misses       14760    14792      +32

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

sonarqubecloud · 2024-08-29T09:59:02Z

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-08-29T10:34:49Z

Builds ready [50a7441]

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, 11, 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 (1793 ± 80 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	321	2303	1730	362	174
		domContentLoaded	1582	2290	1775	168	81
		load	1594	2300	1793	167	80
		domInteractive	23	238	49	48	23

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

background: 0 Bytes (0.00%)
ui: 5.69 KiB (0.08%)
common: 261 Bytes (0.00%)

jpuri · 2024-08-29T14:16:23Z

ui/pages/confirmations/components/confirm/info/approve/approve.tsx

+      }
+      editIconClassName="edit-spending-cap"
+    />
+  );


Putting JSX in variable is INHO not great idea, any reason for not including it inline.

Yes, the idea is to reduce code duplication in the following code block

<ConfirmInfoRow label={t('spendingCap')} tooltip={t('spendingCapTooltipDesc')} > {tokenAmount === UNLIMITED_MSG ? ( <Tooltip title={formattedTokenNum}>{SpendingCapElement}</Tooltip> ) : ( SpendingCapElement )} </ConfirmInfoRow>

This was changed to address feedback on this comment #26606 (comment)

matthewwalsh0 · 2024-09-02T07:49:32Z

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

+  symbol: string;
+};
+
+export const useReceivedToken = () => {


Can this file now be deleted?

will be removed in the next PR

matthewwalsh0 · 2024-09-02T07:50:43Z

ui/pages/confirmations/components/confirm/title/title.tsx

      return t('confirmTitleTransaction');
    case TransactionType.tokenMethodApprove:
-      return t('confirmTitleApproveTransaction');
+      if (isNFT) {


Minor, ternary?

keeping it as is for now since it's the pattern in use in this file

matthewwalsh0 · 2024-09-02T07:53:22Z

ui/pages/confirmations/components/confirm/info/approve/hooks/use-is-nft.ts

+    );
+  }, [transactionMeta]);
+
+  const isNFT = value?.standard !== TokenStandard.ERC20;


Minor, I appreciate we're only using this for labels currently, but one to keep an eye on as some wouldn't consider an ERC-1155 an NFT if it supported multiple of a token ID.

matthewwalsh0 · 2024-09-02T07:55:16Z

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

+
+  const decodedSpendingCap = value
+    ? new BigNumber(value.data[0].params[1].value)
+        .dividedBy(new BigNumber(10).pow(Number(decimals)))


Minor, not the problem of this PR but we do this math a lot so a applyDecimals util or similar would be nice.

to be addressed in a later PR

pedronfigueiredo added the team-confirmations Push issues to confirmations team label Aug 22, 2024

pedronfigueiredo self-assigned this Aug 22, 2024

pedronfigueiredo requested review from a team as code owners August 22, 2024 11:20

pedronfigueiredo force-pushed the pnf/2924 branch 4 times, most recently from fd15e88 to e0713a6 Compare August 22, 2024 14:05

matthewwalsh0 requested changes Aug 22, 2024

View reviewed changes

pedronfigueiredo changed the title ~~feat: Add redesigned ERC20 Approve confirmation~~ feat: Add redesigned ERC20 Approve confirmation and SpendingCap section Aug 23, 2024

pedronfigueiredo force-pushed the pnf/2924 branch 2 times, most recently from d3260a4 to 8c3d68e Compare August 26, 2024 13:13

pedronfigueiredo force-pushed the pnf/2924 branch from 8afe36c to f86909d Compare August 27, 2024 09:17

pedronfigueiredo force-pushed the pnf/2924 branch from c72ec54 to 07b3a7d Compare August 27, 2024 15:31

pedronfigueiredo added 12 commits August 29, 2024 10:19

feat: Redesign Approve confirmation

aa0b077

feat: Redesign Approve confirmation

c3ffa6b

wip

01982fe

address feedback

4548d3d

feat: Add approval static simulation

073e08b

Add redesigned approve and increase allowance confirmations

2e249b8

wip

027aa18

fix linting errors

298148c

fix integration tests

5974004

update snapshot

7881ba9

Update tooltip copy

6c690c2

Fix linting errors in tests

fc22bda

pedronfigueiredo added 12 commits August 29, 2024 10:24

Addressing PR comments

f12d8b6

Addressing PR comments

f248217

Addressing PR comments

4458773

Addressing PR comments

6a94699

fix linting errors

2cfb3c7

fix linting issues

1be1f93

update simulation details heading

326f84d

remove unused import

1285560

Update logic for rendering spending cap component

7ec2234

fix linting error

56c62c3

fix linting error

02dce37

fix linting error

1eb241f

pedronfigueiredo force-pushed the pnf/2924 branch from 07b3a7d to 1eb241f Compare August 29, 2024 09:24

Update test file

50a7441

jpuri reviewed Aug 29, 2024

View reviewed changes

matthewwalsh0 approved these changes Sep 2, 2024

View reviewed changes

jpuri approved these changes Sep 2, 2024

View reviewed changes

pedronfigueiredo merged commit 4844619 into develop Sep 2, 2024

pedronfigueiredo deleted the pnf/2924 branch September 2, 2024 09:46

github-actions bot locked and limited conversation to collaborators Sep 2, 2024

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

gauthierpetetin added release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 11, 2024

Conversation

pedronfigueiredo commented Aug 22, 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 Aug 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bschorchit commented Aug 26, 2024

Uh oh!

metamaskbot commented Aug 27, 2024

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Aug 29, 2024

Quality Gate passed

Uh oh!

metamaskbot commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pedronfigueiredo commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading