feat: Delete legacy signature confirmation code #30038

pedronfigueiredo · 2025-01-31T18:30:14Z

Description

Building up on #29926, the goal of this PR is to remove as much dead code as possible related to the signatures and transactions confirmations that predate the redesign.

Remove signature entry points from confirm-transaction and confirm-transaction-switch.
Remove signature-related components
- signature-request-header
- signature-request-original
- signature-request-siwe
- signature-request
- signature-request-message
- confirm-signature-request
Remove other unused code from confirmations
- confirm-hexdata
- confirm-detail-row
- confirm-page-container
- confirm-page-container-summary
- confirm-page-container-navigation
- confirm-subtitle
- confirm-title
- transaction-decoding
- custom-spending-cap
- review-spending-cap
- security-provider-banner-alert
- transaction-alerts
- ledger-instruction-field
After removing these components, a pass was done on the deleted code to identify deleted uses of selectors, actions, util, and hooks. Some of these become dead code so they were deleted in this PR as well.
I went back and did the same in the diff for the PR to remove dead transactions code.
Removed unit tests, storybook stories, styles and localization files associated with the removed files.

Attempt at automated way to flag dead code for removal

I tried using the import/no-unused-modules ESLint rule to catch unused exports. Here's the config I tried:

{
  files: [
    'ui/**/*.{js,ts,tsx}',
    'shared/**/*.{js,ts,tsx}',
  ],
  rules: {
    'import/no-unused-modules': [
      2,
      {
        unusedExports: true,
        missingExports: false,
        ignoreExports: [
          // Any usage in test or Storybook files is ignored,
          // so if a function is only imported by these files,
          // it will still be flagged as unused.
          '**/*.test.*',
          '**/*.spec.*',
          '**/*.stories.*',
        ],
      },
    ],
  },
}

However, given the way selectors are exported on ui/selectors/index.js, the rule can't correctly pattern match the unused selectors. I commented the index manually and ran the rule to find "dead code" selectors but this time all selectors were flagged as unused.

Finally, I wondered if this rule could still give useful outputs for other folders of the project besides the selectors folder. When running the lint rule against the whole ui directory, the script ran for more than 2 hours before I stopped it.

Unfortunately, using an ESLint rule to detect dead code is not time efficient, so for this PR I resorted to using the manual methods described above.

Related issues

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

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 · 2025-01-31T18:30:28Z

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 · 2025-02-03T10:39:44Z

Builds ready [cee2f77]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1823 ± 106 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	350	2334	1740	388	186
		domContentLoaded	1545	2281	1795	223	107
		load	1559	2335	1823	222	106
		domInteractive	26	108	44	25	12
		backgroundConnect	12	78	33	22	11
		firstReactRender	16	103	45	30	15
		getState	6	80	20	22	10
		initialActions	0	0	0	0	0
		loadScripts	1107	1723	1307	175	84
		setupStore	8	56	14	13	6
		uiStartup	1793	2613	2104	278	134

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: -208.66 KiB (-2.65%)
common: -5.85 KiB (-0.06%)

socket-security · 2025-02-03T10:44:59Z

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/[email protected]

View full report↗︎

pedronfigueiredo · 2025-02-03T11:55:05Z

@metamaskbot update-policies

metamaskbot · 2025-02-03T12:07:04Z

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

pedronfigueiredo · 2025-02-03T12:34:28Z

@MetaMask/policy-reviewers, the policy change results from removing the package react-inspector.

FrederikBolding · 2025-02-03T12:38:12Z

ui/hooks/useTransactionInsights.js

How come the Snaps E2E's didn't need to be adapted for this? IIRC we are testing both the legacy and new confirmations for both signatures and transactions?

The e2e tests for old code were removed in #29695 and #29819, when the toggles were removed because legacy confirmations became no longer accessible.

Then I don't understand what https://github.com/MetaMask/metamask-extension/blob/main/test/e2e/snaps/test-snap-siginsights.spec.js#L258 is testing

Maybe it stayed by mistake, I'll remove this too!

FrederikBolding · 2025-02-03T12:38:51Z

@MetaMask/policy-reviewers, the policy change results from removing the package react-inspector.

Policy changes lgtm

metamaskbot · 2025-02-03T12:40:09Z

Builds ready [a3621e6]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1745 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	253	2063	1673	351	168
		domContentLoaded	1511	2030	1717	125	60
		load	1546	2048	1745	124	59
		domInteractive	24	140	46	31	15
		backgroundConnect	8	71	31	20	9
		firstReactRender	17	93	38	26	12
		getState	4	59	19	18	9
		initialActions	0	1	0	0	0
		loadScripts	1086	1465	1258	108	52
		setupStore	8	59	17	16	8
		uiStartup	1793	2563	2010	180	86

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: -208.66 KiB (-2.65%)
common: -7.72 KiB (-0.08%)

FrederikBolding · 2025-02-03T12:42:41Z

@pedronfigueiredo /ui/components/app/snaps/insight-warnings is dead code now too AFAIK, would be good to remove if you don't mind

jpuri · 2025-02-03T12:53:28Z

package.json

