feat: Enable redesigned transaction confirmations for all users #28321

pedronfigueiredo · 2024-11-06T12:15:54Z

Description

Migrates all users to the redesigned transactions. The setting will be automatically toggled on.

This change initially broke a number of end-to-end tests. After an analysis, I grouped these failing tests into three categories:

User flows that have to be tested before enabling the redesign to all users, 🔴

This includes multichain and transaction insights tests. All tests on these files were tweaked to pass on redesigned confirmations. Specific tests that initially broke for new confirmations were also duplicated and included the temporary helper method tempToggleSettingRedesignedTransactionConfirmations so they were run for the old confirmations that we still need to support.

User flows that we don't need to migrate immediately, but will have to be migrated or adapted when old confirmations flows are deleted, 🟡

For these tests, we simply added tempToggleSettingRedesignedTransactionConfirmations. Once we remove the old flows, we can modify the tests to support the new flows. We didn't think it was as urgent to test these flows with the redesigned screens.

User flows that we don't need to migrate to redesigned confirmations. 🟢

For these tests, we simply added tempToggleSettingRedesignedTransactionConfirmations. Once we remove the old confirmation screens, we can delete these tests because they are no longer relevant, or otherwise already tested in the redesigned confirmation specific tests.

Summary of the e2e tests that broke with the migration

User flows that have to be tested before enabling the redesign to all users, 🔴
- test/e2e/snaps/test-snap-txinsights-v2.spec.js ✅
- test/e2e/snaps/test-snap-txinsights.spec.js ✅
- test/e2e/json-rpc/switchEthereumChain.spec.js ✅
- test/e2e/tests/request-queuing/batch-txs-per-dapp-diff-network.spec.js ✅
- test/e2e/tests/request-queuing/batch-txs-per-dapp-extra-tx.spec.js ✅
- test/e2e/tests/request-queuing/batch-txs-per-dapp-same-network.spec.js ✅
- test/e2e/tests/request-queuing/dapp1-send-dapp2-signTypedData.spec.js ✅
- test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js ✅
- test/e2e/tests/request-queuing/dapp1-switch-dapp2-send.spec.js ✅
- test/e2e/tests/request-queuing/multi-dapp-sendTx-revokePermission.spec.js ✅
- test/e2e/tests/request-queuing/multiple-networks-dapps-txs.spec.js ✅
- test/e2e/tests/request-queuing/switch-network.spec.js ✅
- test/e2e/tests/request-queuing/ui.spec.js ✅
- test/e2e/tests/account/snap-account-transfers.spec.ts ✅
User flows that we don't need to migrate immediately, but will have to be migrated or adapted when old confirmations flows are deleted, 🟡
- test/e2e/json-rpc/eth_sendTransaction.spec.js
- test/e2e/tests/account/add-account.spec.ts
- test/e2e/tests/petnames/petnames-transactions.spec.js
- test/e2e/tests/ppom/ppom-blockaid-alert-simple-send.spec.js
- test/e2e/tests/responsive-ui/metamask-responsive-ui.spec.js
- test/e2e/tests/tokens/custom-token-send-transfer.spec.js
- test/e2e/tests/transaction/change-assets.spec.js
- test/e2e/tests/transaction/edit-gas-fee.spec.js
- test/e2e/tests/transaction/gas-estimates.spec.js
- test/e2e/tests/transaction/multiple-transactions.spec.js
- test/e2e/tests/transaction/navigate-transactions.spec.js
- test/e2e/tests/transaction/send-edit.spec.js
- test/e2e/tests/transaction/send-eth.spec.js
- test/e2e/tests/transaction/send-hex-address.spec.js
User flows that we don't need to migrate to redesigned confirmations. 🟢
- test/e2e/tests/dapp-interactions/contract-interactions.spec.js
- test/e2e/tests/dapp-interactions/dapp-tx-edit.spec.js
- test/e2e/tests/dapp-interactions/failing-contract.spec.js
- test/e2e/tests/network/network-error.spec.js
- test/e2e/tests/settings/4byte-directory.spec.js
- test/e2e/tests/settings/show-hex-data.spec.js
- test/e2e/tests/tokens/custom-token-add-approve.spec.js
- test/e2e/tests/tokens/increase-token-allowance.spec.js
- test/e2e/tests/transaction/simple-send.spec.ts
- test/e2e/tests/tokens/nft/erc721-interaction.spec.js
- test/e2e/tests/tokens/nft/erc1155-interaction.spec.js
- test/e2e/tests/tokens/nft/send-nft.spec.js

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3026

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-11-06T12:16:07Z

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-11-14T16:27:39Z

