diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index 719a019be2e..923e1d4e5ce 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -13,6 +13,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add custom account provider configs ([#6624](https://github.com/MetaMask/core/pull/6624)) - This new config can be set by the clients to update discovery timeout/retry values. +### Fixed + +- No longer create temporary EVM account during discovery ([#6650](https://github.com/MetaMask/core/pull/6650)) + - We used to create the EVM account and remove it if there was no activity for that account. Now we're just deriving the next address directly, which avoids state mutation. + - This prevents `:accountAdded` event from being published, which also prevents account-tree and multichain-account service updates. + - Backup & sync will no longer synchronize this temporary account group, which was causing a bug that persisted it on the user profile and left it permanently. + ## [0.9.0] ### Added diff --git a/packages/multichain-account-service/package.json b/packages/multichain-account-service/package.json index 1989e9dcb30..7c69dc8677d 100644 --- a/packages/multichain-account-service/package.json +++ b/packages/multichain-account-service/package.json @@ -47,6 +47,7 @@ "test:watch": "NODE_OPTIONS=--experimental-vm-modules jest --watch" }, "dependencies": { + "@ethereumjs/util": "^9.1.0", "@metamask/base-controller": "^8.4.0", "@metamask/eth-snap-keyring": "^17.0.0", "@metamask/key-tree": "^10.1.1", diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts index dd5e2e204c7..b05986d4c9b 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts @@ -1,3 +1,5 @@ +/* eslint-disable jsdoc/require-jsdoc */ +import { publicToAddress } from '@ethereumjs/util'; import type { Messenger } from '@metamask/base-controller'; import { type KeyringMetadata } from '@metamask/keyring-controller'; import type { @@ -8,6 +10,8 @@ import type { AutoManagedNetworkClient, CustomNetworkClientConfiguration, } from '@metamask/network-controller'; +import type { Hex } from '@metamask/utils'; +import { createBytes } from '@metamask/utils'; import { EvmAccountProvider } from './EvmAccountProvider'; import { TimeoutError } from './utils'; @@ -26,6 +30,32 @@ import type { MultichainAccountServiceEvents, } from '../types'; +jest.mock('@ethereumjs/util', () => ({ + publicToAddress: jest.fn(), +})); + +function mockNextDiscoveryAddress(address: string) { + jest.mocked(publicToAddress).mockReturnValue(createBytes(address as Hex)); +} + +function mockNextDiscoveryAddressOnce(address: string) { + jest.mocked(publicToAddress).mockReturnValueOnce(createBytes(address as Hex)); +} + +type MockHdKey = { + deriveChild: jest.Mock; +}; + +function mockHdKey(): MockHdKey { + return { + deriveChild: jest.fn().mockImplementation(() => { + return { + publicKey: new Uint8Array(65), + }; + }), + }; +} + class MockEthKeyring implements EthKeyring { readonly type = 'MockEthKeyring'; @@ -36,8 +66,11 @@ class MockEthKeyring implements EthKeyring { readonly accounts: InternalAccount[]; + readonly root: MockHdKey; + constructor(accounts: InternalAccount[]) { this.accounts = accounts; + this.root = mockHdKey(); } async serialize() { @@ -85,17 +118,23 @@ class MockEthKeyring implements EthKeyring { * @param options - Configuration options for setup. * @param options.messenger - An optional messenger instance to use. Defaults to a new Messenger. * @param options.accounts - List of accounts to use. + * @param options.discovery - Discovery options. + * @param options.discovery.transactionCount - Transaction count (use '0x0' to stop the discovery). * @returns An object containing the controller instance and the messenger. */ function setup({ messenger = getRootMessenger(), accounts = [], + discovery, }: { messenger?: Messenger< MultichainAccountServiceActions | AllowedActions, MultichainAccountServiceEvents | AllowedEvents >; accounts?: InternalAccount[]; + discovery?: { + transactionCount: string; + }; } = {}): { provider: EvmAccountProvider; messenger: Messenger< @@ -123,7 +162,7 @@ function setup({ const mockProviderRequest = jest.fn().mockImplementation(({ method }) => { if (method === 'eth_getTransactionCount') { - return '0x2'; + return discovery?.transactionCount ?? '0x2'; } throw new Error(`Unknown method: ${method}`); }); @@ -156,6 +195,8 @@ function setup({ }, ); + mockNextDiscoveryAddress('0x123'); + const provider = new EvmAccountProvider( getMultichainAccountServiceMessenger(messenger), ); @@ -274,13 +315,17 @@ describe('EvmAccountProvider', () => { accounts: [], }); + const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) + .withAddressSuffix('0') + .get(); + const expectedAccount = { - ...MockAccountBuilder.from(MOCK_HD_ACCOUNT_1) - .withAddressSuffix('0') - .get(), + ...account, id: expect.any(String), }; + mockNextDiscoveryAddressOnce(account.address); + expect( await provider.discoverAccounts({ entropySource: MOCK_HD_KEYRING_1.metadata.id, @@ -291,42 +336,28 @@ describe('EvmAccountProvider', () => { expect(provider.getAccounts()).toStrictEqual([expectedAccount]); }); - it('removes discovered account if no transaction history is found', async () => { - const { provider, mocks } = setup({ - accounts: [MOCK_HD_ACCOUNT_1], + it('stops discovery if there is no transaction activity', async () => { + const { provider } = setup({ + accounts: [], + discovery: { + transactionCount: '0x0', + }, }); - mocks.mockProviderRequest.mockReturnValue('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, - groupIndex: 1, + groupIndex: 0, }), ).toStrictEqual([]); - await Promise.resolve(); - - expect(provider.getAccounts()).toStrictEqual([MOCK_HD_ACCOUNT_1]); - }); - - it('removes discovered account if RPC request fails', async () => { - const { provider, mocks } = setup({ - accounts: [MOCK_HD_ACCOUNT_1], - }); - - mocks.mockProviderRequest.mockImplementation(() => { - throw new Error('RPC request failed'); - }); - - await expect( - provider.discoverAccounts({ - entropySource: MOCK_HD_KEYRING_1.metadata.id, - groupIndex: 1, - }), - ).rejects.toThrow('RPC request failed'); - - expect(provider.getAccounts()).toStrictEqual([MOCK_HD_ACCOUNT_1]); + expect(provider.getAccounts()).toStrictEqual([]); }); it('retries RPC request up to 3 times if it fails and throws the last error', async () => { diff --git a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts index 6f2a4172eb8..50c5e256833 100644 --- a/packages/multichain-account-service/src/providers/EvmAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/EvmAccountProvider.ts @@ -1,4 +1,6 @@ +import { publicToAddress } from '@ethereumjs/util'; import type { Bip44Account } from '@metamask/account-api'; +import type { HdKeyring } from '@metamask/eth-hd-keyring'; import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { EthAccountType } from '@metamask/keyring-api'; import { KeyringTypes } from '@metamask/keyring-controller'; @@ -7,7 +9,7 @@ import type { InternalAccount, } from '@metamask/keyring-internal-api'; import type { Provider } from '@metamask/network-controller'; -import type { Hex } from '@metamask/utils'; +import { add0x, assert, bytesToHex, type Hex } from '@metamask/utils'; import type { MultichainAccountServiceMessenger } from 'src/types'; import { @@ -169,6 +171,36 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { return parseInt(countHex, 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. * @@ -184,39 +216,29 @@ export class EvmAccountProvider extends BaseBip44AccountProvider { const provider = this.getEvmProvider(); const { entropySource, groupIndex } = opts; - const [address, didCreate] = await this.#createAccount({ + const addressFromGroupIndex = await this.#getAddressFromGroupIndex({ entropySource, groupIndex, }); - // We don't want to remove the account if it's the first one. - const shouldCleanup = didCreate && groupIndex !== 0; - try { - const count = await this.#getTransactionCount(provider, address); - - if (count === 0 && shouldCleanup) { - await this.withKeyring( - { id: entropySource }, - async ({ keyring }) => { - keyring.removeAccount?.(address); - }, - ); - return []; - } - } catch (error) { - // If the RPC request fails and we just created this account for discovery, - // remove it to avoid leaving a dangling account. - if (shouldCleanup) { - await this.withKeyring( - { id: entropySource }, - async ({ keyring }) => { - keyring.removeAccount?.(address); - }, - ); - } - throw error; + const count = await this.#getTransactionCount( + provider, + addressFromGroupIndex, + ); + if (count === 0) { + return []; } + // We have some activity on this address, we try to create the account. + const [address] = await this.#createAccount({ + entropySource, + groupIndex, + }); + assert( + addressFromGroupIndex === address, + 'Created account does not match address from group index.', + ); + const account = this.messenger.call( 'AccountsController:getAccountByAddress', address, diff --git a/yarn.lock b/yarn.lock index e6754964dfb..1d589b43769 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3836,6 +3836,7 @@ __metadata: version: 0.0.0-use.local resolution: "@metamask/multichain-account-service@workspace:packages/multichain-account-service" dependencies: + "@ethereumjs/util": "npm:^9.1.0" "@metamask/account-api": "npm:^0.12.0" "@metamask/accounts-controller": "npm:^33.1.0" "@metamask/auto-changelog": "npm:^3.4.4"