Jl/caip25 permission migration/move request grant hooks into mmc #29213

jiexi · 2024-12-13T23:07:52Z

Description

Adds requestPermittedChainsPermission, requestPermittedChainsPermissionIncremental, and requestCaip25Permission methods to MetamaskController
Uses these new hooks in wallet_requestPermissions, eth_requestAccounts, wallet_switchEthereumChain, wallet_addEthereumChain
Removes wallet_requestPermissions update caveat flow
Make wallet_requestPermissions no longer support anything but eth_requestAccounts and endowment:permittedChains

Related issues

Manual testing steps

Only user facing change is that wallet_requestPermissions should correctly replace instead update existing permissions again

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.

…on. Use in wallet_requestPermissions and eth_requestAccounts. Remove updateCaveat and support of unexpected permissions in wallet_requestPermissions

…ermission requestPermittedChainsPermissionIncremental to MMC. Use downstream.

github-actions · 2024-12-13T23:08:02Z

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-12-13T23:46:38Z

Builds ready [036a418]

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
- 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 (1518 ± 27 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1397	1616	1518	56	27
		domContentLoaded	1390	1599	1500	52	25
		load	1398	1614	1518	55	27
		domInteractive	23	64	36	14	7
		backgroundConnect	8	53	25	16	8
		firstReactRender	16	95	37	27	13
		getState	4	40	8	8	4
		initialActions	0	1	0	0	0
		loadScripts	1018	1192	1109	51	24
		setupStore	6	43	9	8	4
		uiStartup	1602	2206	1753	143	68

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

background: 15.58 KiB (0.28%)
ui: 590 Bytes (0.01%)
common: 132.75 KiB (1.66%)

adonesky1 · 2024-12-16T18:48:29Z

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.js

+ * @param hooks.requestPermittedChainsPermissionForOrigin
+ * @param hooks.requestPermittedChainsPermissionIncrementalForOrigin


Should we add a jsdoc descriptor for these hooks?

adonesky1 · 2024-12-16T19:09:20Z

app/scripts/metamask-controller.js

+  async requestPermittedChainsPermissionIncremental(
+    origin,
+    chainId,
+    autoApprove,
+  ) {


nit - I find it easier to understand these functions inline when the args are passed in as object rather than by position - like in this case where this is used in ethereum-chain-utils you can see that the isAddFlow boolean is used for the autoApprove arg without having toggle between the declaration to understand ... I suppose the docs also help with this. Anyways I know we do it both ways throughout the codebase. Just putting in for my preference 😅

adonesky1 · 2024-12-16T19:33:16Z

app/scripts/metamask-controller.js

+   *
+   * @param {string} origin - The origin to request approval for.
+   * @param {Hex} chainId - The chainId to add incrementally.
+   * @param {boolean} autoApprove - If the chain should be granted without prompting for user approval.


Should we include an explanation of why we might want to auto approve in some cases: i.e. currently chainPermissions may be granted as part of a wallet_addEthereumChain request in which the permissions and addition of the network are combined into one confirmation

…migration/move-request-grant-hooks-into-mmc

…ionIncremental to object param

metamaskbot · 2024-12-16T20:35:34Z

Builds ready [c70f7b7]

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
- 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 (1507 ± 29 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1387	1598	1511	58	28
		domContentLoaded	1377	1577	1486	56	27
		load	1387	1601	1507	60	29
		domInteractive	20	69	30	12	6
		backgroundConnect	10	59	27	16	7
		firstReactRender	16	93	28	22	11
		getState	4	57	16	18	9
		initialActions	0	1	0	0	0
		loadScripts	996	1193	1109	55	27
		setupStore	6	79	20	24	11
		uiStartup	1587	2146	1779	170	82

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

background: 15.69 KiB (0.29%)
ui: 590 Bytes (0.01%)
common: 132.75 KiB (1.63%)

adonesky1 · 2024-12-16T21:05:27Z

app/scripts/lib/rpc-method-middleware/handlers/ethereum-chain-utils.test.ts

-        expect(mocks.requestPermissionApprovalForOrigin).toHaveBeenCalled();
+        expect(
+          mocks.requestPermittedChainsPermissionIncrementalForOrigin,
+        ).toHaveBeenCalled();
        expect(mocks.setActiveNetwork).not.toHaveBeenCalled();
        expect(end).toHaveBeenCalledWith();
      });
    });

    describe('with an existing CAIP-25 permission granted from the multichain flow (isMultichainOrigin: true) and the chainId is not already permissioned', () => {


🤔 is it weird that we have tests for the multichain API in here already?

yeah. To fix this we could just drop/move all of the isMultichainOrigin checks

adonesky1 · 2024-12-16T21:17:16Z

app/scripts/lib/rpc-method-middleware/handlers/request-accounts.test.ts

-    it('sets the approved chainIds on an empty CAIP-25 caveat with isMultichainOrigin: false if origin is not snapId', async () => {
-      const { handler } = createMockedHandler();
-
-      await handler(baseRequest);
-      expect(MockMultichain.setPermittedEthChainIds).toHaveBeenCalledWith(
-        {
-          requiredScopes: {},
-          optionalScopes: {},
-          isMultichainOrigin: false,
-        },
-        ['0x1', '0x5'],
-      );
-    });
-
-    it('sets the approved accounts on the CAIP-25 caveat after the approved chainIds if origin is not snapId', async () => {
-      const { handler } = createMockedHandler();
-
-      MockMultichain.setPermittedEthChainIds.mockReturnValue({
-        requiredScopes: {},
-        optionalScopes: {},
-        sessionProperties: { caveatValueWithEthChainIdsSet: true },
-        isMultichainOrigin: false,
-      });
-
-      await handler(baseRequest);
-      expect(MockMultichain.setEthAccounts).toHaveBeenCalledWith(
-        {
-          requiredScopes: {},
-          optionalScopes: {},
-          sessionProperties: { caveatValueWithEthChainIdsSet: true },
-          isMultichainOrigin: false,
-        },
-        ['0xdeadbeef'],
-      );
-    });
-
-    it('does not set the approved chainIds on an empty CAIP-25 caveat if origin is snapId', async () => {
-      const { handler } = createMockedHandler();
-
-      await handler({ ...baseRequest, origin: 'npm:snap' });
-      expect(MockMultichain.setPermittedEthChainIds).not.toHaveBeenCalled();
-    });
-
-    it('sets the approved accounts for the `wallet:eip155` scope with isMultichainOrigin: false if origin is snapId', async () => {
-      const { handler } = createMockedHandler();
-
-      await handler({ ...baseRequest, origin: 'npm:snap' });
-      expect(MockMultichain.setEthAccounts).toHaveBeenCalledWith(
-        {
-          requiredScopes: {},
-          optionalScopes: {
-            'wallet:eip155': {
-              accounts: [],
-            },
-          },
-          isMultichainOrigin: false,
-        },
-        ['0xdeadbeef'],
-      );
-    });
-
-    it('grants a CAIP-25 permission', async () => {
-      const { handler, grantPermissions } = createMockedHandler();
-
-      MockMultichain.setEthAccounts.mockReturnValue({
-        requiredScopes: {},
-        optionalScopes: {},
-        sessionProperties: { caveatValueWithEthAccountsSet: true },
-        isMultichainOrigin: false,
-      });
-
-      await handler(baseRequest);
-      expect(grantPermissions).toHaveBeenCalledWith({
-        [Caip25EndowmentPermissionName]: {
-          caveats: [
-            {
-              type: Caip25CaveatType,
-              value: {
-                requiredScopes: {},
-                optionalScopes: {},
-                sessionProperties: { caveatValueWithEthAccountsSet: true },
-                isMultichainOrigin: false,
-              },
-            },
-          ],
-        },
-      });
-    });


Forgive me if its obvious but why aren't these tests relevant anymore? I see they would need to be adjusted but aren't they still test cases?

they've been moved/adjusted into MetaMaskController

adonesky1 · 2024-12-16T21:23:10Z

Screen.Recording.2024-12-16.at.3.21.33.PM.mov

app/scripts/metamask-controller.js

app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts

…ermissions.ts Co-authored-by: Alex Donesky <[email protected]>

jiexi · 2024-12-16T21:49:26Z

Screen.Recording.2024-12-16.at.3.21.33.PM.mov

ticketed here https://github.com/MetaMask/MetaMask-planning/issues/3814

adonesky1 · 2024-12-16T21:51:52Z

app/scripts/metamask-controller.test.js

+            'addAndShowApprovalRequest',
+          )
+          .mockResolvedValue({
+            approvedChainIds: ['0x1', '0x5'],


Suggested change

approvedChainIds: ['0x1', '0x5'],

approvedChainIds: ['0x1', '0x5'],

presumably this isn't currently possible?

yeah, the UI should not showing an Approval veiw that can return approvedChainIds, but I also explicitly filter out / ignore them in code. Is it worth leaving this in to be doubly sure or do you think it just causes confusion?

No its okay!

app/scripts/metamask-controller.test.js

adonesky1 · 2024-12-16T21:56:39Z

app/scripts/metamask-controller.test.js

+        );
+      });
+
+      it('gets the CAIP-25 caveat', async () => {


~~🤔 not sure I follow~~

oh I see. Hmmm, one of those cases where I see you're trying to avoid overly specific implementation detail language but kindof confuses without looking closer

metamaskbot · 2024-12-16T22:01:14Z

❌ API Spec Test Failed. View the report here.

Co-authored-by: Alex Donesky <[email protected]>

…migration/move-request-grant-hooks-into-mmc

…ove-request-grant-hooks-into-mmc' into jl/caip25-permission-migration/move-request-grant-hooks-into-mmc

adonesky1

LGTM

…migration/move-request-grant-hooks-into-mmc

…ove-request-grant-hooks-into-mmc' into jl/caip25-permission-migration/move-request-grant-hooks-into-mmc

jiexi · 2024-12-16T23:10:06Z

@metamaskbot update-policies

metamaskbot · 2024-12-16T23:22:00Z

No policy changes

metamaskbot · 2024-12-17T00:02:46Z

Builds ready [e3a903b]

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
- 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, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1572 ± 63 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	221	1908	1450	408	196
		domContentLoaded	1380	1889	1549	127	61
		load	1393	1909	1572	132	63
		domInteractive	22	171	37	32	16
		backgroundConnect	7	74	27	22	11
		firstReactRender	16	88	38	27	13
		getState	3	55	9	11	5
		initialActions	0	1	0	0	0
		loadScripts	975	1441	1132	114	55
		setupStore	6	83	20	23	11
		uiStartup	1644	2358	1896	203	97

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

background: 15.69 KiB (0.29%)
ui: 590 Bytes (0.01%)
common: 132.94 KiB (1.64%)

jiexi added 2 commits December 13, 2024 13:31

Replace requestPermissionApprovalForOrigin with requestCaip25Permissi…

564234a

…on. Use in wallet_requestPermissions and eth_requestAccounts. Remove updateCaveat and support of unexpected permissions in wallet_requestPermissions

add requestApprovalPermittedChainsPermission, requestPermittedChainsP…

8be4506

…ermission requestPermittedChainsPermissionIncremental to MMC. Use downstream.

metamaskbot added the team-wallet-api-platform label Dec 13, 2024

remove unused hook

036a418

jiexi mentioned this pull request Dec 13, 2024

feat: Migrate eth_accounts and permittedChains to CAIP-25 endowment #27847

Open

7 tasks

adonesky1 reviewed Dec 16, 2024

View reviewed changes

jiexi added 4 commits December 16, 2024 11:36

update switchChain jsdoc

8dae536

Merge branch 'caip25-permission-migration' into jl/caip25-permission-…

fd1f931

…migration/move-request-grant-hooks-into-mmc

update requestPermittedChains jsdoc

e741f74

change requestPermittedChainsPermission requestPermittedChainsPermiss…

c70f7b7

…ionIncremental to object param

adonesky1 reviewed Dec 16, 2024

View reviewed changes

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

adonesky1 reviewed Dec 16, 2024

View reviewed changes

app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Dec 16, 2024

View reviewed changes

app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Dec 16, 2024

View reviewed changes

app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Dec 16, 2024

View reviewed changes

app/scripts/lib/rpc-method-middleware/handlers/wallet-requestPermissions.ts Outdated Show resolved Hide resolved

jiexi and others added 4 commits December 16, 2024 13:48

Update app/scripts/lib/rpc-method-middleware/handlers/wallet-requestP…

cbb649e

…ermissions.ts Co-authored-by: Alex Donesky <[email protected]>

Update app/scripts/lib/rpc-method-middleware/handlers/wallet-requestP…

cb81b1c

…ermissions.ts Co-authored-by: Alex Donesky <[email protected]>

Update app/scripts/lib/rpc-method-middleware/handlers/wallet-requestP…

aa3efc9

…ermissions.ts Co-authored-by: Alex Donesky <[email protected]>

Update app/scripts/lib/rpc-method-middleware/handlers/wallet-requestP…

0495769

…ermissions.ts Co-authored-by: Alex Donesky <[email protected]>

adonesky1 reviewed Dec 16, 2024

View reviewed changes

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

adonesky1 reviewed Dec 16, 2024

View reviewed changes

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

adonesky1 reviewed Dec 16, 2024

View reviewed changes

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

adonesky1 reviewed Dec 16, 2024

View reviewed changes

jiexi and others added 6 commits December 16, 2024 14:03

Update app/scripts/metamask-controller.test.js

a29ce03

Co-authored-by: Alex Donesky <[email protected]>

Update app/scripts/metamask-controller.test.js

2cb35a9

Co-authored-by: Alex Donesky <[email protected]>

Update app/scripts/metamask-controller.test.js

bdf570c

Co-authored-by: Alex Donesky <[email protected]>

Merge branch 'caip25-permission-migration' into jl/caip25-permission-…

08076a6

…migration/move-request-grant-hooks-into-mmc

Merge remote-tracking branch 'origin/jl/caip25-permission-migration/m…

5c53a5e

…ove-request-grant-hooks-into-mmc' into jl/caip25-permission-migration/move-request-grant-hooks-into-mmc

fix wallet_requestPermissions var names

80ddbf1

adonesky1 approved these changes Dec 16, 2024

View reviewed changes

jiexi added 4 commits December 16, 2024 14:52

Merge branch 'caip25-permission-migration' into jl/caip25-permission-…

0103d36

…migration/move-request-grant-hooks-into-mmc

Merge branch 'caip25-permission-migration' into jl/caip25-permission-…

91ee227

…migration/move-request-grant-hooks-into-mmc

Merge remote-tracking branch 'origin/jl/caip25-permission-migration/m…

76e791d

…ove-request-grant-hooks-into-mmc' into jl/caip25-permission-migration/move-request-grant-hooks-into-mmc

lint

9ac2b92

jiexi force-pushed the jl/caip25-permission-migration/move-request-grant-hooks-into-mmc branch from a65eec9 to 9ac2b92 Compare December 16, 2024 23:09

Trigger

e3a903b

jiexi marked this pull request as ready for review December 17, 2024 00:09

jiexi merged commit ff3b272 into caip25-permission-migration Dec 17, 2024
71 checks passed

jiexi deleted the jl/caip25-permission-migration/move-request-grant-hooks-into-mmc branch December 17, 2024 00:09

github-actions bot locked and limited conversation to collaborators Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jl/caip25 permission migration/move request grant hooks into mmc #29213

Jl/caip25 permission migration/move request grant hooks into mmc #29213

jiexi commented Dec 13, 2024 •

edited

Loading

github-actions bot commented Dec 13, 2024

metamaskbot commented Dec 13, 2024

adonesky1 Dec 16, 2024

jiexi Dec 16, 2024

adonesky1 Dec 16, 2024

jiexi Dec 16, 2024

adonesky1 Dec 16, 2024

jiexi Dec 16, 2024

metamaskbot commented Dec 16, 2024

adonesky1 Dec 16, 2024

jiexi Dec 16, 2024

adonesky1 Dec 16, 2024

jiexi Dec 16, 2024

adonesky1 commented Dec 16, 2024

jiexi commented Dec 16, 2024

adonesky1 Dec 16, 2024

jiexi Dec 16, 2024

adonesky1 Dec 16, 2024

adonesky1 Dec 16, 2024 •

edited

Loading

metamaskbot commented Dec 16, 2024

adonesky1 left a comment

jiexi commented Dec 16, 2024

metamaskbot commented Dec 16, 2024

metamaskbot commented Dec 17, 2024

		* @param hooks.requestPermittedChainsPermissionForOrigin
		* @param hooks.requestPermittedChainsPermissionIncrementalForOrigin

	approvedChainIds: ['0x1', '0x5'],
	approvedChainIds: ['0x1', '0x5'],

Jl/caip25 permission migration/move request grant hooks into mmc #29213

Jl/caip25 permission migration/move request grant hooks into mmc #29213

Conversation

jiexi commented Dec 13, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Dec 13, 2024

metamaskbot commented Dec 13, 2024

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

Choose a reason for hiding this comment

metamaskbot commented Dec 16, 2024

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

adonesky1 commented Dec 16, 2024

jiexi commented Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

metamaskbot commented Dec 16, 2024

adonesky1 left a comment

Choose a reason for hiding this comment

jiexi commented Dec 16, 2024

metamaskbot commented Dec 16, 2024

metamaskbot commented Dec 17, 2024

jiexi commented Dec 13, 2024 •

edited

Loading

adonesky1 Dec 16, 2024 •

edited

Loading