diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 053d97aa4..6ae8c43dd 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -9,10 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Reduce `keyring_createAccounts` entropy RPCs from one per account to one per distinct parent path by fetching the account-level parent node once and deriving hardened children locally ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) + - The private parent node is held transiently in memory during the batch — the same trust boundary as the previous per-account implementation — and children are neutered before descriptor construction. + - The creation concurrency throttle is removed: with derivation local, the remaining per-account work is synchronous WASM wallet construction. +- Reduce full-state round trips during batch account creation: the insert step reuses the state snapshot loaded by the existing-accounts lookup instead of re-reading both account maps, and the two state writes now run in parallel ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) +- Process the entire requested account range as a single batch instead of chunks of 100, so the existing-accounts lookup and state I/O happen once per request ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) - Split the chain `stopGap` configuration into `{ discovery: 5, scan: 20 }` so account discovery keeps the cheap probe while full account scans use the BIP44 gap limit ([#224](https://github.com/MetaMask/internal-snaps/pull/224)) ### Fixed +- Coalesce concurrent account synchronization runs so stacked triggers (the 30s cronjob, `onActive`, and background events scheduled by `setSelectedAccounts`) share one run instead of duplicating network fetches, state writes, and keyring events ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) +- Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) + - v2 clients reject v1 lifecycle events, which aborted the deletion before the account was removed from state. Deletion is client-initiated in v2, so no event is needed. - Ensure certain errors are stringified correctly ([#179](https://github.com/MetaMask/internal-snaps/pull/179)) ## [2.0.1] diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 95ba9bc57..ecef0a1d3 100644 --- a/packages/bitcoin-wallet-snap/snap.manifest.json +++ b/packages/bitcoin-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "DhEtGSewa79HmdU+vX8K/GjzP7I+y/99eNLkXjR0URw=", + "shasum": "ZEMZtj4BZq+pj3bzb81K1Vk5WQuqMA0NArfIhrIppds=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/bitcoin-wallet-snap/src/entities/account.ts b/packages/bitcoin-wallet-snap/src/entities/account.ts index 0bb396f5f..8788fb759 100644 --- a/packages/bitcoin-wallet-snap/src/entities/account.ts +++ b/packages/bitcoin-wallet-snap/src/entities/account.ts @@ -17,6 +17,7 @@ import type { } from '@metamask/bitcoindevkit'; import type { Inscription } from './meta-protocols'; +import type { AccountStateSnapshot } from './snap'; import type { TransactionBuilder } from './transaction'; /** @@ -276,11 +277,14 @@ export type BitcoinAccountRepository = { * Get accounts by derivation path. * * @param derivationPaths - derivation paths. - * @returns the accounts or null if they do not exist, in input order + * @returns the accounts or null if they do not exist (in input order), and + * the derivation-path snapshot the lookup was resolved from, reusable by + * `insertMany` within the same account mutation */ - getByDerivationPaths( - derivationPaths: string[][], - ): Promise<(BitcoinAccount | null)[]>; + getByDerivationPaths(derivationPaths: string[][]): Promise<{ + accounts: (BitcoinAccount | null)[]; + snapshot: AccountStateSnapshot; + }>; /** * Create a new account, without persisting it. @@ -296,6 +300,21 @@ export type BitcoinAccountRepository = { addressType: AddressType, ): Promise; + /** + * Create multiple accounts, without persisting them. Fetches entropy once + * per distinct parent path and derives hardened account children locally. + * + * @param requests - Account creation requests. + * @returns the new accounts, in input order + */ + createMany( + requests: { + derivationPath: string[]; + network: Network; + addressType: AddressType; + }[], + ): Promise; + /** * Insert an account. * @@ -307,8 +326,14 @@ export type BitcoinAccountRepository = { * Insert accounts. * * @param accounts - Bitcoin accounts. - */ - insertMany(accounts: BitcoinAccount[]): Promise; + * @param snapshot - Optional state snapshot (from `getByDerivationPaths`) + * used to reuse the derivation-path map. The accounts map is still refreshed + * before writing so concurrent sync updates are preserved. + */ + insertMany( + accounts: BitcoinAccount[], + snapshot?: AccountStateSnapshot, + ): Promise; /** * Update an account. diff --git a/packages/bitcoin-wallet-snap/src/entities/snap.ts b/packages/bitcoin-wallet-snap/src/entities/snap.ts index 386167a5f..7b9817885 100644 --- a/packages/bitcoin-wallet-snap/src/entities/snap.ts +++ b/packages/bitcoin-wallet-snap/src/entities/snap.ts @@ -17,6 +17,17 @@ export type SnapState = { derivationPaths: Record; }; +/** + * In-memory snapshot loaded while resolving derivation paths. The + * derivation-path map can be reused by a subsequent insert within the same + * account mutation; the accounts map is retained for lookup results but + * refreshed before full-map writes to avoid overwriting sync updates. + */ +export type AccountStateSnapshot = { + accounts: SnapState['accounts'] | null; + derivationPaths: SnapState['derivationPaths'] | null; +}; + export type AccountState = { // Split derivation path. derivationPath: string[]; @@ -92,25 +103,6 @@ export type SnapClient = { */ getPublicEntropy(derivationPath: string[]): Promise; - /** - * Emit an event notifying the extension of a newly created Bitcoin account - * - * @param account - The Bitcoin account. - * @param correlationId - The correlation ID to be used for the event. - */ - emitAccountCreatedEvent( - account: BitcoinAccount, - correlationId?: string, - accountName?: string, - ): Promise; - - /** - * Emit an event notifying the extension of a deleted Bitcoin account - * - * @param id - The Bitcoin account id. - */ - emitAccountDeletedEvent(id: string): Promise; - /** * Emit an event notifying the extension of updated balances * diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index 224e9bf12..82931b9bf 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -288,6 +288,80 @@ describe('CronHandler', () => { }); }); + describe('sync coalescing', () => { + it('coalesces concurrent synchronizeAccounts calls into one run', async () => { + (getSelectedAccounts as jest.Mock).mockResolvedValue([]); + mockAccountUseCases.list.mockResolvedValue([]); + + await Promise.all([ + handler.synchronizeAccounts(), + handler.synchronizeAccounts(), + handler.synchronizeAccounts(), + ]); + + expect(mockAccountUseCases.list).toHaveBeenCalledTimes(1); + }); + + it('runs synchronizeAccounts again once the previous run has finished', async () => { + (getSelectedAccounts as jest.Mock).mockResolvedValue([]); + mockAccountUseCases.list.mockResolvedValue([]); + + await handler.synchronizeAccounts(); + await handler.synchronizeAccounts(); + + expect(mockAccountUseCases.list).toHaveBeenCalledTimes(2); + }); + + it('rejects all coalesced synchronizeAccounts callers on a shared failure', async () => { + const mockAccount = mock({ id: 'account-1' }); + (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-1']); + mockAccountUseCases.list.mockResolvedValue([mockAccount]); + mockAccountUseCases.synchronize.mockRejectedValue( + new Error('sync failed'), + ); + + const first = handler.synchronizeAccounts(); + const second = handler.synchronizeAccounts(); + + await expect(first).rejects.toThrow('Account synchronization failures'); + await expect(second).rejects.toThrow('Account synchronization failures'); + expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(1); + }); + + it('coalesces concurrent syncSelectedAccounts calls for the same accounts regardless of order', async () => { + mockAccountUseCases.list.mockResolvedValue([]); + + await Promise.all([ + handler.syncSelectedAccounts(['account-1', 'account-2']), + handler.syncSelectedAccounts(['account-2', 'account-1']), + ]); + + expect(mockAccountUseCases.list).toHaveBeenCalledTimes(1); + }); + + it('coalesces concurrent syncSelectedAccounts calls for duplicate account IDs', async () => { + mockAccountUseCases.list.mockResolvedValue([]); + + await Promise.all([ + handler.syncSelectedAccounts(['account-1']), + handler.syncSelectedAccounts(['account-1', 'account-1']), + ]); + + expect(mockAccountUseCases.list).toHaveBeenCalledTimes(1); + }); + + it('does not coalesce syncSelectedAccounts calls for different accounts', async () => { + mockAccountUseCases.list.mockResolvedValue([]); + + await Promise.all([ + handler.syncSelectedAccounts(['account-1']), + handler.syncSelectedAccounts(['account-2']), + ]); + + expect(mockAccountUseCases.list).toHaveBeenCalledTimes(2); + }); + }); + describe('fullScanAccount', () => { const mockAccount = mock({ id: 'account-1' }); const request = { diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index bf772ec76..c87557425 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -1,4 +1,5 @@ import { getSelectedAccounts } from '@metamask/keyring-snap-sdk'; +import { InFlightCoalescer } from '@metamask/snap-networks-utils/dedupe'; import type { JsonRpcRequest, SnapsProvider } from '@metamask/snaps-sdk'; import { array, assert, object, string } from 'superstruct'; @@ -36,6 +37,8 @@ export class CronHandler { readonly #snap: SnapsProvider; + readonly #syncCoalescer = new InFlightCoalescer(); + constructor( accounts: AccountUseCases, sendFlow: SendFlowUseCases, @@ -78,83 +81,102 @@ export class CronHandler { } async synchronizeAccounts(): Promise { - const selectedAccounts: Set = new Set( - await getSelectedAccounts(this.#snap), - ); + // Sync triggers stack up (the 30s cronjob, `onActive`, background + // events), so concurrent invocations share one in-flight run instead of + // duplicating network fetches, state writes, and keyring events. Note + // that coalesced callers share the run's outcome, including a + // `SynchronizationError` from partial failures. + await this.#syncCoalescer.run('synchronizeAccounts', async () => { + const selectedAccounts: Set = new Set( + await getSelectedAccounts(this.#snap), + ); - const accounts = (await this.#accountsUseCases.list()).filter((account) => { - return selectedAccounts.has(account.id); - }); + const accounts = (await this.#accountsUseCases.list()).filter( + (account) => { + return selectedAccounts.has(account.id); + }, + ); - const results = await Promise.allSettled( - accounts.map(async (account) => - this.#accountsUseCases.synchronize(account, 'cron'), - ), - ); + const results = await Promise.allSettled( + accounts.map(async (account) => + this.#accountsUseCases.synchronize(account, 'cron'), + ), + ); - const successfulResults: SyncResult[] = []; + const successfulResults: SyncResult[] = []; - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const errors: Record = {}; + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const errors: Record = {}; - results.forEach((result, index) => { - if (result.status === 'fulfilled') { - successfulResults.push(result.value); - } else { - const id = accounts[index]?.id; - if (id) { - errors[id] = result.reason; + results.forEach((result, index) => { + if (result.status === 'fulfilled') { + successfulResults.push(result.value); + } else { + const id = accounts[index]?.id; + if (id) { + errors[id] = result.reason; + } } - } - }); + }); - await this.#emitSyncEvents(successfulResults); + await this.#emitSyncEvents(successfulResults); - if (Object.keys(errors).length > 0) { - throw new SynchronizationError( - 'Account synchronization failures', - errors, - ); - } + if (Object.keys(errors).length > 0) { + throw new SynchronizationError( + 'Account synchronization failures', + errors, + ); + } + }); } async syncSelectedAccounts(accountIds: string[]): Promise { - const accountIdSet = new Set(accountIds); - const allAccounts = await this.#accountsUseCases.list(); - - const selectedAccounts = allAccounts.filter((account) => - accountIdSet.has(account.id), - ); - - const results = await Promise.allSettled( - selectedAccounts.map(async (account) => - this.#accountsUseCases.synchronize(account, 'metamask'), - ), - ); + // Every `setSelectedAccounts` call schedules a background event with no + // dedupe, so bursts of identical syncs fire together during onboarding + // and imports. Concurrent invocations for the same account set share one + // in-flight run. + const uniqueAccountIds = [...new Set(accountIds)].sort(); + const key = `syncSelectedAccounts:${JSON.stringify(uniqueAccountIds)}`; + + await this.#syncCoalescer.run(key, async () => { + const accountIdSet = new Set(uniqueAccountIds); + const allAccounts = await this.#accountsUseCases.list(); + + const selectedAccounts = allAccounts.filter((account) => + accountIdSet.has(account.id), + ); - const successfulResults = results - .filter( - (result): result is PromiseFulfilledResult => - result.status === 'fulfilled', - ) - .map((result) => result.value); + const results = await Promise.allSettled( + selectedAccounts.map(async (account) => + this.#accountsUseCases.synchronize(account, 'metamask'), + ), + ); - const rejectedResults = results.filter( - (result): result is PromiseRejectedResult => result.status === 'rejected', - ); + const successfulResults = results + .filter( + (result): result is PromiseFulfilledResult => + result.status === 'fulfilled', + ) + .map((result) => result.value); - if (rejectedResults.length > 0) { - await this.#snapClient.emitTrackingError( - new SynchronizationError( - `Failed to synchronize ${rejectedResults.length} selected accounts`, - undefined, - rejectedResults[0]?.reason, - ), + const rejectedResults = results.filter( + (result): result is PromiseRejectedResult => + result.status === 'rejected', ); - } - await this.#emitSyncEvents(successfulResults); + if (rejectedResults.length > 0) { + await this.#snapClient.emitTrackingError( + new SynchronizationError( + `Failed to synchronize ${rejectedResults.length} selected accounts`, + undefined, + rejectedResults[0]?.reason, + ), + ); + } + + await this.#emitSyncEvents(successfulResults); + }); } /** diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts index 3dc0a3428..aea1f648c 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts @@ -298,7 +298,7 @@ describe('KeyringHandler', () => { expect(mockAccounts.createMany).not.toHaveBeenCalled(); }); - it('splits requests larger than 100 accounts into internal batches', async () => { + it('creates ranges larger than 100 accounts in a single batch', async () => { mockAccounts.createMany.mockImplementation(async (requests) => requests.map(({ index }) => buildMockAccount(index)), ); @@ -309,16 +309,12 @@ describe('KeyringHandler', () => { entropySource, }); - expect(mockAccounts.createMany).toHaveBeenCalledTimes(2); - expect(mockAccounts.createMany).toHaveBeenNthCalledWith( - 1, - Array.from({ length: 100 }, (_, index) => + expect(mockAccounts.createMany).toHaveBeenCalledTimes(1); + expect(mockAccounts.createMany).toHaveBeenCalledWith( + Array.from({ length: 101 }, (_, index) => expect.objectContaining({ index }), ), ); - expect(mockAccounts.createMany).toHaveBeenNthCalledWith(2, [ - expect.objectContaining({ index: 100 }), - ]); expect(result).toHaveLength(101); expect( result.map((account) => mnemonicGroupIndex(account)), diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts index 600cd7e4a..aad24bba1 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts @@ -46,9 +46,6 @@ import { mapToKeyringAccount, mapToTransaction } from './mappings'; import { parseDerivationPath } from './parsers'; import { BtcWalletRequestStruct, validateSelectedAccounts } from './validation'; -/** Maximum number of accounts to create in one internal createMany call. */ -const MAX_CREATE_ACCOUNTS_PER_BATCH = 100; - /** * Scopes declared in the snap manifest's keyring capabilities block. * Used to determine which networks are supported for account discovery. @@ -181,34 +178,27 @@ export class KeyringHandler implements KeyringSnapRpc { // `AccountUseCases.createMany` is idempotent: if an account already // exists for the resolved derivation path, it will be returned as-is. + // The whole range goes in one batch so the existing-accounts lookup and + // state I/O happen once per request; entropy is fetched once per parent + // path regardless of range size, and per-account work is local. const created: KeyringAccount[] = []; for (const scope of SUPPORTED_SCOPES) { const network = scopeToNetwork[scope]; - let chunkFrom = range.from; - - while (chunkFrom <= range.to) { - const chunkTo = Math.min( - chunkFrom + MAX_CREATE_ACCOUNTS_PER_BATCH - 1, - range.to, - ); - const chunkRequests: CreateAccountParams[] = []; - - for (let index = chunkFrom; index <= chunkTo; index += 1) { - chunkRequests.push({ - network, - entropySource, - index, - addressType, - synchronize: false, - }); - } - - const chunk = await this.#accountsUseCases.createMany(chunkRequests); - created.push(...chunk.map(mapToKeyringAccount)); + const requests: CreateAccountParams[] = []; - chunkFrom = chunkTo + 1; + for (let index = range.from; index <= range.to; index += 1) { + requests.push({ + network, + entropySource, + index, + addressType, + synchronize: false, + }); } + + const accounts = await this.#accountsUseCases.createMany(requests); + created.push(...accounts.map(mapToKeyringAccount)); } return created; diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts index 2e195dd87..d6efbe8a7 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts @@ -26,7 +26,7 @@ import { networkToCaip19, networkToScope, } from '../handlers'; -import { mapToKeyringAccount, mapToTransaction } from '../handlers/mappings'; +import { mapToTransaction } from '../handlers/mappings'; export class SnapClientAdapter implements SnapClient { readonly #encrypt: boolean; @@ -86,26 +86,6 @@ export class SnapClientAdapter implements SnapClient { return (await SLIP10Node.fromJSON(slip10)).neuter(); } - async emitAccountCreatedEvent( - account: BitcoinAccount, - correlationId?: string, - accountName?: string, - ): Promise { - return emitSnapKeyringEvent(snap, KeyringEvent.AccountCreated, { - account: mapToKeyringAccount(account), - accountNameSuggestion: accountName, - displayConfirmation: false, - displayAccountNameSuggestion: false, - ...(correlationId ? { metamask: { correlationId } } : {}), - }); - } - - async emitAccountDeletedEvent(id: string): Promise { - return emitSnapKeyringEvent(snap, KeyringEvent.AccountDeleted, { - id, - }); - } - async emitAccountBalancesUpdatedEvent( accounts: BitcoinAccount[], ): Promise { diff --git a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts index e909e902a..aeaa8b7a6 100644 --- a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts +++ b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts @@ -5,10 +5,15 @@ import type { DescriptorPair } from '@metamask/bitcoindevkit'; import { Address, ChangeSet, + slip10_to_extended, xpriv_to_descriptor, xpub_to_descriptor, } from '@metamask/bitcoindevkit'; -import type { SLIP10Node } from '@metamask/key-tree'; +import type { BIP32Node, BIP39Node, SLIP10Node } from '@metamask/key-tree'; +import { + mnemonicPhraseToBytes, + SLIP10Node as RealSlip10Node, +} from '@metamask/key-tree'; import { mock } from 'jest-mock-extended'; import type { @@ -240,7 +245,17 @@ describe('BdkAccountRepository', () => { expect(mockSnapClient.getState).toHaveBeenCalledWith('derivationPaths'); expect(mockSnapClient.getState).toHaveBeenCalledWith('accounts'); expect(mockSnapClient.getState).toHaveBeenCalledTimes(2); - expect(result).toStrictEqual([mockAccount2, mockAccount1]); + expect(result.accounts).toStrictEqual([mockAccount2, mockAccount1]); + expect(result.snapshot).toStrictEqual({ + accounts: { + 'some-id-1': accountState1, + 'some-id-2': accountState2, + }, + derivationPaths: { + "m/84'/0'/1'": 'some-id-1', + "m/84'/0'/2'": 'some-id-2', + }, + }); expect(mockSnapClient.setState).not.toHaveBeenCalled(); }); @@ -265,7 +280,7 @@ describe('BdkAccountRepository', () => { (ChangeSet.from_json as jest.Mock).mockClear(); const result = await repo.getByDerivationPaths([derivationPath1]); - const account = result[0]; + const account = result.accounts[0]; expect(account?.id).toBe('some-id-1'); expect(account?.publicAddress.toString()).toBe('bc1qaddress...'); @@ -296,11 +311,16 @@ describe('BdkAccountRepository', () => { derivationPath2, ]); - expect(result).toStrictEqual([mockAccount1, mockAccount2]); + expect(result.accounts).toStrictEqual([mockAccount1, mockAccount2]); expect(mockSnapClient.setState).toHaveBeenCalledWith('derivationPaths', { "m/84'/0'/1'": 'some-id-1', "m/84'/0'/2'": 'some-id-2', }); + // The snapshot reflects the repaired index so later merges keep it. + expect(result.snapshot.derivationPaths).toStrictEqual({ + "m/84'/0'/1'": 'some-id-1', + "m/84'/0'/2'": 'some-id-2', + }); }); it('repairs a missing derivation path index for a single lookup', async () => { @@ -311,7 +331,7 @@ describe('BdkAccountRepository', () => { const result = await repo.getByDerivationPaths([derivationPath1]); - expect(result).toStrictEqual([mockAccount1]); + expect(result.accounts).toStrictEqual([mockAccount1]); expect(mockSnapClient.setState).toHaveBeenCalledWith('derivationPaths', { "m/84'/0'/1'": 'some-id-1', }); @@ -360,6 +380,81 @@ describe('BdkAccountRepository', () => { }); }); + describe('createMany', () => { + const mnemonic = + 'abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about'; + const parentPath = ['entropy-1', "84'", "0'"]; + const requests = [ + { + derivationPath: ['entropy-1', "84'", "0'", "0'"], + network: 'bitcoin', + addressType: 'p2wpkh', + }, + { + derivationPath: ['entropy-1', "84'", "0'", "1'"], + network: 'bitcoin', + addressType: 'p2wpkh', + }, + ] as Parameters[0]; + + /** + * Derives the real SLIP-10 node for a path from the fixture mnemonic. + * + * @param segments - Hardened path segments below the master node. + * @returns The derived node. + */ + async function deriveFixtureNode( + segments: string[], + ): Promise { + const derivationPath: [BIP39Node, ...BIP32Node[]] = [ + mnemonicPhraseToBytes(mnemonic) as BIP39Node, + ...segments.map((segment) => `bip32:${segment}` as BIP32Node), + ]; + + return RealSlip10Node.fromDerivationPath({ + derivationPath, + curve: 'secp256k1', + }); + } + + beforeEach(async () => { + const parentNode = await deriveFixtureNode(["84'", "0'"]); + mockSnapClient.getPrivateEntropy.mockResolvedValue(parentNode.toJSON()); + }); + + it('fetches entropy once per distinct parent path', async () => { + const result = await repo.createMany(requests); + + expect(mockSnapClient.getPrivateEntropy).toHaveBeenCalledTimes(1); + expect(mockSnapClient.getPrivateEntropy).toHaveBeenCalledWith(parentPath); + expect(mockSnapClient.getPublicEntropy).not.toHaveBeenCalled(); + expect(BdkAccountAdapter.create).toHaveBeenCalledTimes(2); + expect(result).toHaveLength(2); + }); + + it('derives neutered children byte-identical to full-path derivation', async () => { + await repo.createMany([requests[1]] as typeof requests); + + // Independent route: full-path derivation from the same mnemonic, the + // way `snap_getBip32Entropy` would resolve it. + const expected = (await deriveFixtureNode(["84'", "0'", "1'"])).neuter(); + + const passedNode = (slip10_to_extended as jest.Mock).mock + .calls[0]?.[0] as RealSlip10Node; + expect(passedNode.privateKey).toBeUndefined(); + expect(passedNode.publicKey).toStrictEqual(expected.publicKey); + expect(passedNode.chainCode).toStrictEqual(expected.chainCode); + expect(passedNode.masterFingerprint).toBe(expected.masterFingerprint); + }); + + it('returns an empty array without entropy fetches for empty input', async () => { + const result = await repo.createMany([]); + + expect(result).toStrictEqual([]); + expect(mockSnapClient.getPrivateEntropy).not.toHaveBeenCalled(); + }); + }); + describe('insert', () => { it('throws an error if no wallet data', async () => { await expect( @@ -530,6 +625,71 @@ describe('BdkAccountRepository', () => { }, ); }); + + it('reuses the provided derivation-path snapshot while refreshing accounts', async () => { + const staleExistingAccountState: AccountState = { + wallet: mockWalletData, + inscriptions: [], + derivationPath: mockDerivationPath, + }; + const freshExistingAccountState: AccountState = { + ...staleExistingAccountState, + wallet: 'fresh-wallet-data', + }; + const makeInsertableAccount = ( + id: string, + derivationPath: string[], + ): BitcoinAccount => { + const account = mock(); + account.id = id; + account.derivationPath = derivationPath; + account.network = 'bitcoin'; + account.addressType = 'p2wpkh'; + account.publicAddress = mockAddress; + account.publicDescriptor = 'mock-public-descriptor'; + (account.takeStaged as jest.Mock) = jest + .fn() + .mockReturnValue(mockChangeSet); + (account.hasStaged as jest.Mock) = jest.fn().mockReturnValue(true); + return account; + }; + const account1 = makeInsertableAccount('some-id-1', [ + 'm', + "84'", + "0'", + "1'", + ]); + const account2 = makeInsertableAccount('some-id-2', [ + 'm', + "84'", + "0'", + "2'", + ]); + mockSnapClient.getState.mockResolvedValueOnce({ + 'existing-id': freshExistingAccountState, + }); + + await repo.insertMany([account1, account2], { + accounts: { 'existing-id': staleExistingAccountState }, + derivationPaths: { "m/84'/0'/0'": 'existing-id' }, + }); + + expect(mockSnapClient.getState).toHaveBeenCalledTimes(1); + expect(mockSnapClient.getState).toHaveBeenCalledWith('accounts'); + expect(mockSnapClient.setState).toHaveBeenCalledWith( + 'accounts', + expect.objectContaining({ + 'existing-id': freshExistingAccountState, + 'some-id-1': expect.anything(), + 'some-id-2': expect.anything(), + }), + ); + expect(mockSnapClient.setState).toHaveBeenCalledWith('derivationPaths', { + "m/84'/0'/0'": 'existing-id', + "m/84'/0'/1'": 'some-id-1', + "m/84'/0'/2'": 'some-id-2', + }); + }); }); describe('update', () => { diff --git a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts index 44d1232db..37998eabb 100644 --- a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts +++ b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts @@ -8,10 +8,12 @@ import { xpriv_to_descriptor, xpub_to_descriptor, } from '@metamask/bitcoindevkit'; +import { SLIP10Node } from '@metamask/key-tree'; import { v4 } from 'uuid'; import { StorageError } from '../entities'; import type { + AccountStateSnapshot, BitcoinAccountRepository, BitcoinAccount, SnapClient, @@ -117,11 +119,15 @@ export class BdkAccountRepository implements BitcoinAccountRepository { return this.get(id as string); } - async getByDerivationPaths( - derivationPaths: string[][], - ): Promise<(BitcoinAccount | null)[]> { + async getByDerivationPaths(derivationPaths: string[][]): Promise<{ + accounts: (BitcoinAccount | null)[]; + snapshot: AccountStateSnapshot; + }> { if (derivationPaths.length === 0) { - return []; + return { + accounts: [], + snapshot: { accounts: null, derivationPaths: null }, + }; } const [derivationPathIndex, accounts] = await Promise.all([ @@ -166,14 +172,20 @@ export class BdkAccountRepository implements BitcoinAccountRepository { return this.#loadPersistedAccount(id, account); }); - if (Object.keys(repairs).length > 0) { - await this.#snapClient.setState('derivationPaths', { - ...existingDerivationPathIndex, - ...repairs, - }); + const hasRepairs = Object.keys(repairs).length > 0; + const repairedIndex = hasRepairs + ? { ...existingDerivationPathIndex, ...repairs } + : derivationPathIndex; + + if (hasRepairs) { + await this.#snapClient.setState('derivationPaths', repairedIndex); } - return results; + return { + accounts: results, + // Include repairs so a later merge from this snapshot preserves them. + snapshot: { accounts, derivationPaths: repairedIndex }, + }; } async getWithSigner(id: string): Promise { @@ -218,6 +230,78 @@ export class BdkAccountRepository implements BitcoinAccountRepository { addressType: AddressType, ): Promise { const slip10 = await this.#snapClient.getPublicEntropy(derivationPath); + + return BdkAccountRepository.#buildAccount( + slip10, + derivationPath, + network, + addressType, + ); + } + + async createMany( + requests: { + derivationPath: string[]; + network: Network; + addressType: AddressType; + }[], + ): Promise { + if (requests.length === 0) { + return []; + } + + // One entropy RPC per distinct parent path (entropy source + purpose + + // coin type); hardened account children are derived locally. The private + // parent node only lives in this scope — the same trust boundary as + // `getPublicEntropy`, which also fetches private entropy before + // neutering — and is never persisted or logged. + const parentNodes = new Map(); + for (const { derivationPath } of requests) { + const parentPath = derivationPath.slice(0, -1); + const parentKey = getDerivationPathKey(parentPath); + if (!parentNodes.has(parentKey)) { + const parentJson = await this.#snapClient.getPrivateEntropy(parentPath); + parentNodes.set(parentKey, await SLIP10Node.fromJSON(parentJson)); + } + } + + const accounts: BitcoinAccount[] = []; + for (const { derivationPath, network, addressType } of requests) { + const parentKey = getDerivationPathKey(derivationPath.slice(0, -1)); + const parentNode = parentNodes.get(parentKey) as SLIP10Node; + const childSegment = derivationPath[derivationPath.length - 1] as string; + const childNode = ( + await parentNode.derive([`bip32:${childSegment}`]) + ).neuter(); + + accounts.push( + BdkAccountRepository.#buildAccount( + childNode, + derivationPath, + network, + addressType, + ), + ); + } + + return accounts; + } + + /** + * Builds an in-memory BDK account from a neutered SLIP-10 node. + * + * @param slip10 - Neutered node at the account-level derivation path. + * @param derivationPath - The account's derivation path. + * @param network - The account's network. + * @param addressType - The account's address type. + * @returns The new, not yet persisted, account. + */ + static #buildAccount( + slip10: SLIP10Node, + derivationPath: string[], + network: Network, + addressType: AddressType, + ): BitcoinAccount { const id = v4(); const fingerprint = toBdkFingerprint( slip10.masterFingerprint ?? slip10.parentFingerprint, @@ -257,7 +341,10 @@ export class BdkAccountRepository implements BitcoinAccountRepository { return account; } - async insertMany(accounts: BitcoinAccount[]): Promise { + async insertMany( + accounts: BitcoinAccount[], + snapshot?: AccountStateSnapshot, + ): Promise { if (accounts.length === 0) { return []; } @@ -290,24 +377,32 @@ export class BdkAccountRepository implements BitcoinAccountRepository { derivationPathEntries.push([getDerivationPathKey(derivationPath), id]); } + // Re-read accounts before the full-map write so account updates from sync + // are not overwritten by a stale lookup snapshot. Derivation paths are only + // mutated by account lifecycle operations, so the mutation-local snapshot + // can still safely avoid one redundant state read. const [existingAccounts, existingDerivationPaths] = await Promise.all([ this.#snapClient.getState('accounts') as Promise< SnapState['accounts'] | null >, - this.#snapClient.getState('derivationPaths') as Promise< - SnapState['derivationPaths'] | null - >, + snapshot + ? Promise.resolve(snapshot.derivationPaths) + : (this.#snapClient.getState('derivationPaths') as Promise< + SnapState['derivationPaths'] | null + >), ]); - await this.#snapClient.setState('accounts', { - ...(existingAccounts ?? {}), - ...Object.fromEntries(accountStateEntries), - }); - - await this.#snapClient.setState('derivationPaths', { - ...(existingDerivationPaths ?? {}), - ...Object.fromEntries(derivationPathEntries), - }); + // The two maps are independent, so write them in parallel. + await Promise.all([ + this.#snapClient.setState('accounts', { + ...(existingAccounts ?? {}), + ...Object.fromEntries(accountStateEntries), + }), + this.#snapClient.setState('derivationPaths', { + ...(existingDerivationPaths ?? {}), + ...Object.fromEntries(derivationPathEntries), + }), + ]); return accounts; } diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts index d4e4972c5..2a94fa259 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -116,181 +116,6 @@ describe('AccountUseCases', () => { }); }); - describe('create', () => { - const createParams: CreateAccountParams = { - network: 'bitcoin', - entropySource: 'some-source', - index: 1, - addressType: 'p2wpkh', - synchronize: false, - correlationId: 'correlation-id', - accountName: 'My account', - }; - const mockAccount = mock({ network: createParams.network }); - - beforeEach(() => { - mockRepository.create.mockResolvedValue(mockAccount); - }); - - it.each([ - { tAddressType: 'p2pkh', purpose: "44'" }, - { tAddressType: 'p2sh', purpose: "49'" }, - { tAddressType: 'p2wsh', purpose: "45'" }, - { tAddressType: 'p2wpkh', purpose: "84'" }, - { tAddressType: 'p2tr', purpose: "86'" }, - ] as { tAddressType: AddressType; purpose: string }[])( - 'creates an account of type: %s', - async ({ tAddressType, purpose }) => { - const derivationPath = [ - createParams.entropySource, - purpose, - "0'", - `${createParams.index}'`, - ]; - - await useCases.create({ - ...createParams, - addressType: tAddressType, - synchronize: true, - }); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalledWith( - derivationPath, - ); - expect(mockRepository.create).toHaveBeenCalledWith( - derivationPath, - createParams.network, - tAddressType, - ); - expect(mockAccount.revealNextAddress).toHaveBeenCalled(); - expect(mockRepository.insert).toHaveBeenCalledWith(mockAccount); - expect(mockSnapClient.emitAccountCreatedEvent).toHaveBeenCalledWith( - mockAccount, - createParams.correlationId, - createParams.accountName, - ); - expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ - duration: 'PT1S', - method: CronMethod.FullScanAccount, - params: { accountId: mockAccount.id }, - }); - }, - ); - - it.each([ - { tNetwork: 'bitcoin', coinType: "0'" }, - { tNetwork: 'testnet', coinType: "1'" }, - { tNetwork: 'testnet4', coinType: "1'" }, - { tNetwork: 'signet', coinType: "1'" }, - { tNetwork: 'regtest', coinType: "1'" }, - ] as { tNetwork: Network; coinType: string }[])( - 'should create an account on network: %s', - async ({ tNetwork, coinType }) => { - const expectedDerivationPath = [ - createParams.entropySource, - "84'", - coinType, - `${createParams.index}'`, - ]; - - await useCases.create({ - ...createParams, - network: tNetwork, - synchronize: true, - }); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalledWith( - expectedDerivationPath, - ); - expect(mockRepository.create).toHaveBeenCalledWith( - expectedDerivationPath, - tNetwork, - createParams.addressType, - ); - expect(mockRepository.insert).toHaveBeenCalledWith(mockAccount); - expect(mockSnapClient.emitAccountCreatedEvent).toHaveBeenCalledWith( - mockAccount, - createParams.correlationId, - createParams.accountName, - ); - expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ - duration: 'PT1S', - method: CronMethod.FullScanAccount, - params: { accountId: mockAccount.id }, - }); - }, - ); - - it('returns an existing account if one already exists on same network', async () => { - mockRepository.getByDerivationPath.mockResolvedValue(mockAccount); - - const result = await useCases.create(createParams); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalled(); - expect(mockRepository.create).not.toHaveBeenCalled(); - - expect(result).toBe(mockAccount); - }); - - it('propagates an error if getByDerivationPath throws', async () => { - const error = new Error('getByDerivationPath failed'); - mockRepository.getByDerivationPath.mockRejectedValue(error); - - await expect(useCases.create(createParams)).rejects.toBe(error); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalled(); - expect(mockRepository.create).not.toHaveBeenCalled(); - }); - - it('propagates an error if create throws', async () => { - const error = new Error('create failed'); - mockRepository.create.mockRejectedValue(error); - - await expect(useCases.create(createParams)).rejects.toBe(error); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalled(); - expect(mockRepository.create).toHaveBeenCalled(); - }); - - it('propagates an error if insert throws', async () => { - const error = new Error('insert failed'); - mockRepository.insert.mockRejectedValue(error); - - await expect(useCases.create(createParams)).rejects.toBe(error); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalled(); - expect(mockRepository.create).toHaveBeenCalled(); - expect(mockRepository.insert).toHaveBeenCalled(); - }); - - it('propagates an error if emitAccountCreatedEvent throws', async () => { - const error = new Error('emitAccountCreatedEvent failed'); - mockSnapClient.emitAccountCreatedEvent.mockRejectedValue(error); - - await expect(useCases.create(createParams)).rejects.toBe(error); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalled(); - expect(mockRepository.create).toHaveBeenCalled(); - expect(mockRepository.insert).toHaveBeenCalled(); - expect(mockSnapClient.emitAccountCreatedEvent).toHaveBeenCalled(); - }); - - it('propagates an error if scheduleBackgroundEvent throws', async () => { - const error = new Error('scheduleBackgroundEvent failed'); - mockSnapClient.scheduleBackgroundEvent.mockRejectedValue(error); - - await expect( - useCases.create({ ...createParams, synchronize: true }), - ).rejects.toBe(error); - - expect(mockRepository.getByDerivationPath).toHaveBeenCalled(); - expect(mockRepository.create).toHaveBeenCalled(); - expect(mockRepository.insert).toHaveBeenCalled(); - expect(mockSnapClient.emitAccountCreatedEvent).toHaveBeenCalled(); - expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalled(); - }); - }); - describe('createMany', () => { const createParams: CreateAccountParams = { network: 'bitcoin', @@ -311,13 +136,17 @@ describe('AccountUseCases', () => { id: 'new-id', network: createParams.network, }); + const mockSnapshot = { + accounts: null, + derivationPaths: null, + }; it('reuses existing accounts and bulk-inserts newly-created accounts', async () => { - mockRepository.getByDerivationPaths.mockResolvedValue([ - existingAccount, - null, - ]); - mockRepository.create.mockResolvedValue(newAccount); + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [existingAccount, null], + snapshot: mockSnapshot, + }); + mockRepository.createMany.mockResolvedValue([newAccount]); const result = await useCases.createMany([ createParams, @@ -328,14 +157,18 @@ describe('AccountUseCases', () => { firstDerivationPath, secondDerivationPath, ]); - expect(mockRepository.create).toHaveBeenCalledWith( - secondDerivationPath, - createParams.network, - createParams.addressType, - ); + expect(mockRepository.createMany).toHaveBeenCalledWith([ + { + derivationPath: secondDerivationPath, + network: createParams.network, + addressType: createParams.addressType, + }, + ]); expect(newAccount.revealNextAddress).toHaveBeenCalled(); - expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); + expect(mockRepository.insertMany).toHaveBeenCalledWith( + [newAccount], + mockSnapshot, + ); expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ duration: 'PT1S', method: CronMethod.FullScanAccount, @@ -345,89 +178,89 @@ describe('AccountUseCases', () => { }); it('creates only one account for duplicate derivation paths in the same batch', async () => { - mockRepository.getByDerivationPaths.mockResolvedValue([null]); - mockRepository.create.mockResolvedValue(newAccount); + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [null], + snapshot: mockSnapshot, + }); + mockRepository.createMany.mockResolvedValue([newAccount]); const result = await useCases.createMany([createParams, createParams]); expect(mockRepository.getByDerivationPaths).toHaveBeenCalledWith([ firstDerivationPath, ]); - expect(mockRepository.create).toHaveBeenCalledTimes(1); - expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); + expect(mockRepository.createMany).toHaveBeenCalledTimes(1); + expect(mockRepository.createMany).toHaveBeenCalledWith([ + { + derivationPath: firstDerivationPath, + network: createParams.network, + addressType: createParams.addressType, + }, + ]); + expect(mockRepository.insertMany).toHaveBeenCalledWith( + [newAccount], + mockSnapshot, + ); expect(result).toStrictEqual([newAccount, newAccount]); }); it('does not create or insert accounts when all accounts already exist', async () => { - mockRepository.getByDerivationPaths.mockResolvedValue([existingAccount]); + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [existingAccount], + snapshot: mockSnapshot, + }); const result = await useCases.createMany([createParams]); - expect(mockRepository.create).not.toHaveBeenCalled(); + expect(mockRepository.createMany).not.toHaveBeenCalled(); expect(mockRepository.insertMany).not.toHaveBeenCalled(); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); expect(result).toStrictEqual([existingAccount]); }); it('propagates insertMany errors without emitting account-created events', async () => { const error = new Error('insertMany failed'); - mockRepository.getByDerivationPaths.mockResolvedValue([null]); - mockRepository.create.mockResolvedValue(newAccount); + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [null], + snapshot: mockSnapshot, + }); + mockRepository.createMany.mockResolvedValue([newAccount]); mockRepository.insertMany.mockRejectedValue(error); await expect(useCases.createMany([createParams])).rejects.toBe(error); - expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); + expect(mockRepository.insertMany).toHaveBeenCalledWith( + [newAccount], + mockSnapshot, + ); }); - it('waits for in-flight creates before rejecting when one create fails', async () => { - const error = new Error('create failed'); - const slowAccount = mock({ - id: 'slow-id', - network: createParams.network, - }); - let resolveSlowCreate: (account: BitcoinAccount) => void = () => - undefined; - const slowCreate = new Promise((resolve) => { - resolveSlowCreate = resolve; + it('logs phase timings for a batch creation', async () => { + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [null], + snapshot: mockSnapshot, }); - const callOrder: string[] = []; - - mockRepository.getByDerivationPaths.mockResolvedValue([null, null]); - mockRepository.create - .mockImplementationOnce(async () => { - callOrder.push('create-1'); - throw error; - }) - .mockImplementationOnce(async () => { - callOrder.push('create-2'); - const account = await slowCreate; - callOrder.push('resolve-2'); - return account; - }); + mockRepository.createMany.mockResolvedValue([newAccount]); - const createManyPromise = useCases.createMany([ - createParams, - { ...createParams, index: 2 }, - ]); - const onSettled = jest.fn(); - const settlementObserver = createManyPromise.then(onSettled, onSettled); + await useCases.createMany([createParams]); - await new Promise((resolve) => { - setTimeout(resolve, 0); - }); + expect(mockLogger.info).toHaveBeenCalledWith( + expect.stringMatching( + /^\[createMany\] Phase timings \{.*"requested":1.*"created":1.*"lookupMs":\d+.*"deriveMs":\d+.*"persistMs":\d+.*"totalMs":\d+.*\}$/u, + ), + ); + }); - expect(onSettled).not.toHaveBeenCalled(); + it('propagates createMany errors without inserting accounts', async () => { + const error = new Error('createMany failed'); + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [null], + snapshot: mockSnapshot, + }); + mockRepository.createMany.mockRejectedValue(error); - resolveSlowCreate(slowAccount); + await expect(useCases.createMany([createParams])).rejects.toBe(error); - await expect(createManyPromise).rejects.toBe(error); - await settlementObserver; - expect(callOrder).toStrictEqual(['create-1', 'create-2', 'resolve-2']); expect(mockRepository.insertMany).not.toHaveBeenCalled(); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); }); }); @@ -938,11 +771,10 @@ describe('AccountUseCases', () => { ); expect(mockRepository.get).toHaveBeenCalledWith('non-existent-id'); - expect(mockSnapClient.emitAccountDeletedEvent).not.toHaveBeenCalled(); expect(mockRepository.delete).not.toHaveBeenCalled(); }); - it('removes an account', async () => { + it('removes an account without emitting keyring events', async () => { const mockAccount = mock(); mockAccount.id = 'some-id'; @@ -951,27 +783,9 @@ describe('AccountUseCases', () => { await useCases.delete(mockAccount.id); expect(mockRepository.get).toHaveBeenCalledWith(mockAccount.id); - expect(mockSnapClient.emitAccountDeletedEvent).toHaveBeenCalledWith( - mockAccount.id, - ); expect(mockRepository.delete).toHaveBeenCalledWith(mockAccount.id); }); - it('propagates an error if the event emitting fails', async () => { - const mockAccount = mock(); - mockAccount.id = 'some-id'; - const error = new Error('Event emit failed'); - - mockRepository.get.mockResolvedValue(mockAccount); - mockSnapClient.emitAccountDeletedEvent.mockRejectedValue(error); - - await expect(useCases.delete(mockAccount.id)).rejects.toBe(error); - - expect(mockRepository.get).toHaveBeenCalled(); - expect(mockSnapClient.emitAccountDeletedEvent).toHaveBeenCalled(); - expect(mockRepository.delete).not.toHaveBeenCalled(); - }); - it('propagates an error if the repository fails', async () => { const mockAccount = mock(); mockAccount.id = 'some-id'; @@ -983,7 +797,6 @@ describe('AccountUseCases', () => { await expect(useCases.delete(mockAccount.id)).rejects.toBe(error); expect(mockRepository.get).toHaveBeenCalled(); - expect(mockSnapClient.emitAccountDeletedEvent).toHaveBeenCalled(); expect(mockRepository.delete).toHaveBeenCalled(); }); }); diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index 57a9e00b1..e77521f51 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -48,9 +48,6 @@ export type CreateAccountParams = DiscoverAccountParams & { accountName?: string; }; -// Snap entropy derivation can become very spiky under wider parallelism. -const CREATE_ACCOUNTS_CONCURRENCY = 2; - /** * @param req - Account creation or discovery request. * @returns The BIP-44 account derivation path. @@ -72,59 +69,6 @@ function getDerivationPathKey(derivationPath: string[]): string { return derivationPath.join('/'); } -/** - * Map items to results with at most `concurrency` in-flight async operations. - * Output order matches `items` order. - * - * @param items - Values to map in pool order. - * @param concurrency - Maximum number of concurrent mapper executions. - * @param mapper - Async function applied to each item. - * @returns Results in the same order as `items`. - */ -async function runWithConcurrencyLimit( - items: readonly Item[], - concurrency: number, - mapper: (item: Item, index: number) => Promise, -): Promise { - if (items.length === 0) { - return []; - } - - const results: Result[] = new Array(items.length); - let next = 0; - let firstError: unknown; - let hasError = false; - - const worker = async (): Promise => { - while (!hasError) { - const idx = next; - next += 1; - if (idx >= items.length) { - return; - } - - try { - results[idx] = await mapper(items[idx] as Item, idx); - } catch (error) { - if (!hasError) { - firstError = error; - hasError = true; - } - return; - } - } - }; - - const poolSize = Math.min(Math.max(1, concurrency), items.length); - await Promise.all(Array.from({ length: poolSize }, async () => worker())); - - if (hasError) { - throw firstError; - } - - return results; -} - /** * Result of broadcasting a Bitcoin transaction. * @@ -239,69 +183,16 @@ export class AccountUseCases { return newAccount; } - async create(req: CreateAccountParams): Promise { - this.#logger.debug('Creating new Bitcoin account. Request: %o', req); - - return this.#runAccountMutation(async () => { - const { addressType, network, correlationId, accountName, synchronize } = - req; - const derivationPath = getAccountDerivationPath(req); - - // Idempotent account creation + ensures only one account per derivation path - const account = - await this.#repository.getByDerivationPath(derivationPath); - if (account?.network === network) { - this.#logger.debug('Account already exists: %s,', account.id); - await this.#snapClient.emitAccountCreatedEvent( - account, - correlationId, - accountName, - ); - return account; - } - - const newAccount = await this.#repository.create( - derivationPath, - network, - addressType, - ); - - newAccount.revealNextAddress(); - - await this.#repository.insert(newAccount); - - // First notify the event has been created, then schedule full scan. - await this.#snapClient.emitAccountCreatedEvent( - newAccount, - correlationId, - accountName, - ); - - if (synchronize) { - await this.#snapClient.scheduleBackgroundEvent({ - duration: 'PT1S', - method: CronMethod.FullScanAccount, - params: { accountId: newAccount.id }, - }); - } - - this.#logger.info( - 'Bitcoin account created successfully: %s. Public address: %s, Request: %o', - newAccount.id, - newAccount.publicAddress, - req, - ); - return newAccount; - }); - } - async createMany(reqs: CreateAccountParams[]): Promise { if (reqs.length === 0) { return []; } + const startMs = Date.now(); + const { accounts, createdAccountKeys } = await this.#runAccountMutation( async () => { + const lookupStartMs = Date.now(); const entries = reqs.map((req, index) => { const derivationPath = getAccountDerivationPath(req); return { @@ -320,9 +211,10 @@ export class AccountUseCases { } const uniqueEntries = [...uniqueEntriesByPath.values()]; - const existingAccounts = await this.#repository.getByDerivationPaths( - uniqueEntries.map(({ derivationPath }) => derivationPath), - ); + const { accounts: existingAccounts, snapshot } = + await this.#repository.getByDerivationPaths( + uniqueEntries.map(({ derivationPath }) => derivationPath), + ); const existingAccountsByPath = new Map(); uniqueEntries.forEach((entry, index) => { @@ -335,23 +227,49 @@ export class AccountUseCases { const entriesToCreate = uniqueEntries.filter( ({ pathKey }) => !existingAccountsByPath.has(pathKey), ); - const newAccounts = await runWithConcurrencyLimit( - entriesToCreate, - CREATE_ACCOUNTS_CONCURRENCY, - async ({ derivationPath, req }) => { - const newAccount = await this.#repository.create( - derivationPath, - req.network, - req.addressType, - ); - newAccount.revealNextAddress(); - return newAccount; - }, - ); + const lookupMs = Date.now() - lookupStartMs; + + // Batch-create so entropy is fetched once per parent path instead of + // once per account; remaining per-account work is local derivation + // plus synchronous WASM wallet construction, so no throttling needed. + const deriveStartMs = Date.now(); + const newAccounts = + entriesToCreate.length > 0 + ? await this.#repository.createMany( + entriesToCreate.map(({ derivationPath, req }) => ({ + derivationPath, + network: req.network, + addressType: req.addressType, + })), + ) + : []; + + for (const newAccount of newAccounts) { + newAccount.revealNextAddress(); + } + const deriveMs = Date.now() - deriveStartMs; + const persistStartMs = Date.now(); if (newAccounts.length > 0) { - await this.#repository.insertMany(newAccounts); + // Reuse the lookup's derivation-path snapshot: account lifecycle + // mutations are serialized, while `insertMany` refreshes the + // accounts map to preserve concurrent sync updates. + await this.#repository.insertMany(newAccounts, snapshot); } + const persistMs = Date.now() - persistStartMs; + + // Stringified so the values survive in the console after the snap's + // execution environment is torn down. + this.#logger.info( + `[createMany] Phase timings ${JSON.stringify({ + requested: reqs.length, + created: newAccounts.length, + lookupMs, + deriveMs, + persistMs, + totalMs: Date.now() - startMs, + })}`, + ); const newAccountsByPath = new Map( entriesToCreate.map((entry, index) => [ @@ -516,7 +434,9 @@ export class AccountUseCases { throw new NotFoundError('Account not found', { id }); } - await this.#snapClient.emitAccountDeletedEvent(id); + // No AccountDeleted event: deletion is client-initiated in keyring v2, + // and v2 clients reject v1 lifecycle events (which would abort the + // deletion below). await this.#repository.delete(id); this.#logger.info('Account deleted successfully: %s', account.id);