Added navigation between multiple sign prompts and reject all sign prompts #17093

VSaric · 2023-01-06T12:51:07Z

Explanation

With this fix, the users should be able to navigate between multiple sign prompts back and forth and should be able to reject all sign prompts at once for Eth Sign, Personal Sign, SignTypedData, SignTypedData_v3 and SignTypedData_v4. Also we are using a single component for navigation which will show both messages and transactions requests.

Fixes [Bug]: Cannot navigate through multiple Sign prompts for Signed Typed Data V3 and V4 #16848
Blocked by Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822 (ConfirmPageContainerNavigation refactor as it mentioned here)

Screenshots/Screencaps

Before

206234177-845cbb7e-8454-47c6-bcbd-4daedb292ebd.mp4

After

popup view

Screen.Recording.2023-01-06.at.13.58.27.mov

expanded view

Screen.Recording.2023-01-06.at.13.30.06.mov

Using a single component for navigation which will show both messages and transactions requests and visa versa

Screen.Recording.2023-01-25.at.12.29.57.mov

Manual Testing Steps

Connect to Test Dapp https://metamask.github.io/test-dapp/
Click multiple times on Eth Sign or Personal Sign or SignTypedData or SignTypedData_v3or SignTypedData_v4.
The users should be able to navigate between multiple sign prompts back and forth and should be able to reject all sign prompts at once

Using a single component for navigation which will show both messages and transactions requests and visa versa

Connect to Test Dapp https://metamask.github.io/test-dapp/
Click multiple times on Piggy bank contract section (Deploy Contract) or Send Tokens section (Create Token) or Send Eth section (Send Legacy Transaction or Send EIP 1559 Transaction) and than click multiple times on Eth Sign or Personal Sign or SignTypedData or SignTypedData_v3or SignTypedData_v4.
The users should be able to navigate between multiple sign prompts and transactions back and forth

github-actions · 2023-01-06T12:51:19Z

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 · 2023-01-06T13:09:49Z

Builds ready [c403d4e]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1639 ± 164 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	102	1405	195	278	134
		domContentLoaded	1156	2748	1615	343	165
		load	1156	2748	1639	342	164
		domInteractive	1156	2748	1615	343	165

Bundle size diffs

background: 0 bytes
ui: 3294 bytes
common: 0 bytes

highlights:

storybook

metamaskbot · 2023-01-09T10:43:56Z

Builds ready [cb3cf68]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1540 ± 131 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	111	170	133	16	8
		domContentLoaded	1174	2120	1530	265	127
		load	1174	2170	1540	272	131
		domInteractive	1174	2120	1530	265	127

Bundle size diffs

background: 0 bytes
ui: 3266 bytes
common: 0 bytes

highlights:

storybook

dzfjz · 2023-01-09T10:53:32Z

Verified by QA

metamaskbot · 2023-01-11T09:25:46Z

Builds ready [80c11c3]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1260 ± 76 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	89	293	116	43	21
		domContentLoaded	1021	1656	1251	150	72
		load	1039	1729	1260	157	76
		domInteractive	1021	1656	1251	150	72

Bundle size diffs

background: 0 bytes
ui: 3266 bytes
common: 0 bytes

highlights:

storybook

metamaskbot · 2023-01-11T16:25:33Z

Builds ready [53df20e]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1521 ± 113 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	102	177	140	18	8
		domContentLoaded	1244	2105	1502	239	115
		load	1264	2105	1521	236	113
		domInteractive	1244	2105	1502	239	115

Bundle size diffs

background: 0 bytes
ui: 2522 bytes
common: 0 bytes

highlights:

storybook

metamaskbot · 2023-01-13T14:53:40Z

Builds ready [fa3cda9]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1214 ± 90 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	87	126	104	11	5
		domContentLoaded	1074	1740	1197	159	76
		load	1074	1850	1214	186	90
		domInteractive	1074	1740	1197	159	76

Bundle size diffs

background: 0 bytes
ui: 2522 bytes
common: 0 bytes

highlights:

storybook

metamaskbot · 2023-01-16T09:57:52Z

Builds ready [81fe32b]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1179 ± 52 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	91	131	108	12	6
		domContentLoaded	1020	1507	1176	109	53
		load	1039	1507	1179	109	52
		domInteractive	1020	1507	1176	109	53

Bundle size diffs

background: 0 bytes
ui: 2522 bytes
common: 0 bytes

highlights:

storybook

jpuri

Changes in confirm-page-container-navigation.component.js can cause unapproved signature request to be opened from transaction page and visa versa.

An option could be creating separate component for signatures. This new component and confirm-page-container-navigation.component.js can be refactored to use same dumb component for UI rendering.

seaona · 2023-01-19T10:33:13Z

Some behaviour to double-check related to mixing Signatures with Sends/Deploys tx's following up @jpuri comment: after prompting for a couple of signatures without accepting them, if the Dapp now sends Send/Deploy tx's requests, MM starts to behave strangely. Issues:

