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

jiexi · 2024-10-14T19:51:23Z

Description

This PR replaces the replaces the internal eth_accounts and endowment:permittedChains permission structure with a CAIP-25 endowment. It adds adapter logic to translate to and from the new internal CAIP-25 permissions. This change should be transparent to wallet users and to dapps except for ~~one~~ two cases, see below. This change is required in order to support CAIP-25 and CAIP-27 requests in a follow-up PR that enables the Multichain API.

Related issues

Related: MetaMask/core#4784

Manual testing steps

There should be no user or dapp facing difference in behavior except:

When calling wallet_revokePermissions and specifying either eth_accounts or endowment:permitted-chains, the entire CAIP-25 permission will be revoked. It will appear to the dapp as if both eth_accounts and endowment:permitted-chains were revoked.
When calling wallet_getPermissions for a permitted dapp when the wallet is locked, eth_accounts should be returned in addition to endowment:permitted-chains. Currently there is a regression on main where only endowment:permitted-chains gets returned when the wallet is locked.

await window.ethereum.request({
 "method": "wallet_revokePermissions",
 "params": [
  {
    eth_accounts: {}
  }
],
});

await window.ethereum.request({
 "method": "wallet_revokePermissions",
 "params": [
  {
    'endowment:permitted-chains': {}
  }
],
});

await window.ethereum.request({
 "method": "wallet_getPermissions",
 "params": [],
});

Locked Wallet Behavior with dapp connected

Other than the one noted item below, this behavior matches that in main

eth_accounts returns []
wallet_getPermissions returns permissions incl eth_accounts
wallet_revokePermissions works as usual and revokes eth_accounts and revoke permitted-chains together
- Note this fixes a regression in main where eth_accounts and permitted-chains aren't revoked as a pair if either is revoked
eth_requestAccounts prompts for unlock, after unlock returns accounts if any are permitted, otherwise shows connection prompt
wallet_requestPermissions prompts for unlock
signature methods fails with method or accounts not authorized
non-signature methods work as usual
accountsChanged empty array on lock. no event after revokePermissions which makes sense since the dapp was told empty array on lock and now it's actually empty array so no changes have occurred as far as the dapp should be concerned.

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-10-14T19:51:35Z

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.

socket-security · 2024-10-14T19:52:52Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/[email protected]	None	`0`	270 kB	metamaskbot
npm/@metamask/[email protected]	None	`0`	318 kB	metamaskbot
npm/@open-rpc/[email protected]	None	`0`	38.4 kB	belfordz

🚮 Removed packages: npm/@json-schema-spec/[email protected], npm/@json-schema-tools/[email protected], npm/@metamask/[email protected], npm/@open-rpc/[email protected]

View full report↗︎

socket-security · 2024-10-14T19:52:53Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

jiexi · 2024-10-14T20:05:06Z

@metamask-bot update-policies

.yarn/patches/@json-schema-spec-json-pointer-npm-0.1.2-3d06119887.patch

.yarn/patches/@json-schema-tools-reference-resolver-npm-1.2.6-4e1497c16d.patch

jiexi · 2024-10-15T18:15:55Z

@metamaskbot update-policies

metamaskbot · 2024-10-15T18:27:48Z

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

…o caip25-permission-migration

…ge.test.js snapshot

FrederikBolding · 2024-12-18T11:59:45Z

app/scripts/migrations/135.ts

+
+    const isSnap = snapsPrefixes.some((prefix) => origin.startsWith(prefix));
+    const scopes: InternalScopesObject = {};
+    const scopeStrings: CaipChainId[] = isSnap


I don't entirely follow the reasoning here, what about Snaps that have accounts created?

They'll still be stored under the wallet:eip155 scope a few lines below this

Hmm, I don't follow that either, what is that scope supposed to represent?

that scope represents evm wallet methods, i.e. methods that do not belong to a particular evm chain, so something like personal_sign would belong here . It's kind of awkward right now because our current Signing UI implementation does require chainId context and so some signing methods do belong to a chain specific scope (i.e. eip155:1 etc) and so personal_sign does not exist in wallet:eip155 like in my example above, but I won't go into the specifics there

