diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index 88e7f0a38a..7515fd580b 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** Replace `KeyringController:withKeyring` with `KeyringController:withKeyringV2` for the EVM account provider ([#8491](https://github.com/MetaMask/core/pull/8491)) - Bump `@metamask/accounts-controller` from `^37.1.1` to `^38.0.0` ([#8363](https://github.com/MetaMask/core/pull/8363), [#8665](https://github.com/MetaMask/core/pull/8665)) - Bump `@metamask/keyring-controller` from `^25.1.1` to `^25.4.0` ([#8363](https://github.com/MetaMask/core/pull/8363), [#8634](https://github.com/MetaMask/core/pull/8634), [#8665](https://github.com/MetaMask/core/pull/8665)) - Bump `@metamask/messenger` from `^1.0.0` to `^1.2.0` ([#8364](https://github.com/MetaMask/core/pull/8364), [#8373](https://github.com/MetaMask/core/pull/8373), [#8632](https://github.com/MetaMask/core/pull/8632)) diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index d8b3456d11..faa2d954f9 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -11,8 +11,9 @@ import { SolAccountType, TrxAccountType, } from '@metamask/keyring-api'; +import type { Keyring } from '@metamask/keyring-api/v2'; +import { KeyringType } from '@metamask/keyring-api/v2'; import type { KeyringObject } from '@metamask/keyring-controller'; -import type { EthKeyring } from '@metamask/keyring-internal-api'; import { traceFallback } from './analytics'; import { isPerfEnabled, withLocalPerfTrace } from './analytics/perf'; @@ -1537,13 +1538,13 @@ describe('MultichainAccountService', () => { ); rootMessenger.registerActionHandler( - 'KeyringController:withKeyring', + 'KeyringController:withKeyringV2', async (_, operation) => { const newKeyring = mocks.KeyringController.keyrings.find( - (keyring) => keyring.type === 'HD Key Tree', + (keyring) => keyring.type === KeyringType.Hd, ) as KeyringObject; return operation({ - keyring: {} as unknown as EthKeyring, + keyring: {} as unknown as Keyring, metadata: newKeyring.metadata, }); }, @@ -1580,13 +1581,13 @@ describe('MultichainAccountService', () => { ); rootMessenger.registerActionHandler( - 'KeyringController:withKeyring', + 'KeyringController:withKeyringV2', async (_, operation) => { const newKeyring = mocks.KeyringController.keyrings.find( (keyring) => keyring.type === 'HD Key Tree', ) as KeyringObject; return operation({ - keyring: {} as unknown as EthKeyring, + keyring: {} as unknown as Keyring, metadata: newKeyring.metadata, }); }, diff --git a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts index 9f27f2fc72..5ac23e82d7 100644 --- a/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts +++ b/packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts @@ -5,10 +5,14 @@ import type { EntropySourceId, KeyringAccount, } from '@metamask/keyring-api'; -import type { KeyringCapabilities } from '@metamask/keyring-api/v2'; +import type { + Keyring as KeyringV2, + KeyringCapabilities, +} from '@metamask/keyring-api/v2'; import type { KeyringMetadata, KeyringSelector, + KeyringSelectorV2, } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; @@ -155,6 +159,17 @@ export abstract class BaseBip44AccountProvider< ) as unknown as Account; } + /** + * Run an operation against a V1 keyring selected by `selector`. + * + * Forwards to `KeyringController:withKeyring`. Use this for keyrings that + * have not yet migrated to the unified V2 `Keyring` interface (e.g. the + * snap keyring). + * + * @param selector - The selector identifying the keyring. + * @param operation - The operation to run with the selected keyring. + * @returns The result of the operation. + */ protected async withKeyring( selector: KeyringSelector, operation: ({ @@ -178,6 +193,43 @@ export abstract class BaseBip44AccountProvider< return result as CallbackResult; } + /** + * Run an operation against a V2 keyring selected by `selector`. + * + * Forwards to `KeyringController:withKeyringV2`. Use this for keyrings + * that implement the unified V2 `Keyring` interface from + * `@metamask/keyring-api/v2`. + * + * @param selector - The selector identifying the keyring. + * @param operation - The operation to run with the selected keyring. + * @returns The result of the operation. + */ + protected async withKeyringV2< + SelectedKeyring extends KeyringV2 = KeyringV2, + CallbackResult = void, + >( + selector: KeyringSelectorV2, + operation: ({ + keyring, + metadata, + }: { + keyring: SelectedKeyring; + metadata: KeyringMetadata; + }) => Promise, + ): Promise { + const result = await this.messenger.call( + 'KeyringController:withKeyringV2', + selector, + ({ keyring, metadata }) => + operation({ + keyring: keyring as SelectedKeyring, + metadata, + }), + ); + + return result as CallbackResult; + } + abstract get capabilities(): KeyringCapabilities; abstract getName(): string; diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index ccd9cac13c..9c668155dd 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -1,21 +1,24 @@ import { publicToAddress } from '@ethereumjs/util'; import { isBip44Account } from '@metamask/account-api'; -import { getUUIDFromAddressOfNormalAccount } from '@metamask/accounts-controller'; -import { AccountCreationType } from '@metamask/keyring-api'; -import type { KeyringMetadata } from '@metamask/keyring-controller'; +import { HdKeyring as LegacyHdKeyring } from '@metamask/eth-hd-keyring'; +import { AccountCreationType, EthScope } from '@metamask/keyring-api'; import type { - EthKeyring, - InternalAccount, -} from '@metamask/keyring-internal-api'; + CreateAccountOptions, + KeyringAccount, +} from '@metamask/keyring-api'; +import type { Keyring } from '@metamask/keyring-api/v2'; +import { KeyringType } from '@metamask/keyring-api/v2'; +import type { KeyringMetadata } from '@metamask/keyring-controller'; +import type { InternalAccount } from '@metamask/keyring-internal-api'; import type { AutoManagedNetworkClient, CustomNetworkClientConfiguration, } from '@metamask/network-controller'; -import type { Hex } from '@metamask/utils'; -import { createBytes } from '@metamask/utils'; +import { add0x, bytesToHex } from '@metamask/utils'; import { TraceName } from '../analytics/traces'; import { + asKeyringAccount, getMultichainAccountServiceMessenger, getRootMessenger, MOCK_HD_ACCOUNT_1, @@ -34,90 +37,120 @@ import { } from './EvmAccountProvider'; import { TimeoutError } from './utils'; -jest.mock('@ethereumjs/util', () => { - const actual = jest.requireActual('@ethereumjs/util'); - return { - ...actual, - publicToAddress: jest.fn(), - }; -}); +// Real HD root rooted at a valid BIP-39 test mnemonic so the address peeked via +// `keyring.root.deriveChild(groupIndex)` matches the address that the mock's +// `createAccounts` later returns at the same index. +const TEST_MNEMONIC = + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; +let mockHdRoot: NonNullable; -function mockNextDiscoveryAddress(address: string): void { - jest.mocked(publicToAddress).mockReturnValue(createBytes(address as Hex)); +/** + * Derives the EVM address for a given group index using the test mnemonic. + * + * @param groupIndex - The BIP-44 group index. + * @returns The lowercase hex address. + */ +function deriveAddressForIndex(groupIndex: number): string { + const child = mockHdRoot.deriveChild(groupIndex); + if (!child.publicKey) { + throw new Error('Expected derived public key to be set'); + } + return add0x( + bytesToHex(publicToAddress(child.publicKey, true)).toLowerCase(), + ); } -function mockNextDiscoveryAddressOnce(address: string): void { - jest.mocked(publicToAddress).mockReturnValueOnce(createBytes(address as Hex)); +/** + * Builds an HD account fixture whose address matches what + * `mockHdRoot.deriveChild(groupIndex)` would derive. + * + * @param groupIndex - The BIP-44 group index. + * @returns A Bip44 InternalAccount fixture for the index. + */ +function makeDerivedHdAccount(groupIndex: number): InternalAccount { + return MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withUuid() + .withAddress(deriveAddressForIndex(groupIndex)) + .withGroupIndex(groupIndex) + .get(); } -type MockHdKey = { - deriveChild: jest.Mock; -}; +// Mock V2 HD Keyring implementing the Keyring interface from @metamask/keyring-api/v2. +class MockHdKeyringV2 implements Keyring { + readonly type = KeyringType.Hd; -function mockHdKey(): MockHdKey { - return { - deriveChild: jest.fn().mockImplementation(() => { - return { - publicKey: new Uint8Array(65), - }; - }), + readonly capabilities = { + scopes: [EthScope.Eoa], + bip44: { deriveIndex: true }, }; -} -class MockEthKeyring implements EthKeyring { - readonly type = 'MockEthKeyring'; + // Internal test-only state — not part of the Keyring interface. + readonly accounts: KeyringAccount[]; readonly metadata: KeyringMetadata = { id: 'mock-eth-keyring-id', name: '', }; - readonly accounts: InternalAccount[]; - - readonly root: MockHdKey; - constructor(accounts: InternalAccount[]) { - this.accounts = accounts; - this.root = mockHdKey(); + this.accounts = accounts.map( + ({ metadata, ...keyringAccount }) => keyringAccount, + ); } - async serialize(): Promise { - return 'serialized'; + /** + * The HD root that the EVM provider uses to peek the next address + * (via `root.deriveChild(groupIndex)`) without persisting an account. + * + * @returns The HD root derived from the test mnemonic. + */ + get root(): NonNullable { + return mockHdRoot; } - async deserialize(_: string): Promise { - // Not required. - } + getAccounts = jest.fn().mockImplementation(() => this.accounts); - getAccounts = jest - .fn() - .mockImplementation(() => this.accounts.map((account) => account.address)); - - addAccounts = jest.fn().mockImplementation((numberOfAccounts: number) => { - const newAccountsIndex = this.accounts.length; - - // Just generate a new address by appending the number of accounts owned by that fake keyring. - for (let i = 0; i < numberOfAccounts; i++) { - this.accounts.push( - MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withUuid() - .withAddressSuffix(`${this.accounts.length}`) - .withGroupIndex(this.accounts.length) - .get(), - ); + getAccount = jest.fn().mockImplementation((accountId: string) => { + const account = this.accounts.find((a) => a.id === accountId); + if (!account) { + throw new Error(`Account not found: ${accountId}`); } - - return this.accounts - .slice(newAccountsIndex) - .map((account) => account.address); + return account; }); - removeAccount = jest.fn().mockImplementation((address: string) => { - const index = this.accounts.findIndex((a) => a.address === address); + createAccounts = jest + .fn() + .mockImplementation((options: CreateAccountOptions) => { + const newAccounts: KeyringAccount[] = []; + + if (options.type === AccountCreationType.Bip44DeriveIndex) { + // Derive at the caller-supplied `groupIndex` (rather than + // `this.accounts.length`) so that a production bug forwarding the + // wrong index would surface as an address/identity mismatch in + // tests, instead of being masked by the mock re-deriving + // sequentially. + const { groupIndex } = options; + const { metadata, ...keyringAccount } = + makeDerivedHdAccount(groupIndex); + this.accounts.push(keyringAccount); + newAccounts.push(keyringAccount); + } + + return newAccounts; + }); + + deleteAccount = jest.fn().mockImplementation((accountId: string) => { + const index = this.accounts.findIndex((a) => a.id === accountId); if (index >= 0) { this.accounts.splice(index, 1); } }); + + serialize = jest.fn().mockResolvedValue({}); + + deserialize = jest.fn().mockResolvedValue(undefined); + + submitRequest = jest.fn(); } /** @@ -146,32 +179,22 @@ function setup({ } = {}): { provider: EvmAccountProvider; messenger: RootMessenger; - keyring: MockEthKeyring; + keyring: MockHdKeyringV2; mocks: { mockProviderRequest: jest.Mock; mockGetAccount: jest.Mock; }; } { - const keyring = new MockEthKeyring(accounts); + const keyring = new MockHdKeyringV2(accounts); messenger.registerActionHandler( 'AccountsController:getAccounts', (accountIds: string[]) => - keyring.accounts.filter( - (account) => - accountIds.includes(account.id) || - accountIds.includes( - getUUIDFromAddressOfNormalAccount(account.address), - ), - ), + keyring.accounts.filter((account) => accountIds.includes(account.id)), ); const mockGetAccount = jest.fn().mockImplementation((id) => { - return keyring.accounts.find( - (account) => - account.id === id || - getUUIDFromAddressOfNormalAccount(account.address) === id, - ); + return keyring.accounts.find((account) => account.id === id); }); messenger.registerActionHandler( @@ -179,15 +202,6 @@ function setup({ mockGetAccount, ); - const mockGetAccountByAddress = jest.fn().mockImplementation((address) => { - return keyring.accounts.find((account) => account.address === address); - }); - - messenger.registerActionHandler( - 'AccountsController:getAccountByAddress', - mockGetAccountByAddress, - ); - const mockProviderRequest = jest.fn().mockImplementation(({ method }) => { if (method === 'eth_getTransactionCount') { return discovery?.transactionCount ?? '0x2'; @@ -196,7 +210,7 @@ function setup({ }); messenger.registerActionHandler( - 'KeyringController:withKeyring', + 'KeyringController:withKeyringV2', async (_, operation) => operation({ keyring, metadata: keyring.metadata }), ); @@ -218,8 +232,6 @@ function setup({ }, ); - mockNextDiscoveryAddress('0x123'); - const provider = new EvmAccountProvider( getMultichainAccountServiceMessenger(messenger), config, @@ -240,6 +252,15 @@ function setup({ } describe('EvmAccountProvider', () => { + beforeAll(async () => { + const legacy = new LegacyHdKeyring(); + await legacy.deserialize({ mnemonic: TEST_MNEMONIC }); + if (!legacy.root) { + throw new Error('Failed to initialize test HD root'); + } + mockHdRoot = legacy.root; + }); + it('getName returns EVM', () => { const { provider } = setup({ accounts: [] }); expect(provider.getName()).toBe(EVM_ACCOUNT_PROVIDER_NAME); @@ -251,7 +272,9 @@ describe('EvmAccountProvider', () => { accounts, }); - expect(provider.getAccounts()).toStrictEqual(accounts); + expect(provider.getAccounts()).toStrictEqual( + accounts.map(asKeyringAccount), + ); }); it('gets a specific account', () => { @@ -263,7 +286,9 @@ describe('EvmAccountProvider', () => { accounts: [account], }); - expect(provider.getAccount(customId)).toStrictEqual(account); + expect(provider.getAccount(customId)).toStrictEqual( + asKeyringAccount(account), + ); }); it('throws if account does not exist', () => { @@ -306,7 +331,7 @@ describe('EvmAccountProvider', () => { groupIndex: 0, }); expect(newAccounts).toHaveLength(1); - expect(newAccounts[0]).toStrictEqual(MOCK_HD_ACCOUNT_1); + expect(newAccounts[0]).toStrictEqual(asKeyringAccount(MOCK_HD_ACCOUNT_1)); }); it('creates multiple accounts using Bip44DeriveIndexRange', async () => { @@ -326,8 +351,9 @@ describe('EvmAccountProvider', () => { }); expect(newAccounts).toHaveLength(3); - expect(keyring.addAccounts).toHaveBeenCalledTimes(1); - expect(keyring.addAccounts).toHaveBeenCalledWith(3); + // HdKeyringV2 only supports bip44:derive-index, so range creation + // calls createAccounts once per new index. + expect(keyring.createAccounts).toHaveBeenCalledTimes(3); // Verify each account has the correct group index. for (const [index, account] of newAccounts.entries()) { @@ -351,8 +377,12 @@ describe('EvmAccountProvider', () => { }); expect(newAccounts).toHaveLength(3); - expect(keyring.addAccounts).toHaveBeenCalledTimes(1); - expect(keyring.addAccounts).toHaveBeenCalledWith(3); + expect(keyring.createAccounts).toHaveBeenCalledTimes(3); + expect(keyring.createAccounts).toHaveBeenCalledWith({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }); }); it('creates a single account when range from equals to', async () => { @@ -381,7 +411,8 @@ describe('EvmAccountProvider', () => { }); expect(newAccounts).toHaveLength(1); - expect(keyring.addAccounts).toHaveBeenCalledTimes(2); // 1 call for range 0-4, 1 call for account 5. + // 5 calls for range 0-4 + 1 call for account 5. + expect(keyring.createAccounts).toHaveBeenCalledTimes(6); expect( isBip44Account(newAccounts[0]) && newAccounts[0].options.entropy.groupIndex, @@ -427,11 +458,44 @@ describe('EvmAccountProvider', () => { // Should return 4 accounts: 2 existing (indices 0,1) + 2 new (indices 2,3). expect(newAccounts).toHaveLength(4); - expect(newAccounts[0]).toStrictEqual(MOCK_HD_ACCOUNT_1); - expect(newAccounts[1]).toStrictEqual(MOCK_HD_ACCOUNT_2); - // Only 2 new accounts should be created (indices 2 and 3) in a single batched call. - expect(keyring.addAccounts).toHaveBeenCalledTimes(1); - expect(keyring.addAccounts).toHaveBeenCalledWith(2); + expect(newAccounts[0]).toStrictEqual(asKeyringAccount(MOCK_HD_ACCOUNT_1)); + expect(newAccounts[1]).toStrictEqual(asKeyringAccount(MOCK_HD_ACCOUNT_2)); + // Only new accounts (indices 2 and 3) should be created — one call each. + expect(keyring.createAccounts).toHaveBeenCalledTimes(2); + }); + + it('throws when the keyring returns no created account during range creation', async () => { + const { provider, keyring } = setup({ accounts: [] }); + + // Simulate the keyring failing to create an account on the first call. + keyring.createAccounts.mockImplementationOnce(() => []); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + range: { + from: 0, + to: 1, + }, + }), + ).rejects.toThrow('Account creation failed'); + }); + + it('throws when single Bip44DeriveIndex creation returns no account', async () => { + const { provider, keyring } = setup({ accounts: [] }); + + keyring.createAccounts.mockImplementationOnce(() => []); + + await expect( + provider.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Account creation failed'); + // The provider should not register the account when nothing was created. + expect(provider.getAccounts()).toStrictEqual([]); }); it('throws if the created account is not BIP-44 compatible', async () => { @@ -507,17 +571,11 @@ describe('EvmAccountProvider', () => { accounts: [], }); - const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withAddressSuffix('0') - .get(); - const expectedAccount = { - ...account, + ...asKeyringAccount(makeDerivedHdAccount(0)), id: expect.any(String), }; - mockNextDiscoveryAddressOnce(account.address); - expect( await provider.discoverAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, @@ -551,19 +609,13 @@ describe('EvmAccountProvider', () => { }); it('stops discovery if there is no transaction activity', async () => { - const { provider } = setup({ + const { provider, keyring } = setup({ accounts: [], discovery: { transactionCount: '0x0', }, }); - const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withAddressSuffix('0') - .get(); - - mockNextDiscoveryAddressOnce(account.address); - expect( await provider.discoverAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, @@ -572,6 +624,24 @@ describe('EvmAccountProvider', () => { ).toStrictEqual([]); expect(provider.getAccounts()).toStrictEqual([]); + // Address is peeked via `keyring.root.deriveChild`, so no account + // is created (or deleted) when there is no on-chain activity. + expect(keyring.createAccounts).not.toHaveBeenCalled(); + expect(keyring.deleteAccount).not.toHaveBeenCalled(); + }); + + it('throws during discovery if the keyring returns no created account', async () => { + const { provider, keyring } = setup({ accounts: [] }); + + // Transaction count > 0 (default mock), so discovery proceeds to creation. + keyring.createAccounts.mockImplementationOnce(() => []); + + await expect( + provider.discoverAccounts({ + entropySource: MOCK_HD_KEYRING_1.metadata.id, + groupIndex: 0, + }), + ).rejects.toThrow('Account creation failed'); }); it('retries RPC request up to 3 times if it fails and throws the last error', async () => { @@ -632,7 +702,7 @@ describe('EvmAccountProvider', () => { entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, }), - ).toStrictEqual([MOCK_HD_ACCOUNT_1]); + ).toStrictEqual([asKeyringAccount(MOCK_HD_ACCOUNT_1)]); }); it('calls trace callback during account discovery', async () => { @@ -648,17 +718,11 @@ describe('EvmAccountProvider', () => { accounts: [], }); - const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withAddressSuffix('0') - .get(); - const expectedAccount = { - ...account, + ...asKeyringAccount(makeDerivedHdAccount(0)), id: expect.any(String), }; - mockNextDiscoveryAddressOnce(account.address); - // Create provider with custom trace callback const providerWithTrace = new EvmAccountProvider( getMultichainAccountServiceMessenger(messenger), @@ -686,17 +750,11 @@ describe('EvmAccountProvider', () => { accounts: [], }); - const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withAddressSuffix('0') - .get(); - const expectedAccount = { - ...account, + ...asKeyringAccount(makeDerivedHdAccount(0)), id: expect.any(String), }; - mockNextDiscoveryAddressOnce(account.address); - const result = await provider.discoverAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, groupIndex: 0, @@ -721,12 +779,6 @@ describe('EvmAccountProvider', () => { }, }); - const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withAddressSuffix('0') - .get(); - - mockNextDiscoveryAddressOnce(account.address); - const providerWithTrace = new EvmAccountProvider( getMultichainAccountServiceMessenger(messenger), { diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts index 147fe6a768..ef739cf5a3 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts @@ -1,8 +1,7 @@ import { publicToAddress } from '@ethereumjs/util'; import type { Bip44Account } from '@metamask/account-api'; -import { getUUIDFromAddressOfNormalAccount } from '@metamask/accounts-controller'; import type { TraceCallback } from '@metamask/controller-utils'; -import type { HdKeyring } from '@metamask/eth-hd-keyring'; +import type { HdKeyring } from '@metamask/eth-hd-keyring/v2'; import type { CreateAccountOptions, EntropySourceId, @@ -14,12 +13,9 @@ import { EthAccountType, EthScope, } from '@metamask/keyring-api'; -import type { KeyringCapabilities } from '@metamask/keyring-api/v2'; +import type { KeyringCapabilities, Keyring } from '@metamask/keyring-api/v2'; import { KeyringTypes } from '@metamask/keyring-controller'; -import type { - EthKeyring, - InternalAccount, -} from '@metamask/keyring-internal-api'; +import type { InternalAccount } from '@metamask/keyring-internal-api'; import { AccountId } from '@metamask/keyring-utils'; import type { Provider } from '@metamask/network-controller'; import { add0x, assert, bytesToHex, isStrictHexString } from '@metamask/utils'; @@ -130,19 +126,6 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { return provider; } - /** - * Get the account ID for an EVM account. - * - * Note: Since the account ID is deterministic at the AccountsController level, - * we can use this method to get the account ID from the address. - * - * @param address - The address of the account. - * @returns The account ID. - */ - #getAccountId(address: Hex): string { - return getUUIDFromAddressOfNormalAccount(address); - } - /** * Create an EVM account. * @@ -150,23 +133,24 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { * @param opts.entropySource - The entropy source to use for the creation of the account. * @param opts.groupIndex - The index of the group to create the account for. * @param opts.throwOnGap - Whether to throw an error if the account index is not contiguous. - * @returns The account ID and a boolean indicating if the account was created. + * @returns The created or existing keyring account(s) at the requested group + * index. Returns an empty array if the keyring did not create an account. */ async #createAccount({ entropySource, groupIndex, - throwOnGap = false, + throwOnGap, }: { entropySource: EntropySourceId; groupIndex: number; - throwOnGap?: boolean; - }): Promise<[Hex, boolean]> { - const result = await this.withKeyring( + throwOnGap: boolean; + }): Promise { + return await this.withKeyringV2( { id: entropySource }, async ({ keyring }) => { const existing = await keyring.getAccounts(); if (groupIndex < existing.length) { - return [existing[groupIndex], false]; + return [existing[groupIndex]]; } // If the throwOnGap flag is set, we throw an error to prevent index gaps. @@ -174,12 +158,13 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { throw new Error('Trying to create too many accounts'); } - const [added] = await keyring.addAccounts(1); - return [added, true]; + return await keyring.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex, + }); }, ); - - return result; } /** @@ -202,7 +187,7 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { const { range } = options; // Use a single withKeyring call for the entire range. - const accountIds = await this.withKeyring( + const accountIds = await this.withKeyringV2( { id: entropySource }, async ({ keyring }) => { const existing = await keyring.getAccounts(); @@ -224,21 +209,24 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { ) { if (groupIndex < existing.length) { // Account already exists. - result.push(this.#getAccountId(existing[groupIndex])); + result.push(existing[groupIndex].id); } } - // Determine if we need to create new accounts. - const from = Math.max(range.from, existing.length); - if (from <= range.to) { - // Calculate how many new accounts to create. - const accountsToCreate = range.to - existing.length + 1; - - // Create all new accounts in one call. - const newAccounts = await keyring.addAccounts(accountsToCreate); - result.push( - ...newAccounts.map((address) => this.#getAccountId(address)), - ); + // Create new accounts one-by-one since HdKeyringV2 only supports + // bip44:derive-index (not bip44:derive-index-range). + for ( + let groupIndex = Math.max(range.from, existing.length); + groupIndex <= range.to; + groupIndex++ + ) { + const [created] = await keyring.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex, + }); + assert(created, 'Account creation failed'); + result.push(created.id); } return result; @@ -262,17 +250,17 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { // Handle Bip44DeriveIndex (single account creation). const { groupIndex } = options; - const [address] = await this.#createAccount({ + const [created] = await this.#createAccount({ entropySource, groupIndex, throwOnGap: true, }); - const accountId = this.#getAccountId(address); + assert(created, 'Account creation failed'); const account = this.messenger.call( 'AccountsController:getAccount', - accountId, + created.id, ); // We MUST have the associated internal account. @@ -285,6 +273,49 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { return accountsArray; } + /** + * Get the address that would be derived for a given group index without + * persisting an account in the keyring. + * + * Peeks at the next address via the HD `root` so discovery can short-circuit + * (and skip the vault write + `stateChange` event) when there is no on-chain + * activity at this index. + * + * @param opts - The options for the address derivation. + * @param opts.entropySource - The entropy source to derive from. + * @param opts.groupIndex - The group index to derive at. + * @returns The derived address for the given group index. + */ + async #getAddressFromGroupIndex({ + entropySource, + groupIndex, + }: { + entropySource: EntropySourceId; + groupIndex: number; + }): Promise { + // NOTE: To avoid exposing this function at keyring level, we just re-use its internal state + // and compute the derivation here. + return await this.withKeyringV2( + { id: entropySource }, + async ({ keyring }) => { + // If the account already exist, do not re-derive and just re-use that account. + const existing = await keyring.getAccounts(); + if (groupIndex < existing.length) { + return existing[groupIndex].address as Hex; + } + + // If not, then we just "peek" the next address to avoid creating the account. + assert(keyring.root, 'Expected HD keyring.root to be set'); + const hdKey = keyring.root.deriveChild(groupIndex); + assert(hdKey.publicKey, 'Expected public key to be set'); + + return add0x( + bytesToHex(publicToAddress(hdKey.publicKey, true)).toLowerCase(), + ); + }, + ); + } + /** * Get the transaction count for an EVM account. * This method uses a retry and timeout mechanism to handle transient failures. @@ -295,7 +326,7 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { */ async #getTransactionCount( provider: Provider, - address: Hex, + address: string, ): Promise { const method = 'eth_getTransactionCount'; @@ -328,36 +359,6 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { return parseInt(response, 16); } - async #getAddressFromGroupIndex({ - entropySource, - groupIndex, - }: { - entropySource: EntropySourceId; - groupIndex: number; - }): Promise { - // NOTE: To avoid exposing this function at keyring level, we just re-use its internal state - // and compute the derivation here. - return await this.withKeyring( - { id: entropySource }, - async ({ keyring }) => { - // If the account already exist, do not re-derive and just re-use that account. - const existing = await keyring.getAccounts(); - if (groupIndex < existing.length) { - return existing[groupIndex]; - } - - // If not, then we just "peek" the next address to avoid creating the account. - assert(keyring.root, 'Expected HD keyring.root to be set'); - const hdKey = keyring.root.deriveChild(groupIndex); - assert(hdKey.publicKey, 'Expected public key to be set'); - - return add0x( - bytesToHex(publicToAddress(hdKey.publicKey, true)).toLowerCase(), - ); - }, - ); - } - /** * Discover and create accounts for the EVM provider. * @@ -399,20 +400,21 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { } // We have some activity on this address, we try to create the account. - const [address] = await this.#createAccount({ + const [created] = await this.#createAccount({ entropySource, groupIndex, + throwOnGap: false, }); + + assert(created, 'Account creation failed'); assert( - addressFromGroupIndex === address, + addressFromGroupIndex === created.address, 'Created account does not match address from group index.', ); - const accoundId = this.#getAccountId(address); - const account = this.messenger.call( 'AccountsController:getAccount', - accoundId, + created.id, ); assertInternalAccountExists(account); assertIsBip44Account(account); diff --git a/packages/multichain-account-service/src/tests/accounts.ts b/packages/multichain-account-service/src/tests/accounts.ts index a63184a2d3..8239850dba 100644 --- a/packages/multichain-account-service/src/tests/accounts.ts +++ b/packages/multichain-account-service/src/tests/accounts.ts @@ -327,6 +327,11 @@ export class MockAccountBuilder { return this; } + withAddress(address: string): this { + this.#account.address = address; + return this; + } + withAddressSuffix(suffix: string) { this.#account.address += suffix; return this; diff --git a/packages/multichain-account-service/src/tests/messenger.ts b/packages/multichain-account-service/src/tests/messenger.ts index e6f3136b5a..8318ccaede 100644 --- a/packages/multichain-account-service/src/tests/messenger.ts +++ b/packages/multichain-account-service/src/tests/messenger.ts @@ -65,6 +65,7 @@ export function getMultichainAccountServiceMessenger( 'SnapController:getState', 'SnapController:handleRequest', 'KeyringController:withKeyring', + 'KeyringController:withKeyringV2', 'KeyringController:getState', 'KeyringController:getKeyringsByType', 'KeyringController:addNewKeyring', diff --git a/packages/multichain-account-service/src/types.ts b/packages/multichain-account-service/src/types.ts index 8817722e10..6376af180a 100644 --- a/packages/multichain-account-service/src/types.ts +++ b/packages/multichain-account-service/src/types.ts @@ -23,6 +23,7 @@ import type { KeyringControllerRemoveAccountAction, KeyringControllerStateChangeEvent, KeyringControllerWithKeyringAction, + KeyringControllerWithKeyringV2Action, } from '@metamask/keyring-controller'; import type { Messenger } from '@metamask/messenger'; import type { @@ -79,6 +80,7 @@ type AllowedActions = | AccountsControllerGetAccountAction | AccountsControllerGetAccountByAddressAction | KeyringControllerWithKeyringAction + | KeyringControllerWithKeyringV2Action | KeyringControllerGetStateAction | KeyringControllerGetKeyringsByTypeAction | KeyringControllerAddNewKeyringAction