diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 6fa47deddbe..1a9d81b84c5 100644 --- a/packages/accounts-controller/CHANGELOG.md +++ b/packages/accounts-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add support for `SnapKeyring` v2 accounts ([#8513](https://github.com/MetaMask/core/pull/8513)) + ### Changed - Bump `@metamask/messenger` from `^1.0.0` to `^1.1.1` ([#8364](https://github.com/MetaMask/core/pull/8364), [#8373](https://github.com/MetaMask/core/pull/8373)) diff --git a/packages/accounts-controller/package.json b/packages/accounts-controller/package.json index 2ae535e9097..e97458a9601 100644 --- a/packages/accounts-controller/package.json +++ b/packages/accounts-controller/package.json @@ -44,8 +44,8 @@ "build:docs": "typedoc", "changelog:update": "../../scripts/update-changelog.sh @metamask/accounts-controller", "changelog:validate": "../../scripts/validate-changelog.sh @metamask/accounts-controller", - "messenger-action-types:check": "tsx ../../packages/messenger-cli/src/cli.ts --check", - "messenger-action-types:generate": "tsx ../../packages/messenger-cli/src/cli.ts --generate", + "messenger-action-types:check": "tsx ../../packages/messenger-cli/src/cli.ts --formatter oxfmt --check", + "messenger-action-types:generate": "tsx ../../packages/messenger-cli/src/cli.ts --formatter oxfmt --generate", "since-latest-release": "../../scripts/since-latest-release.sh", "test": "NODE_OPTIONS=--experimental-vm-modules jest --reporters=jest-silent-reporter", "test:clean": "NODE_OPTIONS=--experimental-vm-modules jest --clearCache", diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 22150310a29..96595352fd1 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -3,6 +3,7 @@ import { InfuraNetworkType, toChecksumHexAddress, } from '@metamask/controller-utils'; +import { SnapKeyring as SnapKeyringV2 } from '@metamask/eth-snap-keyring/v2'; import type { AccountAssetListUpdatedEventPayload, AccountBalancesUpdatedEventPayload, @@ -16,6 +17,7 @@ import { EthScope, KeyringAccountEntropyTypeOption, } from '@metamask/keyring-api'; +import { KeyringType } from '@metamask/keyring-api/v2'; import { KeyringControllerState, KeyringTypes, @@ -1068,6 +1070,182 @@ describe('AccountsController', () => { ]); }); + it('add Snap v2 accounts', () => { + const mockSnapV2Address = '0xabcdef1234567890abcdef1234567890abcdef12'; + const mockSnapV2SnapId = 'mock-snap-v2-id'; + const mockSnapV2AccountId = 'mock-snap-v2-account-id'; + + const mockSnapV2KeyringAccount = { + id: mockSnapV2AccountId, + address: mockSnapV2Address, + options: {}, + methods: [...ETH_EOA_METHODS], + type: EthAccountType.Eoa, + scopes: [EthScope.Eoa], + }; + + const mockKeyringV2Instance = Object.assign( + Object.create(SnapKeyringV2.prototype), + { + snapId: mockSnapV2SnapId, + lookupByAddress: jest + .fn() + .mockReturnValue(mockSnapV2KeyringAccount), + }, + ); + + const messenger = buildMessenger(); + messenger.registerActionHandler( + 'KeyringController:getKeyringsByType', + mockGetKeyringByType.mockReturnValue([mockKeyringV2Instance]), + ); + + const mockNewKeyringState = { + isUnlocked: true, + keyrings: [ + { + type: KeyringType.Snap, + accounts: [mockSnapV2Address], + metadata: { + id: 'mock-keyring-v2-id', + name: 'mock-keyring-v2-name', + }, + }, + ], + }; + + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + accountIdByAddress: {}, + }, + messenger, + }); + + messenger.publish( + 'KeyringController:stateChange', + mockNewKeyringState, + [], + ); + + const accounts = accountsController.listMultichainAccounts(); + + expect(accounts).toHaveLength(1); + expect(accounts[0]).toMatchObject({ + id: mockSnapV2AccountId, + address: mockSnapV2Address, + metadata: { + keyring: { type: KeyringType.Snap }, + snap: { + id: mockSnapV2SnapId, + name: mockSnapV2SnapId, + enabled: true, + }, + importTime: expect.any(Number), + }, + }); + }); + + it('handles the event when a Snap v2 deleted the account before it was added', () => { + const mockSnapV2Address = '0xabcdef1234567890abcdef1234567890abcdef12'; + + const mockKeyringV2Instance = Object.assign( + Object.create(SnapKeyringV2.prototype), + { + snapId: 'mock-snap-v2-id', + lookupByAddress: jest.fn().mockReturnValue(undefined), + }, + ); + + const messenger = buildMessenger(); + messenger.registerActionHandler( + 'KeyringController:getKeyringsByType', + mockGetKeyringByType.mockReturnValue([mockKeyringV2Instance]), + ); + + const mockNewKeyringState = { + isUnlocked: true, + keyrings: [ + { + type: KeyringType.Snap, + accounts: [mockSnapV2Address], + metadata: { + id: 'mock-keyring-v2-id', + name: 'mock-keyring-v2-name', + }, + }, + ], + }; + + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + accountIdByAddress: {}, + }, + messenger, + }); + + messenger.publish( + 'KeyringController:stateChange', + mockNewKeyringState, + [], + ); + + expect(accountsController.listMultichainAccounts()).toStrictEqual([]); + }); + + it('handles when no SnapKeyringV2 instance matches for a Snap v2 keyring type', () => { + const mockSnapV2Address = '0xabcdef1234567890abcdef1234567890abcdef12'; + + const messenger = buildMessenger(); + messenger.registerActionHandler( + 'KeyringController:getKeyringsByType', + mockGetKeyringByType.mockReturnValue([ + // Plain object — does NOT pass instanceof SnapKeyringV2 + { lookupByAddress: jest.fn() }, + ]), + ); + + const mockNewKeyringState = { + isUnlocked: true, + keyrings: [ + { + type: KeyringType.Snap, + accounts: [mockSnapV2Address], + metadata: { + id: 'mock-keyring-v2-id', + name: 'mock-keyring-v2-name', + }, + }, + ], + }; + + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + accountIdByAddress: {}, + }, + messenger, + }); + + messenger.publish( + 'KeyringController:stateChange', + mockNewKeyringState, + [], + ); + + expect(accountsController.listMultichainAccounts()).toStrictEqual([]); + }); + it('increment the default account number when adding an account', async () => { const messenger = buildMessenger(); @@ -3295,6 +3473,119 @@ describe('AccountsController', () => { expect(mockGetKeyringByType).toHaveBeenCalledTimes(1); }); + it('update accounts with Snap v2 accounts', async () => { + const mockSnapV2Address = '0xabcdef1234567890abcdef1234567890abcdef12'; + const mockSnapV2SnapId = 'mock-snap-v2-id'; + const mockSnapV2AccountId = 'mock-snap-v2-account-id'; + + const mockSnapV2KeyringAccount = { + id: mockSnapV2AccountId, + address: mockSnapV2Address, + options: {}, + methods: [...ETH_EOA_METHODS], + type: EthAccountType.Eoa, + scopes: [EthScope.Eoa], + }; + + const mockKeyringV2Instance = Object.assign( + Object.create(SnapKeyringV2.prototype), + { + snapId: mockSnapV2SnapId, + lookupByAddress: jest.fn().mockReturnValue(mockSnapV2KeyringAccount), + }, + ); + + const messenger = buildMessenger(); + messenger.registerActionHandler( + 'KeyringController:getState', + mockGetState.mockReturnValue({ + keyrings: [ + { + type: KeyringType.Snap, + accounts: [mockSnapV2Address], + metadata: { + id: 'mock-keyring-v2-id', + name: 'mock-keyring-v2-name', + }, + }, + ], + }), + ); + messenger.registerActionHandler( + 'KeyringController:getKeyringsByType', + mockGetKeyringByType.mockReturnValue([mockKeyringV2Instance]), + ); + + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + accountIdByAddress: {}, + }, + messenger, + }); + + await accountsController.updateAccounts(); + + const accounts = accountsController.listMultichainAccounts(); + expect(accounts).toHaveLength(1); + expect(accounts[0]).toMatchObject({ + id: mockSnapV2AccountId, + address: mockSnapV2Address, + metadata: { + name: 'Snap Account 1', + keyring: { type: KeyringType.Snap }, + snap: { + id: mockSnapV2SnapId, + name: mockSnapV2SnapId, + enabled: true, + }, + }, + }); + }); + + it('skips Snap v2 account if no SnapKeyringV2 instance is found', async () => { + const mockSnapV2Address = '0xabcdef1234567890abcdef1234567890abcdef12'; + + const messenger = buildMessenger(); + messenger.registerActionHandler( + 'KeyringController:getState', + mockGetState.mockReturnValue({ + keyrings: [ + { + type: KeyringType.Snap, + accounts: [mockSnapV2Address], + metadata: { + id: 'mock-keyring-v2-id', + name: 'mock-keyring-v2-name', + }, + }, + ], + }), + ); + messenger.registerActionHandler( + 'KeyringController:getKeyringsByType', + mockGetKeyringByType.mockReturnValue([]), + ); + + const { accountsController } = setupAccountsController({ + initialState: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + accountIdByAddress: {}, + }, + messenger, + }); + + await accountsController.updateAccounts(); + + expect(accountsController.listMultichainAccounts()).toStrictEqual([]); + }); + it.todo( 'does not re-fire a accountChanged event if the account is still the same', ); diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts index b94d28f99d7..50b0934f9a0 100644 --- a/packages/accounts-controller/src/AccountsController.ts +++ b/packages/accounts-controller/src/AccountsController.ts @@ -9,6 +9,7 @@ import type { SnapKeyringAccountBalancesUpdatedEvent, SnapKeyringAccountTransactionsUpdatedEvent, } from '@metamask/eth-snap-keyring'; +import { SnapKeyring as SnapKeyringV2 } from '@metamask/eth-snap-keyring/v2'; import type { KeyringAccountEntropyOptions } from '@metamask/keyring-api'; import { EthAccountType, @@ -17,6 +18,7 @@ import { isEvmAccountType, KeyringAccountEntropyTypeOption, } from '@metamask/keyring-api'; +import { KeyringType } from '@metamask/keyring-api/v2'; import type { KeyringControllerState, KeyringControllerGetKeyringsByTypeAction, @@ -52,6 +54,7 @@ import { isHdSnapKeyringAccount, isMoneyKeyringType, isSnapKeyringType, + isSnapKeyringV2Type, keyringTypeToName, } from './utils'; @@ -847,6 +850,49 @@ export class AccountsController extends BaseController< return snapKeyring as SnapKeyring | undefined; } + /** + * Get an account from a Snap keyring v2. + * + * @param address - The address of the account to retrieve. + * @returns The Snap account if available. + */ + #getAccountFromSnapKeyringV2(address: string): InternalAccount | undefined { + const keyrings = this.messenger.call( + 'KeyringController:getKeyringsByType', + KeyringType.Snap, + ); + + // Snap keyring v2 are "per-Snaps", so we need to iterate over all of them to find the account. + for (const keyring of keyrings) { + if (keyring instanceof SnapKeyringV2) { + // We use the synchronous method here since this method is used during `:stateChange` that are + // use synchronous handlers. + const account = keyring.lookupByAddress(address); + if (account) { + return { + ...account, + // We still have to use internal account for now, so we inject some metadata. + metadata: { + name: '', + importTime: Date.now(), + lastSelected: 0, + keyring: { + type: KeyringType.Snap, + }, + snap: { + name: keyring.snapId, + enabled: true, + id: keyring.snapId, + }, + }, + }; + } + } + } + + return undefined; + } + /** * Re-publish an account event. * @@ -887,32 +933,16 @@ export class AccountsController extends BaseController< log('Synchronizing accounts with keyrings (through :stateChange)...'); // State patches. - const generatePatch = (): StatePatch => { - return { - previous: {}, - added: [], - updated: [], - removed: [], - }; - }; - const patches = { - snap: generatePatch(), - normal: generatePatch(), - }; - - // Gets the patch object based on the keyring type (since Snap accounts and other accounts - // are handled differently). - const patchOf = (type: string): StatePatch => { - if (isSnapKeyringType(type)) { - return patches.snap; - } - return patches.normal; + const patch: StatePatch = { + previous: {}, + added: [], + updated: [], + removed: [], }; // Create a map (with lower-cased addresses) of all existing accounts. for (const account of this.listMultichainAccounts()) { const address = account.address.toLowerCase(); - const patch = patchOf(account.metadata.keyring.type); patch.previous[address] = account; } @@ -926,8 +956,6 @@ export class AccountsController extends BaseController< continue; } - const patch = patchOf(keyring.type); - for (const accountAddress of keyring.accounts) { // Lower-case address to use it in the `previous` map. const address = accountAddress.toLowerCase(); @@ -951,12 +979,10 @@ export class AccountsController extends BaseController< // We might have accounts associated with removed keyrings, so we iterate // over all previous known accounts and check against the keyring addresses. - for (const patch of [patches.snap, patches.normal]) { - for (const [address, account] of Object.entries(patch.previous)) { - // If a previous address is not part of the new addesses, then it got removed. - if (!addresses.has(address)) { - patch.removed.push(account); - } + for (const [address, account] of Object.entries(patch.previous)) { + // If a previous address is not part of the new addesses, then it got removed. + if (!addresses.has(address)) { + patch.removed.push(account); } } @@ -970,42 +996,40 @@ export class AccountsController extends BaseController< (state) => { const { internalAccounts, accountIdByAddress } = state; - for (const patch of [patches.snap, patches.normal]) { - for (const account of patch.removed) { - delete internalAccounts.accounts[account.id]; - delete accountIdByAddress[account.address]; + for (const account of patch.removed) { + delete internalAccounts.accounts[account.id]; + delete accountIdByAddress[account.address]; - diff.removed.push(account.id); - } + diff.removed.push(account.id); + } + + for (const added of patch.added) { + const account = this.#getInternalAccountFromAddressAndType( + added.address, + added.keyring, + ); + + if (account) { + const accounts = Object.values( + internalAccounts.accounts, + ) as InternalAccount[]; + + // If it's the first account, we need to select it. + const lastSelected = + accounts.length === 0 ? this.#getLastSelectedIndex() : 0; + + internalAccounts.accounts[account.id] = { + ...account, + metadata: { + ...account.metadata, + importTime: Date.now(), + lastSelected, + }, + }; + + accountIdByAddress[account.address] = account.id; - for (const added of patch.added) { - const account = this.#getInternalAccountFromAddressAndType( - added.address, - added.keyring, - ); - - if (account) { - const accounts = Object.values( - internalAccounts.accounts, - ) as InternalAccount[]; - - // If it's the first account, we need to select it. - const lastSelected = - accounts.length === 0 ? this.#getLastSelectedIndex() : 0; - - internalAccounts.accounts[account.id] = { - ...account, - metadata: { - ...account.metadata, - importTime: Date.now(), - lastSelected, - }, - }; - - accountIdByAddress[account.address] = account.id; - - diff.added.push(internalAccounts.accounts[account.id]); - } + diff.added.push(internalAccounts.accounts[account.id]); } } }, @@ -1192,17 +1216,27 @@ export class AccountsController extends BaseController< address: string, keyring: KeyringObject, ): InternalAccount | undefined { - if (isSnapKeyringType(keyring.type)) { - const snapKeyring = this.#getSnapKeyring(); + const isSnapKeyringV1 = isSnapKeyringType(keyring.type); + const isSnapKeyringV2 = isSnapKeyringV2Type(keyring.type); + + if (isSnapKeyringV1 || isSnapKeyringV2) { + let account: InternalAccount | undefined; + + if (isSnapKeyringV1) { + const snapKeyring = this.#getSnapKeyring(); + + // We need the Snap keyring to retrieve the account from its address. + if (!snapKeyring) { + return undefined; + } - // We need the Snap keyring to retrieve the account from its address. - if (!snapKeyring) { - return undefined; + // This might be undefined if the Snap deleted the account before + // reaching that point. + account = snapKeyring.getAccountByAddress(address); + } else { + account = this.#getAccountFromSnapKeyringV2(address); } - // This might be undefined if the Snap deleted the account before - // reaching that point. - let account = snapKeyring.getAccountByAddress(address); if (account) { // We force the copy here, to avoid mutating the reference returned by the Snap keyring. account = cloneDeep(account); diff --git a/packages/accounts-controller/src/utils.test.ts b/packages/accounts-controller/src/utils.test.ts index ad640ac010d..6081f94af52 100644 --- a/packages/accounts-controller/src/utils.test.ts +++ b/packages/accounts-controller/src/utils.test.ts @@ -1,4 +1,5 @@ import { toChecksumAddress } from '@ethereumjs/util'; +import { KeyringType } from '@metamask/keyring-api/v2'; import type { KeyringObject } from '@metamask/keyring-controller'; import { KeyringTypes } from '@metamask/keyring-controller'; @@ -9,6 +10,7 @@ import { isMoneyKeyringType, isNormalKeyringType, isSimpleKeyringType, + isSnapKeyringV2Type, keyringTypeToName, } from './utils'; @@ -23,6 +25,7 @@ describe('utils', () => { [KeyringTypes.lattice, 'Lattice'], [KeyringTypes.qr, 'QR'], [KeyringTypes.snap, 'Snap Account'], + [KeyringType.Snap, 'Snap Account'], [KeyringTypes.money, 'Money'], ])('returns "%s" for %s keyring type', (keyringType, expectedName) => { expect(keyringTypeToName(keyringType)).toBe(expectedName); @@ -84,6 +87,22 @@ describe('utils', () => { }); }); + describe('isSnapKeyringV2Type', () => { + it('returns true for KeyringType.Snap', () => { + expect(isSnapKeyringV2Type(KeyringType.Snap)).toBe(true); + }); + + it('returns false for the v1 snap keyring type', () => { + expect(isSnapKeyringV2Type(KeyringTypes.snap)).toBe(false); + }); + + it('returns false for non-snap keyring types', () => { + expect(isSnapKeyringV2Type(KeyringTypes.hd)).toBe(false); + expect(isSnapKeyringV2Type(KeyringTypes.simple)).toBe(false); + expect(isSnapKeyringV2Type(KeyringTypes.trezor)).toBe(false); + }); + }); + describe('getGroupIndexFromAddressIndex', () => { const keyring: KeyringObject = { type: KeyringTypes.hd, diff --git a/packages/accounts-controller/src/utils.ts b/packages/accounts-controller/src/utils.ts index c961ace733a..31f5755fbc0 100644 --- a/packages/accounts-controller/src/utils.ts +++ b/packages/accounts-controller/src/utils.ts @@ -1,3 +1,4 @@ +import { KeyringType } from '@metamask/keyring-api/v2'; import type { KeyringObject } from '@metamask/keyring-controller'; import { KeyringTypes } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; @@ -39,6 +40,7 @@ export function keyringTypeToName(keyringType: string): string { case KeyringTypes.qr: { return 'QR'; } + case KeyringType.Snap: case KeyringTypes.snap: { return 'Snap Account'; } @@ -103,6 +105,18 @@ export function isSnapKeyringType(keyringType: KeyringTypes | string): boolean { return keyringType === (KeyringTypes.snap as string); } +/** + * Check if a keyring type is a Snap keyring. + * + * @param keyringType - The account's keyring type. + * @returns True if the keyring type is considered a Snap keyring, false otherwise. + */ +export function isSnapKeyringV2Type( + keyringType: KeyringTypes | KeyringType | string, +): boolean { + return keyringType === (KeyringType.Snap as string); +} + /** * Check if a keyring type is a simple keyring. *