feat: NFT token transfer #27955

pedronfigueiredo · 2024-10-18T11:47:17Z

Description

Implements token transfer confirmation for ERC721 and ERC1155 tokens, in the cases where they are wallet initiated and dApp initiated. Includes e2e tests.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3018 https://github.com/MetaMask/MetaMask-planning/issues/3019

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-18T11:47:31Z

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-21T14:12:39Z

Builds ready [cbc3616]

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 (1958 ± 94 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1665	2476	1957	193	93
		domContentLoaded	1653	2464	1922	196	94
		load	1662	2477	1958	196	94
		domInteractive	15	94	49	23	11
		backgroundConnect	10	164	33	39	19
		firstReactRender	46	298	106	63	30
		getState	5	55	16	16	7
		initialActions	0	0	0	0	0
		loadScripts	1241	2059	1455	182	87
		setupStore	11	93	44	26	12
		uiStartup	1861	2671	2215	235	113

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

background: 0 Bytes (0.00%)
ui: 4.83 KiB (0.06%)
common: 88 Bytes (0.00%)

matthewwalsh0 · 2024-10-24T10:02:10Z

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

@@ -33,7 +33,16 @@ const Header = () => {

  const { currentConfirmation } = useConfirmContext<Confirmation>();

-  if (currentConfirmation?.type === TransactionType.tokenMethodTransfer) {
+  const CONFIRMATIONS_WITH_NEW_HEADER = [


Minor, can this go at the top of the file?

And should we use a more explicit name, what does New mean in this context?

matthewwalsh0 · 2024-10-24T10:03:15Z

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

+  if (
+    currentConfirmation?.type &&
+    CONFIRMATIONS_WITH_NEW_HEADER.includes(currentConfirmation.type)
+  ) {


Minor, would it make the logic clearer if we remove the nesting and have flat conditions for each alternate header?

We could even put the fallback into a local component such as DefaultHeader?

metamaskbot · 2024-10-24T14:14:27Z

Builds ready [af00d32]

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 (1831 ± 86 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	301	2484	1773	380	183
		domContentLoaded	1661	2433	1807	171	82
		load	1666	2485	1831	179	86
		domInteractive	17	175	48	33	16
		backgroundConnect	9	92	28	24	11
		firstReactRender	45	200	90	34	16
		getState	4	80	18	23	11
		initialActions	0	1	0	0	0
		loadScripts	1216	1989	1350	163	78
		setupStore	10	73	26	22	11
		uiStartup	1866	2677	2046	216	104

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

background: 0 Bytes (0.00%)
ui: 4.87 KiB (0.06%)
common: 88 Bytes (0.00%)

matthewwalsh0 · 2024-10-24T15:19:26Z

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

-  }
-
-  return (
+  const DefaultHeader = (


Minor, we've defined this as a nested component within the Header component but is it slightly more readable to define it at the top level and either re-use the same hooks or pass in the properties it needs?

matthewwalsh0 · 2024-10-24T15:20:04Z

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

+    (currentConfirmation as TransactionMeta)?.origin === 'metamask';
+  if (isConfirmationWithNewHeader && isWalletInitiated) {
+    return <WalletInitiatedHeader />;
+  } else if (isConfirmationWithNewHeader && !isWalletInitiated) {


Minor, we're using an else here but no need since we're returning in each condition.

matthewwalsh0 · 2024-10-24T15:20:15Z

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

+    currentConfirmation?.type &&
+    CONFIRMATIONS_WITH_NEW_HEADER.includes(currentConfirmation.type);
+  const isWalletInitiated =
+    (currentConfirmation as TransactionMeta)?.origin === 'metamask';


Minor, could we use ORIGIN_METAMASK here?

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

pedronfigueiredo self-assigned this Oct 18, 2024

pedronfigueiredo requested review from a team as code owners October 18, 2024 11:47

pedronfigueiredo force-pushed the pnf/3019 branch from 091a8c8 to 442b449 Compare October 18, 2024 12:52

jpuri previously approved these changes Oct 18, 2024

View reviewed changes

pedronfigueiredo dismissed jpuri’s stale review via 34a5515 October 21, 2024 09:26

pedronfigueiredo force-pushed the pnf/3019 branch 5 times, most recently from 17c115e to cbc3616 Compare October 21, 2024 13:34

matthewwalsh0 previously approved these changes Oct 24, 2024

View reviewed changes

pedronfigueiredo dismissed matthewwalsh0’s stale review via ab2e5f0 October 24, 2024 10:33

pedronfigueiredo force-pushed the pnf/3019 branch from ab2e5f0 to 7cc1b27 Compare October 24, 2024 10:34

jpuri previously approved these changes Oct 24, 2024

View reviewed changes

pedronfigueiredo dismissed jpuri’s stale review via 67c59fb October 24, 2024 13:09

pedronfigueiredo force-pushed the pnf/3019 branch from 7cc1b27 to 67c59fb Compare October 24, 2024 13:09

pedronfigueiredo added 2 commits October 24, 2024 14:15

feat: NFT token transfer

9f73970

wip

af00d32

pedronfigueiredo force-pushed the pnf/3019 branch from 67c59fb to af00d32 Compare October 24, 2024 13:15

jpuri approved these changes Oct 24, 2024

View reviewed changes

matthewwalsh0 approved these changes Oct 24, 2024

View reviewed changes

pedronfigueiredo added this pull request to the merge queue Oct 24, 2024

Merged via the queue into develop with commit cbfd131 Oct 24, 2024
76 checks passed

pedronfigueiredo deleted the pnf/3019 branch October 24, 2024 15:45

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: NFT token transfer #27955

feat: NFT token transfer #27955

pedronfigueiredo commented Oct 18, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Oct 18, 2024

metamaskbot commented Oct 21, 2024

matthewwalsh0 Oct 24, 2024

matthewwalsh0 Oct 24, 2024

metamaskbot commented Oct 24, 2024

matthewwalsh0 Oct 24, 2024

matthewwalsh0 Oct 24, 2024

matthewwalsh0 Oct 24, 2024

feat: NFT token transfer #27955

feat: NFT token transfer #27955

Conversation

pedronfigueiredo commented Oct 18, 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

github-actions bot commented Oct 18, 2024

metamaskbot commented Oct 21, 2024

matthewwalsh0 Oct 24, 2024

Choose a reason for hiding this comment

matthewwalsh0 Oct 24, 2024

Choose a reason for hiding this comment

metamaskbot commented Oct 24, 2024

matthewwalsh0 Oct 24, 2024

Choose a reason for hiding this comment

matthewwalsh0 Oct 24, 2024

Choose a reason for hiding this comment

matthewwalsh0 Oct 24, 2024

Choose a reason for hiding this comment

pedronfigueiredo commented Oct 18, 2024 •

edited by github-actions bot

Loading