Skip to content

Commit

Permalink
Jl/caip25 permission migration/update mutators (#28709)
Browse files Browse the repository at this point in the history
<!--
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**

Replaces existing caveat mutators. Handles ensuring `wallet:eip155` is
only upserted for permissions granted to snaps.

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

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **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**

- [ ] 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).
- [ ] 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
jiexi authored Nov 26, 2024
1 parent d96d85d commit d2fd282
Show file tree
Hide file tree
Showing 11 changed files with 33 additions and 189 deletions.
29 changes: 0 additions & 29 deletions app/scripts/controllers/permissions/background-api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,6 @@ describe('permission background API methods', () => {
'eip155:1:0x4',
],
},
'wallet:eip155': {
accounts: [
'wallet:eip155:0x1',
'wallet:eip155:0x2',
'wallet:eip155:0x3',
'wallet:eip155:0x4',
],
},
},
isMultichainOrigin: true,
},
Expand Down Expand Up @@ -245,15 +237,6 @@ describe('permission background API methods', () => {
'eip155:1:0x5',
],
},
'wallet:eip155': {
accounts: [
'wallet:eip155:0x1',
'wallet:eip155:0x2',
'wallet:eip155:0x3',
'wallet:eip155:0x4',
'wallet:eip155:0x5',
],
},
},
isMultichainOrigin: true,
},
Expand Down Expand Up @@ -424,9 +407,6 @@ describe('permission background API methods', () => {
'eip155:1': {
accounts: ['eip155:1:0x1', 'eip155:1:0x3'],
},
'wallet:eip155': {
accounts: ['wallet:eip155:0x1', 'wallet:eip155:0x3'],
},
},
isMultichainOrigin: true,
},
Expand Down Expand Up @@ -510,9 +490,6 @@ describe('permission background API methods', () => {
'eip155:5': {
accounts: ['eip155:5:0xdeadbeef'],
},
'wallet:eip155': {
accounts: ['wallet:eip155:0xdeadbeef'],
},
},
isMultichainOrigin: false,
},
Expand Down Expand Up @@ -619,9 +596,6 @@ describe('permission background API methods', () => {
'eip155:1337': {
accounts: ['eip155:1337:0x1', 'eip155:1337:0x2'],
},
'wallet:eip155': {
accounts: ['wallet:eip155:0x1', 'wallet:eip155:0x2'],
},
},
isMultichainOrigin: true,
},
Expand Down Expand Up @@ -727,9 +701,6 @@ describe('permission background API methods', () => {
'eip155:5': {
accounts: ['eip155:5:0x1', 'eip155:5:0x2'],
},
'wallet:eip155': {
accounts: ['wallet:eip155:0x1', 'wallet:eip155:0x2'],
},
},
isMultichainOrigin: true,
},
Expand Down
71 changes: 0 additions & 71 deletions app/scripts/controllers/permissions/caveat-mutators.js

This file was deleted.

67 changes: 0 additions & 67 deletions app/scripts/controllers/permissions/caveat-mutators.test.js

This file was deleted.

1 change: 0 additions & 1 deletion app/scripts/controllers/permissions/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export * from './caveat-mutators';
export * from './background-api';
export * from './enums';
export * from './specifications';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,18 @@ describe('requestEthereumAccountsHandler', () => {
expect(MockMultichain.setPermittedEthChainIds).not.toHaveBeenCalled();
});