@@ -414,7 +414,6 @@
    "react-dom": "^16.12.0",
    "react-focus-lock": "^2.9.4",
    "react-idle-timer": "^4.2.5",
-    "react-inspector": "^6.0.2",


jpuri · 2025-02-03T12:56:27Z

ui/helpers/constants/routes.ts

+PATH_NAME_MAP[
+  `${CONFIRM_TRANSACTION_ROUTE}/:id${CONFIRM_INCREASE_ALLOWANCE_PATH}`
+] = 'Confirm Increase Allowance Transaction Page';
+


Hey @pedronfigueiredo : are these paths still needed, may be we can simplify routing to route all confirmations to Confirm.tsx. I think we no longer need sub-paths like above.

But this can be done in separate PR.

Yes, we have planned work for refactoring the routing separately 👍

jpuri · 2025-02-03T12:59:46Z

ui/selectors/selectors.js

 export function getSnaps(state) {
  return state.metamask.snaps;
 }

+export function testFunctionToSeeIfItsCaught(state) {
+  return state.metamask.snaps;
+}


Is this needed, it looks same as getSnaps above ?

This was committed by mistake, great catch Jyoti. I'll remove this now.

jpuri

Great work, I left some small feedbacks.

metamaskbot · 2025-02-03T15:23:24Z

Builds ready [c958b4d]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1607 ± 56 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	250	1815	1549	318	153
		domContentLoaded	1397	1773	1581	116	56
		load	1447	1817	1607	117	56
		domInteractive	24	163	44	36	18
		backgroundConnect	6	79	29	21	10
		firstReactRender	15	102	41	28	14
		getState	4	13	7	3	1
		initialActions	0	1	0	0	0
		loadScripts	984	1338	1130	107	51
		setupStore	7	92	24	24	12
		uiStartup	1636	2180	1844	154	74

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: -208.66 KiB (-2.65%)
common: -8.56 KiB (-0.09%)

sleepytanya · 2025-02-05T03:17:08Z

@pedronfigueiredo

The PR does not appear to negatively impact any of the signatures and transactions confirmations, all basic flows
and functionalities work as expected for Signatures and Transactions.

## **Description** Building up on [#29926](#29926), the goal of this PR is to remove as much dead code as possible related to the signatures and transactions confirmations that predate the redesign. - Remove signature entry points from `confirm-transaction` and `confirm-transaction-switch`. - Remove signature-related components - `signature-request-header` - `signature-request-original` - `signature-request-siwe` - `signature-request` - `signature-request-message` - `confirm-signature-request` - Remove other unused code from confirmations - `confirm-hexdata` - `confirm-detail-row` - `confirm-page-container` - `confirm-page-container-summary` - `confirm-page-container-navigation` - `confirm-subtitle` - `confirm-title` - `transaction-decoding` - `custom-spending-cap` - `review-spending-cap` - `security-provider-banner-alert` - `transaction-alerts` - `ledger-instruction-field` - After removing these components, a pass was done on the deleted code to identify deleted uses of `selectors`, `actions`, `util`, and `hooks`. Some of these become dead code so they were deleted in this PR as well. - I went back and did the same in the diff for the PR to remove dead transactions code. - Removed unit tests, storybook stories, styles and localization files associated with the removed files. ## Attempt at automated way to flag dead code for removal I tried using the [import/no-unused-modules](https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-unused-modules.md) ESLint rule to catch unused exports. Here's the config I tried: ```js { files: [ 'ui/**/*.{js,ts,tsx}', 'shared/**/*.{js,ts,tsx}', ], rules: { 'import/no-unused-modules': [ 2, { unusedExports: true, missingExports: false, ignoreExports: [ // Any usage in test or Storybook files is ignored, // so if a function is only imported by these files, // it will still be flagged as unused. '**/*.test.*', '**/*.spec.*', '**/*.stories.*', ], }, ], }, } ``` However, given the way selectors are exported on `ui/selectors/index.js`, the rule can't correctly pattern match the unused selectors. I commented the index manually and ran the rule to find "dead code" selectors but this time all selectors were flagged as unused. Finally, I wondered if this rule could still give useful outputs for other folders of the project besides the selectors folder. When running the lint rule against the whole ui directory, the script ran for more than 2 hours before I stopped it. Unfortunately, using an ESLint rule to detect dead code is not time efficient, so for this PR I resorted to using the manual methods described above. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30038?quickstart=1) ## **Related issues** Fixes: MetaMask/MetaMask-planning#4062 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). 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.

metamaskbot · 2025-02-05T10:08:54Z

Builds ready [b0d4839]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1688 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1532	2111	1687	154	74
		domContentLoaded	1523	2105	1656	154	74
		load	1532	2111	1688	154	74
		domInteractive	23	70	36	15	7
		backgroundConnect	9	80	34	23	11
		firstReactRender	16	74	33	22	10
		getState	5	57	17	19	9
		initialActions	0	1	0	0	0
		loadScripts	1063	1582	1199	137	66
		setupStore	8	53	16	14	7
		uiStartup	1754	2392	1923	170	82

Bundle size diffs [🚀 Bundle size reduced!]

