fix(multichain-account-service): prevent creating EVM account during discovery

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 we nore activity for that account, now we're just deriving the next address directly, which avoid 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 of persisting it on the user profile and to leave forever.
+
 ## [0.9.0]
 
 ### Added

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",

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,28 @@ import type {
   MultichainAccountServiceEvents,
 } from '../types';
 
+jest.mock('@ethereumjs/util', () => ({
+  publicToAddress: jest.fn(),
+}));
+
+function mockNextDiscoveryAddress(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 +62,11 @@ class MockEthKeyring implements EthKeyring {
 
   readonly accounts: InternalAccount[];
 
+  readonly root: MockHdKey;
+
   constructor(accounts: InternalAccount[]) {
     this.accounts = accounts;
+    this.root = mockHdKey();
   }
 
   async serialize() {
@@ -85,17 +114,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 +158,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}`);
   });
@@ -274,13 +309,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),
     };
 
+    mockNextDiscoveryAddress(account.address);
+
     expect(
       await provider.discoverAccounts({
         entropySource: MOCK_HD_KEYRING_1.metadata.id,
@@ -291,42 +330,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();
+
+    mockNextDiscoveryAddress(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 () => {
@@ -348,6 +373,8 @@ describe('EvmAccountProvider', () => {
         throw new Error('RPC request failed 4');
       });
 
+    mockNextDiscoveryAddress('0x123');
+
     await expect(
       provider.discoverAccounts({
         entropySource: MOCK_HD_KEYRING_1.metadata.id,
@@ -369,6 +396,8 @@ describe('EvmAccountProvider', () => {
       });
     });
 
+    mockNextDiscoveryAddress('0x123');
+
     await expect(
       provider.discoverAccounts({
         entropySource: MOCK_HD_KEYRING_1.metadata.id,

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<Hex> {
+    // 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<HdKeyring, Hex>(
+      { 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<EthKeyring>(
-          { 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<EthKeyring>(
-          { 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,

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"