Skip to content

Commit

Permalink
feat: add wallet_switchEthereumChain permission and enforce it (beh…
Browse files Browse the repository at this point in the history
…ind feature flag) (MetaMask#24415)

## **Description**

This PR introduces the `wallet_switchEthereumChain` permission (as an
`endowment` type permission) behind a feature flag
(`CHAIN_PERMISSIONS`), allowing us to remove the `wallet_switchEthereum`
confirmation. Now instead of seeing this confirmation everytime
`wallet_switchEthereumChain` is called for a given chain, users sees a
confirmation to add a permission for the dapp to switch to this chain
after which they will no longer see any confirmation at all when the
dapp next attempts to switch to this chain.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24415?quickstart=1)


## **Related issues**

Fixes: MetaMask/MetaMask-planning#2327

## **Manual testing steps**

1. `CHAIN_PERMISSIONS=1 yarn start`
2. Go to a dapp (uniswap is good for this test) and connect the wallet
3. Use the in dapp switch chain UI
4. You should see a permission confirmation (instead of the old switch
chain confirmation)
- The UI is not fully implemented, this will be completed by the
`wallet-ux` team.
5. Manually switch to another chain in the wallet
6. Go back to the dapp and switch to the same chain you just added
permissions for
7. You should not see any confirmation but the network should change

## **Screenshots/Recordings**

### **Before**


https://github.com/MetaMask/metamask-extension/assets/34557516/6f6321ef-5fb5-48db-84a2-790cf178ea7f

### **After**


https://github.com/MetaMask/metamask-extension/assets/34557516/c94c67be-f17d-4b9d-aec0-431ea89500f5

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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/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.
  • Loading branch information
adonesky1 authored May 24, 2024
1 parent 40919a0 commit 671c997
Show file tree
Hide file tree
Showing 15 changed files with 1,396 additions and 529 deletions.
4 changes: 4 additions & 0 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions app/scripts/controllers/permissions/caveat-mutators.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ export const CaveatMutatorFactories = {
[CaveatTypes.restrictReturnedAccounts]: {
removeAccount,
},
[CaveatTypes.restrictNetworkSwitching]: {
removeChainId,
},
};

