chore: centralize redesigned confirmation decision logic #28445

cryptotavares · 2024-11-13T17:57:28Z

Description

This PR centralizes the redesigned confirmation decision logic to improve code organization and reduce duplication across the codebase. Currently, redesign confirmation decision logic is scattered across multiple files, making it harder to maintain and more prone to inconsistencies.

Motivation

The existing implementation has several issues:

Duplicate logic for handling confirmation decisions across different transaction types
Prevent inconsistent handling of redesigned confirmation flows

Key changes:

Move supported redesigned confirmation decision logic to the shared dir (to be used both in the ui and app bundles)
Updated signature metrics tracking to support developer mode enabled

Types of changes:

Code style update (changes that do not affect the meaning of the code)

Related issues

Fixes:

Manual testing steps

Ensure that when either the experimental confirmations redesign toggle, developer option confirmations redesign toggle or the env ENABLE_CONFIRMATIONS_REDESIGN is enabled the we are presenting the new redesign confirmations.

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-13T17:57:39Z

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.

sentry-io · 2024-11-13T17:57:53Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: app/scripts/lib/createRPCMethodTrackingMiddleware.js

Function	Unhandled Issue
`finalizeSignatureFragment`	Error: Event fragment with id signature-911707255 does not exist. /sc... `Event Count:` 1 `Affected Users:` 0

📄 File: app/scripts/lib/transaction/metrics.ts (Click to Expand)

Function	Unhandled Issue
`buildEventFragmentProperties`	TypeError: null has no properties /background.html `Event Count:` 20 `Affected Users:` 0
`buildEventFragmentProperties`	SyntaxError: Invalid or unexpected token /scripts... `Event Count:` 7 `Affected Users:` 0
`buildEventFragmentProperties`	TypeError: Cannot destructure property 'name' of '(intermediate value)' as it is null. ... `Event Count:` 5 `Affected Users:` 0
`buildEventFragmentProperties`	TypeError: e.ￇ槌ￔ榫ￅ槝iceModel is not a function /sc... `Event Count:` 4 `Affected Users:` 0

---

_{Did you find this useful? React with a 👍 or 👎}

…rmation-decision-logic

metamaskbot · 2024-11-15T11:44:05Z

