Display a warning and gas fee component for token allowance and NFT flow when transaction is expected to fail #17437

VSaric · 2023-01-26T15:11:07Z

Explanation

With this fixes, when MM loads a confirmation screen for a transaction that is expected to fail, it's now displaying the gas component. This has been seen so far on both the NFT approve and the new token allowance confirmation screens. Also when a transaction is expected to fail, it displays a warning. Approving a second time USDT drives to a failed transaction where should display a warning and the gas component.

Further explanation:

The gas component isn't displayed because we aren't set userAcknowledgedGasMissing in GasDetailsItem and EditGasFeeButton component in approve-content-card.js (this is content for token allowance flow) and confirm-approve-content.component.js (this is for NFT flow). Default value for userAcknowledgedGasMissing is false.

Code where default value for userAcknowledgedGasMissing is false in GasDisplayItem component: https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/gas-details-item/gas-details-item.js#L19
Code where it is returning null in GasDisplayItem component: https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/gas-details-item/gas-details-item.js#L35

Code where it is returning null in EditGasFeeButton component: https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/edit-gas-fee-button/edit-gas-fee-button.js#L35

Now we are using renderSimulationFailureWarning to show a warning if it is true and if we want to proceed with a transaction that is expected to fail, we are setting userAcknowledgedGasMissing to true when we click on I want to proceed anyway link and then gas component will displays.

I also changed setshowContractDetails to setShowContractDetails what @darkwing suggested here.

Screenshots/Screencaps

Before

ERC20 token (token allowance flow)

Screen.Recording.2023-01-26.at.16.47.01.mov

ERC721 or ERC1155 token (NFT flow)

Screen.Recording.2023-01-26.at.16.48.24.mov

After

ERC20 token (token allowance flow)

Screen.Recording.2023-01-26.at.15.01.52.mov

ERC721 or ERC1155 token (NFT flow)

Screen.Recording.2023-01-26.at.15.06.14.mov

Manual Testing Steps

ERC20 token (token allowance)

Go to USDT https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#writeContract
Click Write Contract
Connect MM
Click Approve
Set amount and address
Click on Write
Approve on MM
Click again Approve -- this transaction will fail. See how it is displays a warning and when user click on I want to proceed anyway link displays the gas component for this failing transaction (on the second page of token allowance flow)

ERC721 or ERC1155 token (NFT flow)

Go to (erc721) https://etherscan.io/address/0x2a187453064356c898cae034eaed119e1663acb8#writeContract or (erc1155) https://etherscan.io/address/0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d#writeContract
Connect your wallet
Go to approve and input a random address and a random token id (I used 1, it's important that you don't actually own that NFT)
Click on Write
See how it is displays a warning and when user click on I want to proceed anyway link displays the gas component for this failing transaction

github-actions · 2023-01-26T15:11:21Z

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-27T11:17:13Z

Builds ready [41c2404]

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 (1361 ± 109 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	171	131	19	9
		domContentLoaded	1049	1883	1313	228	110
		load	1084	1955	1361	227	109
		domInteractive	1049	1882	1313	228	110

Bundle size diffs

background: 0 bytes
ui: 1382 bytes
common: 0 bytes

danjm · 2023-01-27T11:26:49Z

I think that another fix is required for the confirmation screens other than token allowance and NFT approvals. I have made a PR for that here: #17458

VSaric · 2023-01-27T11:40:11Z

I think that another fix is required for the confirmation screens other than token allowance and NFT approvals. I have made a PR for that here: #17458

Oh I see, sorry for that @danjm! Do you want maybe to delete that line in confirm-transaction-base.component.js in this PR or it would be in separate PR that you opened?

I tried transfer and deprecate from etherscan here: https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#writeContract and if we delete that condition in confirm-transaction-base.component.js it will display a warning message twice. You can see it on screenshots below:

dzfjz · 2023-01-27T13:45:10Z

Verified by QA

metamaskbot · 2023-01-30T15:11:25Z

Builds ready [6070ff2]

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 (1619 ± 138 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	119	284	159	37	18
		domContentLoaded	1135	2043	1593	280	134
		load	1202	2085	1619	288	138
		domInteractive	1135	2043	1593	280	134

Bundle size diffs

background: 0 bytes
ui: 1382 bytes
common: 0 bytes

jpuri · 2023-01-30T23:43:37Z

ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js

+              }
+            />
+          </Box>
+        )}


Might be useful to extract out a small component for above warning display. It seems to be repeated a few places.

@jpuri Done here.

jpuri · 2023-01-30T23:44:46Z

Will be nice to have more test coverage in the PR.

jpuri · 2023-01-30T23:46:41Z

ui/pages/confirm-approve/confirm-approve-content/confirm-approve-content.component.js