after some pending Signatures, you try a couple of Sends, and the top navigation displays now the number of pending Sends, but with the Signatures screens. Imagine you had 3 pending Signatures and 2 pending Sends: you'll see navigation for the Sends (2), but the screen for Signatures
if you then navigate to the Next page, you are now seeing the pending Sends with their navigation and the Signatures won't appears anymore until you reject/accept the pending Sends.

sign-reqs-tx-reqs.mp4

I am not sure what should be the expect behaviour though:

are we aiming to consider all Dapp API calls into one navigation flow? I think that would be good for the usability, but not sure how complex might be to implement, depending on how the code is at the moment
if we want to separate the different Dapp requests, we should fix the navigation mix and the clicking Next, making switch from pending Signatures to pending Sends

VSaric · 2023-01-19T15:17:27Z

Changes in confirm-page-container-navigation.component.js can cause unapproved signature request to be opened from transaction page and visa versa.

An option could be creating separate component for signatures. This new component and confirm-page-container-navigation.component.js can be refactored to use same dumb component for UI rendering.

You are right @jpuri! I created separate component for signatures and this new component for signatures and confirm-page-container-navigation.component.js are refactored now. This two components are using same component for UI rendering (in this case navigation.js).
I will appreciate your re-review on this and if there is some new suggestions please comment and I will address them. You can check latest changes here.

VSaric · 2023-01-25T12:25:52Z

Hey @VSaric : me and @bschorchit had chat about this and we decided that this approach will be better:

are we aiming to consider all Dapp API calls into one navigation flow? I think that would be good for the usability, but not sure how complex might be to implement, depending on how the code is at the moment

It seems that this will increase your effort in the task and you almost need to re-do it. Can you plz check this.

We are aiming to have a single component for navigation which will show both messages and transactions and any other DAPP request.

@jpuri @bschorchit @seaona I added a single component for navigation which will show both messages and transactions requests. Here are those changes. You can also check in the PR description, I attached screencast with latest changes. I will appreciate your re-review on this. Thanks!

jpuri · 2023-01-25T13:26:59Z

Hey @VSaric : code changes look great. Will be n ice to have some more test coverage also.

dzfjz · 2023-01-26T09:25:00Z

Verified by QA

…okens

…ompts

…endering part into a single component

…ransactions requests

VSaric · 2023-01-30T10:12:36Z

Hey @VSaric : code changes look great. Will be n ice to have some more test coverage also.

Hey @jpuri, I added more unit tests.

metamaskbot · 2023-01-30T10:53:43Z

Builds ready [621269d]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1193 ± 94 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	151	115	13	6
		domContentLoaded	990	1578	1176	195	94
		load	1004	1579	1193	195	94
		domInteractive	990	1578	1176	195	94

Bundle size diffs

background: 0 bytes
ui: 2385 bytes
common: 0 bytes

jpuri · 2023-01-31T05:27:08Z

...e-container/confirm-page-container-navigation/confirm-page-container-navigation.component.js