it('sets the approved accounts on an empty CAIP-25 caveat with isMultichainOrigin: false if origin is snapId', async () => {
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: {},
optionalScopes: {
'wallet:eip155': {
accounts: [],
},
},
isMultichainOrigin: false,
},
['0xdeadbeef'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,13 @@ async function requestEthereumAccountsHandler(
isMultichainOrigin: false,
};

if (!isSnapId(origin)) {
if (isSnapId(origin)) {
caveatValue.optionalScopes = {
'wallet:eip155': {
accounts: [],
},
};
} else {
caveatValue = setPermittedEthChainIds(
caveatValue,
legacyApproval.approvedChainIds,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -579,14 +579,18 @@ describe('requestPermissionsHandler', () => {
expect(MockMultichain.setPermittedEthChainIds).not.toHaveBeenCalled();
});

it('sets the approved accounts on an empty CAIP-25 caveat with isMultichainOrigin: false if origin is snapId', async () => {
it('sets the approved accounts for the `wallet:eip155` scope with isMultichainOrigin: false if origin is snapId', async () => {
const { handler } = createMockedHandler();

await handler({ ...getBaseRequest(), origin: 'npm:snapm' });
expect(MockMultichain.setEthAccounts).toHaveBeenCalledWith(
{
requiredScopes: {},
optionalScopes: {},
optionalScopes: {
'wallet:eip155': {
accounts: [],
},
},
isMultichainOrigin: false,
},
['0xdeadbeef'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,14 @@ async function requestPermissionsImplementation(
optionalScopes: {},
isMultichainOrigin: false,
};
if (!isSnapId(origin)) {

if (isSnapId(origin)) {
newCaveatValue.optionalScopes = {
'wallet:eip155': {
accounts: [],
},
};
} else {
newCaveatValue = setPermittedEthChainIds(
newCaveatValue,
legacyApproval.approvedChainIds,
Expand Down
9 changes: 0 additions & 9 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,6 @@ import {
} from '../../shared/constants/hardware-wallets';
import { KeyringType } from '../../shared/constants/keyring';
import {
CaveatTypes,
RestrictedMethods,
EndowmentPermissions,
ExcludedSnapPermissions,
Expand Down Expand Up @@ -325,7 +324,6 @@ import EncryptionPublicKeyController from './controllers/encryption-public-key';
import AppMetadataController from './controllers/app-metadata';

import {
CaveatMutatorFactories,
getCaveatSpecifications,
diffMap,
getPermissionBackgroundApiMethods,
Expand Down Expand Up @@ -5148,13 +5146,6 @@ export default class MetamaskController extends EventEmitter {
* to third parties.
*/
removeAllAccountPermissions(targetAccount) {
this.permissionController.updatePermissionsByCaveat(
CaveatTypes.restrictReturnedAccounts,
(existingAccounts) =>
CaveatMutatorFactories[
CaveatTypes.restrictReturnedAccounts
].removeAccount(targetAccount, existingAccounts),
);
this.permissionController.updatePermissionsByCaveat(
Caip25CaveatType,
(existingScopes) =>
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@
"@metamask/message-manager": "^10.1.0",
"@metamask/message-signing-snap": "^0.4.0",
"@metamask/metamask-eth-abis": "^3.1.1",
"@metamask/multichain": "^1.0.0",
"@metamask/multichain": "^1.1.0",
"@metamask/name-controller": "^8.0.0",
"@metamask/network-controller": "patch:@metamask/network-controller@npm%3A21.0.0#~/.yarn/patches/@metamask-network-controller-npm-21.0.0-559aa8e395.patch",
"@metamask/notification-controller": "^6.0.0",
Expand Down
10 changes: 5 additions & 5 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5951,9 +5951,9 @@ __metadata:
languageName: node
linkType: hard

"@metamask/multichain@npm:^1.0.0":
version: 1.0.0
resolution: "@metamask/multichain@npm:1.0.0"
"@metamask/multichain@npm:^1.1.0":
version: 1.1.0
resolution: "@metamask/multichain@npm:1.1.0"
dependencies:
"@metamask/api-specs": "npm:^0.10.12"
"@metamask/controller-utils": "npm:^11.4.3"
Expand All @@ -5964,7 +5964,7 @@ __metadata:
peerDependencies:
"@metamask/network-controller": ^22.0.0
"@metamask/permission-controller": ^11.0.0
checksum: 10/ec4ae86bb91a002863bf2b50900488bcb3851981d9b5e91f4d14bba05acf89dd7fce23738b11b1c5079fd3e0edc3702849e28e5ecd59b99a1f9ddedf19d3fd14
checksum: 10/dab9a223c6cf1b11705f53cab3c1d6f2ecfb5f0c2b7b44d32847ea42b5cd2c3a9936b10239d3dbf0215b2656b3584a08a2ee979004cedd2edfb27813ec25665d
languageName: node
linkType: hard

Expand Down Expand Up @@ -26883,7 +26883,7 @@ __metadata:
"@metamask/message-manager": "npm:^10.1.0"
"@metamask/message-signing-snap": "npm:^0.4.0"
"@metamask/metamask-eth-abis": "npm:^3.1.1"
"@metamask/multichain": "npm:^1.0.0"
"@metamask/multichain": "npm:^1.1.0"
"@metamask/name-controller": "npm:^8.0.0"
"@metamask/network-controller": "patch:@metamask/network-controller@npm%3A21.0.0#~/.yarn/patches/@metamask-network-controller-npm-21.0.0-559aa8e395.patch"
"@metamask/notification-controller": "npm:^6.0.0"
Expand Down

0 comments on commit d2fd282

Please sign in to comment.