/**
Expand Down Expand Up @@ -39,3 +42,30 @@ function removeAccount(targetAccount, existingAccounts) {
}
return { operation: CaveatMutatorOperation.revokePermission };
}

/**
* Removes the target chain ID from the value arrays of all
* `restrictNetworkSwitching` caveats. No-ops if the target chain ID is not in
* the array, and revokes the parent permission if it's the only chain ID in
* the array.
*
* @param {string} targetChainId - The chain ID to remove from
* all network switching permissions.
* @param {string[]} existingChainIds - The chain ID array from the
* network switching permissions.
*/
function removeChainId(targetChainId, existingChainIds) {
const newChainIds = existingChainIds.filter(
(chainId) => chainId !== targetChainId,
);

if (newChainIds.length === existingChainIds.length) {
return { operation: CaveatMutatorOperation.noop };
} else if (newChainIds.length > 0) {
return {
operation: CaveatMutatorOperation.updateValue,
value: newChainIds,
};
}
return { operation: CaveatMutatorOperation.revokePermission };
}
90 changes: 85 additions & 5 deletions app/scripts/controllers/permissions/specifications.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
constructPermission,
PermissionType,
SubjectType,
} from '@metamask/permission-controller';
///: BEGIN:ONLY_INCLUDE_IF(snaps)
import {
Expand All @@ -23,18 +24,23 @@ import {
* The "keys" of all of permissions recognized by the PermissionController.
* Permission keys and names have distinct meanings in the permission system.
*/
const PermissionNames = Object.freeze({
export const PermissionNames = Object.freeze({
...RestrictedMethods,
permittedChains: 'permittedChains',
});

/**
* Factory functions for all caveat types recognized by the
* PermissionController.
*/
const CaveatFactories = Object.freeze({
export const CaveatFactories = Object.freeze({
[CaveatTypes.restrictReturnedAccounts]: (accounts) => {
return { type: CaveatTypes.restrictReturnedAccounts, value: accounts };
},

[CaveatTypes.restrictNetworkSwitching]: (chainIds) => {
return { type: CaveatTypes.restrictNetworkSwitching, value: chainIds };
},
});

/**
Expand All @@ -45,7 +51,10 @@ const CaveatFactories = Object.freeze({
* getInternalAccounts: () => Record<string, import('@metamask/keyring-api').InternalAccount>,
* }} options - Options bag.
*/
export const getCaveatSpecifications = ({ getInternalAccounts }) => {
export const getCaveatSpecifications = ({
getInternalAccounts,
findNetworkClientIdByChainId,
}) => {
return {
[CaveatTypes.restrictReturnedAccounts]: {
type: CaveatTypes.restrictReturnedAccounts,
Expand All @@ -60,6 +69,11 @@ export const getCaveatSpecifications = ({ getInternalAccounts }) => {
validator: (caveat, _origin, _target) =>
validateCaveatAccounts(caveat.value, getInternalAccounts),
},
[CaveatTypes.restrictNetworkSwitching]: {
type: CaveatTypes.restrictNetworkSwitching,
validator: (caveat, _origin, _target) =>
validateCaveatNetworks(caveat.value, findNetworkClientIdByChainId),
},

///: BEGIN:ONLY_INCLUDE_IF(snaps)
...snapsCaveatsSpecifications,
Expand Down Expand Up @@ -113,7 +127,6 @@ export const getPermissionSpecifications = ({
],
});
},

methodImplementation: async (_args) => {
const accounts = await getAllAccounts();
const internalAccounts = getInternalAccounts();
Expand Down Expand Up @@ -162,7 +175,6 @@ export const getPermissionSpecifications = ({
);
});
},

validator: (permission, _origin, _target) => {
const { caveats } = permission;
if (
Expand All @@ -176,6 +188,43 @@ export const getPermissionSpecifications = ({
}
},
},

[PermissionNames.permittedChains]: {
permissionType: PermissionType.Endowment,
targetName: PermissionNames.permittedChains,
allowedCaveats: [CaveatTypes.restrictNetworkSwitching],
subjectTypes: [SubjectType.Website],

factory: (permissionOptions, requestData) => {
if (!requestData.approvedChainIds) {
throw new Error(
`${PermissionNames.permittedChains}: No approved networks specified.`,
);
}

return constructPermission({
...permissionOptions,
caveats: [
CaveatFactories[CaveatTypes.restrictNetworkSwitching](
requestData.approvedChainIds,
),
],
});
},
endowmentGetter: async (_getterOptions) => undefined,
validator: (permission, _origin, _target) => {
const { caveats } = permission;
if (
!caveats ||
caveats.length !== 1 ||
caveats[0].type !== CaveatTypes.restrictNetworkSwitching
) {
throw new Error(
`${PermissionNames.permittedChains} error: Invalid caveats. There must be a single caveat of type "${CaveatTypes.restrictNetworkSwitching}".`,
);
}
},
},
};
};

Expand Down Expand Up @@ -216,6 +265,36 @@ function validateCaveatAccounts(accounts, getInternalAccounts) {
});
}

/**
* Validates the networks associated with a caveat. Ensures that
* the networks value is an array of valid chain IDs.
*
* @param {string[]} chainIdsForCaveat - The list of chain IDs to validate.
* @param {function(string): string} findNetworkClientIdByChainId - Function to find network client ID by chain ID.
* @throws {Error} If the chainIdsForCaveat is not a non-empty array of valid chain IDs.
*/
function validateCaveatNetworks(
chainIdsForCaveat,
findNetworkClientIdByChainId,
) {
if (!Array.isArray(chainIdsForCaveat) || chainIdsForCaveat.length === 0) {
throw new Error(
`${PermissionNames.permittedChains} error: Expected non-empty array of chainIds.`,
);
}

chainIdsForCaveat.forEach((chainId) => {
try {
findNetworkClientIdByChainId(chainId);
} catch (e) {
console.error(e);
throw new Error(
`${PermissionNames.permittedChains} error: Received unrecognized chainId: "${chainId}". Please try adding the network first via wallet_addEthereumChain.`,
);
}
});
}

/**
* All unrestricted methods recognized by the PermissionController.
* Unrestricted methods are ignored by the permission system, but every
Expand Down Expand Up @@ -278,6 +357,7 @@ export const unrestrictedMethods = Object.freeze([
'net_version',
'personal_ecRecover',
'personal_sign',
'wallet_switchEthereumChain',
'wallet_watchAsset',
'web3_clientVersion',
'web3_sha3',
Expand Down
10 changes: 8 additions & 2 deletions app/scripts/controllers/permissions/specifications.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@ describe('PermissionController specifications', () => {
describe('caveat specifications', () => {
it('getCaveatSpecifications returns the expected specifications object', () => {
const caveatSpecifications = getCaveatSpecifications({});
expect(Object.keys(caveatSpecifications)).toHaveLength(12);
expect(Object.keys(caveatSpecifications)).toHaveLength(13);
expect(
caveatSpecifications[CaveatTypes.restrictReturnedAccounts].type,
).toStrictEqual(CaveatTypes.restrictReturnedAccounts);
expect(
caveatSpecifications[CaveatTypes.restrictNetworkSwitching].type,
).toStrictEqual(CaveatTypes.restrictNetworkSwitching);

expect(caveatSpecifications.permittedDerivationPaths.type).toStrictEqual(
SnapCaveatType.PermittedDerivationPaths,
Expand Down Expand Up @@ -181,10 +184,13 @@ describe('PermissionController specifications', () => {
describe('permission specifications', () => {
it('getPermissionSpecifications returns the expected specifications object', () => {
const permissionSpecifications = getPermissionSpecifications({});
expect(Object.keys(permissionSpecifications)).toHaveLength(1);
expect(Object.keys(permissionSpecifications)).toHaveLength(2);
expect(
permissionSpecifications[RestrictedMethods.eth_accounts].targetName,
).toStrictEqual(RestrictedMethods.eth_accounts);
expect(permissionSpecifications.permittedChains.targetName).toStrictEqual(
'permittedChains',
);
});

describe('eth_accounts', () => {
Expand Down
Loading

0 comments on commit 671c997

Please sign in to comment.