From 64b22ec3e2ba6a4a72fca7357b891d4f6daf3faa Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 3 Feb 2026 17:22:21 +0100 Subject: [PATCH 01/21] feat: add createAccounts --- packages/keyring-api/CHANGELOG.md | 3 ++ packages/keyring-api/src/api/keyring.ts | 14 ++++++++ packages/keyring-api/src/rpc.ts | 18 ++++++++++ packages/keyring-snap-bridge/CHANGELOG.md | 5 +++ .../keyring-snap-bridge/src/SnapKeyring.ts | 34 +++++++++++++++++++ packages/keyring-snap-client/CHANGELOG.md | 5 +++ .../keyring-snap-client/src/KeyringClient.ts | 14 ++++++++ 7 files changed, 93 insertions(+) diff --git a/packages/keyring-api/CHANGELOG.md b/packages/keyring-api/CHANGELOG.md index df9aeced4..5e53cf9a7 100644 --- a/packages/keyring-api/CHANGELOG.md +++ b/packages/keyring-api/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Add `Keyring.createAccounts` optional method ([#448](https://github.com/MetaMask/accounts/pull/448)) + - This method is part of the keyring v2 specification and set as optional for backwards compatibility. + - This method can be used to create one or more accounts using the new keyring v2 account creation typed options. - Add support for account derivations using range of indices in `KeyringV2` ([#451](https://github.com/MetaMask/accounts/pull/451)) - Add `bip44:derive-index-range` capability to `KeyringCapabilities`. - Add `AccountCreationType.Bip44DeriveIndexRange` and `CreateAccountBip44DeriveIndexRangeOptions`. diff --git a/packages/keyring-api/src/api/keyring.ts b/packages/keyring-api/src/api/keyring.ts index 4276fca25..81b4965a8 100644 --- a/packages/keyring-api/src/api/keyring.ts +++ b/packages/keyring-api/src/api/keyring.ts @@ -13,6 +13,7 @@ import type { Paginated, Pagination } from './pagination'; import type { KeyringRequest } from './request'; import type { KeyringResponse } from './response'; import type { Transaction } from './transaction'; +import type { CreateAccountOptions } from './v2'; /** * Keyring interface. @@ -56,6 +57,19 @@ export type Keyring = { options?: Record & MetaMaskOptions, ): Promise; + /** + * Creates one or more new accounts according to the provided options. + * + * Deterministic account creation MUST be idempotent, meaning that for + * deterministic algorithms, like BIP-44, calling this method with the same + * options should always return the same accounts, even if the accounts + * already exist in the keyring. + * + * @param options - Options describing how to create the account(s). + * @returns A promise that resolves to an array of the created account objects. + */ + createAccounts?(options: CreateAccountOptions): Promise; + /** * Lists the assets of an account (fungibles and non-fungibles) represented * by their respective CAIP-19: diff --git a/packages/keyring-api/src/rpc.ts b/packages/keyring-api/src/rpc.ts index 7e9589576..7579ace74 100644 --- a/packages/keyring-api/src/rpc.ts +++ b/packages/keyring-api/src/rpc.ts @@ -36,6 +36,7 @@ import { export enum KeyringRpcMethod { // Account management CreateAccount = 'keyring_createAccount', + CreateAccounts = 'keyring_createAccounts', DeleteAccount = 'keyring_deleteAccount', DiscoverAccounts = 'keyring_discoverAccounts', ExportAccount = 'keyring_exportAccount', @@ -126,6 +127,23 @@ export const CreateAccountResponseStruct = KeyringAccountStruct; export type CreateAccountResponse = Infer; +// ---------------------------------------------------------------------------- +// Create accounts + +export const CreateAccountsRequestStruct = object({ + ...CommonHeader, + method: literal('keyring_createAccounts'), + params: object({ + options: record(string(), JsonStruct), + }), +}); + +export type CreateAccountsRequest = Infer; + +export const CreateAccountsResponseStruct = array(KeyringAccountStruct); + +export type CreateAccountsResponse = Infer; + // ---------------------------------------------------------------------------- // Set selected accounts diff --git a/packages/keyring-snap-bridge/CHANGELOG.md b/packages/keyring-snap-bridge/CHANGELOG.md index 0be31ab76..a98b15e30 100644 --- a/packages/keyring-snap-bridge/CHANGELOG.md +++ b/packages/keyring-snap-bridge/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `SnapKeyring.createAccounts` method ([#448](https://github.com/MetaMask/accounts/pull/448)) + - This method can be used to create one or more accounts using the new keyring v2 account creation typed options. + ### Changed - Bump `@metamask/snaps-controllers` from `^14.0.1` to `^17.2.0` ([#422](https://github.com/MetaMask/accounts/pull/422)) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 986c84554..26e8ba67f 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -14,6 +14,7 @@ import type { CaipChainId, MetaMaskOptions, KeyringResponse, + CreateAccountOptions, } from '@metamask/keyring-api'; import { EthBytesStruct, @@ -891,6 +892,39 @@ export class SnapKeyring { }); } + /** + * Create one or more accounts according to the provided options. + * + * This method supports batch account creation for BIP-44 derivation paths, + * allowing the creation of multiple accounts up to a specified maximum index. + * + * @param snapId - Snap ID to create the accounts for. + * @param options - Account creation options. + * @returns A promise that resolves to an array of the created account objects. + */ + async createAccounts( + snapId: SnapId, + options: CreateAccountOptions, + ): Promise { + const client = new KeyringInternalSnapClient({ + messenger: this.#messenger, + snapId, + }); + + // Add each returned account to the internal accounts map. + // NOTE: This method DOES NOT rely on the `AccountCreated` event to add + // accounts to the keyring, since those accounts are created in batch. + const accounts = await client.createAccounts(options); + for (const account of accounts) { + this.#accounts.set(account.id, { account, snapId }); + } + + // Save the state after adding all accounts. + await this.#callbacks.saveState(); + + return accounts; + } + /** * Checks if a Snap ID is known from the keyring. * diff --git a/packages/keyring-snap-client/CHANGELOG.md b/packages/keyring-snap-client/CHANGELOG.md index 8b5fc54d4..f36e13454 100644 --- a/packages/keyring-snap-client/CHANGELOG.md +++ b/packages/keyring-snap-client/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `KeyringClient.createAccounts` method ([#448](https://github.com/MetaMask/accounts/pull/448)) + - This method can be used to create one or more accounts using the new keyring v2 account creation typed options. + ## [8.1.1] ### Changed diff --git a/packages/keyring-snap-client/src/KeyringClient.ts b/packages/keyring-snap-client/src/KeyringClient.ts index f603091bc..f6e4a43c8 100644 --- a/packages/keyring-snap-client/src/KeyringClient.ts +++ b/packages/keyring-snap-client/src/KeyringClient.ts @@ -1,6 +1,7 @@ import { ApproveRequestResponseStruct, CreateAccountResponseStruct, + CreateAccountsResponseStruct, DeleteAccountResponseStruct, ExportAccountResponseStruct, FilterAccountChainsResponseStruct, @@ -34,6 +35,7 @@ import type { CaipAssetTypeOrId, EntropySourceId, DiscoveredAccount, + CreateAccountOptions, } from '@metamask/keyring-api'; import type { AccountId, JsonRpcRequest } from '@metamask/keyring-utils'; import { strictMask } from '@metamask/keyring-utils'; @@ -117,6 +119,18 @@ export class KeyringClient implements Keyring { ); } + async createAccounts( + options: CreateAccountOptions, + ): Promise { + return strictMask( + await this.send({ + method: KeyringRpcMethod.CreateAccounts, + params: options, + }), + CreateAccountsResponseStruct, + ); + } + async discoverAccounts( scopes: CaipChainId[], entropySource: EntropySourceId, From 3840f34a35dc20e6c6fb0c36b43fac2320cd3316 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 3 Feb 2026 18:12:58 +0100 Subject: [PATCH 02/21] test: add missing tests --- .../src/SnapKeyring.test.ts | 220 ++++++++++++++++++ .../src/KeyringClient.test.ts | 109 ++++++++- 2 files changed, 328 insertions(+), 1 deletion(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index 7db510cfb..98aaccefe 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -10,6 +10,7 @@ import type { AccountTransactionsUpdatedEventPayload, AccountAssetListUpdatedEventPayload, MetaMaskOptions, + CreateAccountOptions, } from '@metamask/keyring-api'; import { EthScope, @@ -28,6 +29,7 @@ import { TrxScope, TrxMethod, TrxAccountType, + AccountCreationType, } from '@metamask/keyring-api'; import { SnapManageAccountsMethod } from '@metamask/keyring-snap-sdk'; import type { JsonRpcRequest } from '@metamask/keyring-utils'; @@ -2520,6 +2522,224 @@ describe('SnapKeyring', () => { }); }); + describe('createAccounts', () => { + const newAccount1 = { + ...newEthEoaAccount, + id: 'aa11bb22-cc33-4d44-8e55-ff6677889900', + address: '0xaabbccddee00112233445566778899aabbccddee', + }; + const newAccount2 = { + ...newEthEoaAccount, + id: 'bb11bb22-cc33-4d44-9e55-ff6677889900', + address: '0xbbccddee00112233445566778899aabbccddeeff', + }; + const newAccount3 = { + ...newEthEoaAccount, + id: 'cc11bb22-cc33-4d44-ae55-ff6677889900', + address: '0xccddee00112233445566778899aabbccddee0011', + }; + + const entropySource = '01JQCAKR17JARQXZ0NDP760N1K'; + + const snapMetadata = { + manifest: { + proposedName: 'snap-name', + }, + id: snapId, + enabled: true, + }; + + it('creates multiple accounts', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + mockMessenger.get.mockReturnValue(snapMetadata); + + const accountsToCreate = [newAccount1, newAccount2, newAccount3]; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => { + // Unlike createAccount, createAccounts does NOT emit AccountCreated events + // for each account. It returns all accounts directly. + return accountsToCreate; + }, + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource, + range: { + from: 0, + to: 2, + }, + }; + const result = await keyring.createAccounts(snapId, options); + + expect(mockMessenger.handleRequest).toHaveBeenLastCalledWith( + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, options), + ); + + // Verify all accounts were returned + expect(result).toStrictEqual(accountsToCreate); + + // Verify all accounts were added to the internal state + for (const account of accountsToCreate) { + expect(keyring.getAccountByAddress(account.address)).toMatchObject({ + ...account, + metadata: expect.objectContaining({ + snap: expect.objectContaining({ + id: snapId, + }), + }), + }); + } + + // Verify state was saved once after adding all accounts + expect(mockCallbacks.saveState).toHaveBeenCalled(); + + // IMPORTANT: Unlike createAccount, createAccounts does NOT call addAccount callback + // because accounts are created in batch + expect(mockCallbacks.addAccount).not.toHaveBeenCalled(); + }); + + it('creates a single account through createAccounts', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + mockMessenger.get.mockReturnValue(snapMetadata); + + const accountToCreate = [newAccount1]; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountToCreate, + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + groupIndex: 0, + entropySource, + }; + const result = await keyring.createAccounts(snapId, options); + + expect(mockMessenger.handleRequest).toHaveBeenLastCalledWith( + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, options), + ); + + expect(result).toStrictEqual(accountToCreate); + expect(result).toHaveLength(1); + + // Verify the account was added to the internal state + expect(keyring.getAccountByAddress(newAccount1.address)).toMatchObject({ + ...newAccount1, + metadata: expect.objectContaining({ + snap: expect.objectContaining({ + id: snapId, + }), + }), + }); + + expect(mockCallbacks.saveState).toHaveBeenCalled(); + expect(mockCallbacks.addAccount).not.toHaveBeenCalled(); + }); + + it('creates accounts with custom options', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + const accountsToCreate = [newAccount1, newAccount2]; + const options: CreateAccountOptions = { + type: AccountCreationType.Custom, + }; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountsToCreate, + }); + + const result = await keyring.createAccounts(snapId, options); + + expect(mockMessenger.handleRequest).toHaveBeenLastCalledWith( + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, options), + ); + + expect(result).toStrictEqual(accountsToCreate); + expect(mockCallbacks.saveState).toHaveBeenCalled(); + expect(mockCallbacks.addAccount).not.toHaveBeenCalled(); + }); + + it('handles empty response from Snap', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => [], + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + const result = await keyring.createAccounts(snapId, options); + + expect(result).toStrictEqual([]); + expect(mockCallbacks.saveState).toHaveBeenCalled(); + expect(mockCallbacks.addAccount).not.toHaveBeenCalled(); + }); + + it('handles errors from Snap', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + const errorMessage = 'Failed to create accounts'; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => { + throw new Error(errorMessage); + }, + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + await expect(keyring.createAccounts(snapId, options)).rejects.toThrow( + errorMessage, + ); + + // State should not be saved if account creation fails + expect(mockCallbacks.saveState).not.toHaveBeenCalled(); + expect(mockCallbacks.addAccount).not.toHaveBeenCalled(); + }); + + it('adds all accounts to the internal map with correct snapId', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + mockMessenger.get.mockReturnValue(snapMetadata); + + const accountsToCreate = [newAccount1, newAccount2]; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountsToCreate, + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + await keyring.createAccounts(snapId, options); + + // Verify each account is mapped to the correct snapId + for (const account of accountsToCreate) { + const createdAccount = keyring.getAccountByAddress(account.address); + expect(createdAccount).toBeDefined(); + expect(createdAccount?.metadata.snap?.id).toBe(snapId); + } + }); + }); + describe('resolveAccountAddress', () => { const scope = toCaipChainId( KnownCaipNamespace.Eip155, diff --git a/packages/keyring-snap-client/src/KeyringClient.test.ts b/packages/keyring-snap-client/src/KeyringClient.test.ts index 71ed612b3..6fe32c3ce 100644 --- a/packages/keyring-snap-client/src/KeyringClient.test.ts +++ b/packages/keyring-snap-client/src/KeyringClient.test.ts @@ -1,4 +1,9 @@ -import { BtcMethod, BtcScope, KeyringRpcMethod } from '@metamask/keyring-api'; +import { + AccountCreationType, + BtcMethod, + BtcScope, + KeyringRpcMethod, +} from '@metamask/keyring-api'; import type { KeyringAccount, KeyringRequest, @@ -7,6 +12,7 @@ import type { CaipAssetType, CaipAssetTypeOrId, DiscoveredAccount, + CreateAccountOptions, } from '@metamask/keyring-api'; import type { JsonRpcRequest } from '@metamask/keyring-utils'; @@ -270,6 +276,107 @@ describe('KeyringClient', () => { const client = keyringClient; + describe('createAccounts', () => { + const entropySource = '01JQCAKR17JARQXZ0NDP760N1K'; + + it('should send a request to create multiple accounts and return the response', async () => { + const expectedResponse: KeyringAccount[] = [ + { + id: '49116980-0712-4fa5-b045-e4294f1d440e', + address: '0xE9A74AACd7df8112911ca93260fC5a046f8a64Ae', + options: {}, + methods: [], + scopes: ['eip155:0'], + type: 'eip155:eoa', + }, + { + id: '6d9e5e9a-8f9c-4b3a-9e3a-1e5c7f8a9b0c', + address: '0x1234567890123456789012345678901234567890', + options: {}, + methods: [], + scopes: ['eip155:0'], + type: 'eip155:eoa', + }, + ]; + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource, + range: { + from: 0, + to: 2, + }, + }; + + mockSender.send.mockResolvedValue(expectedResponse); + const accounts = await client.createAccounts(options); + expect(mockSender.send).toHaveBeenCalledWith({ + jsonrpc: '2.0', + id: expect.any(String), + method: 'keyring_createAccounts', + params: options, + }); + expect(accounts).toStrictEqual(expectedResponse); + }); + + it('should handle creating accounts with a single index', async () => { + const expectedResponse: KeyringAccount[] = [ + { + id: '49116980-0712-4fa5-b045-e4294f1d440e', + address: '0xE9A74AACd7df8112911ca93260fC5a046f8a64Ae', + options: {}, + methods: [], + scopes: ['eip155:0'], + type: 'eip155:eoa', + }, + ]; + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + groupIndex: 0, + entropySource, + }; + + mockSender.send.mockResolvedValue(expectedResponse); + const accounts = await client.createAccounts(options); + expect(mockSender.send).toHaveBeenCalledWith({ + jsonrpc: '2.0', + id: expect.any(String), + method: 'keyring_createAccounts', + params: options, + }); + expect(accounts).toStrictEqual(expectedResponse); + expect(accounts).toHaveLength(1); + }); + + it('should handle creating accounts with custom options', async () => { + const expectedResponse: KeyringAccount[] = [ + { + id: '49116980-0712-4fa5-b045-e4294f1d440e', + address: '0xE9A74AACd7df8112911ca93260fC5a046f8a64Ae', + options: {}, + methods: [], + scopes: ['eip155:0'], + type: 'eip155:eoa', + }, + ]; + + const options: CreateAccountOptions = { + type: AccountCreationType.Custom, + }; + + mockSender.send.mockResolvedValue(expectedResponse); + const accounts = await client.createAccounts(options); + expect(mockSender.send).toHaveBeenCalledWith({ + jsonrpc: '2.0', + id: expect.any(String), + method: 'keyring_createAccounts', + params: options, + }); + expect(accounts).toStrictEqual(expectedResponse); + }); + }); + describe('discoverAccounts', () => { const scopes = [BtcScope.Mainnet, BtcScope.Testnet]; const entropySource = '01JQCAKR17JARQXZ0NDP760N1K'; From 3d7740fd518f2b901dc71a316df5f782e2058b80 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 3 Feb 2026 22:08:26 +0100 Subject: [PATCH 03/21] fix: use proper options struct --- packages/keyring-api/src/rpc.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/keyring-api/src/rpc.ts b/packages/keyring-api/src/rpc.ts index 7579ace74..6f444c442 100644 --- a/packages/keyring-api/src/rpc.ts +++ b/packages/keyring-api/src/rpc.ts @@ -28,6 +28,7 @@ import { PaginationStruct, CaipAccountIdStruct, DiscoveredAccountStruct, + CreateAccountOptionsStruct, } from './api'; /** @@ -134,7 +135,7 @@ export const CreateAccountsRequestStruct = object({ ...CommonHeader, method: literal('keyring_createAccounts'), params: object({ - options: record(string(), JsonStruct), + options: CreateAccountOptionsStruct, }), }); From ff2729c3c8b24f5c8d01b4c51f93813d18ac5875 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 3 Feb 2026 22:08:33 +0100 Subject: [PATCH 04/21] fix: properly wrap options --- packages/keyring-snap-client/src/KeyringClient.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/keyring-snap-client/src/KeyringClient.ts b/packages/keyring-snap-client/src/KeyringClient.ts index f6e4a43c8..3e948d103 100644 --- a/packages/keyring-snap-client/src/KeyringClient.ts +++ b/packages/keyring-snap-client/src/KeyringClient.ts @@ -125,7 +125,7 @@ export class KeyringClient implements Keyring { return strictMask( await this.send({ method: KeyringRpcMethod.CreateAccounts, - params: options, + params: { options }, }), CreateAccountsResponseStruct, ); From 30e2fa6d483698ece18ccc9506f1775a219c46b5 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 3 Feb 2026 22:28:30 +0100 Subject: [PATCH 05/21] feat: add dispatch --- .../keyring-snap-sdk/src/rpc-handler.test.ts | 50 +++++++++++++++++++ packages/keyring-snap-sdk/src/rpc-handler.ts | 9 ++++ 2 files changed, 59 insertions(+) diff --git a/packages/keyring-snap-sdk/src/rpc-handler.test.ts b/packages/keyring-snap-sdk/src/rpc-handler.test.ts index 682cb2371..c470a7e7a 100644 --- a/packages/keyring-snap-sdk/src/rpc-handler.test.ts +++ b/packages/keyring-snap-sdk/src/rpc-handler.test.ts @@ -14,6 +14,7 @@ describe('handleKeyringRequest', () => { listAccounts: jest.fn(), getAccount: jest.fn(), createAccount: jest.fn(), + createAccounts: jest.fn(), discoverAccounts: jest.fn(), listAccountTransactions: jest.fn(), listAccountAssets: jest.fn(), @@ -120,6 +121,55 @@ describe('handleKeyringRequest', () => { expect(result).toBe('CreateAccount result'); }); + it('calls `keyring_createAccounts`', async () => { + const options = { + type: 'bip44:derive-index-range', + entropySource: '01JQCAKR17JARQXZ0NDP760N1K', + range: { + from: 0, + to: 2, + }, + }; + + const request: JsonRpcRequest = { + jsonrpc: '2.0', + id: '7c507ff0-365f-4de0-8cd5-eb83c30ebda4', + method: 'keyring_createAccounts', + params: { options }, + }; + + keyring.createAccounts.mockResolvedValue('CreateAccounts result'); + const result = await handleKeyringRequest(keyring, request); + + expect(keyring.createAccounts).toHaveBeenCalledWith(options); + expect(result).toBe('CreateAccounts result'); + }); + + it('throws an error if `keyring_createAccounts` is not implemented', async () => { + const options = { + type: 'bip44:derive-index-range', + entropySource: '01JQCAKR17JARQXZ0NDP760N1K', + range: { + from: 0, + to: 2, + }, + }; + + const request: JsonRpcRequest = { + jsonrpc: '2.0', + id: '7c507ff0-365f-4de0-8cd5-eb83c30ebda4', + method: 'keyring_createAccounts', + params: { options }, + }; + + const partialKeyring: Keyring = { ...keyring }; + delete partialKeyring.createAccounts; + + await expect(handleKeyringRequest(partialKeyring, request)).rejects.toThrow( + 'Method not supported: keyring_createAccounts', + ); + }); + it('calls `keyring_discoverAccounts`', async () => { const scopes = [BtcScope.Mainnet, BtcScope.Testnet]; const entropySource = '01JQCAKR17JARQXZ0NDP760N1K'; diff --git a/packages/keyring-snap-sdk/src/rpc-handler.ts b/packages/keyring-snap-sdk/src/rpc-handler.ts index 02a759e25..c921b4072 100644 --- a/packages/keyring-snap-sdk/src/rpc-handler.ts +++ b/packages/keyring-snap-sdk/src/rpc-handler.ts @@ -3,6 +3,7 @@ import { KeyringRpcMethod, GetAccountRequestStruct, CreateAccountRequestStruct, + CreateAccountsRequestStruct, ListAccountTransactionsRequestStruct, ApproveRequestRequestStruct, DeleteAccountRequestStruct, @@ -66,6 +67,14 @@ async function dispatchRequest( return keyring.createAccount(request.params.options); } + case `${KeyringRpcMethod.CreateAccounts}`: { + if (keyring.createAccounts === undefined) { + throw new MethodNotSupportedError(request.method); + } + assert(request, CreateAccountsRequestStruct); + return keyring.createAccounts(request.params.options); + } + case `${KeyringRpcMethod.DiscoverAccounts}`: { if (keyring.discoverAccounts === undefined) { throw new MethodNotSupportedError(request.method); From ba6ed3d3729df420b92c6e080d3d196f05c2a1f0 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 3 Feb 2026 22:51:18 +0100 Subject: [PATCH 06/21] fix: check for AnyAccount type --- .../src/SnapKeyring.test.ts | 40 +++++++++++++++++-- .../keyring-snap-bridge/src/SnapKeyring.ts | 9 +++++ 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index 98aaccefe..ce4e2f32c 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2576,7 +2576,7 @@ describe('SnapKeyring', () => { const result = await keyring.createAccounts(snapId, options); expect(mockMessenger.handleRequest).toHaveBeenLastCalledWith( - mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, options), + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, { options }), ); // Verify all accounts were returned @@ -2622,7 +2622,7 @@ describe('SnapKeyring', () => { const result = await keyring.createAccounts(snapId, options); expect(mockMessenger.handleRequest).toHaveBeenLastCalledWith( - mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, options), + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, { options }), ); expect(result).toStrictEqual(accountToCreate); @@ -2658,7 +2658,7 @@ describe('SnapKeyring', () => { const result = await keyring.createAccounts(snapId, options); expect(mockMessenger.handleRequest).toHaveBeenLastCalledWith( - mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, options), + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, { options }), ); expect(result).toStrictEqual(accountsToCreate); @@ -2738,6 +2738,40 @@ describe('SnapKeyring', () => { expect(createdAccount?.metadata.snap?.id).toBe(snapId); } }); + + it('throws an error when creating generic accounts if not allowed', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + // Create a keyring with isAnyAccountTypeAllowed = false + const restrictedKeyring = new SnapKeyring({ + messenger: mockSnapKeyringMessenger, + callbacks: mockCallbacks, + isAnyAccountTypeAllowed: false, + }); + + const genericAccount = { + ...newAccount1, + type: AnyAccountType.Account, + }; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => [genericAccount], + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + + await expect( + restrictedKeyring.createAccounts(snapId, options), + ).rejects.toThrow(`Cannot create generic account '${genericAccount.id}'`); + + // State should not be saved if validation fails + expect(mockCallbacks.saveState).not.toHaveBeenCalled(); + }); }); describe('resolveAccountAddress', () => { diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 26e8ba67f..b86ea4470 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -916,6 +916,15 @@ export class SnapKeyring { // accounts to the keyring, since those accounts are created in batch. const accounts = await client.createAccounts(options); for (const account of accounts) { + // The `AnyAccountType.Account` generic account type is allowed only during + // development, so we check whether it's allowed before continuing. + if ( + !this.#isAnyAccountTypeAllowed && + account.type === AnyAccountType.Account + ) { + throw new Error(`Cannot create generic account '${account.id}'`); + } + this.#accounts.set(account.id, { account, snapId }); } From e7cb4d869219eef69acde0a2a501ca814ccb3e41 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 12:11:01 +0100 Subject: [PATCH 07/21] docs: fix doc --- packages/keyring-snap-bridge/src/SnapKeyring.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index b86ea4470..315537b4e 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -893,14 +893,16 @@ export class SnapKeyring { } /** - * Create one or more accounts according to the provided options. + * Creates one or more new accounts according to the provided options. * - * This method supports batch account creation for BIP-44 derivation paths, - * allowing the creation of multiple accounts up to a specified maximum index. + * Deterministic account creation MUST be idempotent, meaning that for + * deterministic algorithms, like BIP-44, calling this method with the same + * options should always return the same accounts, even if the accounts + * already exist in the keyring. * - * @param snapId - Snap ID to create the accounts for. - * @param options - Account creation options. - * @returns A promise that resolves to an array of the created account objects. + * @param snapId - Snap ID to create the account(s) for. + * @param options - Options describing how to create the account(s). + * @returns An array of the created account objects. */ async createAccounts( snapId: SnapId, From f9c3f0de92cdeb8e045bd1ab469296cb0e3e5ffe Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 12:22:39 +0100 Subject: [PATCH 08/21] fix: skip generic accounts if not allowed --- .../src/SnapKeyring.test.ts | 50 ++++++++++++++++--- .../keyring-snap-bridge/src/SnapKeyring.ts | 32 ++++++++++-- 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index ce4e2f32c..13fdad2ae 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2739,10 +2739,12 @@ describe('SnapKeyring', () => { } }); - it('throws an error when creating generic accounts if not allowed', async () => { + it('skips and cleans up unsupported generic accounts when not allowed', async () => { mockCallbacks.addAccount.mockClear(); mockCallbacks.saveState.mockClear(); + mockMessenger.get.mockReturnValue(snapMetadata); + // Create a keyring with isAnyAccountTypeAllowed = false const restrictedKeyring = new SnapKeyring({ messenger: mockSnapKeyringMessenger, @@ -2752,11 +2754,26 @@ describe('SnapKeyring', () => { const genericAccount = { ...newAccount1, + id: 'aa11bb22-cc33-4d44-8e55-ff6677889900', + address: '0xaabbccddee00112233445566778899aabbccddee', type: AnyAccountType.Account, }; + const supportedAccount = { + ...newAccount2, + id: 'bb11bb22-cc33-4d44-9e55-ff6677889900', + address: '0xbbccddee00112233445566778899aabbccddeeff', + type: EthAccountType.Eoa, + }; + + const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); + mockMessengerHandleRequest({ - [KeyringRpcMethod.CreateAccounts]: async () => [genericAccount], + [KeyringRpcMethod.CreateAccounts]: async () => [ + genericAccount, + supportedAccount, + ], + [KeyringRpcMethod.DeleteAccount]: async () => null, }); const options: CreateAccountOptions = { @@ -2765,12 +2782,31 @@ describe('SnapKeyring', () => { groupIndex: 0, }; - await expect( - restrictedKeyring.createAccounts(snapId, options), - ).rejects.toThrow(`Cannot create generic account '${genericAccount.id}'`); + const result = await restrictedKeyring.createAccounts(snapId, options); - // State should not be saved if validation fails - expect(mockCallbacks.saveState).not.toHaveBeenCalled(); + expect(result).toHaveLength(1); + expect(result).not.toContain(genericAccount); + expect(result).toContain(supportedAccount); + expect( + restrictedKeyring.getAccountByAddress(genericAccount.address), + ).toBeUndefined(); + expect( + restrictedKeyring.getAccountByAddress(supportedAccount.address), + ).toBeDefined(); + + expect(consoleSpy).toHaveBeenCalledWith( + `SnapKeyring - Found an unsupported generic account: ${genericAccount.id}`, + ); + + expect(mockMessenger.handleRequest).toHaveBeenCalledWith( + mockKeyringRpcRequest(KeyringRpcMethod.DeleteAccount, { + id: genericAccount.id, + }), + ); + + expect(mockCallbacks.saveState).toHaveBeenCalled(); + + consoleSpy.mockRestore(); }); }); diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 315537b4e..f5e262a30 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -900,6 +900,9 @@ export class SnapKeyring { * options should always return the same accounts, even if the accounts * already exist in the keyring. * + * NOTE: If generic accounts are not allowed, this method will skip their + * creation and ask the Snap to remove them from its state. + * * @param snapId - Snap ID to create the account(s) for. * @param options - Options describing how to create the account(s). * @returns An array of the created account objects. @@ -913,24 +916,43 @@ export class SnapKeyring { snapId, }); - // Add each returned account to the internal accounts map. + const unsupportedAccountIds = []; + // NOTE: This method DOES NOT rely on the `AccountCreated` event to add // accounts to the keyring, since those accounts are created in batch. - const accounts = await client.createAccounts(options); - for (const account of accounts) { + const snapAccounts = await client.createAccounts(options); + + // Add each returned account to the internal accounts map and maybe filter + // unsupported generic accounts. + const accounts = []; + for (const account of snapAccounts) { // The `AnyAccountType.Account` generic account type is allowed only during // development, so we check whether it's allowed before continuing. if ( !this.#isAnyAccountTypeAllowed && account.type === AnyAccountType.Account ) { - throw new Error(`Cannot create generic account '${account.id}'`); + console.warn( + `SnapKeyring - Found an unsupported generic account: ${account.id}`, + ); + unsupportedAccountIds.push(account.id); + + continue; // Skip adding this account. } + // Update the internal accounts map since `createAccounts` does not register + // them through the `AccountCreated` event. this.#accounts.set(account.id, { account, snapId }); + + accounts.push(account); + } + + // Cleanup if we found unsupported accounts to avoid avoid "dangling" accounts + // on the Snap side. + for (const accountId of unsupportedAccountIds) { + await client.deleteAccount(accountId); } - // Save the state after adding all accounts. await this.#callbacks.saveState(); return accounts; From cf5300031e13c0843e69f7f4bc4ba4c13fd145d0 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 12:39:00 +0100 Subject: [PATCH 09/21] chore: update changelogs --- packages/keyring-api/CHANGELOG.md | 1 + packages/keyring-snap-bridge/CHANGELOG.md | 1 + packages/keyring-snap-sdk/CHANGELOG.md | 6 ++++++ 3 files changed, 8 insertions(+) diff --git a/packages/keyring-api/CHANGELOG.md b/packages/keyring-api/CHANGELOG.md index 5e53cf9a7..750100249 100644 --- a/packages/keyring-api/CHANGELOG.md +++ b/packages/keyring-api/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add `Keyring.createAccounts` optional method ([#448](https://github.com/MetaMask/accounts/pull/448)) - This method is part of the keyring v2 specification and set as optional for backwards compatibility. - This method can be used to create one or more accounts using the new keyring v2 account creation typed options. + - Add RPC support for this method through `KeyringRpcMethod.CreateAccounts`. - Add support for account derivations using range of indices in `KeyringV2` ([#451](https://github.com/MetaMask/accounts/pull/451)) - Add `bip44:derive-index-range` capability to `KeyringCapabilities`. - Add `AccountCreationType.Bip44DeriveIndexRange` and `CreateAccountBip44DeriveIndexRangeOptions`. diff --git a/packages/keyring-snap-bridge/CHANGELOG.md b/packages/keyring-snap-bridge/CHANGELOG.md index a98b15e30..059472fec 100644 --- a/packages/keyring-snap-bridge/CHANGELOG.md +++ b/packages/keyring-snap-bridge/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add `SnapKeyring.createAccounts` method ([#448](https://github.com/MetaMask/accounts/pull/448)) - This method can be used to create one or more accounts using the new keyring v2 account creation typed options. + - Generic accounts will be filtered out if they are not allowed by the keyring configuration. ### Changed diff --git a/packages/keyring-snap-sdk/CHANGELOG.md b/packages/keyring-snap-sdk/CHANGELOG.md index f655f263d..ee7350fd4 100644 --- a/packages/keyring-snap-sdk/CHANGELOG.md +++ b/packages/keyring-snap-sdk/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `keyring_createAccounts` method to RPC dispatcher ([#448](https://github.com/MetaMask/accounts/pull/448)) + - This method SHOULD NOT use `notify:accountCreated`. + - Accounts returned by this method are will be automatically added to the internal keyring state. + ### Changed - Bump `@metamask/snaps-sdk` from `^9.0.0` to `^10.3.0` ([#422](https://github.com/MetaMask/accounts/pull/422)) From d7647dc28145570ff33624a2d247de63b1117fa4 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 12:47:30 +0100 Subject: [PATCH 10/21] refactor: cosmetic --- .../keyring-snap-bridge/src/SnapKeyring.ts | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index f5e262a30..630ccff1b 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -262,6 +262,18 @@ export class SnapKeyring { ); } + /** + * Checks whether an account is supported by this keyring. + * + * @param account - The account to check. + * @returns True if the account is supported, false otherwise. + */ + #isGenericAccountSupported(account: KeyringAccount): boolean { + return ( + !this.#isAnyAccountTypeAllowed && account.type === AnyAccountType.Account + ); + } + /** * Get internal options given a correlation ID. * @@ -328,10 +340,7 @@ export class SnapKeyring { // The `AnyAccountType.Account` generic account type is allowed only during // development, so we check whether it's allowed before continuing. - if ( - !this.#isAnyAccountTypeAllowed && - account.type === AnyAccountType.Account - ) { + if (!this.#isGenericAccountSupported(account)) { throw new Error(`Cannot create generic account '${account.id}'`); } @@ -928,10 +937,7 @@ export class SnapKeyring { for (const account of snapAccounts) { // The `AnyAccountType.Account` generic account type is allowed only during // development, so we check whether it's allowed before continuing. - if ( - !this.#isAnyAccountTypeAllowed && - account.type === AnyAccountType.Account - ) { + if (!this.#isGenericAccountSupported(account)) { console.warn( `SnapKeyring - Found an unsupported generic account: ${account.id}`, ); From 9ce6b7f431443c0a2a9c430d866dabe25844555b Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 17:26:20 +0100 Subject: [PATCH 11/21] fix: check account preconditions + call saveState only when necessary --- .../src/SnapKeyring.test.ts | 83 ++++++++- .../keyring-snap-bridge/src/SnapKeyring.ts | 163 ++++++++++++------ packages/keyring-snap-bridge/src/errors.ts | 14 ++ 3 files changed, 201 insertions(+), 59 deletions(-) create mode 100644 packages/keyring-snap-bridge/src/errors.ts diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index 13fdad2ae..2cf884a16 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2682,7 +2682,7 @@ describe('SnapKeyring', () => { const result = await keyring.createAccounts(snapId, options); expect(result).toStrictEqual([]); - expect(mockCallbacks.saveState).toHaveBeenCalled(); + expect(mockCallbacks.saveState).toHaveBeenCalledTimes(0); expect(mockCallbacks.addAccount).not.toHaveBeenCalled(); }); @@ -2795,7 +2795,7 @@ describe('SnapKeyring', () => { ).toBeDefined(); expect(consoleSpy).toHaveBeenCalledWith( - `SnapKeyring - Found an unsupported generic account: ${genericAccount.id}`, + `SnapKeyring - Cannot create generic account '${genericAccount.id}' - Skipping account: ${genericAccount.id}.`, ); expect(mockMessenger.handleRequest).toHaveBeenCalledWith( @@ -2808,6 +2808,85 @@ describe('SnapKeyring', () => { consoleSpy.mockRestore(); }); + + it('handles idempotent account creation by skipping existing accounts', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + mockMessenger.get.mockReturnValue(snapMetadata); + + const accountsToCreate = [newAccount1, newAccount2]; + + // First call: create accounts + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountsToCreate, + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + + const firstResult = await keyring.createAccounts(snapId, options); + + // Verify accounts were created + expect(firstResult).toHaveLength(2); + expect(mockCallbacks.saveState).toHaveBeenCalledTimes(1); + + // Clear mocks for second call + mockCallbacks.saveState.mockClear(); + + // Second call: same accounts should be skipped (idempotent) + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountsToCreate, + }); + + const secondResult = await keyring.createAccounts(snapId, options); + + // Should return the same accounts (idempotent behavior) + expect(secondResult).toHaveLength(2); + expect(secondResult).toStrictEqual(accountsToCreate); + + // No new accounts should be added, so saveState should not be called again + expect(mockCallbacks.saveState).toHaveBeenCalledTimes(0); + + // Verify the original accounts still exist and weren't duplicated + for (const account of accountsToCreate) { + const existingAccount = keyring.getAccountByAddress(account.address); + expect(existingAccount).toBeDefined(); + expect(existingAccount?.id).toBe(account.id); + } + }); + + it('throws non-AccountError exceptions during account validation', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + const accountToCreate = [newAccount1]; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountToCreate, + }); + + // Mock addressExists to throw a non-AccountError + const error = 'KeyringController is locked'; + mockCallbacks.addressExists.mockRejectedValue(new Error(error)); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + + // Should propagate non-AccountError exceptions + await expect(keyring.createAccounts(snapId, options)).rejects.toThrow( + error, + ); + + // State should not be saved when an unexpected error occurs + expect(mockCallbacks.saveState).not.toHaveBeenCalled(); + }); }); describe('resolveAccountAddress', () => { diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 630ccff1b..af8ac5544 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -53,6 +53,7 @@ import { v4 as uuid } from 'uuid'; import { transformAccount } from './account'; import { DeferredPromise } from './DeferredPromise'; +import { AccountAssertError } from './errors'; import { AccountCreatedEventStruct, AccountUpdatedEventStruct, @@ -263,17 +264,70 @@ export class SnapKeyring { } /** - * Checks whether an account is supported by this keyring. + * Asserts that an account can be used within the Snap keyring. (e.g. generic accounts). * * @param account - The account to check. - * @returns True if the account is supported, false otherwise. + * @throws If the account cannot be used. */ - #isGenericAccountSupported(account: KeyringAccount): boolean { + #assertAccountCanBeUsed(account: KeyringAccount): void { + // The `AnyAccountType.Account` generic account type is allowed only during + // development, so we check whether it's allowed before continuing. + if ( + !this.#isAnyAccountTypeAllowed && + account.type === AnyAccountType.Account + ) { + throw new AccountAssertError(`Cannot create generic account '${account.id}'`); + } + } + + /** + * Checks whether an account is known. + * + * @param snapId - The account's Snap ID. + * @param account - The account to check. + * @returns True if the account is known, false otherwise. + */ + #isAccountAlreadyKnown(snapId: SnapId, account: KeyringAccount): boolean { + const address = normalizeAccountAddress(account); + + // Acount creation is idempotent, so we need to check whether the account already exists + // and that the right Snap is trying to "create" it again. + // NOTE: We are not checking account object equality here. If a Snap + // re-send this event with different account data, we will ignore it. + const accountEntry = this.#accounts.get(snapId, account.id); return ( - !this.#isAnyAccountTypeAllowed && account.type === AnyAccountType.Account + accountEntry !== undefined && + normalizeAccountAddress(accountEntry.account) === address ); } + /** + * Asserts that an account is unique and can be added to the keyring. + * + * @param snapId - The account's Snap ID. + * @param account - The account to check. + * @throws If the account is not unique. + */ + async #assertAccountIsUnique( + snapId: SnapId, + account: KeyringAccount, + ): Promise { + const address = normalizeAccountAddress(account); + + // The UI still uses the account address to identify accounts, so we need + // to block the creation of duplicate accounts for now to prevent accounts + // from being overwritten. + if (await this.#callbacks.addressExists(address)) { + throw new AccountAssertError(`Account address '${address}' already exists`); + } + + // A Snap could try to create an account with a different address but with + // an existing ID, so the above test only is not enough. + if (this.#accounts.has(snapId, account.id)) { + throw new AccountAssertError(`Account '${account.id}' already exists`); + } + } + /** * Get internal options given a correlation ID. * @@ -338,36 +392,16 @@ export class SnapKeyring { const account = transformAccount(newAccountFromEvent); const address = normalizeAccountAddress(account); - // The `AnyAccountType.Account` generic account type is allowed only during - // development, so we check whether it's allowed before continuing. - if (!this.#isGenericAccountSupported(account)) { - throw new Error(`Cannot create generic account '${account.id}'`); - } - - // This is idempotent, so we need to check whether the account already exists - // and that the right Snap is trying to "create" it again. - const accountEntry = this.#accounts.get(snapId, account.id); - if ( - accountEntry && - normalizeAccountAddress(accountEntry.account) === address - ) { - // NOTE: We are not checking account object equality here. If a Snap - // re-send this event with different account data, we will ignore it. + // Check for account preconditions. Order matters here: + // 1. Account type validity (e.g. generic accounts). + // 2. Account existence (idempotency). + // 3. Account uniqueness (e.g. address and ID uniqueness). + this.#assertAccountCanBeUsed(account); + if (this.#isAccountAlreadyKnown(snapId, account)) { + // If the account already exists, we skip it. return null; } - - // The UI still uses the account address to identify accounts, so we need - // to block the creation of duplicate accounts for now to prevent accounts - // from being overwritten. - if (await this.#callbacks.addressExists(address)) { - throw new Error(`Account address '${address}' already exists`); - } - - // A Snap could try to create an account with a different address but with - // an existing ID, so the above test only is not enough. - if (this.#accounts.has(snapId, account.id)) { - throw new Error(`Account '${account.id}' already exists`); - } + await this.#assertAccountIsUnique(snapId, account); // A deferred promise that will be resolved once the Snap keyring has saved // its internal state. @@ -909,8 +943,9 @@ export class SnapKeyring { * options should always return the same accounts, even if the accounts * already exist in the keyring. * - * NOTE: If generic accounts are not allowed, this method will skip their - * creation and ask the Snap to remove them from its state. + * NOTE: If some accounts are not allowed (non-unique address, unsupported + * generic account), this method will skip their creation and ask the Snap + * to remove them from its state. * * @param snapId - Snap ID to create the account(s) for. * @param options - Options describing how to create the account(s). @@ -925,32 +960,38 @@ export class SnapKeyring { snapId, }); + const accounts = []; + const newAccounts = []; const unsupportedAccountIds = []; + for (const account of await client.createAccounts(options)) { + try { + // Check for account preconditions. Order matters here: + // 1. Account type validity (e.g. generic accounts). + // 2. Account existence (idempotency). + // 3. Account uniqueness (e.g. address and ID uniqueness). + this.#assertAccountCanBeUsed(account); + if (!this.#isAccountAlreadyKnown(snapId, account)) { + await this.#assertAccountIsUnique(snapId, account); + + newAccounts.push(account); + } - // NOTE: This method DOES NOT rely on the `AccountCreated` event to add - // accounts to the keyring, since those accounts are created in batch. - const snapAccounts = await client.createAccounts(options); - - // Add each returned account to the internal accounts map and maybe filter - // unsupported generic accounts. - const accounts = []; - for (const account of snapAccounts) { - // The `AnyAccountType.Account` generic account type is allowed only during - // development, so we check whether it's allowed before continuing. - if (!this.#isGenericAccountSupported(account)) { - console.warn( - `SnapKeyring - Found an unsupported generic account: ${account.id}`, - ); - unsupportedAccountIds.push(account.id); + // New AND existing accounts are returned to the caller no matter what. + accounts.push(account); + } catch (error) { + // Any account error means that the account cannot be used, so we + // just log a warning and continue. + if (error instanceof AccountAssertError) { + console.warn( + `SnapKeyring - ${error.message} - Skipping account: ${account.id}.`, + ); - continue; // Skip adding this account. + // Keep track of unsupported accounts to ask the Snap to delete them later. + unsupportedAccountIds.push(account.id); + } else { + throw error; + } } - - // Update the internal accounts map since `createAccounts` does not register - // them through the `AccountCreated` event. - this.#accounts.set(account.id, { account, snapId }); - - accounts.push(account); } // Cleanup if we found unsupported accounts to avoid avoid "dangling" accounts @@ -959,7 +1000,15 @@ export class SnapKeyring { await client.deleteAccount(accountId); } - await this.#callbacks.saveState(); + // NOTE: This method DOES NOT rely on the `AccountCreated` event to add + // accounts to the keyring, since those accounts are created in batch. + if (newAccounts.length > 0) { + for (const account of newAccounts) { + this.#accounts.set(account.id, { account, snapId }); + } + + await this.#callbacks.saveState(); + } return accounts; } diff --git a/packages/keyring-snap-bridge/src/errors.ts b/packages/keyring-snap-bridge/src/errors.ts new file mode 100644 index 000000000..bf5adcae0 --- /dev/null +++ b/packages/keyring-snap-bridge/src/errors.ts @@ -0,0 +1,14 @@ +/** + * Custom error class for account-related errors. + */ +export class AccountAssertError extends Error { + constructor(message: string) { + super(message); + this.name = 'AccountError'; + + // Maintains proper stack trace for where our error was thrown (only available on V8) + if (Error.captureStackTrace) { + Error.captureStackTrace(this, AccountAssertError); + } + } +} From 23bdc61d144e85a487db1cdb2e0e6995c7894767 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 17:41:50 +0100 Subject: [PATCH 12/21] fix: rollback all accounts on error --- .../src/SnapKeyring.test.ts | 59 +++++++++++-------- .../keyring-snap-bridge/src/SnapKeyring.ts | 45 ++++++-------- packages/keyring-snap-bridge/src/errors.ts | 14 ----- 3 files changed, 50 insertions(+), 68 deletions(-) delete mode 100644 packages/keyring-snap-bridge/src/errors.ts diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index 2cf884a16..45e9bb755 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2739,12 +2739,10 @@ describe('SnapKeyring', () => { } }); - it('skips and cleans up unsupported generic accounts when not allowed', async () => { + it('throws error and rolls back Snap state when encountering unsupported accounts', async () => { mockCallbacks.addAccount.mockClear(); mockCallbacks.saveState.mockClear(); - mockMessenger.get.mockReturnValue(snapMetadata); - // Create a keyring with isAnyAccountTypeAllowed = false const restrictedKeyring = new SnapKeyring({ messenger: mockSnapKeyringMessenger, @@ -2752,26 +2750,24 @@ describe('SnapKeyring', () => { isAnyAccountTypeAllowed: false, }); - const genericAccount = { + const supportedAccount = { ...newAccount1, id: 'aa11bb22-cc33-4d44-8e55-ff6677889900', address: '0xaabbccddee00112233445566778899aabbccddee', - type: AnyAccountType.Account, + type: EthAccountType.Eoa, }; - const supportedAccount = { + const genericAccount = { ...newAccount2, id: 'bb11bb22-cc33-4d44-9e55-ff6677889900', address: '0xbbccddee00112233445566778899aabbccddeeff', - type: EthAccountType.Eoa, + type: AnyAccountType.Account, }; - const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); - mockMessengerHandleRequest({ [KeyringRpcMethod.CreateAccounts]: async () => [ - genericAccount, supportedAccount, + genericAccount, ], [KeyringRpcMethod.DeleteAccount]: async () => null, }); @@ -2782,31 +2778,42 @@ describe('SnapKeyring', () => { groupIndex: 0, }; - const result = await restrictedKeyring.createAccounts(snapId, options); - - expect(result).toHaveLength(1); - expect(result).not.toContain(genericAccount); - expect(result).toContain(supportedAccount); - expect( - restrictedKeyring.getAccountByAddress(genericAccount.address), - ).toBeUndefined(); - expect( - restrictedKeyring.getAccountByAddress(supportedAccount.address), - ).toBeDefined(); + // Should throw error when encountering unsupported account + await expect( + restrictedKeyring.createAccounts(snapId, options), + ).rejects.toThrow(`Cannot create generic account '${genericAccount.id}'`); - expect(consoleSpy).toHaveBeenCalledWith( - `SnapKeyring - Cannot create generic account '${genericAccount.id}' - Skipping account: ${genericAccount.id}.`, + // We still have sent a Snap request to create accounts + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 1, + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, { options }), ); - expect(mockMessenger.handleRequest).toHaveBeenCalledWith( + // BUT, we should roll back Snap state by deleting the accounts that were created + // since generic accounts are not allowed in this keyring configuration + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 2, + mockKeyringRpcRequest(KeyringRpcMethod.DeleteAccount, { + id: supportedAccount.id, + }), + ); + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 3, mockKeyringRpcRequest(KeyringRpcMethod.DeleteAccount, { id: genericAccount.id, }), ); - expect(mockCallbacks.saveState).toHaveBeenCalled(); + // No accounts should be added to the keyring state + expect( + restrictedKeyring.getAccountByAddress(supportedAccount.address), + ).toBeUndefined(); + expect( + restrictedKeyring.getAccountByAddress(genericAccount.address), + ).toBeUndefined(); - consoleSpy.mockRestore(); + // State should not be saved when operation fails + expect(mockCallbacks.saveState).not.toHaveBeenCalled(); }); it('handles idempotent account creation by skipping existing accounts', async () => { diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index af8ac5544..99ae3374d 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -53,7 +53,6 @@ import { v4 as uuid } from 'uuid'; import { transformAccount } from './account'; import { DeferredPromise } from './DeferredPromise'; -import { AccountAssertError } from './errors'; import { AccountCreatedEventStruct, AccountUpdatedEventStruct, @@ -276,7 +275,7 @@ export class SnapKeyring { !this.#isAnyAccountTypeAllowed && account.type === AnyAccountType.Account ) { - throw new AccountAssertError(`Cannot create generic account '${account.id}'`); + throw new Error(`Cannot create generic account '${account.id}'`); } } @@ -318,13 +317,13 @@ export class SnapKeyring { // to block the creation of duplicate accounts for now to prevent accounts // from being overwritten. if (await this.#callbacks.addressExists(address)) { - throw new AccountAssertError(`Account address '${address}' already exists`); + throw new Error(`Account address '${address}' already exists`); } // A Snap could try to create an account with a different address but with // an existing ID, so the above test only is not enough. if (this.#accounts.has(snapId, account.id)) { - throw new AccountAssertError(`Account '${account.id}' already exists`); + throw new Error(`Account '${account.id}' already exists`); } } @@ -960,11 +959,11 @@ export class SnapKeyring { snapId, }); - const accounts = []; + const allAccounts = []; const newAccounts = []; - const unsupportedAccountIds = []; - for (const account of await client.createAccounts(options)) { - try { + const snapAccounts = await client.createAccounts(options); + try { + for (const account of snapAccounts) { // Check for account preconditions. Order matters here: // 1. Account type validity (e.g. generic accounts). // 2. Account existence (idempotency). @@ -973,31 +972,21 @@ export class SnapKeyring { if (!this.#isAccountAlreadyKnown(snapId, account)) { await this.#assertAccountIsUnique(snapId, account); + // Keep track of newly created accounts to add them to the keyring + // state later. newAccounts.push(account); } // New AND existing accounts are returned to the caller no matter what. - accounts.push(account); - } catch (error) { - // Any account error means that the account cannot be used, so we - // just log a warning and continue. - if (error instanceof AccountAssertError) { - console.warn( - `SnapKeyring - ${error.message} - Skipping account: ${account.id}.`, - ); - - // Keep track of unsupported accounts to ask the Snap to delete them later. - unsupportedAccountIds.push(account.id); - } else { - throw error; - } + allAccounts.push(account); + } + } catch (error) { + // Rollback Snap state if something went wrong during account creation. + for (const account of snapAccounts) { + await this.#deleteAccount(snapId, account); } - } - // Cleanup if we found unsupported accounts to avoid avoid "dangling" accounts - // on the Snap side. - for (const accountId of unsupportedAccountIds) { - await client.deleteAccount(accountId); + throw error; } // NOTE: This method DOES NOT rely on the `AccountCreated` event to add @@ -1010,7 +999,7 @@ export class SnapKeyring { await this.#callbacks.saveState(); } - return accounts; + return allAccounts; } /** diff --git a/packages/keyring-snap-bridge/src/errors.ts b/packages/keyring-snap-bridge/src/errors.ts deleted file mode 100644 index bf5adcae0..000000000 --- a/packages/keyring-snap-bridge/src/errors.ts +++ /dev/null @@ -1,14 +0,0 @@ -/** - * Custom error class for account-related errors. - */ -export class AccountAssertError extends Error { - constructor(message: string) { - super(message); - this.name = 'AccountError'; - - // Maintains proper stack trace for where our error was thrown (only available on V8) - if (Error.captureStackTrace) { - Error.captureStackTrace(this, AccountAssertError); - } - } -} From be8657142fe87d775151cc23f6f7e5630f820be5 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 18:24:06 +0100 Subject: [PATCH 13/21] fix: only rollback non-existing accounts (from the state) --- packages/keyring-snap-bridge/src/SnapKeyring.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 99ae3374d..4d65eee54 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -983,7 +983,10 @@ export class SnapKeyring { } catch (error) { // Rollback Snap state if something went wrong during account creation. for (const account of snapAccounts) { - await this.#deleteAccount(snapId, account); + // Make sure to only delete accounts that were not part of the keyring state. + if (!this.#isAccountAlreadyKnown(snapId, account)) { + await this.#deleteAccount(snapId, account); + } } throw error; From 8d2294cd68c8d021ee146ae930366e6def446108 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Wed, 4 Feb 2026 18:25:48 +0100 Subject: [PATCH 14/21] test: fix KeyringSnapClient tests --- packages/keyring-snap-client/src/KeyringClient.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/keyring-snap-client/src/KeyringClient.test.ts b/packages/keyring-snap-client/src/KeyringClient.test.ts index 6fe32c3ce..65424d911 100644 --- a/packages/keyring-snap-client/src/KeyringClient.test.ts +++ b/packages/keyring-snap-client/src/KeyringClient.test.ts @@ -314,7 +314,7 @@ describe('KeyringClient', () => { jsonrpc: '2.0', id: expect.any(String), method: 'keyring_createAccounts', - params: options, + params: { options }, }); expect(accounts).toStrictEqual(expectedResponse); }); @@ -343,7 +343,7 @@ describe('KeyringClient', () => { jsonrpc: '2.0', id: expect.any(String), method: 'keyring_createAccounts', - params: options, + params: { options }, }); expect(accounts).toStrictEqual(expectedResponse); expect(accounts).toHaveLength(1); @@ -371,7 +371,7 @@ describe('KeyringClient', () => { jsonrpc: '2.0', id: expect.any(String), method: 'keyring_createAccounts', - params: options, + params: { options }, }); expect(accounts).toStrictEqual(expectedResponse); }); From 77c703ec88b55bff22d24a05ace1d6fad25c7239 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 5 Feb 2026 09:22:27 +0100 Subject: [PATCH 15/21] fix: proper rollback + test --- .../src/SnapKeyring.test.ts | 118 ++++++++++++++++++ .../keyring-snap-bridge/src/SnapKeyring.ts | 43 ++++--- 2 files changed, 142 insertions(+), 19 deletions(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index 45e9bb755..b9598070a 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2894,6 +2894,124 @@ describe('SnapKeyring', () => { // State should not be saved when an unexpected error occurs expect(mockCallbacks.saveState).not.toHaveBeenCalled(); }); + + it('rolls back keyring state and only deletes new Snap accounts when saveState fails', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + mockCallbacks.addressExists.mockClear(); + + mockMessenger.get.mockReturnValue(snapMetadata); + + // First, create an existing account that's already in the keyring + const existingAccount = { + ...newAccount1, + id: 'aa11bb22-cc33-4d44-ae55-ff6677889900', + address: '0xaabbccddee00112233445566778899aabbccdd00', + }; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => [existingAccount], + }); + + mockCallbacks.addressExists.mockResolvedValue(false); + + await keyring.createAccounts(snapId, { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }); + + // Verify the existing account is in the keyring + expect(keyring.getAccountByAddress(existingAccount.address)).toBeDefined(); + + // Clear the mocks for the second call + mockCallbacks.saveState.mockClear(); + mockCallbacks.addressExists.mockClear(); + mockMessenger.handleRequest.mockClear(); + + // Now try to create new accounts, but make saveState fail + const newAccount3 = { + ...newAccount1, + id: 'bb11bb22-cc33-4d44-be55-ff6677889911', + address: '0xbbccddee00112233445566778899aabbccdd11', + }; + + const newAccount4 = { + ...newAccount2, + id: 'cc11bb22-cc33-4d44-8e55-ff6677889922', + address: '0xccddee00112233445566778899aabbccddee22', + }; + + const accountsFromSnap = [existingAccount, newAccount3, newAccount4]; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountsFromSnap, + [KeyringRpcMethod.DeleteAccount]: async () => null, + }); + + mockCallbacks.addressExists.mockResolvedValue(false); + + // Make saveState fail + const saveStateError = new Error('Failed to save state to disk'); + mockCallbacks.saveState.mockRejectedValue(saveStateError); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 1, + }; + + // Should throw the saveState error + await expect(keyring.createAccounts(snapId, options)).rejects.toThrow( + saveStateError, + ); + + // Verify that createAccounts was called on the Snap + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + request: expect.objectContaining({ + method: KeyringRpcMethod.CreateAccounts, + }), + }), + ); + + // Verify that only the NEW accounts (not the pre-existing one) were deleted from the Snap + // The existing account should NOT be deleted because it was already in the keyring state + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + request: expect.objectContaining({ + method: KeyringRpcMethod.DeleteAccount, + params: { id: newAccount3.id }, + }), + }), + ); + + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 3, + expect.objectContaining({ + request: expect.objectContaining({ + method: KeyringRpcMethod.DeleteAccount, + params: { id: newAccount4.id }, + }), + }), + ); + + // Should only have 3 delete calls (for the 2 new accounts, not the existing one) + expect(mockMessenger.handleRequest).toHaveBeenCalledTimes(3); + + // Verify the keyring state was rolled back + // The existing account should still be there + expect(keyring.getAccountByAddress(existingAccount.address)).toBeDefined(); + + // The new accounts should NOT be in the keyring + expect(keyring.getAccountByAddress(newAccount3.address)).toBeUndefined(); + expect(keyring.getAccountByAddress(newAccount4.address)).toBeUndefined(); + + // Verify saveState was called (and failed) + expect(mockCallbacks.saveState).toHaveBeenCalledTimes(1); + }); }); describe('resolveAccountAddress', () => { diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 4d65eee54..31234025b 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -959,8 +959,15 @@ export class SnapKeyring { snapId, }); - const allAccounts = []; - const newAccounts = []; + // Make a backup so we can rollback if something goes wrong during the + // account creation process. + const backup = await this.serialize(); + + // Flag to track whether we need to update the keyring state at the end of the process. + // NOTE: To avoid unecessary state updates. + let updated = false; + + const accounts = []; const snapAccounts = await client.createAccounts(options); try { for (const account of snapAccounts) { @@ -972,16 +979,26 @@ export class SnapKeyring { if (!this.#isAccountAlreadyKnown(snapId, account)) { await this.#assertAccountIsUnique(snapId, account); - // Keep track of newly created accounts to add them to the keyring - // state later. - newAccounts.push(account); + // NOTE: This method does not rely on the `AccountCreated` event to add + // accounts to the keyring, so we have to add them to the state manually. + this.#accounts.set(account.id, { account, snapId }); + updated = true; } // New AND existing accounts are returned to the caller no matter what. - allAccounts.push(account); + accounts.push(account); } + + if (updated) { + await this.#callbacks.saveState(); + } + + return accounts; } catch (error) { - // Rollback Snap state if something went wrong during account creation. + // Rollback keyring state. + await this.deserialize(backup); + + // Rollback Snap state. for (const account of snapAccounts) { // Make sure to only delete accounts that were not part of the keyring state. if (!this.#isAccountAlreadyKnown(snapId, account)) { @@ -991,18 +1008,6 @@ export class SnapKeyring { throw error; } - - // NOTE: This method DOES NOT rely on the `AccountCreated` event to add - // accounts to the keyring, since those accounts are created in batch. - if (newAccounts.length > 0) { - for (const account of newAccounts) { - this.#accounts.set(account.id, { account, snapId }); - } - - await this.#callbacks.saveState(); - } - - return allAccounts; } /** From 6ad3f24a243f76b04b1b5b2e124705d5b630d9e6 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 5 Feb 2026 09:32:24 +0100 Subject: [PATCH 16/21] fix: use transformAccount --- packages/keyring-snap-bridge/src/SnapKeyring.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 31234025b..10f9132fd 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -970,7 +970,9 @@ export class SnapKeyring { const accounts = []; const snapAccounts = await client.createAccounts(options); try { - for (const account of snapAccounts) { + for (const snapAccount of snapAccounts) { + const account = transformAccount(snapAccount); + // Check for account preconditions. Order matters here: // 1. Account type validity (e.g. generic accounts). // 2. Account existence (idempotency). From 77b00c17414b5711ac96117707af1658f88311df Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 5 Feb 2026 09:35:33 +0100 Subject: [PATCH 17/21] chore: lint --- .../src/SnapKeyring.test.ts | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index b9598070a..c8e2c4c8b 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2538,6 +2538,12 @@ describe('SnapKeyring', () => { id: 'cc11bb22-cc33-4d44-ae55-ff6677889900', address: '0xccddee00112233445566778899aabbccddee0011', }; + const newAccount4 = { + ...newAccount2, + ...newEthEoaAccount, + id: 'dd11bb22-cc33-4d44-ae55-ff6677889900', + address: '0xddddee00112233445566778899aabbccddee0011', + }; const entropySource = '01JQCAKR17JARQXZ0NDP760N1K'; @@ -2922,7 +2928,9 @@ describe('SnapKeyring', () => { }); // Verify the existing account is in the keyring - expect(keyring.getAccountByAddress(existingAccount.address)).toBeDefined(); + expect( + keyring.getAccountByAddress(existingAccount.address), + ).toBeDefined(); // Clear the mocks for the second call mockCallbacks.saveState.mockClear(); @@ -2930,17 +2938,6 @@ describe('SnapKeyring', () => { mockMessenger.handleRequest.mockClear(); // Now try to create new accounts, but make saveState fail - const newAccount3 = { - ...newAccount1, - id: 'bb11bb22-cc33-4d44-be55-ff6677889911', - address: '0xbbccddee00112233445566778899aabbccdd11', - }; - - const newAccount4 = { - ...newAccount2, - id: 'cc11bb22-cc33-4d44-8e55-ff6677889922', - address: '0xccddee00112233445566778899aabbccddee22', - }; const accountsFromSnap = [existingAccount, newAccount3, newAccount4]; @@ -3003,7 +3000,9 @@ describe('SnapKeyring', () => { // Verify the keyring state was rolled back // The existing account should still be there - expect(keyring.getAccountByAddress(existingAccount.address)).toBeDefined(); + expect( + keyring.getAccountByAddress(existingAccount.address), + ).toBeDefined(); // The new accounts should NOT be in the keyring expect(keyring.getAccountByAddress(newAccount3.address)).toBeUndefined(); From e7ec21fe33c5fa62d85649a518931d91879240e1 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 5 Feb 2026 10:39:42 +0100 Subject: [PATCH 18/21] test: silent some test log noises --- packages/keyring-snap-bridge/src/SnapKeyring.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index c8e2c4c8b..cf72cb82e 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2879,6 +2879,7 @@ describe('SnapKeyring', () => { const accountToCreate = [newAccount1]; mockMessengerHandleRequest({ + [KeyringRpcMethod.DeleteAccount]: async () => null, [KeyringRpcMethod.CreateAccounts]: async () => accountToCreate, }); From 5ede4092427370492c5eef6608845ddf662c0f02 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 5 Feb 2026 10:40:09 +0100 Subject: [PATCH 19/21] fix: check for duplicated addresses within an accounts batch --- .../src/SnapKeyring.test.ts | 98 +++++++++++++++++++ .../keyring-snap-bridge/src/SnapKeyring.ts | 14 +++ 2 files changed, 112 insertions(+) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index cf72cb82e..c7e1919b7 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -3012,6 +3012,104 @@ describe('SnapKeyring', () => { // Verify saveState was called (and failed) expect(mockCallbacks.saveState).toHaveBeenCalledTimes(1); }); + + it('throws error and rolls back when batch contains duplicate addresses', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + mockCallbacks.addressExists.mockClear(); + + mockMessenger.get.mockReturnValue(snapMetadata); + + // Create accounts where two accounts have the same address + const account1 = { + ...newAccount1, + address: '0xddee001122334455667788990abbccddee334455', + }; + + const account2 = { + ...newAccount2, + // Same address as account1 (duplicate) + address: '0xddee001122334455667788990abbccddee334455', + }; + + const account3 = { + ...newAccount3, + id: 'ff11bb22-cc33-4d44-be55-ff6677889955', + address: '0xffee001122334455667788990abbccddee556677', + }; + + const accountsWithDuplicates = [account1, account2, account3]; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => accountsWithDuplicates, + [KeyringRpcMethod.DeleteAccount]: async () => null, + }); + + mockCallbacks.addressExists.mockResolvedValue(false); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + + // Should throw error when encountering duplicate address in batch + await expect(keyring.createAccounts(snapId, options)).rejects.toThrow( + `Account '${account2.id}' already exists (part of this batch)`, + ); + + // Verify that createAccounts was called on the Snap + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + request: expect.objectContaining({ + method: KeyringRpcMethod.CreateAccounts, + }), + }), + ); + + // Verify that ALL accounts in the batch were rolled back (deleted from Snap) + // since the operation failed + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + request: expect.objectContaining({ + method: KeyringRpcMethod.DeleteAccount, + params: { id: account1.id }, + }), + }), + ); + + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 3, + expect.objectContaining({ + request: expect.objectContaining({ + method: KeyringRpcMethod.DeleteAccount, + params: { id: account2.id }, + }), + }), + ); + + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 4, + expect.objectContaining({ + request: expect.objectContaining({ + method: KeyringRpcMethod.DeleteAccount, + params: { id: account3.id }, + }), + }), + ); + + // Should have 4 calls: 1 createAccounts + 3 deleteAccount + expect(mockMessenger.handleRequest).toHaveBeenCalledTimes(4); + + // Verify that no accounts were added to the keyring + expect(keyring.getAccountByAddress(account1.address)).toBeUndefined(); + expect(keyring.getAccountByAddress(account3.address)).toBeUndefined(); + + // Verify that saveState was NOT called (since operation failed before saving) + expect(mockCallbacks.saveState).not.toHaveBeenCalled(); + }); }); describe('resolveAccountAddress', () => { diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 10f9132fd..39b5311bd 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -967,11 +967,16 @@ export class SnapKeyring { // NOTE: To avoid unecessary state updates. let updated = false; + // Keep track of the addresses of each new accounts to prevent + // duplicate accounts addresses. + const addresses = new Set(); + const accounts = []; const snapAccounts = await client.createAccounts(options); try { for (const snapAccount of snapAccounts) { const account = transformAccount(snapAccount); + const address = normalizeAccountAddress(account); // Check for account preconditions. Order matters here: // 1. Account type validity (e.g. generic accounts). @@ -981,6 +986,15 @@ export class SnapKeyring { if (!this.#isAccountAlreadyKnown(snapId, account)) { await this.#assertAccountIsUnique(snapId, account); + // Also check for transient accounts that are not yet part of the keyring + // state. + if (addresses.has(address)) { + throw new Error( + `Account '${account.id}' already exists (part of this batch)`, + ); + } + addresses.add(address); + // NOTE: This method does not rely on the `AccountCreated` event to add // accounts to the keyring, so we have to add them to the state manually. this.#accounts.set(account.id, { account, snapId }); From 6b0c86860ce6c1057b059778822da4ac0db98808 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 5 Feb 2026 13:59:58 +0100 Subject: [PATCH 20/21] refactor: remove saveState rollback + check for unique account IDs within batch --- .../src/SnapKeyring.test.ts | 119 +--------------- .../keyring-snap-bridge/src/SnapKeyring.ts | 129 +++++++++--------- 2 files changed, 63 insertions(+), 185 deletions(-) diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index c7e1919b7..d6440f35b 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2538,12 +2538,6 @@ describe('SnapKeyring', () => { id: 'cc11bb22-cc33-4d44-ae55-ff6677889900', address: '0xccddee00112233445566778899aabbccddee0011', }; - const newAccount4 = { - ...newAccount2, - ...newEthEoaAccount, - id: 'dd11bb22-cc33-4d44-ae55-ff6677889900', - address: '0xddddee00112233445566778899aabbccddee0011', - }; const entropySource = '01JQCAKR17JARQXZ0NDP760N1K'; @@ -2902,117 +2896,6 @@ describe('SnapKeyring', () => { expect(mockCallbacks.saveState).not.toHaveBeenCalled(); }); - it('rolls back keyring state and only deletes new Snap accounts when saveState fails', async () => { - mockCallbacks.addAccount.mockClear(); - mockCallbacks.saveState.mockClear(); - mockCallbacks.addressExists.mockClear(); - - mockMessenger.get.mockReturnValue(snapMetadata); - - // First, create an existing account that's already in the keyring - const existingAccount = { - ...newAccount1, - id: 'aa11bb22-cc33-4d44-ae55-ff6677889900', - address: '0xaabbccddee00112233445566778899aabbccdd00', - }; - - mockMessengerHandleRequest({ - [KeyringRpcMethod.CreateAccounts]: async () => [existingAccount], - }); - - mockCallbacks.addressExists.mockResolvedValue(false); - - await keyring.createAccounts(snapId, { - type: AccountCreationType.Bip44DeriveIndex, - entropySource, - groupIndex: 0, - }); - - // Verify the existing account is in the keyring - expect( - keyring.getAccountByAddress(existingAccount.address), - ).toBeDefined(); - - // Clear the mocks for the second call - mockCallbacks.saveState.mockClear(); - mockCallbacks.addressExists.mockClear(); - mockMessenger.handleRequest.mockClear(); - - // Now try to create new accounts, but make saveState fail - - const accountsFromSnap = [existingAccount, newAccount3, newAccount4]; - - mockMessengerHandleRequest({ - [KeyringRpcMethod.CreateAccounts]: async () => accountsFromSnap, - [KeyringRpcMethod.DeleteAccount]: async () => null, - }); - - mockCallbacks.addressExists.mockResolvedValue(false); - - // Make saveState fail - const saveStateError = new Error('Failed to save state to disk'); - mockCallbacks.saveState.mockRejectedValue(saveStateError); - - const options: CreateAccountOptions = { - type: AccountCreationType.Bip44DeriveIndex, - entropySource, - groupIndex: 1, - }; - - // Should throw the saveState error - await expect(keyring.createAccounts(snapId, options)).rejects.toThrow( - saveStateError, - ); - - // Verify that createAccounts was called on the Snap - expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( - 1, - expect.objectContaining({ - request: expect.objectContaining({ - method: KeyringRpcMethod.CreateAccounts, - }), - }), - ); - - // Verify that only the NEW accounts (not the pre-existing one) were deleted from the Snap - // The existing account should NOT be deleted because it was already in the keyring state - expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( - 2, - expect.objectContaining({ - request: expect.objectContaining({ - method: KeyringRpcMethod.DeleteAccount, - params: { id: newAccount3.id }, - }), - }), - ); - - expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( - 3, - expect.objectContaining({ - request: expect.objectContaining({ - method: KeyringRpcMethod.DeleteAccount, - params: { id: newAccount4.id }, - }), - }), - ); - - // Should only have 3 delete calls (for the 2 new accounts, not the existing one) - expect(mockMessenger.handleRequest).toHaveBeenCalledTimes(3); - - // Verify the keyring state was rolled back - // The existing account should still be there - expect( - keyring.getAccountByAddress(existingAccount.address), - ).toBeDefined(); - - // The new accounts should NOT be in the keyring - expect(keyring.getAccountByAddress(newAccount3.address)).toBeUndefined(); - expect(keyring.getAccountByAddress(newAccount4.address)).toBeUndefined(); - - // Verify saveState was called (and failed) - expect(mockCallbacks.saveState).toHaveBeenCalledTimes(1); - }); - it('throws error and rolls back when batch contains duplicate addresses', async () => { mockCallbacks.addAccount.mockClear(); mockCallbacks.saveState.mockClear(); @@ -3055,7 +2938,7 @@ describe('SnapKeyring', () => { // Should throw error when encountering duplicate address in batch await expect(keyring.createAccounts(snapId, options)).rejects.toThrow( - `Account '${account2.id}' already exists (part of this batch)`, + `Account '${account2.id}' is already part of this batch (same address or account ID)`, ); // Verify that createAccounts was called on the Snap diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 39b5311bd..aa14aee85 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -263,12 +263,19 @@ export class SnapKeyring { } /** - * Asserts that an account can be used within the Snap keyring. (e.g. generic accounts). + * Asserts that an account can be used within the Snap keyring. (e.g. generic accounts, unique + * addresses, etc...). * + * @param snapId - The account's Snap ID. * @param account - The account to check. * @throws If the account cannot be used. */ - #assertAccountCanBeUsed(account: KeyringAccount): void { + async #assertAccountCanBeUsed( + snapId: SnapId, + account: KeyringAccount, + ): Promise { + const address = normalizeAccountAddress(account); + // The `AnyAccountType.Account` generic account type is allowed only during // development, so we check whether it's allowed before continuing. if ( @@ -277,6 +284,19 @@ export class SnapKeyring { ) { throw new Error(`Cannot create generic account '${account.id}'`); } + + // A Snap could try to create an account with a different address but with + // an existing ID, so the above test only is not enough. + if (this.#accounts.has(snapId, account.id)) { + throw new Error(`Account '${account.id}' already exists`); + } + + // The UI still uses the account address to identify accounts, so we need + // to block the creation of duplicate accounts for now to prevent accounts + // from being overwritten. + if (await this.#callbacks.addressExists(address)) { + throw new Error(`Account address '${address}' already exists`); + } } /** @@ -284,9 +304,12 @@ export class SnapKeyring { * * @param snapId - The account's Snap ID. * @param account - The account to check. - * @returns True if the account is known, false otherwise. + * @returns The existing account, or `undefined` if the account is not known. */ - #isAccountAlreadyKnown(snapId: SnapId, account: KeyringAccount): boolean { + #getExistingAccount( + snapId: SnapId, + account: KeyringAccount, + ): KeyringAccount | undefined { const address = normalizeAccountAddress(account); // Acount creation is idempotent, so we need to check whether the account already exists @@ -294,37 +317,14 @@ export class SnapKeyring { // NOTE: We are not checking account object equality here. If a Snap // re-send this event with different account data, we will ignore it. const accountEntry = this.#accounts.get(snapId, account.id); - return ( + if ( accountEntry !== undefined && normalizeAccountAddress(accountEntry.account) === address - ); - } - - /** - * Asserts that an account is unique and can be added to the keyring. - * - * @param snapId - The account's Snap ID. - * @param account - The account to check. - * @throws If the account is not unique. - */ - async #assertAccountIsUnique( - snapId: SnapId, - account: KeyringAccount, - ): Promise { - const address = normalizeAccountAddress(account); - - // The UI still uses the account address to identify accounts, so we need - // to block the creation of duplicate accounts for now to prevent accounts - // from being overwritten. - if (await this.#callbacks.addressExists(address)) { - throw new Error(`Account address '${address}' already exists`); + ) { + return accountEntry.account; } - // A Snap could try to create an account with a different address but with - // an existing ID, so the above test only is not enough. - if (this.#accounts.has(snapId, account.id)) { - throw new Error(`Account '${account.id}' already exists`); - } + return undefined; // Not a known account. } /** @@ -391,16 +391,13 @@ export class SnapKeyring { const account = transformAccount(newAccountFromEvent); const address = normalizeAccountAddress(account); - // Check for account preconditions. Order matters here: - // 1. Account type validity (e.g. generic accounts). - // 2. Account existence (idempotency). - // 3. Account uniqueness (e.g. address and ID uniqueness). - this.#assertAccountCanBeUsed(account); - if (this.#isAccountAlreadyKnown(snapId, account)) { + if (this.#getExistingAccount(snapId, account)) { // If the account already exists, we skip it. return null; } - await this.#assertAccountIsUnique(snapId, account); + + // Make sure this new account is valid. + await this.#assertAccountCanBeUsed(snapId, account); // A deferred promise that will be resolved once the Snap keyring has saved // its internal state. @@ -959,65 +956,63 @@ export class SnapKeyring { snapId, }); - // Make a backup so we can rollback if something goes wrong during the - // account creation process. - const backup = await this.serialize(); - - // Flag to track whether we need to update the keyring state at the end of the process. - // NOTE: To avoid unecessary state updates. - let updated = false; - - // Keep track of the addresses of each new accounts to prevent - // duplicate accounts addresses. - const addresses = new Set(); + // Keep track of address/account ID part of this batch, to avoid having duplicates. + const accountAddresses = new Set(); + const accountIds = new Set(); const accounts = []; + const newAccounts = []; const snapAccounts = await client.createAccounts(options); try { for (const snapAccount of snapAccounts) { - const account = transformAccount(snapAccount); + let account = transformAccount(snapAccount); const address = normalizeAccountAddress(account); - // Check for account preconditions. Order matters here: - // 1. Account type validity (e.g. generic accounts). - // 2. Account existence (idempotency). - // 3. Account uniqueness (e.g. address and ID uniqueness). - this.#assertAccountCanBeUsed(account); - if (!this.#isAccountAlreadyKnown(snapId, account)) { - await this.#assertAccountIsUnique(snapId, account); + // Check for idempotency. + const existingAccount = this.#getExistingAccount(snapId, account); + if (existingAccount) { + // NOTE: We re-use the account from the internal state to avoid having the Snap + // mutating the account object without updating the map. + account = existingAccount; + } else { + await this.#assertAccountCanBeUsed(snapId, account); // Also check for transient accounts that are not yet part of the keyring // state. - if (addresses.has(address)) { + if (accountAddresses.has(address) || accountIds.has(account.id)) { throw new Error( - `Account '${account.id}' already exists (part of this batch)`, + `Account '${account.id}' is already part of this batch (same address or account ID)`, ); } - addresses.add(address); + accountAddresses.add(address); + accountIds.add(account.id); // NOTE: This method does not rely on the `AccountCreated` event to add // accounts to the keyring, so we have to add them to the state manually. - this.#accounts.set(account.id, { account, snapId }); - updated = true; + newAccounts.push(account); } // New AND existing accounts are returned to the caller no matter what. accounts.push(account); } - if (updated) { + // We update the keyring state only if needed. + if (newAccounts.length > 0) { + for (const account of newAccounts) { + this.#accounts.set(account.id, { account, snapId }); + } + + // NOTE: We assume this will never fail, thus, we don't need to rollback the + // keyring state if anything goes wrong here. await this.#callbacks.saveState(); } return accounts; } catch (error) { - // Rollback keyring state. - await this.deserialize(backup); - // Rollback Snap state. for (const account of snapAccounts) { // Make sure to only delete accounts that were not part of the keyring state. - if (!this.#isAccountAlreadyKnown(snapId, account)) { + if (!this.#getExistingAccount(snapId, account)) { await this.#deleteAccount(snapId, account); } } From 7ec3108a3e3a1a4f7d1f41e32c7c6a115e64dc22 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 5 Feb 2026 15:05:35 +0100 Subject: [PATCH 21/21] fix: add #withLock for createAccounts --- packages/keyring-snap-bridge/package.json | 1 + .../src/SnapKeyring.test.ts | 128 ++++++++++++++++ .../keyring-snap-bridge/src/SnapKeyring.ts | 138 ++++++++++-------- yarn.lock | 1 + 4 files changed, 209 insertions(+), 59 deletions(-) diff --git a/packages/keyring-snap-bridge/package.json b/packages/keyring-snap-bridge/package.json index 645899f0f..6ed8da73b 100644 --- a/packages/keyring-snap-bridge/package.json +++ b/packages/keyring-snap-bridge/package.json @@ -48,6 +48,7 @@ "@metamask/superstruct": "^3.1.0", "@metamask/utils": "^11.1.0", "@types/uuid": "^9.0.8", + "async-mutex": "^0.5.0", "uuid": "^9.0.1" }, "devDependencies": { diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts index d6440f35b..60440935b 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.test.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.test.ts @@ -2993,6 +2993,134 @@ describe('SnapKeyring', () => { // Verify that saveState was NOT called (since operation failed before saving) expect(mockCallbacks.saveState).not.toHaveBeenCalled(); }); + + it('serializes concurrent createAccounts calls using a lock', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + mockCallbacks.addressExists.mockClear(); + + mockMessenger.get.mockReturnValue(snapMetadata); + mockCallbacks.addressExists.mockResolvedValue(false); + + // Track the execution order to verify serial execution + const executionOrder: string[] = []; + let activeExecutions = 0; + let maxConcurrentExecutions = 0; + + // Create different accounts for each concurrent call + const batch1Accounts: KeyringAccount[] = [ + { + ...newAccount1, + id: 'a0000000-0000-4000-a000-000000000001', + address: '0xa000000000000000000000000000000000000001', + }, + ]; + + const batch2Accounts: KeyringAccount[] = [ + { + ...newAccount1, + id: 'b0000000-0000-4000-a000-000000000002', + address: '0xb000000000000000000000000000000000000002', + }, + ]; + + const batch3Accounts: KeyringAccount[] = [ + { + ...newAccount1, + id: 'c0000000-0000-4000-a000-000000000003', + address: '0xc000000000000000000000000000000000000003', + }, + ]; + + // Mock the Snap's createAccounts to track execution and add artificial delay + const mockConcurrentCreateAccounts = ( + batchNumber: number, + batchAccounts: KeyringAccount[], + ): (() => Promise) => { + return async () => { + // Determine which batch based on the call order + const batchId = `batch${batchNumber + 1}`; + + // Track when this execution starts + executionOrder.push(`${batchId}-start`); + activeExecutions += 1; + + // Track max concurrent executions to verify that they are sequential (should never be >1) + maxConcurrentExecutions = Math.max( + maxConcurrentExecutions, + activeExecutions, + ); + + // Add a delay to simulate async work and increase chance of detecting concurrency issues + await new Promise((resolve) => setTimeout(resolve, 50)); + + // Track when this execution ends + activeExecutions -= 1; + executionOrder.push(`${batchId}-end`); + + // Return the appropriate batch based on call order + return batchAccounts; + }; + }; + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: jest + .fn() + .mockImplementationOnce( + mockConcurrentCreateAccounts(0, batch1Accounts), + ) + .mockImplementationOnce( + mockConcurrentCreateAccounts(1, batch2Accounts), + ) + .mockImplementationOnce( + mockConcurrentCreateAccounts(2, batch3Accounts), + ), + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + + // Launch 3 concurrent createAccounts calls + const [result1, result2, result3] = await Promise.all([ + keyring.createAccounts(snapId, options), + keyring.createAccounts(snapId, options), + keyring.createAccounts(snapId, options), + ]); + + // Verify that all calls completed successfully + expect(result1).toStrictEqual(batch1Accounts); + expect(result2).toStrictEqual(batch2Accounts); + expect(result3).toStrictEqual(batch3Accounts); + + // Verify that operations were serialized (never more than 1 active at a time) + expect(maxConcurrentExecutions).toBe(1); + + // Verify the execution order: each batch must complete before the next starts + expect(executionOrder).toStrictEqual([ + 'batch1-start', + 'batch1-end', + 'batch2-start', + 'batch2-end', + 'batch3-start', + 'batch3-end', + ]); + + // Verify all accounts were created + expect( + keyring.getAccountByAddress(batch1Accounts[0]?.address ?? ''), + ).toBeDefined(); + expect( + keyring.getAccountByAddress(batch2Accounts[0]?.address ?? ''), + ).toBeDefined(); + expect( + keyring.getAccountByAddress(batch3Accounts[0]?.address ?? ''), + ).toBeDefined(); + + // Verify saveState was called for each batch (3 times) + expect(mockCallbacks.saveState).toHaveBeenCalledTimes(3); + }); }); describe('resolveAccountAddress', () => { diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index aa14aee85..b634f4e39 100644 --- a/packages/keyring-snap-bridge/src/SnapKeyring.ts +++ b/packages/keyring-snap-bridge/src/SnapKeyring.ts @@ -49,6 +49,7 @@ import { KnownCaipNamespace, toCaipChainId, } from '@metamask/utils'; +import { Mutex } from 'async-mutex'; import { v4 as uuid } from 'uuid'; import { transformAccount } from './account'; @@ -215,6 +216,12 @@ export class SnapKeyring { */ readonly #isAnyAccountTypeAllowed: boolean; + /** + * Mutex to ensure exclusive access to the inner keyring during + * operations that mutate its state. + */ + readonly #lock; + /** * Create a new Snap keyring. * @@ -242,6 +249,17 @@ export class SnapKeyring { this.#callbacks = callbacks; this.#isAnyAccountTypeAllowed = isAnyAccountTypeAllowed; this.#selectedAccounts = new Map(); + this.#lock = new Mutex(); + } + + /** + * Execute an operation behind a lock. + * + * @param callback - A function that performs the operation. + * @returns The result of the callback. + */ + async #withLock(callback: () => Promise): Promise { + return this.#lock.runExclusive(callback); } /** @@ -951,74 +969,76 @@ export class SnapKeyring { snapId: SnapId, options: CreateAccountOptions, ): Promise { - const client = new KeyringInternalSnapClient({ - messenger: this.#messenger, - snapId, - }); - - // Keep track of address/account ID part of this batch, to avoid having duplicates. - const accountAddresses = new Set(); - const accountIds = new Set(); - - const accounts = []; - const newAccounts = []; - const snapAccounts = await client.createAccounts(options); - try { - for (const snapAccount of snapAccounts) { - let account = transformAccount(snapAccount); - const address = normalizeAccountAddress(account); - - // Check for idempotency. - const existingAccount = this.#getExistingAccount(snapId, account); - if (existingAccount) { - // NOTE: We re-use the account from the internal state to avoid having the Snap - // mutating the account object without updating the map. - account = existingAccount; - } else { - await this.#assertAccountCanBeUsed(snapId, account); - - // Also check for transient accounts that are not yet part of the keyring - // state. - if (accountAddresses.has(address) || accountIds.has(account.id)) { - throw new Error( - `Account '${account.id}' is already part of this batch (same address or account ID)`, - ); + return this.#withLock(async () => { + const client = new KeyringInternalSnapClient({ + messenger: this.#messenger, + snapId, + }); + + // Keep track of address/account ID part of this batch, to avoid having duplicates. + const accountAddresses = new Set(); + const accountIds = new Set(); + + const accounts = []; + const newAccounts = []; + const snapAccounts = await client.createAccounts(options); + try { + for (const snapAccount of snapAccounts) { + let account = transformAccount(snapAccount); + const address = normalizeAccountAddress(account); + + // Check for idempotency. + const existingAccount = this.#getExistingAccount(snapId, account); + if (existingAccount) { + // NOTE: We re-use the account from the internal state to avoid having the Snap + // mutating the account object without updating the map. + account = existingAccount; + } else { + await this.#assertAccountCanBeUsed(snapId, account); + + // Also check for transient accounts that are not yet part of the keyring + // state. + if (accountAddresses.has(address) || accountIds.has(account.id)) { + throw new Error( + `Account '${account.id}' is already part of this batch (same address or account ID)`, + ); + } + accountAddresses.add(address); + accountIds.add(account.id); + + // NOTE: This method does not rely on the `AccountCreated` event to add + // accounts to the keyring, so we have to add them to the state manually. + newAccounts.push(account); } - accountAddresses.add(address); - accountIds.add(account.id); - // NOTE: This method does not rely on the `AccountCreated` event to add - // accounts to the keyring, so we have to add them to the state manually. - newAccounts.push(account); + // New AND existing accounts are returned to the caller no matter what. + accounts.push(account); } - // New AND existing accounts are returned to the caller no matter what. - accounts.push(account); - } + // We update the keyring state only if needed. + if (newAccounts.length > 0) { + for (const account of newAccounts) { + this.#accounts.set(account.id, { account, snapId }); + } - // We update the keyring state only if needed. - if (newAccounts.length > 0) { - for (const account of newAccounts) { - this.#accounts.set(account.id, { account, snapId }); + // NOTE: We assume this will never fail, thus, we don't need to rollback the + // keyring state if anything goes wrong here. + await this.#callbacks.saveState(); } - // NOTE: We assume this will never fail, thus, we don't need to rollback the - // keyring state if anything goes wrong here. - await this.#callbacks.saveState(); - } - - return accounts; - } catch (error) { - // Rollback Snap state. - for (const account of snapAccounts) { - // Make sure to only delete accounts that were not part of the keyring state. - if (!this.#getExistingAccount(snapId, account)) { - await this.#deleteAccount(snapId, account); + return accounts; + } catch (error) { + // Rollback Snap state. + for (const account of snapAccounts) { + // Make sure to only delete accounts that were not part of the keyring state. + if (!this.#getExistingAccount(snapId, account)) { + await this.#deleteAccount(snapId, account); + } } - } - throw error; - } + throw error; + } + }); } /** diff --git a/yarn.lock b/yarn.lock index 81a8e5f45..ed0fa64ed 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1871,6 +1871,7 @@ __metadata: "@types/jest": "npm:^29.5.12" "@types/node": "npm:^20.12.12" "@types/uuid": "npm:^9.0.8" + async-mutex: "npm:^0.5.0" deepmerge: "npm:^4.2.2" depcheck: "npm:^1.4.7" jest: "npm:^29.5.0"