basically, it's a scope for evm stuff, but not chain specific

I understand that, I'm just having trouble following whether the permissions are being migrated correctly.

Is granting wallet:eip155 the same as granting access to all chains by default? E.g. the old eth_accounts behaviour? If not, then I don't understand fully how the migration works for Snaps.

The only reason is that snaps currently do not have permittedChains permissions and so granting permissions to eip155:<chainId> scopes would effectively be giving snaps permittedChains permissions. My understanding was that we wanted to hold off on doing this until we are ready to release snap selected networks and chain switching

That's fine, but it still doesn't answer my question as to what permission are we then actually granting. What does giving them wallet:eip155 grant them permission to do?

permission to call any EIP-1193 window.ethereum exposed method, incl signing methods for the accounts specified in the wallet:eip155 scope, on the globally selected network

OK sorry obviously I misunderstood your line of questioning here in my previous answer.

So given that Snaps may already have eth_accounts permissions on technically all chains, why aren't we granting that permission in the same way as for dapps?

I think (?) I understand the question better now.

So the new permission format (modeled closely after CAIP-25 request structure) nests accounts under specific chain permissions:

"eip155:10": { "accounts:" ["eip155:10:0x0910e12C68d02B561a34569E1367c9AAb42bd810" ,"eip155:10:0x806627172aF48bD5B0765D3449A7DEf80d6576ff"] }, "eip155:42161": { "accounts":["eip155:42161:0x0910e12C68d02B561a34569E1367c9AAb42bd810", "eip155:42161:0x806627172aF48bD5B0765D3449A7DEf80d6576ff"], },

so if we were to grant permissions to snaps in the same way we would necessarily be constraining these account permissions to the enumerated chains.

Putting snaps account permissions into wallet:eip155 is a temporary way to get around this entanglement of chain and account permissions. Really in the new permissions structure accounts permissions shouldn't really be separable from chain permissions.

Now its also the case we currently don't have any cases enabled where any accounts are not included in all the chain scopes so, until we do, the way it works is all permissioned accounts are permissioned on all permissioned chains. But this still doesn't lead to as broad of a permission as the legacy eth_accounts permission (before chain Permissions) since it was unconstrained by what subset of enabled chains the dapp has been granted. That's why we've carved out this temporary solution for snaps until they have chain permissions as well.

FrederikBolding · 2024-12-18T12:08:30Z

app/scripts/controllers/permissions/background-api.js