background: 0 Bytes (0.00%)
ui: -208.66 KiB (-2.64%)
common: -8.56 KiB (-0.09%)

pedronfigueiredo added the team-confirmations Push issues to confirmations team label Jan 31, 2025

pedronfigueiredo self-assigned this Jan 31, 2025

pedronfigueiredo requested review from a team as code owners January 31, 2025 18:30

pedronfigueiredo force-pushed the pnf/4062 branch from 54fb939 to cee2f77 Compare February 3, 2025 10:04

pedronfigueiredo force-pushed the pnf/4062 branch from cee2f77 to bcdaf5f Compare February 3, 2025 10:41

pedronfigueiredo force-pushed the pnf/4062 branch from bcdaf5f to 9db188b Compare February 3, 2025 11:47

metamaskbot requested review from a team as code owners February 3, 2025 12:07

FrederikBolding reviewed Feb 3, 2025

View reviewed changes

jpuri reviewed Feb 3, 2025

View reviewed changes

jpuri previously approved these changes Feb 3, 2025

View reviewed changes

pedronfigueiredo dismissed jpuri’s stale review via f6b2604 February 3, 2025 14:25

pedronfigueiredo force-pushed the pnf/4062 branch from a3621e6 to f6b2604 Compare February 3, 2025 14:25

FrederikBolding previously approved these changes Feb 3, 2025

View reviewed changes

pedronfigueiredo dismissed FrederikBolding’s stale review via 64e0c3f February 3, 2025 14:40

pedronfigueiredo force-pushed the pnf/4062 branch 2 times, most recently from 64e0c3f to c958b4d Compare February 3, 2025 14:49

jpuri previously approved these changes Feb 4, 2025

View reviewed changes

FrederikBolding previously approved these changes Feb 4, 2025

View reviewed changes

sleepytanya added the QA Passed label Feb 5, 2025

pedronfigueiredo added this pull request to the merge queue Feb 5, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 5, 2025

pedronfigueiredo added this pull request to the merge queue Feb 5, 2025

pedronfigueiredo added 2 commits February 5, 2025 09:31

feat: Delete legacy signature confirmation code

6277180

Update mmi policy

b0d4839

pedronfigueiredo removed this pull request from the merge queue due to a manual request Feb 5, 2025

pedronfigueiredo dismissed stale reviews from FrederikBolding and jpuri via b0d4839 February 5, 2025 09:32

pedronfigueiredo force-pushed the pnf/4062 branch from c958b4d to b0d4839 Compare February 5, 2025 09:32

jpuri approved these changes Feb 5, 2025

View reviewed changes

FrederikBolding approved these changes Feb 5, 2025

View reviewed changes

pedronfigueiredo added this pull request to the merge queue Feb 5, 2025

Merged via the queue into main with commit b4fc88e Feb 5, 2025
72 checks passed

pedronfigueiredo deleted the pnf/4062 branch February 5, 2025 11:02

github-actions bot locked and limited conversation to collaborators Feb 5, 2025

metamaskbot added the release-12.13.0 Issue or pull request that will be included in release 12.13.0 label Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Delete legacy signature confirmation code #30038

feat: Delete legacy signature confirmation code #30038

pedronfigueiredo commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025

metamaskbot commented Feb 3, 2025

socket-security bot commented Feb 3, 2025 •

edited

Loading

pedronfigueiredo commented Feb 3, 2025

metamaskbot commented Feb 3, 2025

pedronfigueiredo commented Feb 3, 2025

FrederikBolding Feb 3, 2025

pedronfigueiredo Feb 3, 2025

FrederikBolding Feb 3, 2025

pedronfigueiredo Feb 3, 2025

FrederikBolding commented Feb 3, 2025

metamaskbot commented Feb 3, 2025

FrederikBolding commented Feb 3, 2025

jpuri Feb 3, 2025

jpuri Feb 3, 2025

pedronfigueiredo Feb 3, 2025

jpuri Feb 3, 2025

pedronfigueiredo Feb 3, 2025

jpuri left a comment

metamaskbot commented Feb 3, 2025

sleepytanya commented Feb 5, 2025

metamaskbot commented Feb 5, 2025

feat: Delete legacy signature confirmation code #30038

feat: Delete legacy signature confirmation code #30038

Conversation

pedronfigueiredo commented Jan 31, 2025 • edited Loading

Description

Attempt at automated way to flag dead code for removal

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 31, 2025

metamaskbot commented Feb 3, 2025

socket-security bot commented Feb 3, 2025 • edited Loading

pedronfigueiredo commented Feb 3, 2025

metamaskbot commented Feb 3, 2025

pedronfigueiredo commented Feb 3, 2025

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

FrederikBolding commented Feb 3, 2025

metamaskbot commented Feb 3, 2025

FrederikBolding commented Feb 3, 2025

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

jpuri left a comment

Choose a reason for hiding this comment

metamaskbot commented Feb 3, 2025

sleepytanya commented Feb 5, 2025

metamaskbot commented Feb 5, 2025

pedronfigueiredo commented Jan 31, 2025 •

edited

Loading

socket-security bot commented Feb 3, 2025 •

edited

Loading