fix: bring back exception if invalid address passed in eth_accounts #30213

ffmcgee725 · 2025-02-07T18:59:25Z

This commit fixes the following issue which is currently a release blocker for v12.12.9

The eth accounts and permitted chains caveat validation in this PR was originally removed as part of the CAIP-25 Permission Migration PR here, but is being restored in order to fix a regression in the API behavior. Originally, making a wallet_requestPermissions request with invalid caveat values for eth_accounts or endowment:permitted-chains would result in an immediate error returned back to the dapp specifying which value was invalid, i.e. not found in the wallet. This PR restores that behavior.
Additionally, the changes in this PR are temporary and will be removed as part of a follow up refactor that cleans up the CAIP-25 approval/request permission flow.

Description

Related issues

Fixes:

Manual testing steps

Unlock MM
Open test dapp console
Paste the following command
See error

Screenshots/Recordings

Before

After

Screen.Recording.2025-02-07.at.19.54.15.mov

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.

jiexi · 2025-02-07T19:01:46Z

app/scripts/controllers/permissions/specifications.js

+ * @param {string[]} accounts - The accounts associated with the caveat.
+ * @param {() => Record<string, import('@metamask/keyring-internal-api').InternalAccount>} getInternalAccounts -
+ * Gets all AccountsController InternalAccounts.
+ * TODO: Remove this function once permission refactor/factory differ work is merged into main


Suggested change

* TODO: Remove this function once permission refactor/factory differ work is merged into main

* TODO: Remove this function once the CAIP-25 permission refactor/factory differ work is merged into main

resolved in c9df79b

app/scripts/controllers/permissions/specifications.js

app/scripts/metamask-controller.js

app/scripts/metamask-controller.test.js

jiexi · 2025-02-07T19:05:30Z

app/scripts/metamask-controller.test.js

+            PermissionSpecifications.validateCaveatAccounts.mockImplementation(
+              () => ({}),
+            );


do we need to specify anything at all for validateCaveatAccounts? The default mock already does not throw

in this case we do, otherwise the mock from the previous block persists and we throw the accounts error instead of the chains one

odd... well being extra explicit in tests is fine with me. can we make this return undefined instead of empty object? (nit)

resolved in 7a9a4a3

app/scripts/metamask-controller.test.js

metamaskbot · 2025-02-07T19:27:36Z

❌ API Spec Test Failed. View the report here.

metamaskbot · 2025-02-07T21:38:33Z

❌ API Spec Test Failed. View the report here.

metamaskbot · 2025-02-07T22:31:34Z

Builds ready [84df286]

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 (1743 ± 70 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	494	2038	1674	308	148
		domContentLoaded	1487	2021	1718	151	73
		load	1534	2040	1743	147	70
		domInteractive	25	93	37	21	10
		backgroundConnect	11	60	27	16	8
		firstReactRender	16	95	36	25	12
		getState	5	58	23	20	10
		initialActions	0	0	0	0	0
		loadScripts	1068	1516	1249	125	60
		setupStore	6	63	21	21	10
		uiStartup	1719	2404	2025	205	98

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

background: 363 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 841 Bytes (0.01%)

metamaskbot · 2025-02-08T00:28:35Z

Builds ready [6a7ef36]

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 (1725 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	231	1877	1652	336	161
		domContentLoaded	1590	1823	1686	77	37
		load	1604	1879	1725	87	42
		domInteractive	24	86	39	20	10
		backgroundConnect	10	109	42	28	13
		firstReactRender	17	93	34	25	12
		getState	5	58	20	19	9
		initialActions	0	1	0	0	0
		loadScripts	1101	1369	1213	82	39
		setupStore	7	59	17	17	8
		uiStartup	1794	2320	1974	136	65

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

background: 365 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 841 Bytes (0.01%)

metamaskbot · 2025-02-08T10:53:56Z

Builds ready [7a9a4a3]

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 (1515 ± 45 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	293	1775	1461	283	136
		domContentLoaded	1361	1718	1496	86	42
		load	1369	1779	1515	95	45
		domInteractive	17	95	47	25	12
		backgroundConnect	8	63	23	15	7
		firstReactRender	14	82	40	25	12
		getState	3	50	9	10	5
		initialActions	0	1	0	0	0
		loadScripts	959	1299	1091	78	38
		setupStore	7	51	10	9	5
		uiStartup	1539	2171	1722	143	69

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

background: 365 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 841 Bytes (0.01%)

fix: bring back exception if invalid address passed in eth_accounts

7ac1f69

ffmcgee725 requested a review from jiexi February 7, 2025 18:59

metamaskbot added the team-wallet-api-platform label Feb 7, 2025

ffmcgee725 marked this pull request as ready for review February 7, 2025 19:00

jiexi reviewed Feb 7, 2025

View reviewed changes

app/scripts/controllers/permissions/specifications.js Outdated Show resolved Hide resolved

jiexi reviewed Feb 7, 2025

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

jiexi reviewed Feb 7, 2025

View reviewed changes

app/scripts/metamask-controller.test.js Outdated Show resolved Hide resolved

jiexi reviewed Feb 7, 2025

View reviewed changes

app/scripts/metamask-controller.test.js Outdated Show resolved Hide resolved

ffmcgee725 added 2 commits February 7, 2025 20:12

code review changes

c9df79b

minor fix

ed828d5

ffmcgee725 requested a review from jiexi February 7, 2025 19:15

address failing e2e tests

4c1e0dc

address broken wallet_requestPermissions.spec.ts

84df286

test: fix

6a7ef36

address nit

7a9a4a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bring back exception if invalid address passed in eth_accounts #30213

fix: bring back exception if invalid address passed in eth_accounts #30213

ffmcgee725 commented Feb 7, 2025 •

edited

Loading

jiexi Feb 7, 2025

ffmcgee725 Feb 7, 2025

jiexi Feb 7, 2025

ffmcgee725 Feb 7, 2025

jiexi Feb 8, 2025

ffmcgee725 Feb 8, 2025

metamaskbot commented Feb 7, 2025

metamaskbot commented Feb 7, 2025

metamaskbot commented Feb 7, 2025

metamaskbot commented Feb 8, 2025

metamaskbot commented Feb 8, 2025

	* TODO: Remove this function once permission refactor/factory differ work is merged into main
	* TODO: Remove this function once the CAIP-25 permission refactor/factory differ work is merged into main

fix: bring back exception if invalid address passed in eth_accounts #30213

Are you sure you want to change the base?

fix: bring back exception if invalid address passed in eth_accounts #30213

Conversation

ffmcgee725 commented Feb 7, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

jiexi Feb 7, 2025

Choose a reason for hiding this comment

ffmcgee725 Feb 7, 2025

Choose a reason for hiding this comment

jiexi Feb 7, 2025

Choose a reason for hiding this comment

ffmcgee725 Feb 7, 2025

Choose a reason for hiding this comment

jiexi Feb 8, 2025

Choose a reason for hiding this comment

ffmcgee725 Feb 8, 2025

Choose a reason for hiding this comment

metamaskbot commented Feb 7, 2025

metamaskbot commented Feb 7, 2025

metamaskbot commented Feb 7, 2025

metamaskbot commented Feb 8, 2025

metamaskbot commented Feb 8, 2025

ffmcgee725 commented Feb 7, 2025 •

edited

Loading