diff --git a/packages/accounts-controller/CHANGELOG.md b/packages/accounts-controller/CHANGELOG.md index 2c861585c1..375c685d30 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/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index 080da331f3..ff8ff99cbc 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, @@ -250,6 +252,29 @@ function setExpectedLastSelectedAsAny( .get(); } +/** + * Builds a mock SnapKeyringV2 instance with the given `snapId` and overrides. + * + * @param snapId - The snap ID to use for the mock instance. + * @param overrides - Optional overrides for the mock instance. + * @returns A mock SnapKeyringV2 instance. + */ +function buildMockSnapKeyringV2( + snapId: string, + overrides: Partial = {}, +): SnapKeyringV2 { + // We need to use the same prototype as we use some `instanceof` checks. + const instance = Object.create(SnapKeyringV2.prototype) as SnapKeyringV2; + + // The `snapId` is usually injected via `deserialize`, but here we just mock the getter directly. + Object.defineProperty(instance, 'snapId', { + value: snapId, + configurable: true, + }); + + return Object.assign(instance, overrides); +} + /** * Builds a new instance of the root messenger. * @@ -831,6 +856,178 @@ 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 mockSnapKeyringV2Instance = buildMockSnapKeyringV2( + mockSnapV2SnapId, + { + lookupByAddress: jest + .fn() + .mockReturnValue(mockSnapV2KeyringAccount), + }, + ); + + const messenger = buildMessenger(); + messenger.registerActionHandler( + 'KeyringController:getKeyringsByType', + mockGetKeyringByType.mockReturnValue([mockSnapKeyringV2Instance]), + ); + + 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, + }, + importTime: expect.any(Number), + }, + }); + }); + + it('handles the event when a Snap v2 deleted the account before it was added', () => { + const mockSnapV2Address = '0xabcdef1234567890abcdef1234567890abcdef12'; + + const mockSnapKeyringV2Instance = buildMockSnapKeyringV2( + 'mock-snap-v2-id', + { + lookupByAddress: jest.fn().mockReturnValue(undefined), + }, + ); + + const messenger = buildMessenger(); + messenger.registerActionHandler( + 'KeyringController:getKeyringsByType', + mockGetKeyringByType.mockReturnValue([mockSnapKeyringV2Instance]), + ); + + 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(); @@ -3058,6 +3255,116 @@ 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 mockSnapKeyringV2Instance = buildMockSnapKeyringV2( + 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([mockSnapKeyringV2Instance]), + ); + + 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, + }, + }, + }); + }); + + 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 5ea25624cb..f4befa2b70 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, @@ -47,6 +49,7 @@ import { isHdSnapKeyringAccount, isMoneyKeyringType, isSnapKeyringType, + isSnapKeyringV2Type, keyringTypeToName, } from './utils'; @@ -841,6 +844,66 @@ export class AccountsController extends BaseController< return snapKeyring as SnapKeyring | undefined; } + /** + * Get an account from a Snap keyring v1. + * + * @param address - The address of the account to retrieve. + * @returns The Snap account if available. + */ + #getAccountFromSnapKeyringV1(address: string): InternalAccount | undefined { + const snapKeyring = this.#getSnapKeyring(); + + // 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. + return snapKeyring.getAccountByAddress(address); + } + + /** + * 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: { + id: keyring.snapId, + }, + }, + }; + } + } + } + + return undefined; + } + /** * Re-publish an account event. * @@ -881,32 +944,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; } @@ -920,8 +967,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(); @@ -945,12 +990,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); } } @@ -964,42 +1007,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, + ); - 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]); - } + 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]); } } }, @@ -1145,17 +1186,18 @@ 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; - // We need the Snap keyring to retrieve the account from its address. - if (!snapKeyring) { - return undefined; + if (isSnapKeyringV1) { + account = this.#getAccountFromSnapKeyringV1(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 ad640ac010..66d22cd462 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); @@ -52,6 +55,10 @@ describe('utils', () => { expect(isNormalKeyringType(snapKeyringType)).toBe(false); }); + it('returns false for snap keyring type (v2)', () => { + expect(isNormalKeyringType(KeyringType.Snap)).toBe(false); + }); + it('returns false for money keyring type', () => { expect(isNormalKeyringType(moneyKeyringType)).toBe(false); }); @@ -84,6 +91,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 c961ace733..d10ec9538c 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'; } @@ -90,7 +92,11 @@ export function getUUIDFromAddressOfNormalAccount(address: string): string { export function isNormalKeyringType( keyringType: KeyringTypes | string, ): boolean { - return !isSnapKeyringType(keyringType) && !isMoneyKeyringType(keyringType); + return ( + !isSnapKeyringType(keyringType) && + !isSnapKeyringV2Type(keyringType) && + !isMoneyKeyringType(keyringType) + ); } /** @@ -103,6 +109,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. *