Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

filipsekulic · 2022-12-06T15:03:22Z

Explanation

Fixed navigation through multiple unapproved transactions for ERC20 tokens.

Fixes [Bug]: Users cannot navigate through multiple unapproved approve transactions #16768

Screenshots/Screencaps

Before

Before.mov

After

After.mov

Manual Testing Steps

Navigate to the test dapp
Connect your wallet
Deploy the ERC20 token
Click APPROVE TOKENS multiple times
Navigate back to the wallet

github-actions · 2022-12-06T15:03:38Z

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.

mirjanaKukic · 2022-12-07T13:40:35Z

Verified by QA

jpuri

Look good 👍

metamaskbot · 2022-12-13T14:12:41Z

Builds ready [b8553f9]

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

Page Load Metrics (2233 ± 140 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	113	176	131	14	7
		domContentLoaded	1714	2475	2179	195	94
		load	1714	3177	2233	292	140
		domInteractive	1714	2475	2179	195	94

Bundle size diffs

background: 0 bytes
ui: 1324 bytes
common: 0 bytes

highlights:

storybook

brad-decker · 2022-12-13T17:03:35Z

ui/pages/token-allowance/token-allowance.js

+  const network = hexToDecimal(fullTxData.chainId);
+
+  const currentNetworkUnapprovedTxs = Object.keys(unapprovedTxs)
+    .filter((key) =>
+      transactionMatchesNetwork(
+        unapprovedTxs[key],
+        fullTxData.chainId,
+        network,
+      ),
+    )
+    .reduce((acc, key) => ({ ...acc, [key]: unapprovedTxs[key] }), {});
+
+  const enumUnapprovedTxs = Object.keys(currentNetworkUnapprovedTxs);
+  const currentPosition = enumUnapprovedTxs.indexOf(
+    fullTxData.id ? fullTxData.id.toString() : '',
+  );
+
+  const totalTx = enumUnapprovedTxs.length;
+  const positionOfCurrentTx = currentPosition + 1;
+  const nextTxId = enumUnapprovedTxs[currentPosition + 1];
+  const prevTxId = enumUnapprovedTxs[currentPosition - 1];
+  const showNavigation = enumUnapprovedTxs.length > 1;
+  const firstTx = enumUnapprovedTxs[0];
+  const lastTx = enumUnapprovedTxs[enumUnapprovedTxs.length - 1];
+
+  const handleNextTx = (txId) => {
+    if (txId) {
+      dispatch(clearConfirmTransaction());
+      history.push(`${CONFIRM_TRANSACTION_ROUTE}/${txId}`);
+    }
+  };


I don't think this is problematic but what I'm wondering is if there's an opportunity to improve the readability of our code by relocating and consolidating this logic and the 'confirm-transaction-base.container.js' files logic and move it into the <ConfirmPageContainerNavigation> component which appears to already be a functional component. Doing this would make that specific part of our code much more understandable and perhaps prevent needing to do this selector logic in multiple places.

Could you take a look into this and see how much effort it'll be to consolidate? Thanks @filipsekulic

Hey @brad-decker! Good suggestion!
I refactored the code and you can see the changes here: f4b0938
However, there is a problem and you can see it in the attached screenshot, which is the reason why the pipeline is failing (e2e test).
This happens when there are multiple token approvals and when a user confirms one of them, the navigation disappears for some reason and throws the error (screenshot attached). I think the problem is in the confirm-page-container.component.js within the async componentDidMount, which the error message is also suggesting (...cancel all subscriptions and asynchronous tasks...).
I would appreciate if you could take a look and give your opinion on this one.

danjm · 2022-12-20T06:04:53Z

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

+  const txData = useSelector(getTxData);
+
+  const network = hexToDecimal(txData.chainId);
+


The "Navigate transactions - should confirm and remove an unapproved transaction" e2e test is failing because: TimeoutError: Waiting for element to be located By(xpath, .//*[contains(concat(' ', normalize-space(./@class), ' '), ' confirm-page-container-navigation ')][(contains(string(.), '1 of 3') or contains(string(.), '1of3'))])

This probably means that during the e2e test run showNavigation is false, which probably means that enumUnapprovedTxs is an empty array. Looking at this new code for getting currentNetworkUnapprovedTxs compared to the old code in confirm-transaction-base.container.js, I notice that the chainId and network are here derived from the transaction but in confirm-transaction-base.container.js they are taken directly from state.metamask.

If this feature is working when running it manually in the browser, for both custom approvals and erc-20 token transfers, then the problem is likely in e2e test fixtures (unless there is something wrong with getting the chainId and network from the transaction?... I don't think so, but worth a double check)

You are right @danjm. Good catch!
I fixed that and also made a small change that solved the problem, so now txData is being passed from a parent component.
You can check it here: 1d005ae

why did you have to change it so that txData is being passed in, instead of being selected? what problem did that solve?

When a user tries to confirm one of the transactions, it now shows the navigation, but doesn't get updated as it should - txData object being selected becomes empty, next tx cannot be seen on the next page and currentPosition is -1 as the currentTx cannot be found from enumUnapprovedTxs, because there is no txId from txData object which is empty at that moment.
Screencast:

Screen.Recording.2022-12-20.at.16.05.36.mov

txData is derived from state in confirm-transaction-base.container.js which means we can get it here without drilling the prop down the line

I get it, but the problem with it is that somehow the txData gets empty when one of the transactions is confirmed.
You can see it in the above attached video where txDataFromSelector is actually the transaction data derived from state. I console.log-ged it in order to show this problem...

what was the code for getting txDataFromSelector? was it derived from state in the exact same way as it is in confirm-transaction-base.container.js?

metamaskbot · 2022-12-20T11:59:22Z

Builds ready [344d0df]

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

Page Load Metrics (2226 ± 369 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	1785	195	365	175
		domContentLoaded	1597	5476	2209	771	370
		load	1664	5477	2226	768	369
		domInteractive	1597	5476	2209	771	370

Bundle size diffs

background: 0 bytes
ui: 726 bytes
common: 0 bytes

highlights:

storybook

metamaskbot · 2022-12-20T13:36:06Z

Builds ready [1d005ae]

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

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	212	126	27	13
		domContentLoaded	1805	2484	2093	171	82
		load	1805	2484	2115	179	86
		domInteractive	1805	2484	2093	171	82

Bundle size diffs

background: 0 bytes
ui: 722 bytes
common: 0 bytes

highlights:

storybook

ui/pages/token-allowance/token-allowance.stories.js

brad-decker · 2022-12-21T19:07:25Z

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

+  const txData = useSelector(getTxData);
+
+  const network = hexToDecimal(txData.chainId);
+


txData is derived from state in confirm-transaction-base.container.js which means we can get it here without drilling the prop down the line

metamaskbot · 2022-12-27T09:47:37Z

Builds ready [094315d]

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 (1455 ± 93 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	97	161	122	16	7
		domContentLoaded	1185	1888	1430	178	85
		load	1185	1976	1455	195	93
		domInteractive	1185	1888	1430	178	85

Bundle size diffs

background: 0 bytes
ui: 722 bytes
common: 0 bytes

highlights:

storybook

ui/pages/token-allowance/token-allowance.stories.js

…okens

metamaskbot · 2023-01-09T14:15:41Z

Builds ready [453de3c]

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 (1627 ± 157 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	105	168	133	16	8
		domContentLoaded	1233	2376	1624	330	159
		load	1233	2376	1627	327	157
		domInteractive	1233	2376	1624	330	159

Bundle size diffs

background: 0 bytes
ui: 497 bytes
common: 0 bytes

highlights:

storybook

ui/pages/token-allowance/token-allowance.stories.js

filipsekulic · 2023-01-09T14:21:38Z

@brad-decker
After today's conversation with @danjm and his suggestion, I managed to get the transaction id by using the useParams, so I removed the txData as the component's property and everything seems to work fine.
You can check the changes here: 453de3c

filipsekulic self-assigned this Dec 6, 2022

filipsekulic marked this pull request as ready for review December 7, 2022 14:55

filipsekulic requested a review from a team as a code owner December 7, 2022 14:55

filipsekulic requested a review from danjm December 7, 2022 14:55

bschorchit mentioned this pull request Dec 8, 2022

[Bug]: Cannot navigate through multiple Sign prompts for Signed Typed Data V3 and V4 #16848

Closed

jpuri previously approved these changes Dec 9, 2022

View reviewed changes

PeterYinusa added this to the v10.24.0 milestone Dec 12, 2022

filipsekulic dismissed jpuri’s stale review via b8553f9 December 13, 2022 13:40

filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 991f5d8 to b8553f9 Compare December 13, 2022 13:40

brad-decker suggested changes Dec 13, 2022

View reviewed changes

filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch 2 times, most recently from 4309ec0 to f4b0938 Compare December 15, 2022 08:21

filipsekulic requested a review from brad-decker December 19, 2022 14:04

danjm reviewed Dec 20, 2022

View reviewed changes

filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 344d0df to 1d005ae Compare December 20, 2022 13:05

filipsekulic requested review from danjm and jpuri December 20, 2022 13:19

jpuri previously approved these changes Dec 21, 2022

View reviewed changes

brad-decker suggested changes Dec 21, 2022

View reviewed changes

filipsekulic dismissed jpuri’s stale review via 094315d December 27, 2022 09:22

filipsekulic force-pushed the fix/navigate-through-multiple-unapproved-approved-transactions branch from 1d005ae to 094315d Compare December 27, 2022 09:22

filipsekulic added 3 commits January 9, 2023 10:08

Fixed navigation through multiple unapproved transactions for ERC20 t…

43685b5

…okens

Fixed tx details activity-log currency

5665d5d

Fixed e2e test failure

8220055

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

Fixed a problem with txId

453de3c

filipsekulic requested review from brad-decker and jpuri January 9, 2023 14:21

danjm approved these changes Jan 10, 2023

View reviewed changes

VSaric mentioned this pull request Jan 11, 2023

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

Merged

brad-decker approved these changes Jan 11, 2023

View reviewed changes

brad-decker merged commit fc83a1b into develop Jan 11, 2023

brad-decker deleted the fix/navigate-through-multiple-unapproved-approved-transactions branch January 11, 2023 15:01

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

PeterYinusa added the rc-cherry-picked label Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

filipsekulic commented Dec 6, 2022

github-actions bot commented Dec 6, 2022

mirjanaKukic commented Dec 7, 2022

jpuri left a comment

metamaskbot commented Dec 13, 2022

brad-decker Dec 13, 2022

filipsekulic Dec 15, 2022

danjm Dec 20, 2022

filipsekulic Dec 20, 2022

danjm Dec 20, 2022

filipsekulic Dec 20, 2022

brad-decker Dec 21, 2022

filipsekulic Dec 22, 2022

danjm Jan 6, 2023

metamaskbot commented Dec 20, 2022

metamaskbot commented Dec 20, 2022

brad-decker Dec 21, 2022

metamaskbot commented Dec 27, 2022

metamaskbot commented Jan 9, 2023

filipsekulic commented Jan 9, 2023

		const txData = useSelector(getTxData);

		const network = hexToDecimal(txData.chainId);

Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

Fixed navigation through multiple unapproved transactions for ERC20 tokens #16822

Conversation

filipsekulic commented Dec 6, 2022

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

github-actions bot commented Dec 6, 2022

mirjanaKukic commented Dec 7, 2022

jpuri left a comment

Choose a reason for hiding this comment

metamaskbot commented Dec 13, 2022

highlights:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metamaskbot commented Dec 20, 2022

highlights:

metamaskbot commented Dec 20, 2022

highlights:

Choose a reason for hiding this comment

metamaskbot commented Dec 27, 2022

highlights:

metamaskbot commented Jan 9, 2023

highlights:

filipsekulic commented Jan 9, 2023