Skip to content

Commit d5cd7fd

Browse files
jieximetamaskbotadonesky1mcmireGudahtt
authored
feat: Migrate eth_accounts and permittedChains to CAIP-25 endowment (#27847)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This PR replaces the replaces the internal `eth_accounts` and `endowment:permittedChains` permission structure with a [CAIP-25](https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-25.md) 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. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27847?quickstart=1) ## **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 two noted items 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. - **CHANGED**: for dapps that were granted chain permissions via the `wallet_addEthereum` or `wallet_switchEthereumChain` flows without account permissions, these permissions will be removed with this migration. We think this ok because: - This is a very uncommon scenario for dapps to request chain switches without account permissions. - These permissions can be regained very trivially with subsequent chain switch requests. ### Testing the migration * Create a dev build from `main` * Install the dev build from the `dist/chrome` directory and proceed through onboarding * Run this command in the background console: ``` chrome.storage.local.get( null, (state) => { state.data.PermissionController = {}; // Replace this line based on instructions below chrome.storage.local.set(state, () => chrome.runtime.reload()); } ); ``` * Disable the extension * Switch to `main` and create a dev build * Enable and reload the extension * You should see in the console that migration 139 has failed Repeat the above steps but with the line above replaced with the following for example: * `state.data.NetworkController = {}; ` * `state.data.NetworkController = 'foobar'; ` * `state.data.NetworkController.selectedNetworkClientId = null;` * `state.data.NetworkController.networkConfigurationsByChainId = 'foobar';` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.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. --------- Co-authored-by: MetaMask Bot <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: Elliot Winkler <[email protected]> Co-authored-by: Mark Stacey <[email protected]> Co-authored-by: Erik Marks <[email protected]> Co-authored-by: Frederik Bolding <[email protected]>
1 parent 5ae45ab commit d5cd7fd

File tree

71 files changed

+7958
-2343
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

71 files changed

+7958
-2343
lines changed

.storybook/test-data.js

+17-7
Original file line numberDiff line numberDiff line change
@@ -1424,17 +1424,27 @@ const state = {
14241424
subjects: {
14251425
'https://app.uniswap.org': {
14261426
permissions: {
1427-
eth_accounts: {
1428-
invoker: 'https://app.uniswap.org',
1429-
parentCapability: 'eth_accounts',
1430-
id: 'a7342e4b-beae-4525-a36c-c0635fd03359',
1431-
date: 1620710693178,
1427+
'endowment:caip25': {
14321428
caveats: [
14331429
{
1434-
type: 'restrictReturnedAccounts',
1435-
value: ['0x64a845a5b02460acf8a3d84503b0d68d028b4bb4'],
1430+
type: 'authorizedScopes',
1431+
value: {
1432+
requiredScopes: {},
1433+
optionalScopes: {
1434+
'eip155:1': {
1435+
accounts: [
1436+
'eip155:1:0x64a845a5b02460acf8a3d84503b0d68d028b4bb4',
1437+
],
1438+
},
1439+
},
1440+
isMultichainOrigin: false,
1441+
},
14361442
},
14371443
],
1444+
invoker: 'https://app.uniswap.org',
1445+
id: 'a7342e4b-beae-4525-a36c-c0635fd03359',
1446+
date: 1620710693178,
1447+
parentCapability: 'endowment:caip25',
14381448
},
14391449
},
14401450
},
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
diff --git a/lib/index.js b/lib/index.js
2+
index f5795884311124b221d91f488ed45750eb6e9c80..e030d6f8d8e85e6d1350c565d36ad48bc49af881 100644
3+
--- a/lib/index.js
4+
+++ b/lib/index.js
5+
@@ -25,7 +25,7 @@ class Ptr {
6+
});
7+
return `/${tokens.join("/")}`;
8+
}
9+
- eval(instance) {
10+
+ shmeval(instance) {
11+
for (const token of this.tokens) {
12+
if (instance.hasOwnProperty(token)) {
13+
instance = instance[token];
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
diff --git a/build/resolve-pointer.js b/build/resolve-pointer.js
2+
index d5a8ec7486250cd17572eb0e0449725643fc9842..044e74bb51a46e9bf3547f6d7a84763b93260613 100644
3+
--- a/build/resolve-pointer.js
4+
+++ b/build/resolve-pointer.js
5+
@@ -27,7 +27,7 @@ exports.default = (function (ref, root) {
6+
try {
7+
var withoutHash = ref.replace("#", "");
8+
var pointer = json_pointer_1.default.parse(withoutHash);
9+
- return pointer.eval(root);
10+
+ return pointer.shmeval(root);
11+
}
12+
catch (e) {
13+
throw new InvalidJsonPointerRefError(ref, e.message);

app/scripts/background.js

+2-6
Original file line numberDiff line numberDiff line change
@@ -677,13 +677,9 @@ function emitDappViewedMetricEvent(origin) {
677677
return;
678678
}
679679

680-
const permissions = controller.controllerMessenger.call(
681-
'PermissionController:getPermissions',
682-
origin,
683-
);
684680
const numberOfConnectedAccounts =
685-
permissions?.eth_accounts?.caveats[0]?.value.length;
686-
if (!numberOfConnectedAccounts) {
681+
controller.getPermittedAccounts(origin).length;
682+
if (numberOfConnectedAccounts === 0) {
687683
return;
688684
}
689685

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

+183-58
Original file line numberDiff line numberDiff line change
@@ -1,45 +1,177 @@
11
import { nanoid } from 'nanoid';
22
import {
3-
CaveatTypes,
4-
RestrictedMethods,
5-
} from '../../../../shared/constants/permissions';
6-
import { CaveatFactories, PermissionNames } from './specifications';
3+
MethodNames,
4+
PermissionDoesNotExistError,
5+
} from '@metamask/permission-controller';
6+
import {
7+
Caip25CaveatType,
8+
Caip25EndowmentPermissionName,
9+
getEthAccounts,
10+
setEthAccounts,
11+
getPermittedEthChainIds,
12+
setPermittedEthChainIds,
13+
} from '@metamask/multichain';
14+
import { isSnapId } from '@metamask/snaps-utils';
15+
import { RestrictedMethods } from '../../../../shared/constants/permissions';
16+
import { PermissionNames } from './specifications';
17+
18+
export function getPermissionBackgroundApiMethods({
19+
permissionController,
20+
approvalController,
21+
}) {
22+
// Returns the CAIP-25 caveat or undefined if it does not exist
23+
const getCaip25Caveat = (origin) => {
24+
let caip25Caveat;
25+
try {
26+
caip25Caveat = permissionController.getCaveat(
27+
origin,
28+
Caip25EndowmentPermissionName,
29+
Caip25CaveatType,
30+
);
31+
} catch (err) {
32+
if (err instanceof PermissionDoesNotExistError) {
33+
// suppress expected error in case that the origin
34+
// does not have the target permission yet
35+
} else {
36+
throw err;
37+
}
38+
}
39+
return caip25Caveat;
40+
};
741

8-
export function getPermissionBackgroundApiMethods(permissionController) {
42+
// To add more than one account when already connected to the dapp
943
const addMoreAccounts = (origin, accounts) => {
10-
const caveat = CaveatFactories.restrictReturnedAccounts(accounts);
44+
const caip25Caveat = getCaip25Caveat(origin);
45+
if (!caip25Caveat) {
46+
throw new Error(
47+
`Cannot add account permissions for origin "${origin}": no permission currently exists for this origin.`,
48+
);
49+
}
1150

12-
permissionController.grantPermissionsIncremental({
13-
subject: { origin },
14-
approvedPermissions: {
15-
[RestrictedMethods.eth_accounts]: { caveats: [caveat] },
16-
},
17-
});
51+
const ethAccounts = getEthAccounts(caip25Caveat.value);
52+
53+
const updatedEthAccounts = Array.from(
54+
new Set([...ethAccounts, ...accounts]),
55+
);
56+
57+
const updatedCaveatValue = setEthAccounts(
58+
caip25Caveat.value,
59+
updatedEthAccounts,
60+
);
61+
62+
permissionController.updateCaveat(
63+
origin,
64+
Caip25EndowmentPermissionName,
65+
Caip25CaveatType,
66+
updatedCaveatValue,
67+
);
1868
};
1969

2070
const addMoreChains = (origin, chainIds) => {
21-
const caveat = CaveatFactories.restrictNetworkSwitching(chainIds);
71+
const caip25Caveat = getCaip25Caveat(origin);
72+
if (!caip25Caveat) {
73+
throw new Error(
74+
`Cannot add chain permissions for origin "${origin}": no permission currently exists for this origin.`,
75+
);
76+
}
77+
78+
const ethChainIds = getPermittedEthChainIds(caip25Caveat.value);
79+
80+
const updatedEthChainIds = Array.from(
81+
new Set([...ethChainIds, ...chainIds]),
82+
);
83+
84+
const caveatValueWithChains = setPermittedEthChainIds(
85+
caip25Caveat.value,
86+
updatedEthChainIds,
87+
);
88+
89+
// ensure that the list of permitted eth accounts is set for the newly added eth scopes
90+
const ethAccounts = getEthAccounts(caveatValueWithChains);
91+
const caveatValueWithAccountsSynced = setEthAccounts(
92+
caveatValueWithChains,
93+
ethAccounts,
94+
);
95+
96+
permissionController.updateCaveat(
97+
origin,
98+
Caip25EndowmentPermissionName,
99+
Caip25CaveatType,
100+
caveatValueWithAccountsSynced,
101+
);
102+
};
103+
104+
const requestAccountsAndChainPermissions = async (origin, id) => {
105+
// Note that we are purposely requesting an approval from the ApprovalController
106+
// and then manually forming the permission that is then granted via the
107+
// PermissionController rather than calling the PermissionController.requestPermissions()
108+
// directly because the Approval UI is still dependent on the notion of there
109+
// being separate "eth_accounts" and "endowment:permitted-chains" permissions.
110+
// After that depedency is refactored, we can move to requesting "endowment:caip25"
111+
// directly from the PermissionController instead.
112+
const legacyApproval = await approvalController.addAndShowApprovalRequest({
113+
id,
114+
origin,
115+
requestData: {
116+
metadata: {
117+
id,
118+
origin,
119+
},
120+
permissions: {
121+
[RestrictedMethods.eth_accounts]: {},
122+
[PermissionNames.permittedChains]: {},
123+
},
124+
},
125+
type: MethodNames.RequestPermissions,
126+
});
127+
128+
const newCaveatValue = {
129+
requiredScopes: {},
130+
optionalScopes: {},
131+
isMultichainOrigin: false,
132+
};
133+
134+
const caveatValueWithChains = setPermittedEthChainIds(
135+
newCaveatValue,
136+
legacyApproval.approvedChainIds,
137+
);
138+
139+
const caveatValueWithAccounts = setEthAccounts(
140+
caveatValueWithChains,
141+
legacyApproval.approvedAccounts,
142+
);
22143

23-
permissionController.grantPermissionsIncremental({
144+
permissionController.grantPermissions({
24145
subject: { origin },
25146
approvedPermissions: {
26-
[PermissionNames.permittedChains]: { caveats: [caveat] },
147+
[Caip25EndowmentPermissionName]: {
148+
caveats: [
149+
{
150+
type: Caip25CaveatType,
151+
value: caveatValueWithAccounts,
152+
},
153+
],
154+
},
27155
},
28156
});
29157
};
30158

31159
return {
32160
addPermittedAccount: (origin, account) =>
33161
addMoreAccounts(origin, [account]),
162+
34163
addPermittedAccounts: (origin, accounts) =>
35164
addMoreAccounts(origin, accounts),
36165

37166
removePermittedAccount: (origin, account) => {
38-
const { value: existingAccounts } = permissionController.getCaveat(
39-
origin,
40-
RestrictedMethods.eth_accounts,
41-
CaveatTypes.restrictReturnedAccounts,
42-
);
167+
const caip25Caveat = getCaip25Caveat(origin);
168+
if (!caip25Caveat) {
169+
throw new Error(
170+
`Cannot remove account "${account}": No permissions exist for origin "${origin}".`,
171+
);
172+
}
173+
174+
const existingAccounts = getEthAccounts(caip25Caveat.value);
43175

44176
const remainingAccounts = existingAccounts.filter(
45177
(existingAccount) => existingAccount !== account,
@@ -52,73 +184,66 @@ export function getPermissionBackgroundApiMethods(permissionController) {
52184
if (remainingAccounts.length === 0) {
53185
permissionController.revokePermission(
54186
origin,
55-
RestrictedMethods.eth_accounts,
187+
Caip25EndowmentPermissionName,
56188
);
57189
} else {
190+
const updatedCaveatValue = setEthAccounts(
191+
caip25Caveat.value,
192+
remainingAccounts,
193+
);
58194
permissionController.updateCaveat(
59195
origin,
60-
RestrictedMethods.eth_accounts,
61-
CaveatTypes.restrictReturnedAccounts,
62-
remainingAccounts,
196+
Caip25EndowmentPermissionName,
197+
Caip25CaveatType,
198+
updatedCaveatValue,
63199
);
64200
}
65201
},
66202

67203
addPermittedChain: (origin, chainId) => addMoreChains(origin, [chainId]),
204+
68205
addPermittedChains: (origin, chainIds) => addMoreChains(origin, chainIds),
69206

70207
removePermittedChain: (origin, chainId) => {
71-
const { value: existingChains } = permissionController.getCaveat(
72-
origin,
73-
PermissionNames.permittedChains,
74-
CaveatTypes.restrictNetworkSwitching,
75-
);
208+
const caip25Caveat = getCaip25Caveat(origin);
209+
if (!caip25Caveat) {
210+
throw new Error(
211+
`Cannot remove permission for chainId "${chainId}": No permissions exist for origin "${origin}".`,
212+
);
213+
}
76214

77-
const remainingChains = existingChains.filter(
78-
(existingChain) => existingChain !== chainId,
215+
const existingEthChainIds = getPermittedEthChainIds(caip25Caveat.value);
216+
217+
const remainingChainIds = existingEthChainIds.filter(
218+
(existingChainId) => existingChainId !== chainId,
79219
);
80220

81-
if (remainingChains.length === existingChains.length) {
221+
if (remainingChainIds.length === existingEthChainIds.length) {
82222
return;
83223
}
84224

85-
if (remainingChains.length === 0) {
225+
if (remainingChainIds.length === 0 && !isSnapId(origin)) {
86226
permissionController.revokePermission(
87227
origin,
88-
PermissionNames.permittedChains,
228+
Caip25EndowmentPermissionName,
89229
);
90230
} else {
231+
const updatedCaveatValue = setPermittedEthChainIds(
232+
caip25Caveat.value,
233+
remainingChainIds,
234+
);
91235
permissionController.updateCaveat(
92236
origin,
93-
PermissionNames.permittedChains,
94-
CaveatTypes.restrictNetworkSwitching,
95-
remainingChains,
237+
Caip25EndowmentPermissionName,
238+
Caip25CaveatType,
239+
updatedCaveatValue,
96240
);
97241
}
98242
},
99243

100-
requestAccountsAndChainPermissionsWithId: async (origin) => {
244+
requestAccountsAndChainPermissionsWithId: (origin) => {
101245
const id = nanoid();
102-
permissionController.requestPermissions(
103-
{ origin },
104-
{
105-
[PermissionNames.eth_accounts]: {},
106-
[PermissionNames.permittedChains]: {},
107-
},
108-
{ id },
109-
);
110-
return id;
111-
},
112-
113-
requestAccountsPermissionWithId: async (origin) => {
114-
const id = nanoid();
115-
permissionController.requestPermissions(
116-
{ origin },
117-
{
118-
eth_accounts: {},
119-
},
120-
{ id },
121-
);
246+
requestAccountsAndChainPermissions(origin, id);
122247
return id;
123248
},
124249
};

0 commit comments

Comments
 (0)