+      updatedEthAccounts,
+    );
+
+    permissionController.updateCaveat(


Is it not simpler to do this via grantPermissionsIncremental?

Mark, Alex, and I discussed a path to get to potentially get to that point, but prerequisite work would involve making a lot more component level changes that we are opting not to do at this time

At the very least we should create a ticket to follow-up on this!

Making a ticket for this now

Ticket here: https://app.zenhub.com/workspaces/wallet-api-platform-63bee08a4e3b9d001108416e/issues/gh/metamask/metamask-planning/3833

app/scripts/controllers/permissions/background-api.js

# Conflicts: # app/scripts/controllers/permissions/specifications.js # lavamoat/browserify/beta/policy.json # lavamoat/browserify/flask/policy.json # lavamoat/browserify/main/policy.json

jiexi · 2024-12-18T15:53:55Z

@metamaskbot update-policies

metamaskbot · 2024-12-18T16:06:46Z

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

…o caip25-permission-migration

metamaskbot · 2024-12-18T18:33:07Z

Builds ready [059ecad]

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 (1969 ± 74 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1671	2170	1959	152	73
		domContentLoaded	1621	2149	1928	150	72
		load	1681	2249	1969	154	74
		domInteractive	27	124	52	29	14
		backgroundConnect	9	96	38	29	14
		firstReactRender	18	82	52	24	11
		getState	7	92	21	23	11
		initialActions	0	1	0	0	0
		loadScripts	1160	1702	1450	136	65
		setupStore	7	85	24	26	12
		uiStartup	1953	3019	2292	249	119

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

background: 15.68 KiB (0.28%)
ui: 590 Bytes (0.01%)
common: 132.94 KiB (1.66%)

jiexi · 2024-12-19T23:47:09Z

One thing I remembered is that the removeNetwork flow doesn't properly revoke scopes from existing CAIP-25 permissions because that case just isn't being handled in main right now. @Gudahtt I just want to confirm again that that particular problem will be resolved after this PR is merged (and is not expected to be included as part of this PR).

metamaskbot · 2024-12-20T00:51:12Z

Builds ready [a7ba043]

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

Page Load Metrics (1768 ± 64 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	287	2023	1704	351	169
		domContentLoaded	1542	1983	1735	128	61
		load	1567	2033	1768	132	64
		domInteractive	25	123	44	25	12
		backgroundConnect	7	75	34	22	11
		firstReactRender	16	99	53	29	14
		getState	5	62	19	18	9
		initialActions	0	1	0	0	0
		loadScripts	1151	1454	1312	95	45
		setupStore	6	80	16	21	10
		uiStartup	1751	2678	2126	303	145

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

background: 15.75 KiB (0.29%)
ui: 832 Bytes (0.01%)
common: 132.92 KiB (1.65%)

metamaskbot · 2024-12-20T18:05:30Z

Builds ready [352f964]

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

Page Load Metrics (1711 ± 78 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1534	2217	1700	162	78
		domContentLoaded	1525	2184	1678	162	78
		load	1534	2220	1711	163	78
		domInteractive	25	155	50	30	15
		backgroundConnect	9	88	33	25	12
		firstReactRender	16	97	49	27	13
		getState	6	68	18	20	10
		initialActions	0	0	0	0	0
		loadScripts	1115	1513	1234	103	49
		setupStore	7	57	10	11	5
		uiStartup	1695	2450	2022	210	101

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

background: 15.75 KiB (0.28%)
ui: 691 Bytes (0.01%)
common: 132.92 KiB (1.59%)

metamaskbot · 2024-12-20T22:30:52Z

Builds ready [e749a8a]

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

Page Load Metrics (1887 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	341	2045	1797	350	168
		domContentLoaded	1603	2184	1861	127	61
		load	1629	2205	1887	124	60
		domInteractive	29	116	58	23	11
		backgroundConnect	10	81	27	20	10
		firstReactRender	17	103	41	29	14
		getState	6	66	24	21	10
		initialActions	0	1	0	0	0
		loadScripts	1183	1643	1417	114	55
		setupStore	7	71	26	24	11
		uiStartup	1798	2640	2173	214	103

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

background: 15.75 KiB (0.28%)
ui: 691 Bytes (0.01%)
common: 132.92 KiB (1.59%)

jiexi added 3 commits October 14, 2024 12:36

lint

7e61aa7

bring migration

590a206

patch json-schema modules

b507802

github-actions bot added the team-wallet-api-platform label Oct 14, 2024

shanejonas reviewed Oct 15, 2024

View reviewed changes

.yarn/patches/@json-schema-spec-json-pointer-npm-0.1.2-3d06119887.patch Show resolved Hide resolved

shanejonas reviewed Oct 15, 2024

View reviewed changes

.yarn/patches/@json-schema-tools-reference-resolver-npm-1.2.6-4e1497c16d.patch Show resolved Hide resolved

jiexi added 2 commits October 15, 2024 09:01

Bring in migration fixes from #27849

bb96bc3

Merge branch 'develop' into caip25-permission-migration

2bcbf1b

This was referenced Oct 15, 2024

feat: CAIP Multichain (OLD ignore. Only for reference now) #25665

Closed

feat: CAIP Multichain (New) #27782

Draft

jiexi added 4 commits October 15, 2024 10:28

bring in new preview build

d2dd1a3

Merge branch 'develop' into caip25-permission-migration

1ceeecf

convert wallet_revokePermissions to ts

859f60b

convert wallet_getPermissions to ts

419572f

jiexi mentioned this pull request Oct 15, 2024

feat: Add CAIP-25 permission and adapters to @metamask/multichain MetaMask/core#4784

Merged

4 tasks

Update LavaMoat policies

81027d0

jiexi and others added 8 commits October 15, 2024 12:05

convert wallet_requestPermissions to ts

48388a2

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

c8d1c71

…o caip25-permission-migration

Merge branch 'develop' into caip25-permission-migration

3264c17

yarn dedupe

06663e9

Merge branch 'develop' into caip25-permission-migration

1cbbc28

update ui/components/multichain/pages/permissions-page/permissions-pa…

273c523

…ge.test.js snapshot

improve errors + caveate mutator logic

3d83a85

more error improvements

92c2cf3

FrederikBolding reviewed Dec 18, 2024

View reviewed changes

app/scripts/controllers/permissions/background-api.js Show resolved Hide resolved

Merge branch 'main' into caip25-permission-migration

f2f0dbd

# Conflicts: # app/scripts/controllers/permissions/specifications.js # lavamoat/browserify/beta/policy.json # lavamoat/browserify/flask/policy.json # lavamoat/browserify/main/policy.json

Update LavaMoat policies

60d02c9

jiexi added 3 commits December 18, 2024 09:47

fix validateChainId guard

2657bfe

Merge branch 'main' into caip25-permission-migration

88b1bd1

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

059ecad

…o caip25-permission-migration

jiexi added 8 commits December 19, 2024 12:31

Merge branch 'main' into caip25-permission-migration

cb4126d

Merge branch 'main' into caip25-permission-migration

85d0e94

convert getDescriptionNode params to object params

2c40696

Merge branch 'main' into caip25-permission-migration

10ad69f

Update wallet_requestPermissions to handle other permissions again

a8f4b55

Fix Snaps permission UI

f460b5c

incremental permitted chains view still needed

8ac50a4

replace updatedCaveatValue let assignment with const

ca74a5d

fix wallet_requestPermissions e2e spec

a7ba043

jiexi added 3 commits December 20, 2024 08:20

Merge branch 'main' into caip25-permission-migration

8953a07

remove incremental redirect for permittedChains

eb08761

Merge branch 'main' into caip25-permission-migration

352f964

jiexi added 3 commits December 20, 2024 10:23

Merge branch 'main' into caip25-permission-migration

9657527

Merge branch 'main' into caip25-permission-migration

0ab7368

Merge branch 'main' into caip25-permission-migration

e749a8a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jiexi commented Oct 14, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024

socket-security bot commented Oct 14, 2024 •

edited

Loading

socket-security bot commented Oct 14, 2024 •

edited

Loading

jiexi commented Oct 14, 2024

jiexi commented Oct 15, 2024

metamaskbot commented Oct 15, 2024

FrederikBolding Dec 18, 2024

jiexi Dec 18, 2024

FrederikBolding Dec 19, 2024

jiexi Dec 19, 2024

jiexi Dec 19, 2024

FrederikBolding Dec 19, 2024

jiexi Dec 19, 2024

FrederikBolding Dec 19, 2024

jiexi Dec 20, 2024

adonesky1 Dec 20, 2024

FrederikBolding Dec 18, 2024

jiexi Dec 18, 2024

FrederikBolding Dec 19, 2024

adonesky1 Dec 19, 2024

adonesky1 Dec 19, 2024

jiexi commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

jiexi commented Dec 19, 2024

metamaskbot commented Dec 20, 2024

metamaskbot commented Dec 20, 2024

metamaskbot commented Dec 20, 2024

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

Are you sure you want to change the base?

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

Conversation

jiexi commented Oct 14, 2024 • edited Loading

Description

Related issues

Manual testing steps

Locked Wallet Behavior with dapp connected

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 14, 2024

socket-security bot commented Oct 14, 2024 • edited Loading

socket-security bot commented Oct 14, 2024 • edited Loading

jiexi commented Oct 14, 2024

jiexi commented Oct 15, 2024

metamaskbot commented Oct 15, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiexi commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

metamaskbot commented Dec 18, 2024

jiexi commented Dec 19, 2024

metamaskbot commented Dec 20, 2024

metamaskbot commented Dec 20, 2024

metamaskbot commented Dec 20, 2024

jiexi commented Oct 14, 2024 •

edited

Loading

socket-security bot commented Oct 14, 2024 •

edited

Loading

socket-security bot commented Oct 14, 2024 •

edited

Loading