Builds ready [806f6d6]

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, 7
- common: 0, 1, 10, 11, 12, 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 (1936 ± 89 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	376	2217	1848	384	184
		domContentLoaded	1587	2199	1913	187	90
		load	1637	2219	1936	185	89
		domInteractive	30	195	74	43	20
		backgroundConnect	8	75	22	20	10
		firstReactRender	46	200	115	37	18
		getState	4	47	12	12	6
		initialActions	0	1	0	0	0
		loadScripts	1153	1690	1411	158	76
		setupStore	5	55	13	15	7
		uiStartup	1858	2493	2147	199	96

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

background: 876 Bytes (0.01%)
ui: 69 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-11-20T15:22:22Z

Builds ready [9365487]

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, 7
- common: 0, 1, 10, 11, 12, 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 (2353 ± 128 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	264	2661	1818	826	397
		domContentLoaded	1924	3138	2318	257	124
		load	1935	3154	2353	266	128
		domInteractive	32	141	52	23	11
		backgroundConnect	10	162	38	37	18
		firstReactRender	62	489	130	103	50
		getState	59	258	121	54	26
		initialActions	0	1	0	0	0
		loadScripts	1425	2472	1742	223	107
		setupStore	6	42	11	7	4
		uiStartup	2216	4250	2776	445	213

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

background: 876 Bytes (0.01%)
ui: 69 Bytes (0.00%)
common: 0 Bytes (0.00%)

digiwand · 2024-11-21T10:21:39Z

test/e2e/helpers.js

+  // fix race condition with mmi build
+  if (process.env.MMI) {
+    await driver.waitForSelector('[data-testid="global-menu-mmi-portfolio"]');
+  }


digiwand · 2024-11-21T10:24:34Z

app/scripts/migrations/132.ts

+    | Record<string, unknown>
+    | undefined;
+
+  if (isObject(preferencesControllerState) && isObject(preferences)) {


I think there's no need to check isObject(preferencesControllerState) since preferences is derived from preferencesControllerState

Suggested change

if (isObject(preferencesControllerState) && isObject(preferences)) {

if (isObject(preferences)) {

digiwand · 2024-11-21T10:29:35Z

app/scripts/migrations/132.ts

+): Record<string, unknown> {
+  const preferencesControllerState = state?.PreferencesController as
+    | Record<string, unknown>
+    | undefined;


We have supported the case when no preferences state is found in previous migrations. It may be a good idea to support it here as well. Then we can ensure we set redesignedTransactionsEnabled for this migration. Maybe it's automatically set to true by default later, in which case we wouldn't need this.

example: migration 122

if (!isObject(state.PreferencesController?.preferences)) { state.PreferencesController = { preferences: {}, }; }

metamaskbot · 2024-11-26T10:18:14Z

Builds ready [7b198b9]

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, 7
- common: 0, 1, 10, 11, 12, 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 (1704 ± 90 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1465	2054	1705	185	89
		domContentLoaded	1453	2047	1678	183	88
		load	1462	2053	1704	187	90
		domInteractive	25	114	47	25	12
		backgroundConnect	7	61	30	20	10
		firstReactRender	17	75	41	24	11
		getState	4	46	10	9	4
		initialActions	0	0	0	0	0
		loadScripts	1016	1553	1220	162	78
		setupStore	6	68	19	19	9
		uiStartup	1647	2266	1896	194	93

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

background: 993 Bytes (0.02%)
ui: 69 Bytes (0.00%)
common: 0 Bytes (0.00%)

addressed

pedronfigueiredo added the team-confirmations Push issues to confirmations team label Nov 6, 2024

pedronfigueiredo self-assigned this Nov 6, 2024

pedronfigueiredo force-pushed the pnf/3026 branch 11 times, most recently from 03d87cf to 806f6d6 Compare November 14, 2024 09:48

pedronfigueiredo force-pushed the pnf/3026 branch 3 times, most recently from 8ece0b5 to 819c05b Compare November 19, 2024 10:26

pedronfigueiredo marked this pull request as ready for review November 19, 2024 14:59

pedronfigueiredo requested review from a team as code owners November 19, 2024 14:59

pedronfigueiredo requested review from kumavis and a team as code owners November 20, 2024 12:20

pedronfigueiredo force-pushed the pnf/3026 branch 2 times, most recently from b8cfa35 to 3a69a65 Compare November 21, 2024 08:38

jpuri previously approved these changes Nov 21, 2024

View reviewed changes

digiwand reviewed Nov 21, 2024

View reviewed changes

pedronfigueiredo added 16 commits November 26, 2024 09:44

Add tests for redesigned flows

721e69b

wip

5fec298

wip

24b0b3c

wip

7c90dc2

wip

d2504e3

Update migration

47d4b32

Rename withRedesignConfirmationFixtures

1913c38

Migrate snap-account-transfers.spec.ts tests

4b26411

Revert test spacing

0ef5fc8

Revert test run timeout

2d2398f

remove unnecessary test

4171b58

fix whitespace

23dea5b

fix linting errors

a419656

Refactor closing the settings panel

e8b230d

fix linting errors

d708e8b

Add comment on accounts tests

7b198b9

pedronfigueiredo dismissed ccharly’s stale review via 7b198b9 November 26, 2024 09:44

pedronfigueiredo force-pushed the pnf/3026 branch from 0f54e15 to 7b198b9 Compare November 26, 2024 09:44

jpuri approved these changes Nov 26, 2024

View reviewed changes

digiwand approved these changes Nov 26, 2024

View reviewed changes

ccharly approved these changes Nov 26, 2024

View reviewed changes

FrederikBolding approved these changes Nov 26, 2024

View reviewed changes

pedronfigueiredo added this pull request to the merge queue Nov 26, 2024

Merged via the queue into develop with commit 6dda444 Nov 26, 2024
75 checks passed

pedronfigueiredo deleted the pnf/3026 branch November 26, 2024 16:25

github-actions bot locked and limited conversation to collaborators Nov 26, 2024

metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable redesigned transaction confirmations for all users #28321

feat: Enable redesigned transaction confirmations for all users #28321

pedronfigueiredo commented Nov 6, 2024 •

edited

Loading

github-actions bot commented Nov 6, 2024

metamaskbot commented Nov 14, 2024

metamaskbot commented Nov 20, 2024

digiwand Nov 21, 2024

digiwand Nov 21, 2024

digiwand Nov 21, 2024

metamaskbot commented Nov 26, 2024

	if (isObject(preferencesControllerState) && isObject(preferences)) {
	if (isObject(preferences)) {

feat: Enable redesigned transaction confirmations for all users #28321

feat: Enable redesigned transaction confirmations for all users #28321

Conversation

pedronfigueiredo commented Nov 6, 2024 • edited Loading

Description

Summary of the e2e tests that broke with the migration

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Nov 6, 2024

metamaskbot commented Nov 14, 2024

metamaskbot commented Nov 20, 2024

digiwand Nov 21, 2024

Choose a reason for hiding this comment

digiwand Nov 21, 2024

Choose a reason for hiding this comment

digiwand Nov 21, 2024

Choose a reason for hiding this comment

metamaskbot commented Nov 26, 2024

pedronfigueiredo commented Nov 6, 2024 •

edited

Loading