+      supportsEIP1559 &&
+      !renderSimulationFailureWarning
+    ) {
+      return <GasDetailsItem userAcknowledgedGasMissing />;


A re-usable hook to get this value renderSimulationFailureWarning might be useful. I see this logic repeated a few places.

@jpuri Done here.

…low when transaction is expected to fail

…nit tests

…ransactions

metamaskbot · 2023-02-01T14:16:05Z

Builds ready [2bfc2ca]

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 (1388 ± 138 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	101	259	136	33	16
		domContentLoaded	1074	2090	1358	286	137
		load	1074	2090	1388	288	138
		domInteractive	1074	2090	1358	286	137

Bundle size diffs

background: 0 bytes
ui: 5094 bytes
common: 0 bytes

…ransactions

ryanml

lgtm!

…ransactions

metamaskbot · 2023-02-01T23:04:07Z

Builds ready [7e71360]

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 (1313 ± 124 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	101	271	141	53	26
		domContentLoaded	954	1786	1281	256	123
		load	972	1895	1313	258	124
		domInteractive	954	1786	1281	256	123

Bundle size diffs

background: 0 bytes
ui: 5094 bytes
common: 0 bytes

jpuri · 2023-02-02T03:57:25Z

ui/components/ui/simulation-error-message/index.js

@@ -0,0 +1 @@
+export { default } from './simulation-error-message';


A better place for this will be ui/components/app

VSaric self-assigned this Jan 26, 2023

VSaric force-pushed the display-gas-fee-and-warning-for-failing-transactions branch from ca8dc8a to 41c2404 Compare January 27, 2023 10:54

danjm mentioned this pull request Jan 27, 2023

Ensure simulation failure warning is shown on all networks and accounts #17458

Merged

VSaric marked this pull request as ready for review January 27, 2023 13:53

VSaric requested a review from a team as a code owner January 27, 2023 13:53

VSaric requested review from Gtonizuka and danjm January 27, 2023 13:53

jpuri reviewed Jan 30, 2023

View reviewed changes

VSaric added 4 commits February 1, 2023 14:02

Display a warning and gas fee component for token allowance and NFT f…

15458af

…low when transaction is expected to fail

Fixing test-storybook and test-unit-jest-main

9554af7

Fixing failing e2e tests

b351f59

Added small component for simulation error, re-usable hook and more u…

cabe23b

…nit tests

VSaric force-pushed the display-gas-fee-and-warning-for-failing-transactions branch from 6070ff2 to cabe23b Compare February 1, 2023 13:05

VSaric requested a review from jpuri February 1, 2023 13:08

Merge branch 'develop' into display-gas-fee-and-warning-for-failing-t…

2bfc2ca

…ransactions

danjm added the release-blocker This bug is blocking the next release label Feb 1, 2023

pedronfigueiredo and others added 2 commits February 1, 2023 19:08

Merge branch 'develop' into display-gas-fee-and-warning-for-failing-t…

044131f

…ransactions

Merge branch 'develop' into display-gas-fee-and-warning-for-failing-t…

0526f9a

…ransactions

danjm approved these changes Feb 1, 2023

View reviewed changes

ryanml approved these changes Feb 1, 2023

View reviewed changes

Merge branch 'develop' into display-gas-fee-and-warning-for-failing-t…

7e71360

…ransactions

brad-decker merged commit 308151c into develop Feb 1, 2023

brad-decker deleted the display-gas-fee-and-warning-for-failing-transactions branch February 1, 2023 23:14

github-actions bot locked and limited conversation to collaborators Feb 1, 2023

jpuri reviewed Feb 2, 2023

View reviewed changes

PeterYinusa added the rc-cherry-picked label Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display a warning and gas fee component for token allowance and NFT flow when transaction is expected to fail #17437

Display a warning and gas fee component for token allowance and NFT flow when transaction is expected to fail #17437

VSaric commented Jan 26, 2023 •

edited

Loading

github-actions bot commented Jan 26, 2023

metamaskbot commented Jan 27, 2023

danjm commented Jan 27, 2023

VSaric commented Jan 27, 2023 •

edited

Loading

dzfjz commented Jan 27, 2023

metamaskbot commented Jan 30, 2023

jpuri Jan 30, 2023

VSaric Feb 1, 2023

jpuri commented Jan 30, 2023

jpuri Jan 30, 2023

VSaric Feb 1, 2023

metamaskbot commented Feb 1, 2023

ryanml left a comment

metamaskbot commented Feb 1, 2023

jpuri Feb 2, 2023

		@@ -0,0 +1 @@
		export { default } from './simulation-error-message';

Display a warning and gas fee component for token allowance and NFT flow when transaction is expected to fail #17437

Display a warning and gas fee component for token allowance and NFT flow when transaction is expected to fail #17437

Conversation

VSaric commented Jan 26, 2023 • edited Loading

Explanation

Screenshots/Screencaps

Before

ERC20 token (token allowance flow)

ERC721 or ERC1155 token (NFT flow)

After

ERC20 token (token allowance flow)

ERC721 or ERC1155 token (NFT flow)

Manual Testing Steps

ERC20 token (token allowance)

ERC721 or ERC1155 token (NFT flow)

github-actions bot commented Jan 26, 2023

metamaskbot commented Jan 27, 2023

danjm commented Jan 27, 2023

VSaric commented Jan 27, 2023 • edited Loading

dzfjz commented Jan 27, 2023

metamaskbot commented Jan 30, 2023

jpuri Jan 30, 2023

Choose a reason for hiding this comment

VSaric Feb 1, 2023

Choose a reason for hiding this comment

jpuri commented Jan 30, 2023

jpuri Jan 30, 2023

Choose a reason for hiding this comment

VSaric Feb 1, 2023

Choose a reason for hiding this comment

metamaskbot commented Feb 1, 2023

ryanml left a comment

Choose a reason for hiding this comment

metamaskbot commented Feb 1, 2023

jpuri Feb 2, 2023

Choose a reason for hiding this comment

VSaric commented Jan 26, 2023 •

edited

Loading

VSaric commented Jan 27, 2023 •

edited

Loading