+      history.push(
+        uncofirmedTransactions[txId]?.msgParams
+          ? `${CONFIRM_TRANSACTION_ROUTE}/${txId}${SIGNATURE_REQUEST_PATH}`
+          : `${CONFIRM_TRANSACTION_ROUTE}/${txId}`,


I think if you route all request to ${CONFIRM_TRANSACTION_ROUTE}/${txId} it takes care to re-route to specific page depending on request type and should work for any confirmation request in fact, but above also looks good 👍

jpuri

Great work 👍

metamaskbot · 2023-01-31T15:28:13Z

Builds ready [ba47ee3]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1564 ± 158 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	92	177	140	19	9
		domContentLoaded	994	2157	1526	331	159
		load	1065	2183	1564	328	158
		domInteractive	994	2157	1526	331	159

Bundle size diffs

background: 0 bytes
ui: 2385 bytes
common: 0 bytes

VSaric self-assigned this Jan 6, 2023

VSaric changed the base branch from develop to fix/navigate-through-multiple-unapproved-approved-transactions January 6, 2023 13:24

filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 094315d to 8220055 Compare January 9, 2023 09:09

VSaric changed the base branch from fix/navigate-through-multiple-unapproved-approved-transactions to develop January 9, 2023 09:12

VSaric changed the base branch from develop to fix/navigate-through-multiple-unapproved-approved-transactions January 9, 2023 09:34

VSaric changed the base branch from fix/navigate-through-multiple-unapproved-approved-transactions to develop January 9, 2023 09:35

VSaric force-pushed the navigation-and-reject-all-signature-requests branch from c403d4e to cb3cf68 Compare January 9, 2023 09:47

VSaric marked this pull request as ready for review January 9, 2023 12:25

VSaric requested a review from a team as a code owner January 9, 2023 12:25

VSaric requested review from PeterYinusa and danjm January 9, 2023 12:25

VSaric force-pushed the navigation-and-reject-all-signature-requests branch from 80c11c3 to 53df20e Compare January 11, 2023 15:54

VSaric requested a review from brad-decker January 12, 2023 14:47

VSaric force-pushed the navigation-and-reject-all-signature-requests branch from 53df20e to 2c924a0 Compare January 12, 2023 16:21

VSaric requested a review from jpuri January 13, 2023 14:50

VSaric force-pushed the navigation-and-reject-all-signature-requests branch from fa3cda9 to 81fe32b Compare January 16, 2023 09:30

pedronfigueiredo previously approved these changes Jan 17, 2023

View reviewed changes

jpuri requested changes Jan 17, 2023

View reviewed changes

VSaric dismissed pedronfigueiredo’s stale review via d929dde January 19, 2023 15:11

VSaric force-pushed the navigation-and-reject-all-signature-requests branch from 81fe32b to d929dde Compare January 19, 2023 15:11

This was referenced Jan 24, 2023

Add "<<" and ">>" icons #17367

Closed

[Figma] Create "<<" and ">>" icons MetaMask/design-tokens#283

Closed

Add "<<" and ">>" icons MetaMask/metamask-mobile#5584

Closed

VSaric force-pushed the navigation-and-reject-all-signature-requests branch from b84b9ae to 1f59ce3 Compare January 25, 2023 12:01

filipsekulic and others added 11 commits January 30, 2023 10:54

Fixed navigation through multiple unapproved transactions for ERC20 t…

a430ef0

…okens

Fixed tx details activity-log currency

12b69e7

Fixed e2e test failure

b722f2d

Added navigation between multiple sign prompts and reject all sign pr…

f6a5f70

…ompts

Resolving conflicts

0cab276

Creating SignatureRequestNavigation component and extracting the UI r…

3bf0bb9

…endering part into a single component

Fixing e2e tests and updating snapshot

f0d17fd

Using single component for navigation which shows both messages and t…

94a44c3

…ransactions requests

Fixing test-unit-jest-main

5514a44

Added more unit tests

2c51f16

Fixing test-storybook

8bc9ed8

VSaric force-pushed the navigation-and-reject-all-signature-requests branch from 6fa54d2 to 8bc9ed8 Compare January 30, 2023 10:03

Fixing test-storybook

621269d

jpuri reviewed Jan 31, 2023

View reviewed changes

jpuri approved these changes Jan 31, 2023

View reviewed changes

pedronfigueiredo approved these changes Jan 31, 2023

View reviewed changes

Merge branch 'develop' into navigation-and-reject-all-signature-requests

ba47ee3

VSaric merged commit 8aa3263 into develop Jan 31, 2023

VSaric deleted the navigation-and-reject-all-signature-requests branch January 31, 2023 15:29

github-actions bot locked and limited conversation to collaborators Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added navigation between multiple sign prompts and reject all sign prompts #17093

Added navigation between multiple sign prompts and reject all sign prompts #17093

VSaric commented Jan 6, 2023 •

edited

Loading

github-actions bot commented Jan 6, 2023

metamaskbot commented Jan 6, 2023

metamaskbot commented Jan 9, 2023

dzfjz commented Jan 9, 2023

metamaskbot commented Jan 11, 2023

metamaskbot commented Jan 11, 2023

metamaskbot commented Jan 13, 2023

metamaskbot commented Jan 16, 2023

jpuri left a comment •

edited

Loading

seaona commented Jan 19, 2023 •

edited

Loading

VSaric commented Jan 19, 2023

VSaric commented Jan 25, 2023 •

edited

Loading

jpuri commented Jan 25, 2023

dzfjz commented Jan 26, 2023

VSaric commented Jan 30, 2023

metamaskbot commented Jan 30, 2023

jpuri Jan 31, 2023 •

edited

Loading

jpuri left a comment

metamaskbot commented Jan 31, 2023

Added navigation between multiple sign prompts and reject all sign prompts #17093

Added navigation between multiple sign prompts and reject all sign prompts #17093

Conversation

VSaric commented Jan 6, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Before

After

popup view

expanded view

Using a single component for navigation which will show both messages and transactions requests and visa versa

Manual Testing Steps

Using a single component for navigation which will show both messages and transactions requests and visa versa

github-actions bot commented Jan 6, 2023

metamaskbot commented Jan 6, 2023

highlights:

metamaskbot commented Jan 9, 2023

highlights:

dzfjz commented Jan 9, 2023

metamaskbot commented Jan 11, 2023

highlights:

metamaskbot commented Jan 11, 2023

highlights:

metamaskbot commented Jan 13, 2023

highlights:

metamaskbot commented Jan 16, 2023

highlights:

jpuri left a comment • edited Loading

Choose a reason for hiding this comment

seaona commented Jan 19, 2023 • edited Loading

VSaric commented Jan 19, 2023

VSaric commented Jan 25, 2023 • edited Loading

jpuri commented Jan 25, 2023

dzfjz commented Jan 26, 2023

VSaric commented Jan 30, 2023

metamaskbot commented Jan 30, 2023

jpuri Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

jpuri left a comment

Choose a reason for hiding this comment

metamaskbot commented Jan 31, 2023

VSaric commented Jan 6, 2023 •

edited

Loading

jpuri left a comment •

edited

Loading

seaona commented Jan 19, 2023 •

edited

Loading

VSaric commented Jan 25, 2023 •

edited

Loading

jpuri Jan 31, 2023 •

edited

Loading