diff --git a/packages/keyring-api/CHANGELOG.md b/packages/keyring-api/CHANGELOG.md index df9aeced4..750100249 100644 --- a/packages/keyring-api/CHANGELOG.md +++ b/packages/keyring-api/CHANGELOG.md @@ -9,6 +9,10 @@ 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 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-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..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'; /** @@ -36,6 +37,7 @@ import { export enum KeyringRpcMethod { // Account management CreateAccount = 'keyring_createAccount', + CreateAccounts = 'keyring_createAccounts', DeleteAccount = 'keyring_deleteAccount', DiscoverAccounts = 'keyring_discoverAccounts', ExportAccount = 'keyring_exportAccount', @@ -126,6 +128,23 @@ export const CreateAccountResponseStruct = KeyringAccountStruct; export type CreateAccountResponse = Infer; +// ---------------------------------------------------------------------------- +// Create accounts + +export const CreateAccountsRequestStruct = object({ + ...CommonHeader, + method: literal('keyring_createAccounts'), + params: object({ + options: CreateAccountOptionsStruct, + }), +}); + +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..059472fec 100644 --- a/packages/keyring-snap-bridge/CHANGELOG.md +++ b/packages/keyring-snap-bridge/CHANGELOG.md @@ -7,6 +7,12 @@ 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. + - Generic accounts will be filtered out if they are not allowed by the keyring configuration. + ### 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/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 7db510cfb..60440935b 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,607 @@ 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).toHaveBeenCalledTimes(0); + 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); + } + }); + + it('throws error and rolls back Snap state when encountering unsupported accounts', async () => { + mockCallbacks.addAccount.mockClear(); + mockCallbacks.saveState.mockClear(); + + // Create a keyring with isAnyAccountTypeAllowed = false + const restrictedKeyring = new SnapKeyring({ + messenger: mockSnapKeyringMessenger, + callbacks: mockCallbacks, + isAnyAccountTypeAllowed: false, + }); + + const supportedAccount = { + ...newAccount1, + id: 'aa11bb22-cc33-4d44-8e55-ff6677889900', + address: '0xaabbccddee00112233445566778899aabbccddee', + type: EthAccountType.Eoa, + }; + + const genericAccount = { + ...newAccount2, + id: 'bb11bb22-cc33-4d44-9e55-ff6677889900', + address: '0xbbccddee00112233445566778899aabbccddeeff', + type: AnyAccountType.Account, + }; + + mockMessengerHandleRequest({ + [KeyringRpcMethod.CreateAccounts]: async () => [ + supportedAccount, + genericAccount, + ], + [KeyringRpcMethod.DeleteAccount]: async () => null, + }); + + const options: CreateAccountOptions = { + type: AccountCreationType.Bip44DeriveIndex, + entropySource, + groupIndex: 0, + }; + + // Should throw error when encountering unsupported account + await expect( + restrictedKeyring.createAccounts(snapId, options), + ).rejects.toThrow(`Cannot create generic account '${genericAccount.id}'`); + + // We still have sent a Snap request to create accounts + expect(mockMessenger.handleRequest).toHaveBeenNthCalledWith( + 1, + mockKeyringRpcRequest(KeyringRpcMethod.CreateAccounts, { options }), + ); + + // 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, + }), + ); + + // No accounts should be added to the keyring state + expect( + restrictedKeyring.getAccountByAddress(supportedAccount.address), + ).toBeUndefined(); + expect( + restrictedKeyring.getAccountByAddress(genericAccount.address), + ).toBeUndefined(); + + // State should not be saved when operation fails + expect(mockCallbacks.saveState).not.toHaveBeenCalled(); + }); + + 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.DeleteAccount]: async () => null, + [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(); + }); + + 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}' is already part of this batch (same address or account ID)`, + ); + + // 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(); + }); + + 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', () => { const scope = toCaipChainId( KnownCaipNamespace.Eip155, diff --git a/packages/keyring-snap-bridge/src/SnapKeyring.ts b/packages/keyring-snap-bridge/src/SnapKeyring.ts index 986c84554..b634f4e39 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, @@ -48,6 +49,7 @@ import { KnownCaipNamespace, toCaipChainId, } from '@metamask/utils'; +import { Mutex } from 'async-mutex'; import { v4 as uuid } from 'uuid'; import { transformAccount } from './account'; @@ -214,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. * @@ -241,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); } /** @@ -261,6 +280,71 @@ export class SnapKeyring { ); } + /** + * 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. + */ + 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 ( + !this.#isAnyAccountTypeAllowed && + account.type === AnyAccountType.Account + ) { + 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`); + } + } + + /** + * Checks whether an account is known. + * + * @param snapId - The account's Snap ID. + * @param account - The account to check. + * @returns The existing account, or `undefined` if the account is not known. + */ + #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 + // 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); + if ( + accountEntry !== undefined && + normalizeAccountAddress(accountEntry.account) === address + ) { + return accountEntry.account; + } + + return undefined; // Not a known account. + } + /** * Get internal options given a correlation ID. * @@ -325,39 +409,13 @@ 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.#isAnyAccountTypeAllowed && - account.type === AnyAccountType.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. + if (this.#getExistingAccount(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`); - } + // 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. @@ -891,6 +949,98 @@ export class SnapKeyring { }); } + /** + * 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. + * + * 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). + * @returns An array of the created account objects. + */ + async createAccounts( + snapId: SnapId, + options: CreateAccountOptions, + ): Promise { + 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); + } + + // 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 }); + } + + // 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); + } + } + + throw error; + } + }); + } + /** * 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.test.ts b/packages/keyring-snap-client/src/KeyringClient.test.ts index 71ed612b3..65424d911 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'; diff --git a/packages/keyring-snap-client/src/KeyringClient.ts b/packages/keyring-snap-client/src/KeyringClient.ts index f603091bc..3e948d103 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, 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)) 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); 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"