refactor(multichain-account-service)!: replace withKeyring usage with withKeyringV2

packages/multichain-account-service/CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **BREAKING:** Replace `KeyringController:withKeyring` with `KeyringController:withKeyringV2` across all account providers ([#8491](https://github.com/MetaMask/core/pull/8491))
 - Bump `@metamask/accounts-controller` from `^37.1.1` to `^37.2.0` ([#8363](https://github.com/MetaMask/core/pull/8363))
 - Bump `@metamask/keyring-controller` from `^25.1.1` to `^25.2.0` ([#8363](https://github.com/MetaMask/core/pull/8363))
 - 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))

packages/multichain-account-service/src/MultichainAccountService.test.ts

@@ -1537,7 +1537,7 @@ describe('MultichainAccountService', () => {
         );
 
         rootMessenger.registerActionHandler(
-          'KeyringController:withKeyring',
+          'KeyringController:withKeyringV2',
           async (_, operation) => {
             const newKeyring = mocks.KeyringController.keyrings.find(
               (keyring) => keyring.type === 'HD Key Tree',
@@ -1580,7 +1580,7 @@ describe('MultichainAccountService', () => {
         );
 
         rootMessenger.registerActionHandler(
-          'KeyringController:withKeyring',
+          'KeyringController:withKeyringV2',
           async (_, operation) => {
             const newKeyring = mocks.KeyringController.keyrings.find(
               (keyring) => keyring.type === 'HD Key Tree',

packages/multichain-account-service/src/providers/BaseBip44AccountProvider.ts

@@ -8,7 +8,7 @@ import type {
 import type { KeyringCapabilities } from '@metamask/keyring-api/v2';
 import type {
   KeyringMetadata,
-  KeyringSelector,
+  KeyringSelectorV2,
 } from '@metamask/keyring-controller';
 import type { InternalAccount } from '@metamask/keyring-internal-api';
 
@@ -156,7 +156,7 @@ export abstract class BaseBip44AccountProvider<
   }
 
   protected async withKeyring<SelectedKeyring, CallbackResult = void>(
-    selector: KeyringSelector,
+    selector: KeyringSelectorV2,
     operation: ({
       keyring,
       metadata,
@@ -166,7 +166,7 @@ export abstract class BaseBip44AccountProvider<
     }) => Promise<CallbackResult>,
   ): Promise<CallbackResult> {
     const result = await this.messenger.call(
-      'KeyringController:withKeyring',
+      'KeyringController:withKeyringV2',
       selector,
       ({ keyring, metadata }) =>
         operation({

packages/multichain-account-service/src/providers/EvmAccountProvider.test.ts

@@ -1,18 +1,15 @@
-import { publicToAddress } from '@ethereumjs/util';
 import { isBip44Account } from '@metamask/account-api';
 import { getUUIDFromAddressOfNormalAccount } from '@metamask/accounts-controller';
-import { AccountCreationType } from '@metamask/keyring-api';
+import { AccountCreationType, EthScope } from '@metamask/keyring-api';
+import type { 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 {
-  EthKeyring,
-  InternalAccount,
-} from '@metamask/keyring-internal-api';
+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 { TraceName } from '../analytics/traces';
 import {
@@ -34,90 +31,87 @@ import {
 } from './EvmAccountProvider';
 import { TimeoutError } from './utils';
 
-jest.mock('@ethereumjs/util', () => {
-  const actual = jest.requireActual('@ethereumjs/util');
+/**
+ * Converts an InternalAccount to a minimal KeyringAccount shape
+ * that satisfies what the controller accesses from the V2 keyring.
+ *
+ * @param account - The internal account to convert.
+ * @returns A KeyringAccount-shaped object.
+ */
+function toKeyringAccount(account: InternalAccount): KeyringAccount {
   return {
-    ...actual,
-    publicToAddress: jest.fn(),
-  };
-});
-
-function mockNextDiscoveryAddress(address: string): void {
-  jest.mocked(publicToAddress).mockReturnValue(createBytes(address as Hex));
+    id: account.id,
+    address: account.address,
+    type: account.type,
+    options: account.options,
+    methods: account.methods,
+    scopes: account.scopes ?? [],
+  } as unknown as KeyringAccount;
 }
 
-function mockNextDiscoveryAddressOnce(address: string): void {
-  jest.mocked(publicToAddress).mockReturnValueOnce(createBytes(address as Hex));
-}
-
-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: InternalAccount[];
 
   readonly metadata: KeyringMetadata = {
     id: 'mock-eth-keyring-id',
     name: '',
   };
 
-  readonly accounts: InternalAccount[];
-
-  readonly root: MockHdKey;
-
   constructor(accounts: InternalAccount[]) {
     this.accounts = accounts;
-    this.root = mockHdKey();
-  }
-
-  async serialize(): Promise<string> {
-    return 'serialized';
-  }
-
-  async deserialize(_: string): Promise<void> {
-    // Not required.
   }
 
   getAccounts = jest
     .fn()
-    .mockImplementation(() => this.accounts.map((account) => account.address));
+    .mockImplementation(() => this.accounts.map(toKeyringAccount));
+
+  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 toKeyringAccount(account);
+  });
 
-  addAccounts = jest.fn().mockImplementation((numberOfAccounts: number) => {
-    const newAccountsIndex = this.accounts.length;
+  createAccounts = jest
+    .fn()
+    .mockImplementation((options: Record<string, unknown>) => {
+      const newAccounts: InternalAccount[] = [];
 
-    // 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)
+      if (options.type === `${AccountCreationType.Bip44DeriveIndex}`) {
+        const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1)
           .withUuid()
           .withAddressSuffix(`${this.accounts.length}`)
           .withGroupIndex(this.accounts.length)
-          .get(),
-      );
-    }
+          .get();
+        this.accounts.push(account);
+        newAccounts.push(account);
+      }
 
-    return this.accounts
-      .slice(newAccountsIndex)
-      .map((account) => account.address);
-  });
+      return newAccounts.map(toKeyringAccount);
+    });
 
-  removeAccount = jest.fn().mockImplementation((address: string) => {
-    const index = this.accounts.findIndex((a) => a.address === address);
+  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,13 +140,13 @@ 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',
@@ -196,7 +190,7 @@ function setup({
   });
 
   messenger.registerActionHandler(
-    'KeyringController:withKeyring',
+    'KeyringController:withKeyringV2',
     async (_, operation) => operation({ keyring, metadata: keyring.metadata }),
   );
 
@@ -218,8 +212,6 @@ function setup({
     },
   );
 
-  mockNextDiscoveryAddress('0x123');
-
   const provider = new EvmAccountProvider(
     getMultichainAccountServiceMessenger(messenger),
     config,
@@ -326,8 +318,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 +344,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 +378,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,
@@ -429,9 +427,8 @@ describe('EvmAccountProvider', () => {
     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);
+    // Only new accounts (indices 2 and 3) should be created — one call each.
+    expect(keyring.createAccounts).toHaveBeenCalledTimes(2);
   });
 
   it('throws if the created account is not BIP-44 compatible', async () => {
@@ -516,8 +513,6 @@ describe('EvmAccountProvider', () => {
       id: expect.any(String),
     };
 
-    mockNextDiscoveryAddressOnce(account.address);
-
     expect(
       await provider.discoverAccounts({
         entropySource: MOCK_HD_KEYRING_1.metadata.id,
@@ -550,20 +545,14 @@ describe('EvmAccountProvider', () => {
     );
   });
 
-  it('stops discovery if there is no transaction activity', async () => {
-    const { provider } = setup({
+  it('stops discovery if there is no transaction activity and deletes the created account', async () => {
+    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 +561,9 @@ describe('EvmAccountProvider', () => {
     ).toStrictEqual([]);
 
     expect(provider.getAccounts()).toStrictEqual([]);
+    // The account was created to peek at the address, then deleted
+    // because there was no on-chain activity.
+    expect(keyring.deleteAccount).toHaveBeenCalledTimes(1);
   });
 
   it('retries RPC request up to 3 times if it fails and throws the last error', async () => {
@@ -657,8 +649,6 @@ describe('EvmAccountProvider', () => {
       id: expect.any(String),
     };
 
-    mockNextDiscoveryAddressOnce(account.address);
-
     // Create provider with custom trace callback
     const providerWithTrace = new EvmAccountProvider(
       getMultichainAccountServiceMessenger(messenger),
@@ -695,8 +685,6 @@ describe('EvmAccountProvider', () => {
       id: expect.any(String),
     };
 
-    mockNextDiscoveryAddressOnce(account.address);
-
     const result = await provider.discoverAccounts({
       entropySource: MOCK_HD_KEYRING_1.metadata.id,
       groupIndex: 0,
@@ -721,12 +709,6 @@ describe('EvmAccountProvider', () => {
       },
     });
 
-    const account = MockAccountBuilder.from(MOCK_HD_ACCOUNT_1)
-      .withAddressSuffix('0')
-      .get();
-
-    mockNextDiscoveryAddressOnce(account.address);
-
     const providerWithTrace = new EvmAccountProvider(
       getMultichainAccountServiceMessenger(messenger),
       {