Builds ready [7702e54]

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 (1929 ± 125 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	379	2782	1877	437	210
		domContentLoaded	1609	2675	1904	261	125
		load	1662	2712	1929	260	125
		domInteractive	17	114	52	21	10
		backgroundConnect	6	51	21	14	7
		firstReactRender	48	278	114	47	22
		getState	4	82	23	26	13
		initialActions	0	1	0	0	0
		loadScripts	1172	2017	1413	201	96
		setupStore	5	58	17	18	9
		uiStartup	1812	3049	2165	307	147

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

background: 287 Bytes (0.00%)
ui: 1.31 KiB (0.02%)
common: 280 Bytes (0.00%)

shared/modules/confirmation.utils.ts

ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx

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

ui/pages/confirmations/utils/confirm.ts

…rmation-decision-logic

digiwand · 2024-11-20T14:12:13Z

looks like there are a few more lint errors to fix @cryptotavares

jsdoc is not picking up the destruct plus the TS inline type

…rmation-decision-logic

metamaskbot · 2024-11-20T15:48:57Z

Builds ready [746342a]

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 (1976 ± 72 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1720	2274	1977	151	73
		domContentLoaded	1678	2190	1938	138	66
		load	1710	2261	1976	151	72
		domInteractive	30	109	47	18	9
		backgroundConnect	9	113	35	27	13
		firstReactRender	48	303	99	50	24
		getState	51	169	95	23	11
		initialActions	0	1	0	0	0
		loadScripts	1248	1723	1448	126	61
		setupStore	6	10	8	1	1
		uiStartup	1977	2686	2304	198	95

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

background: 472 Bytes (0.01%)
ui: 1.46 KiB (0.02%)
common: 201 Bytes (0.00%)

…rmation-decision-logic

digiwand · 2024-11-21T04:32:19Z

shared/lib/confirmation.utils.ts

+  const isRedesignedConfirmationsDeveloperSettingEnabled =
+    process.env.ENABLE_CONFIRMATION_REDESIGN === 'true' ||
+    isRedesignedConfirmationsDeveloperEnabled;


I was confused about how ENABLE_CONFIRMATION_REDESIGN was being handled. Thanks for clearing this up and for the helpful tests!

metamaskbot · 2024-11-21T04:44:09Z

Builds ready [6c56694]

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 (2161 ± 119 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	455	2521	1953	536	257
		domContentLoaded	1832	2711	2139	246	118
		load	1843	2744	2161	248	119
		domInteractive	30	227	65	53	25
		backgroundConnect	10	67	25	18	9
		firstReactRender	65	201	101	33	16
		getState	46	180	100	26	12
		initialActions	0	1	0	0	0
		loadScripts	1388	2043	1630	196	94
		setupStore	6	45	11	8	4
		uiStartup	2148	3429	2516	296	142

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

background: 472 Bytes (0.01%)
ui: 1.46 KiB (0.02%)
common: 201 Bytes (0.00%)

chore: centralize redesigned confirmation decision logic

ee24909

metamaskbot added the team-confirmations Push issues to confirmations team label Nov 13, 2024

cryptotavares added 5 commits November 14, 2024 14:53

Merge branch 'develop' into cryptotavares/centralize-redesigned-confi…

83ae8ca

…rmation-decision-logic

fix: import redesign dev transaction types

b493085

fix: lint

acee5ae

test:fix createRPCMethodTrackingMiddleware unit tests

487c1f0

test: reomve unnecessary unit test

f8fa89f

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 14, 2024

chore: improve signature redesign approval type readability

7702e54

cryptotavares force-pushed the cryptotavares/centralize-redesigned-confirmation-decision-logic branch from 58fce0b to 7702e54 Compare November 14, 2024 22:23

cryptotavares marked this pull request as ready for review November 14, 2024 22:41

cryptotavares requested review from a team as code owners November 14, 2024 22:41

jpuri previously approved these changes Nov 15, 2024

View reviewed changes

matthewwalsh0 requested changes Nov 15, 2024

View reviewed changes

cryptotavares added 6 commits November 19, 2024 14:00

Merge branch 'develop' into cryptotavares/centralize-redesigned-confi…

ce7bc59

…rmation-decision-logic

chore: use function declarations instead of expressions

f35fac3

chore: add jsdoc comments

5fc5817

chore: remove useless comment

af3819b

chore: stop exporting REDESIGN_SIGNATURE_APPROVAL_TYPES

a59ed51

chore: stop exporting REDESIGN_DEV_TRANSACTION_TYPES

2cd99ef

cryptotavares dismissed jpuri’s stale review via 2cd99ef November 19, 2024 16:32

cryptotavares added 4 commits November 19, 2024 16:45

chore: move confirmation utils from shared/modules to shared/lib

03b5a3c

improve shouldUseRedesignForTransactions args readability

18e2fde

improve shouldUseRedesignForSignatures args readability

74939a6

fix: lint

0154d58

cryptotavares added 2 commits November 20, 2024 15:15

fix: disable jsdoc lint

34dc9df

jsdoc is not picking up the destruct plus the TS inline type

Merge branch 'develop' into cryptotavares/centralize-redesigned-confi…

746342a

…rmation-decision-logic

Merge branch 'develop' into cryptotavares/centralize-redesigned-confi…

6c56694

…rmation-decision-logic

matthewwalsh0 approved these changes Nov 20, 2024

View reviewed changes

digiwand reviewed Nov 21, 2024

View reviewed changes

digiwand approved these changes Nov 21, 2024

View reviewed changes

cryptotavares added this pull request to the merge queue Nov 21, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024

cryptotavares added this pull request to the merge queue Nov 21, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024

cryptotavares added this pull request to the merge queue Nov 21, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024

cryptotavares added this pull request to the merge queue Nov 21, 2024

Merged via the queue into develop with commit 6023787 Nov 21, 2024
75 checks passed

cryptotavares deleted the cryptotavares/centralize-redesigned-confirmation-decision-logic branch November 21, 2024 15:31

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: centralize redesigned confirmation decision logic #28445

chore: centralize redesigned confirmation decision logic #28445

cryptotavares commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024

sentry-io bot commented Nov 13, 2024

metamaskbot commented Nov 15, 2024

digiwand commented Nov 20, 2024

metamaskbot commented Nov 20, 2024

digiwand Nov 21, 2024

metamaskbot commented Nov 21, 2024

chore: centralize redesigned confirmation decision logic #28445

chore: centralize redesigned confirmation decision logic #28445

Conversation

cryptotavares commented Nov 13, 2024 • edited Loading

Description

Motivation

Related issues

Manual testing steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Nov 13, 2024

sentry-io bot commented Nov 13, 2024

🔍 Existing Issues For Review

metamaskbot commented Nov 15, 2024

digiwand commented Nov 20, 2024

metamaskbot commented Nov 20, 2024

digiwand Nov 21, 2024

Choose a reason for hiding this comment

metamaskbot commented Nov 21, 2024

cryptotavares commented Nov 13, 2024 •

edited

Loading