diff --git a/packages/site/package.json b/packages/site/package.json index 2ddbd71af..89781150b 100644 --- a/packages/site/package.json +++ b/packages/site/package.json @@ -29,7 +29,7 @@ ] }, "dependencies": { - "@metamask/keyring-api": "^21.3.0", + "@metamask/keyring-api": "^22.0.0", "@metamask/providers": "^22.1.0", "react": "^18.2.0", "react-dom": "^18.2.0", diff --git a/packages/snap/CHANGELOG.md b/packages/snap/CHANGELOG.md index bf02a33ae..1563695d2 100644 --- a/packages/snap/CHANGELOG.md +++ b/packages/snap/CHANGELOG.md @@ -7,10 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add support for `keyring_createAccounts` method to enable batch account creation ([#601](https://github.com/MetaMask/snap-bitcoin-wallet/pull/601)) + ### Changed - Show a confirmation dialog before signing a PSBT from KeyringHandler and sending a transfer ([#591](https://github.com/MetaMask/snap-bitcoin-wallet/pull/591)) - Add `resolveAccountAddress` method to KeyringHandler for dApp connectivity ([#590](https://github.com/MetaMask/snap-bitcoin-wallet/pull/590)) +- Bump `@metamask/keyring-api` from `^21.3.0` to `^22.0.0` ([#601](https://github.com/MetaMask/snap-bitcoin-wallet/pull/601)) +- Bump `@metamask/keyring-snap-sdk` from `^7.1.1` to `^8.0.0` ([#601](https://github.com/MetaMask/snap-bitcoin-wallet/pull/601)) ### Fixed diff --git a/packages/snap/package.json b/packages/snap/package.json index 34c1132ac..8a0e8bbbe 100644 --- a/packages/snap/package.json +++ b/packages/snap/package.json @@ -43,8 +43,8 @@ "@metamask/auto-changelog": "^3.4.4", "@metamask/bitcoindevkit": "^0.1.13", "@metamask/key-tree": "^10.1.1", - "@metamask/keyring-api": "^21.3.0", - "@metamask/keyring-snap-sdk": "^7.1.1", + "@metamask/keyring-api": "^22.0.0", + "@metamask/keyring-snap-sdk": "^8.0.0", "@metamask/slip44": "^4.2.0", "@metamask/snaps-cli": "^8.3.0", "@metamask/snaps-jest": "^9.8.0", diff --git a/packages/snap/src/entities/account.ts b/packages/snap/src/entities/account.ts index f7f30af65..dc17ef789 100644 --- a/packages/snap/src/entities/account.ts +++ b/packages/snap/src/entities/account.ts @@ -262,6 +262,16 @@ export type BitcoinAccountRepository = { */ getByDerivationPath(derivationPath: string[]): Promise; + /** + * Get accounts by derivation path. + * + * @param derivationPaths - derivation paths. + * @returns the accounts or null if they do not exist, in input order + */ + getByDerivationPaths( + derivationPaths: string[][], + ): Promise<(BitcoinAccount | null)[]>; + /** * Create a new account, without persisting it. * @@ -283,6 +293,13 @@ export type BitcoinAccountRepository = { */ insert(account: BitcoinAccount): Promise; + /** + * Insert accounts. + * + * @param accounts - Bitcoin accounts. + */ + insertMany(accounts: BitcoinAccount[]): Promise; + /** * Update an account. * diff --git a/packages/snap/src/entities/snap.ts b/packages/snap/src/entities/snap.ts index 9d8e9f42d..f84bcfa0d 100644 --- a/packages/snap/src/entities/snap.ts +++ b/packages/snap/src/entities/snap.ts @@ -1,4 +1,4 @@ -import type { WalletTx } from '@metamask/bitcoindevkit'; +import type { AddressType, Network, WalletTx } from '@metamask/bitcoindevkit'; import type { JsonSLIP10Node, SLIP10Node } from '@metamask/key-tree'; import type { ComponentOrElement, @@ -13,7 +13,7 @@ import type { Inscription } from './meta-protocols'; export type SnapState = { // accountId -> account state. This is the main state of the snap. - accounts: Record; + accounts: Record; // derivationPath -> accountId. Only needed for fast lookup. derivationPaths: Record; }; @@ -25,6 +25,19 @@ export type AccountState = { wallet: string; // Wallet inscriptions for meta protocols (ordinals, etc.) inscriptions: Inscription[]; + // Metadata used by keyring account responses without loading the BDK wallet. + metadata?: AccountMetadata; +}; + +export type AccountMetadata = { + // Public receive address at account address index 0. + address: string; + // Account address type. + addressType: AddressType; + // Bitcoin network. + network: Network; + // Public descriptor for read-only descriptor requests. + publicDescriptor: string; }; export type SyncResult = { diff --git a/packages/snap/src/handlers/KeyringHandler.test.ts b/packages/snap/src/handlers/KeyringHandler.test.ts index 8691a519f..2993dcf80 100644 --- a/packages/snap/src/handlers/KeyringHandler.test.ts +++ b/packages/snap/src/handlers/KeyringHandler.test.ts @@ -11,12 +11,17 @@ import type { import { Address } from '@metamask/bitcoindevkit'; import type { DiscoveredAccount, + KeyringAccount, KeyringResponse, Transaction as KeyringTransaction, KeyringRequest, - KeyringAccount, } from '@metamask/keyring-api'; -import { BtcAccountType, BtcMethod, BtcScope } from '@metamask/keyring-api'; +import { + AccountCreationType, + BtcAccountType, + BtcMethod, + BtcScope, +} from '@metamask/keyring-api'; import { mock } from 'jest-mock-extended'; import { assert } from 'superstruct'; @@ -51,6 +56,17 @@ jest.mock('@metamask/bitcoindevkit', () => { }; }); +/** + * Narrows `T | undefined` after `expect(value).toBeDefined()` for use in tests. + * + * @param value - Possibly undefined value. + * @returns The same value narrowed to `T`. + */ +function expectDefined(value: T | undefined): T { + expect(value).toBeDefined(); + return value as T; +} + describe('KeyringHandler', () => { const mockKeyringRequest = mock(); const mockAccounts = mock(); @@ -93,6 +109,7 @@ describe('KeyringHandler', () => { const correlationId = 'correlation-id'; // non-P2WPKH address types as we are not supporting them for v1 + // eslint-disable-next-line jest/no-disabled-tests it.skip('respects provided params', async () => { const options = { scope: BtcScope.Signet, @@ -123,6 +140,7 @@ describe('KeyringHandler', () => { }); // only P2WPKH (BIP-84) derivation paths are now supported for v1 + // eslint-disable-next-line jest/no-disabled-tests it.skip('extracts index from derivationPath', async () => { const options = { scope: BtcScope.Signet, @@ -222,6 +240,7 @@ describe('KeyringHandler', () => { ); // skip non-P2WPKH address types as they are not supported on v1 + // eslint-disable-next-line jest/no-disabled-tests it.skip.each([ { purpose: Purpose.Legacy, addressType: 'p2pkh' }, { purpose: Purpose.Segwit, addressType: 'p2sh' }, @@ -390,6 +409,285 @@ describe('KeyringHandler', () => { }); }); + describe('createAccounts', () => { + const entropySource = 'some-source'; + + const mnemonicGroupIndex = (account: KeyringAccount): number => { + const { entropy } = account.options; + if (entropy?.type !== 'mnemonic') { + throw new Error('expected mnemonic entropy'); + } + return entropy.groupIndex; + }; + + const buildMockAccount = (index: number): BitcoinAccount => + mock({ + id: `id-${index}`, + addressType: 'p2wpkh', + balance: { trusted_spendable: { to_btc: () => 1 } }, + network: 'bitcoin', + derivationPath: ['myEntropy', "84'", "0'", `${index}'`], + entropySource: 'myEntropy', + accountIndex: index, + publicAddress: mockAddress, + capabilities: [ + AccountCapability.SignPsbt, + AccountCapability.ComputeFee, + ], + }); + + it('creates a single account for Bip44DeriveIndex', async () => { + const bitcoinAccount = buildMockAccount(2); + mockAccounts.createMany.mockResolvedValue([bitcoinAccount]); + + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + groupIndex: 2, + entropySource, + }); + + expect(mockAccounts.createMany).toHaveBeenCalledTimes(1); + expect(mockAccounts.createMany).toHaveBeenCalledWith([ + { + network: 'bitcoin', + entropySource, + index: 2, + addressType: 'p2wpkh', + synchronize: false, + }, + ]); + expect(result).toHaveLength(1); + const keyringAccount = expectDefined(result[0]); + expect(keyringAccount.id).toBe('id-2'); + expect(mnemonicGroupIndex(keyringAccount)).toBe(2); + }); + + it('creates an inclusive range of accounts for Bip44DeriveIndexRange', async () => { + mockAccounts.createMany.mockResolvedValue( + [0, 1, 2].map(buildMockAccount), + ); + + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range: { from: 0, to: 2 }, + entropySource, + }); + + expect(mockAccounts.createMany).toHaveBeenCalledTimes(1); + expect(mockAccounts.createMany).toHaveBeenCalledWith( + [0, 1, 2].map((index) => ({ + network: 'bitcoin', + entropySource, + index, + addressType: 'p2wpkh', + synchronize: false, + })), + ); + expect(result).toHaveLength(3); + expect(result.map((a) => mnemonicGroupIndex(a))).toStrictEqual([0, 1, 2]); + }); + + it('creates one account when range from and to are equal', async () => { + mockAccounts.createMany.mockResolvedValue([buildMockAccount(9)]); + + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range: { from: 9, to: 9 }, + entropySource, + }); + + expect(mockAccounts.createMany).toHaveBeenCalledTimes(1); + expect(mockAccounts.createMany).toHaveBeenCalledWith([ + expect.objectContaining({ index: 9 }), + ]); + expect(result).toHaveLength(1); + expect(mnemonicGroupIndex(expectDefined(result[0]))).toBe(9); + }); + + it('creates accounts for a non-zero index range', async () => { + mockAccounts.createMany.mockResolvedValue( + [10, 11, 12].map(buildMockAccount), + ); + + await handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range: { from: 10, to: 12 }, + entropySource, + }); + + expect(mockAccounts.createMany).toHaveBeenCalledWith( + [10, 11, 12].map((index) => expect.objectContaining({ index })), + ); + }); + + it('allows the maximum batch size of 100 accounts in one RPC', async () => { + const indices = Array.from({ length: 100 }, (_, index) => index); + mockAccounts.createMany.mockResolvedValue(indices.map(buildMockAccount)); + + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range: { from: 0, to: 99 }, + entropySource, + }); + + expect(mockAccounts.createMany).toHaveBeenCalledTimes(1); + expect(mockAccounts.createMany).toHaveBeenCalledWith( + indices.map((index) => expect.objectContaining({ index })), + ); + expect(result).toHaveLength(100); + expect(mnemonicGroupIndex(expectDefined(result[0]))).toBe(0); + expect(mnemonicGroupIndex(expectDefined(result[99]))).toBe(99); + }); + + it('returns accounts in the order returned by createMany', async () => { + mockAccounts.createMany.mockResolvedValue( + [0, 1, 2, 3, 4].map(buildMockAccount), + ); + + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range: { from: 0, to: 4 }, + entropySource, + }); + + expect(result.map((a) => mnemonicGroupIndex(a))).toStrictEqual([ + 0, 1, 2, 3, 4, + ]); + }); + + it('rejects when the handler default address type is not P2WPKH', async () => { + const handlerNonSegwit = new KeyringHandler( + mockKeyringRequest, + mockAccounts, + 'p2tr' as AddressType, + mockSnapClient, + mockLogger, + ); + + await expect( + handlerNonSegwit.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + groupIndex: 0, + entropySource, + }), + ).rejects.toThrow( + /Only native segwit \(P2WPKH\) addresses are supported/iu, + ); + expect(mockAccounts.createMany).not.toHaveBeenCalled(); + }); + + it('rejects an invalid index range when from is greater than to', async () => { + await expect( + handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range: { from: 2, to: 0 }, + entropySource, + }), + ).rejects.toThrow(/invalid.*range|from must be/iu); + expect(mockAccounts.createMany).not.toHaveBeenCalled(); + }); + + it.each([ + { from: -1, to: 0 }, + { from: 0.5, to: 1 }, + { from: 0, to: Number.MAX_SAFE_INTEGER + 1 }, + ])('rejects invalid account index bounds %#', async (range) => { + await expect( + handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range, + entropySource, + }), + ).rejects.toThrow(/non-negative integers/iu); + expect(mockAccounts.createMany).not.toHaveBeenCalled(); + }); + + it('splits requests larger than 100 accounts into internal batches', async () => { + mockAccounts.createMany.mockImplementation(async (requests) => + requests.map(({ index }) => buildMockAccount(index)), + ); + + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + range: { from: 0, to: 100 }, + entropySource, + }); + + expect(mockAccounts.createMany).toHaveBeenCalledTimes(2); + expect(mockAccounts.createMany).toHaveBeenNthCalledWith( + 1, + Array.from({ length: 100 }, (_, index) => + expect.objectContaining({ index }), + ), + ); + expect(mockAccounts.createMany).toHaveBeenNthCalledWith(2, [ + expect.objectContaining({ index: 100 }), + ]); + expect(result).toHaveLength(101); + expect( + result.map((account) => mnemonicGroupIndex(account)), + ).toStrictEqual(Array.from({ length: 101 }, (_, index) => index)); + }); + + it('rejects unsupported creation types', async () => { + await expect( + handler.createAccounts({ + type: AccountCreationType.Bip44DerivePath, + derivationPath: "m/84'/0'/0'", + entropySource, + }), + ).rejects.toThrow(/not supported|unsupported/iu); + expect(mockAccounts.createMany).not.toHaveBeenCalled(); + }); + + it('propagates errors from createMany', async () => { + const error = new Error('create error'); + mockAccounts.createMany.mockRejectedValue(error); + + await expect( + handler.createAccounts({ + type: AccountCreationType.Bip44DeriveIndex, + groupIndex: 0, + entropySource, + }), + ).rejects.toThrow(error); + }); + + describe('tracing', () => { + const options = { + type: AccountCreationType.Bip44DeriveIndex as const, + groupIndex: 0, + entropySource, + }; + + beforeEach(() => { + mockSnapClient.startTrace.mockResolvedValue(undefined); + mockSnapClient.endTrace.mockResolvedValue(undefined); + mockAccounts.createMany.mockResolvedValue([buildMockAccount(0)]); + }); + + it('calls startTrace and endTrace with correct trace name', async () => { + await handler.createAccounts(options); + + expect(mockSnapClient.startTrace).toHaveBeenCalledWith( + 'Create Bitcoin Accounts Batch', + ); + expect(mockSnapClient.endTrace).toHaveBeenCalledWith( + 'Create Bitcoin Accounts Batch', + ); + }); + + it('calls endTrace even if create fails', async () => { + mockAccounts.createMany.mockRejectedValue(new Error('boom')); + + await expect(handler.createAccounts(options)).rejects.toThrow('boom'); + expect(mockSnapClient.endTrace).toHaveBeenCalledWith( + 'Create Bitcoin Accounts Batch', + ); + }); + }); + }); + describe('discoverAccounts', () => { const entropySource = 'some-source'; const groupIndex = 0; diff --git a/packages/snap/src/handlers/KeyringHandler.ts b/packages/snap/src/handlers/KeyringHandler.ts index 7fc5a764e..c3596ce51 100644 --- a/packages/snap/src/handlers/KeyringHandler.ts +++ b/packages/snap/src/handlers/KeyringHandler.ts @@ -1,8 +1,11 @@ import type { AddressType } from '@metamask/bitcoindevkit'; import { + AccountCreationType, + assertCreateAccountOptionIsSupported, BtcAccountType, BtcScope, CreateAccountRequestStruct, + CreateAccountsRequestStruct, DeleteAccountRequestStruct, DiscoverAccountsRequestStruct, FilterAccountChainsStruct, @@ -18,6 +21,7 @@ import { SubmitRequestRequestStruct, } from '@metamask/keyring-api'; import type { + CreateAccountOptions, Keyring, KeyringAccount, KeyringResponse, @@ -43,13 +47,15 @@ import { string, } from 'superstruct'; -import type { BitcoinAccount, Logger, SnapClient } from '../entities'; import { + type BitcoinAccount, FormatError, InexistentMethodError, + type Logger, networkToCurrencyUnit, Purpose, purposeToAddressType, + type SnapClient, } from '../entities'; import { networkToCaip19, @@ -66,7 +72,10 @@ import { mapToTransaction, } from './mappings'; import { BtcWalletRequestStruct, validateSelectedAccounts } from './validation'; -import type { AccountUseCases } from '../use-cases/AccountUseCases'; +import type { + AccountUseCases, + CreateAccountParams, +} from '../use-cases/AccountUseCases'; import { runSnapActionSafely } from '../utils/snapHelpers'; export const CreateAccountRequest = object({ @@ -80,6 +89,9 @@ export const CreateAccountRequest = object({ ...MetaMaskOptionsStruct.schema, }); +/** Maximum number of accounts to create in one internal createMany call. */ +const MAX_CREATE_ACCOUNTS_PER_BATCH = 100; + export class KeyringHandler implements Keyring { readonly #accountsUseCases: AccountUseCases; @@ -119,6 +131,10 @@ export class KeyringHandler implements Keyring { assert(request, CreateAccountRequestStruct); return this.createAccount(request.params.options); } + case `${KeyringRpcMethod.CreateAccounts}`: { + assert(request, CreateAccountsRequestStruct); + return this.createAccounts(request.params.options); + } case `${KeyringRpcMethod.DiscoverAccounts}`: { assert(request, DiscoverAccountsRequestStruct); return this.discoverAccounts( @@ -287,6 +303,105 @@ export class KeyringHandler implements Keyring { } } + async createAccounts( + options: CreateAccountOptions, + ): Promise { + assertCreateAccountOptionIsSupported(options, [ + `${AccountCreationType.Bip44DeriveIndex}`, + `${AccountCreationType.Bip44DeriveIndexRange}`, + ]); + + const { entropySource } = options; + + const range = + options.type === AccountCreationType.Bip44DeriveIndex + ? { from: options.groupIndex, to: options.groupIndex } + : options.range; + + if ( + !Number.isSafeInteger(range.from) || + !Number.isSafeInteger(range.to) || + range.from < 0 || + range.to < 0 + ) { + throw new FormatError( + 'Account index range is invalid: from and to must be non-negative integers', + ); + } + + if (range.from > range.to) { + throw new FormatError( + 'Account index range is invalid: from must be less than or equal to to', + ); + } + + // Only P2WPKH (BIP-84) on bitcoin mainnet is supported for v1, mirroring + // the defaults used by `createAccount` when no scope is provided. + const network = scopeToNetwork[BtcScope.Mainnet]; + const addressType = this.#defaultAddressType; + if (addressType !== 'p2wpkh') { + throw new FormatError( + 'Only native segwit (P2WPKH) addresses are supported', + ); + } + + const traceName = 'Create Bitcoin Accounts Batch'; + let traceStarted = false; + + try { + await runSnapActionSafely( + async () => { + await this.#snapClient.startTrace(traceName); + traceStarted = true; + }, + this.#logger, + 'startTrace', + ); + + // `AccountUseCases.createMany` is idempotent: if an account already exists + // for the resolved derivation path, it will be returned as-is. + const accounts: BitcoinAccount[] = []; + 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, + }); + } + + accounts.push( + ...(await this.#accountsUseCases.createMany(chunkRequests)), + ); + + if (chunkTo === range.to) { + break; + } + chunkFrom = chunkTo + 1; + } + + return accounts.map(mapToKeyringAccount); + } finally { + if (traceStarted) { + await runSnapActionSafely( + async () => this.#snapClient.endTrace(traceName), + this.#logger, + 'endTrace', + ); + } + } + } + async discoverAccounts( scopes: BtcScope[], entropySource: string, diff --git a/packages/snap/src/infra/StoredAccountAdapter.ts b/packages/snap/src/infra/StoredAccountAdapter.ts new file mode 100644 index 000000000..92f12464f --- /dev/null +++ b/packages/snap/src/infra/StoredAccountAdapter.ts @@ -0,0 +1,181 @@ +import type { + AddressInfo, + Amount, + Balance, + FullScanRequest, + LocalOutput, + Psbt, + ScriptBuf, + SyncRequest, + Transaction, + Update, + WalletTx, + ChangeSet, + AddressType, + Network, +} from '@metamask/bitcoindevkit'; +import { Address } from '@metamask/bitcoindevkit'; + +import { + AccountCapability, + WalletError, + type AccountMetadata, + type AccountState, + type BitcoinAccount, + type TransactionBuilder, +} from '../entities'; + +type AccountStateWithMetadata = AccountState & { + metadata: AccountMetadata; +}; + +export class StoredAccountAdapter implements BitcoinAccount { + readonly #id: string; + + readonly #derivationPath: string[]; + + readonly #metadata: AccountMetadata; + + readonly #capabilities: AccountCapability[]; + + constructor(id: string, account: AccountStateWithMetadata) { + this.#id = id; + this.#derivationPath = account.derivationPath; + this.#metadata = account.metadata; + this.#capabilities = Object.values(AccountCapability); + } + + static canLoad(account: AccountState): account is AccountStateWithMetadata { + return Boolean(account.metadata); + } + + static load( + id: string, + account: AccountStateWithMetadata, + ): StoredAccountAdapter { + return new StoredAccountAdapter(id, account); + } + + get id(): string { + return this.#id; + } + + get derivationPath(): string[] { + return this.#derivationPath; + } + + get entropySource(): string { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + return this.#derivationPath[0]!; + } + + get accountIndex(): number { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const segment = this.#derivationPath[3]!; + const numericPart = segment.endsWith("'") ? segment.slice(0, -1) : segment; + return Number(numericPart); + } + + get balance(): Balance { + return this.#unsupported(); + } + + get addressType(): AddressType { + return this.#metadata.addressType; + } + + get network(): Network { + return this.#metadata.network; + } + + get publicAddress(): Address { + return Address.from_string(this.#metadata.address, this.#metadata.network); + } + + get publicDescriptor(): string { + return this.#metadata.publicDescriptor; + } + + get capabilities(): AccountCapability[] { + return this.#capabilities; + } + + peekAddress(_index: number): AddressInfo { + return this.#unsupported(); + } + + nextUnusedAddress(): AddressInfo { + return this.#unsupported(); + } + + revealNextAddress(): AddressInfo { + return this.#unsupported(); + } + + startFullScan(): FullScanRequest { + return this.#unsupported(); + } + + startSync(): SyncRequest { + return this.#unsupported(); + } + + applyUpdate(_update: Update): void { + this.#unsupported(); + } + + takeStaged(): ChangeSet | undefined { + return this.#unsupported(); + } + + buildTx(): TransactionBuilder { + return this.#unsupported(); + } + + sign(_psbt: Psbt): Psbt { + return this.#unsupported(); + } + + extractTransaction(_psbt: Psbt, _maxFeeRate?: number): Transaction { + return this.#unsupported(); + } + + getUtxo(_outpoint: string): LocalOutput | undefined { + return this.#unsupported(); + } + + listUnspent(): LocalOutput[] { + return this.#unsupported(); + } + + listTransactions(): WalletTx[] { + return this.#unsupported(); + } + + getTransaction(_txid: string): WalletTx | undefined { + return this.#unsupported(); + } + + calculateFee(_tx: Transaction): Amount { + return this.#unsupported(); + } + + isMine(_script: ScriptBuf): boolean { + return this.#unsupported(); + } + + sentAndReceived(_tx: Transaction): [Amount, Amount] { + return this.#unsupported(); + } + + applyUnconfirmedTx(_tx: Transaction, _lastSeen: number): void { + this.#unsupported(); + } + + #unsupported(): never { + throw new WalletError( + 'Stored account metadata cannot be used for wallet operations; load the full account first.', + { id: this.#id }, + ); + } +} diff --git a/packages/snap/src/infra/index.ts b/packages/snap/src/infra/index.ts index dbc4a2f30..fa4ce4fd2 100644 --- a/packages/snap/src/infra/index.ts +++ b/packages/snap/src/infra/index.ts @@ -1,4 +1,5 @@ export * from './BdkAccountAdapter'; +export * from './StoredAccountAdapter'; export * from './SnapClientAdapter'; export * from './EsploraClientAdapter'; export * from './PriceApiClientAdapter'; diff --git a/packages/snap/src/store/BdkAccountRepository.test.ts b/packages/snap/src/store/BdkAccountRepository.test.ts index a89e12411..c6c684f24 100644 --- a/packages/snap/src/store/BdkAccountRepository.test.ts +++ b/packages/snap/src/store/BdkAccountRepository.test.ts @@ -3,6 +3,7 @@ import type { DescriptorPair } from '@metamask/bitcoindevkit'; import { + Address, ChangeSet, xpriv_to_descriptor, xpub_to_descriptor, @@ -26,6 +27,9 @@ jest.mock('@metamask/bitcoindevkit', () => { ChangeSet: { from_json: jest.fn(), }, + Address: { + from_string: jest.fn(), + }, slip10_to_extended: jest.fn().mockReturnValue('mock-extended'), xpub_to_descriptor: jest.fn(), xpriv_to_descriptor: jest.fn(), @@ -57,11 +61,16 @@ describe('BdkAccountRepository', () => { derivationPath: mockDerivationPath, }); const mockChangeSet = mock(); + const mockAddress = mock
({ + toString: () => 'bc1qaddress...', + }); const mockAccount = mock({ id: 'some-id', derivationPath: mockDerivationPath, network: 'bitcoin', addressType: 'p2wpkh', + publicAddress: mockAddress, + publicDescriptor: 'mock-public-descriptor', }); const repo = new BdkAccountRepository(mockSnapClient); @@ -74,6 +83,7 @@ describe('BdkAccountRepository', () => { mockSnapClient.getPublicEntropy.mockResolvedValue(mockSlip10Node); (xpriv_to_descriptor as jest.Mock).mockReturnValue(mockDescriptors); (xpub_to_descriptor as jest.Mock).mockReturnValue(mockDescriptors); + jest.mocked(Address.from_string).mockReturnValue(mockAddress); (mockAccount.takeStaged as jest.Mock) = jest .fn() .mockReturnValue(mockChangeSet); @@ -185,6 +195,128 @@ describe('BdkAccountRepository', () => { }); }); + describe('getByDerivationPaths', () => { + const derivationPath1 = ['m', "84'", "0'", "1'"]; + const derivationPath2 = ['m', "84'", "0'", "2'"]; + const accountState1 = { + ...mockAccountState, + derivationPath: derivationPath1, + }; + const accountState2 = { + ...mockAccountState, + derivationPath: derivationPath2, + }; + const mockAccount1 = mock({ + ...mockAccount, + id: 'some-id-1', + derivationPath: derivationPath1, + }); + const mockAccount2 = mock({ + ...mockAccount, + id: 'some-id-2', + derivationPath: derivationPath2, + }); + + it('returns accounts in derivation path order with one state read per namespace', async () => { + mockSnapClient.getState + .mockResolvedValueOnce({ + "m/84'/0'/1'": 'some-id-1', + "m/84'/0'/2'": 'some-id-2', + }) + .mockResolvedValueOnce({ + 'some-id-1': accountState1, + 'some-id-2': accountState2, + }); + (BdkAccountAdapter.load as jest.Mock) + .mockReturnValueOnce(mockAccount2) + .mockReturnValueOnce(mockAccount1); + + const result = await repo.getByDerivationPaths([ + derivationPath2, + derivationPath1, + ]); + + expect(mockSnapClient.getState).toHaveBeenCalledWith('derivationPaths'); + expect(mockSnapClient.getState).toHaveBeenCalledWith('accounts'); + expect(mockSnapClient.getState).toHaveBeenCalledTimes(2); + expect(result).toStrictEqual([mockAccount2, mockAccount1]); + expect(mockSnapClient.setState).not.toHaveBeenCalled(); + }); + + it('uses cached account metadata without loading BDK wallets', async () => { + const accountStateWithMetadata: AccountState = { + ...accountState1, + metadata: { + address: 'bc1qcached...', + addressType: 'p2wpkh', + network: 'bitcoin', + publicDescriptor: 'cached-public-descriptor', + }, + }; + mockSnapClient.getState + .mockResolvedValueOnce({ + "m/84'/0'/1'": 'some-id-1', + }) + .mockResolvedValueOnce({ + 'some-id-1': accountStateWithMetadata, + }); + (BdkAccountAdapter.load as jest.Mock).mockClear(); + (ChangeSet.from_json as jest.Mock).mockClear(); + + const result = await repo.getByDerivationPaths([derivationPath1]); + const account = result[0]; + + expect(account?.id).toBe('some-id-1'); + expect(account?.publicAddress.toString()).toBe('bc1qaddress...'); + expect(account?.publicDescriptor).toBe('cached-public-descriptor'); + expect(jest.mocked(Address.from_string)).toHaveBeenCalledWith( + 'bc1qcached...', + 'bitcoin', + ); + expect(ChangeSet.from_json).not.toHaveBeenCalled(); + expect(BdkAccountAdapter.load).not.toHaveBeenCalled(); + }); + + it('repairs missing derivation path indexes from account state', async () => { + mockSnapClient.getState + .mockResolvedValueOnce({ + "m/84'/0'/1'": 'some-id-1', + }) + .mockResolvedValueOnce({ + 'some-id-1': accountState1, + 'some-id-2': accountState2, + }); + (BdkAccountAdapter.load as jest.Mock) + .mockReturnValueOnce(mockAccount1) + .mockReturnValueOnce(mockAccount2); + + const result = await repo.getByDerivationPaths([ + derivationPath1, + derivationPath2, + ]); + + expect(result).toStrictEqual([mockAccount1, mockAccount2]); + expect(mockSnapClient.setState).toHaveBeenCalledWith('derivationPaths', { + "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 () => { + mockSnapClient.getState.mockResolvedValueOnce({}).mockResolvedValueOnce({ + 'some-id-1': accountState1, + }); + (BdkAccountAdapter.load as jest.Mock).mockReturnValueOnce(mockAccount1); + + const result = await repo.getByDerivationPaths([derivationPath1]); + + expect(result).toStrictEqual([mockAccount1]); + expect(mockSnapClient.setState).toHaveBeenCalledWith('derivationPaths', { + "m/84'/0'/1'": 'some-id-1', + }); + }); + }); + describe('getWithSigner', () => { it('returns null if account not found', async () => { mockSnapClient.getState.mockResolvedValue(null); @@ -253,6 +385,111 @@ describe('BdkAccountRepository', () => { wallet: mockWalletData, inscriptions: [], derivationPath: mockDerivationPath, + metadata: { + address: 'bc1qaddress...', + addressType: 'p2wpkh', + network: 'bitcoin', + publicDescriptor: 'mock-public-descriptor', + }, + }, + ); + }); + }); + + describe('insertMany', () => { + it('returns an empty array when there are no accounts to insert', async () => { + const result = await repo.insertMany([]); + + expect(result).toStrictEqual([]); + expect(mockSnapClient.getState).not.toHaveBeenCalled(); + expect(mockSnapClient.setState).not.toHaveBeenCalled(); + }); + + it('throws an error if any account has no wallet data', async () => { + await expect( + repo.insertMany([ + { + ...mockAccount, + id: 'missing-wallet', + takeStaged: jest.fn().mockReturnValue(undefined), + }, + mockAccount, + ]), + ).rejects.toThrow( + 'Missing changeset data for account "missing-wallet" for insertion.', + ); + expect(mockSnapClient.setState).not.toHaveBeenCalled(); + }); + + it('inserts multiple accounts with one accounts write and one derivation path write', async () => { + const existingAccountState: AccountState = { + wallet: mockWalletData, + inscriptions: [], + derivationPath: mockDerivationPath, + }; + const account1 = mock(); + account1.id = 'some-id-1'; + account1.derivationPath = ['m', "84'", "0'", "1'"]; + account1.network = 'bitcoin'; + account1.addressType = 'p2wpkh'; + account1.publicAddress = mockAddress; + account1.publicDescriptor = 'mock-public-descriptor-1'; + const account2 = mock(); + account2.id = 'some-id-2'; + account2.derivationPath = ['m', "84'", "0'", "2'"]; + account2.network = 'bitcoin'; + account2.addressType = 'p2wpkh'; + account2.publicAddress = mockAddress; + account2.publicDescriptor = 'mock-public-descriptor-2'; + (account1.takeStaged as jest.Mock) = jest + .fn() + .mockReturnValue(mockChangeSet); + (account2.takeStaged as jest.Mock) = jest + .fn() + .mockReturnValue(mockChangeSet); + mockSnapClient.getState + .mockResolvedValueOnce({ + 'existing-id': existingAccountState, + }) + .mockResolvedValueOnce({ + "m/84'/0'/0'": 'existing-id', + }); + + const result = await repo.insertMany([account1, account2]); + + expect(result).toStrictEqual([account1, account2]); + expect(mockSnapClient.setState).toHaveBeenNthCalledWith(1, 'accounts', { + 'existing-id': existingAccountState, + 'some-id-1': { + wallet: mockWalletData, + inscriptions: [], + derivationPath: ['m', "84'", "0'", "1'"], + metadata: { + address: 'bc1qaddress...', + addressType: 'p2wpkh', + network: 'bitcoin', + publicDescriptor: 'mock-public-descriptor-1', + }, + }, + 'some-id-2': { + wallet: mockWalletData, + inscriptions: [], + derivationPath: ['m', "84'", "0'", "2'"], + metadata: { + address: 'bc1qaddress...', + addressType: 'p2wpkh', + network: 'bitcoin', + publicDescriptor: 'mock-public-descriptor-2', + }, + }, + }); + expect(mockSnapClient.setState).toHaveBeenNthCalledWith( + 2, + 'derivationPaths', + { + "m/84'/0'/0'": 'existing-id', + "m/84'/0'/1'": 'some-id-1', + "m/84'/0'/2'": 'some-id-2', }, ); }); diff --git a/packages/snap/src/store/BdkAccountRepository.ts b/packages/snap/src/store/BdkAccountRepository.ts index 37b5d4442..7eab93b31 100644 --- a/packages/snap/src/store/BdkAccountRepository.ts +++ b/packages/snap/src/store/BdkAccountRepository.ts @@ -16,10 +16,11 @@ import { type SnapClient, type Inscription, type AccountState, + type AccountMetadata, type SnapState, StorageError, } from '../entities'; -import { BdkAccountAdapter } from '../infra'; +import { BdkAccountAdapter, StoredAccountAdapter } from '../infra'; /** * Encode a fingerprint to a 4-bytes hex-string (required by the BDK). @@ -34,6 +35,44 @@ function toBdkFingerprint(fingerprint: number): string { return fingerprint.toString(16).padStart(8, '0'); } +/** + * @param derivationPath - Split derivation path. + * @returns Storage key for a derivation path. + */ +function getDerivationPathKey(derivationPath: string[]): string { + return derivationPath.join('/'); +} + +/** + * @param account - Account to persist. + * @returns Metadata needed for keyring account responses. + */ +function getAccountMetadata(account: BitcoinAccount): AccountMetadata { + return { + address: account.publicAddress.toString(), + addressType: account.addressType, + network: account.network, + publicDescriptor: account.publicDescriptor, + }; +} + +/** + * @param account - Account to persist. + * @param walletData - Serialized wallet data. + * @returns Account state with cached response metadata. + */ +function getAccountState( + account: BitcoinAccount, + walletData: ChangeSet, +): AccountState { + return { + wallet: walletData.to_json(), + inscriptions: [], + derivationPath: account.derivationPath, + metadata: getAccountMetadata(account), + }; +} + export class BdkAccountRepository implements BitcoinAccountRepository { readonly #snapClient: SnapClient; @@ -49,11 +88,7 @@ export class BdkAccountRepository implements BitcoinAccountRepository { return null; } - return BdkAccountAdapter.load( - id, - account.derivationPath, - ChangeSet.from_json(account.wallet), - ); + return this.#loadAccount(id, account); } async getAll(): Promise { @@ -64,22 +99,16 @@ export class BdkAccountRepository implements BitcoinAccountRepository { return []; } - return Object.entries(accounts) - .filter(([, account]) => account !== null) - .map(([id, account]) => - BdkAccountAdapter.load( - id, - account.derivationPath, - ChangeSet.from_json(account.wallet), - ), - ); + return Object.entries(accounts).flatMap(([id, account]) => + account ? [this.#loadAccount(id, account)] : [], + ); } async getByDerivationPath( derivationPath: string[], ): Promise { const id = await this.#snapClient.getState( - `derivationPaths.${derivationPath.join('/')}`, + `derivationPaths.${getDerivationPathKey(derivationPath)}`, ); if (!id) { return null; @@ -88,6 +117,65 @@ export class BdkAccountRepository implements BitcoinAccountRepository { return this.get(id as string); } + async getByDerivationPaths( + derivationPaths: string[][], + ): Promise<(BitcoinAccount | null)[]> { + if (derivationPaths.length === 0) { + return []; + } + + const [derivationPathIndex, accounts] = await Promise.all([ + this.#snapClient.getState('derivationPaths') as Promise< + SnapState['derivationPaths'] | null + >, + this.#snapClient.getState('accounts') as Promise< + SnapState['accounts'] | null + >, + ]); + + const accountsById = accounts ?? {}; + const existingDerivationPathIndex = derivationPathIndex ?? {}; + const accountsByDerivationPath = new Map(); + + for (const [id, account] of Object.entries(accountsById)) { + if (account) { + accountsByDerivationPath.set( + getDerivationPathKey(account.derivationPath), + [id, account], + ); + } + } + + const repairs: Record = {}; + const results = derivationPaths.map((derivationPath) => { + const pathKey = getDerivationPathKey(derivationPath); + const indexedId = existingDerivationPathIndex[pathKey]; + const indexedAccount = indexedId ? accountsById[indexedId] : null; + + if (indexedId && indexedAccount) { + return this.#loadPersistedAccount(indexedId, indexedAccount); + } + + const fallback = accountsByDerivationPath.get(pathKey); + if (!fallback) { + return null; + } + + const [id, account] = fallback; + repairs[pathKey] = id; + return this.#loadPersistedAccount(id, account); + }); + + if (Object.keys(repairs).length > 0) { + await this.#snapClient.setState('derivationPaths', { + ...existingDerivationPathIndex, + ...repairs, + }); + } + + return results; + } + async getWithSigner(id: string): Promise { const accountState = (await this.#snapClient.getState( `accounts.${id}`, @@ -148,7 +236,6 @@ export class BdkAccountRepository implements BitcoinAccountRepository { async insert(account: BitcoinAccount): Promise { const { id, derivationPath } = account; - const walletData = account.takeStaged(); if (!walletData) { throw new StorageError( @@ -158,19 +245,66 @@ export class BdkAccountRepository implements BitcoinAccountRepository { await Promise.all([ this.#snapClient.setState( - `derivationPaths.${derivationPath.join('/')}`, + `derivationPaths.${getDerivationPathKey(derivationPath)}`, id, ), - this.#snapClient.setState(`accounts.${id}`, { - wallet: walletData.to_json(), - inscriptions: [], - derivationPath, - }), + this.#snapClient.setState( + `accounts.${id}`, + getAccountState(account, walletData), + ), ]); return account; } + async insertMany(accounts: BitcoinAccount[]): Promise { + if (accounts.length === 0) { + return []; + } + + if (accounts.length === 1) { + return [await this.insert(accounts[0] as BitcoinAccount)]; + } + + const accountStateEntries: [string, AccountState][] = []; + const derivationPathEntries: [string, string][] = []; + + for (const account of accounts) { + const { id, derivationPath } = account; + const walletData = account.takeStaged(); + + if (!walletData) { + throw new StorageError( + `Missing changeset data for account "${id}" for insertion.`, + ); + } + + accountStateEntries.push([id, getAccountState(account, walletData)]); + derivationPathEntries.push([getDerivationPathKey(derivationPath), id]); + } + + const [existingAccounts, existingDerivationPaths] = await Promise.all([ + this.#snapClient.getState('accounts') as Promise< + SnapState['accounts'] | null + >, + 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), + }); + + return accounts; + } + async update( account: BitcoinAccount, inscriptions?: Inscription[], @@ -246,4 +380,20 @@ export class BdkAccountRepository implements BitcoinAccountRepository { return `${txid}:${vout}`; }); } + + #loadAccount(id: string, account: AccountState): BitcoinAccount { + return BdkAccountAdapter.load( + id, + account.derivationPath, + ChangeSet.from_json(account.wallet), + ); + } + + #loadPersistedAccount(id: string, account: AccountState): BitcoinAccount { + if (StoredAccountAdapter.canLoad(account)) { + return StoredAccountAdapter.load(id, account); + } + + return this.#loadAccount(id, account); + } } diff --git a/packages/snap/src/use-cases/AccountUseCases.test.ts b/packages/snap/src/use-cases/AccountUseCases.test.ts index 210f9f9c9..1a25b6c91 100644 --- a/packages/snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/snap/src/use-cases/AccountUseCases.test.ts @@ -291,6 +291,146 @@ describe('AccountUseCases', () => { }); }); + describe('createMany', () => { + const createParams: CreateAccountParams = { + network: 'bitcoin', + entropySource: 'some-source', + index: 1, + addressType: 'p2wpkh', + synchronize: false, + correlationId: 'correlation-id', + accountName: 'My account', + }; + const firstDerivationPath = ['some-source', "84'", "0'", "1'"]; + const secondDerivationPath = ['some-source', "84'", "0'", "2'"]; + const existingAccount = mock({ + id: 'existing-id', + network: createParams.network, + }); + const newAccount = mock({ + id: 'new-id', + network: createParams.network, + }); + + it('reuses existing accounts and bulk-inserts newly-created accounts', async () => { + mockRepository.getByDerivationPaths.mockResolvedValue([ + existingAccount, + null, + ]); + mockRepository.create.mockResolvedValue(newAccount); + + const result = await useCases.createMany([ + createParams, + { ...createParams, index: 2, synchronize: true }, + ]); + + expect(mockRepository.getByDerivationPaths).toHaveBeenCalledWith([ + firstDerivationPath, + secondDerivationPath, + ]); + expect(mockRepository.create).toHaveBeenCalledWith( + secondDerivationPath, + createParams.network, + createParams.addressType, + ); + expect(newAccount.revealNextAddress).toHaveBeenCalled(); + expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); + expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); + expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ + duration: 'PT1S', + method: CronMethod.FullScanAccount, + params: { accountId: newAccount.id }, + }); + expect(result).toStrictEqual([existingAccount, newAccount]); + }); + + it('creates only one account for duplicate derivation paths in the same batch', async () => { + mockRepository.getByDerivationPaths.mockResolvedValue([null]); + mockRepository.create.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(result).toStrictEqual([newAccount, newAccount]); + }); + + it('does not create or insert accounts when all accounts already exist', async () => { + mockRepository.getByDerivationPaths.mockResolvedValue([existingAccount]); + + const result = await useCases.createMany([createParams]); + + expect(mockRepository.create).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.insertMany.mockRejectedValue(error); + + await expect(useCases.createMany([createParams])).rejects.toBe(error); + + expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); + expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); + }); + + 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; + }); + 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; + }); + + const createManyPromise = useCases.createMany([ + createParams, + { ...createParams, index: 2 }, + ]); + const onSettled = jest.fn(); + const settlementObserver = createManyPromise.then(onSettled, onSettled); + + await new Promise((resolve) => { + setTimeout(resolve, 0); + }); + + expect(onSettled).not.toHaveBeenCalled(); + + resolveSlowCreate(slowAccount); + + 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(); + }); + }); + describe('discover', () => { const discoverParams: DiscoverAccountParams = { network: 'bitcoin', diff --git a/packages/snap/src/use-cases/AccountUseCases.ts b/packages/snap/src/use-cases/AccountUseCases.ts index 345fe472a..871247cf4 100644 --- a/packages/snap/src/use-cases/AccountUseCases.ts +++ b/packages/snap/src/use-cases/AccountUseCases.ts @@ -48,6 +48,83 @@ 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. + */ +function getAccountDerivationPath(req: DiscoverAccountParams): string[] { + return [ + req.entropySource, + `${addressTypeToPurpose[req.addressType]}'`, + `${networkToCoinType[req.network]}'`, + `${req.index}'`, + ]; +} + +/** + * @param derivationPath - Split derivation path. + * @returns Storage key for a derivation path. + */ +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; +} + export class AccountUseCases { readonly #logger: Logger; @@ -65,6 +142,8 @@ export class AccountUseCases { readonly #targetBlocksConfirmation: number; + #accountMutationQueue: Promise = Promise.resolve(); + constructor( logger: Logger, snapClient: SnapClient, @@ -109,14 +188,8 @@ export class AccountUseCases { async discover(req: DiscoverAccountParams): Promise { this.#logger.debug('Discovering Bitcoin account. Request: %o', req); - const { addressType, index, network, entropySource } = req; - - const derivationPath = [ - entropySource, - `${addressTypeToPurpose[addressType]}'`, - `${networkToCoinType[network]}'`, - `${index}'`, - ]; + const { addressType, network } = req; + const derivationPath = getAccountDerivationPath(req); // Idempotent account creation + ensures only one account per derivation path const account = await this.#repository.getByDerivationPath(derivationPath); @@ -146,67 +219,165 @@ export class AccountUseCases { async create(req: CreateAccountParams): Promise { this.#logger.debug('Creating new Bitcoin account. Request: %o', req); - const { - addressType, - index, - network, - entropySource, - correlationId, - accountName, - synchronize, - } = req; - - const derivationPath = [ - entropySource, - `${addressTypeToPurpose[addressType]}'`, - `${networkToCoinType[network]}'`, - `${index}'`, - ]; + 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 && account.network === network) { + this.#logger.debug('Account already exists: %s,', account.id); + await this.#snapClient.emitAccountCreatedEvent( + account, + correlationId, + accountName, + ); + return account; + } - // Idempotent account creation + ensures only one account per derivation path - const account = await this.#repository.getByDerivationPath(derivationPath); - if (account && account.network === network) { - this.#logger.debug('Account already exists: %s,', account.id); + 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( - account, + newAccount, correlationId, accountName, ); - return account; + + 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 newAccount = await this.#repository.create( - derivationPath, - network, - addressType, - ); + const { accounts, createdAccountKeys } = await this.#runAccountMutation( + async () => { + const entries = reqs.map((req, index) => { + const derivationPath = getAccountDerivationPath(req); + return { + req, + index, + derivationPath, + pathKey: getDerivationPathKey(derivationPath), + }; + }); + + const uniqueEntriesByPath = new Map(); + for (const entry of entries) { + if (!uniqueEntriesByPath.has(entry.pathKey)) { + uniqueEntriesByPath.set(entry.pathKey, entry); + } + } + const uniqueEntries = [...uniqueEntriesByPath.values()]; - newAccount.revealNextAddress(); + const existingAccounts = await this.#repository.getByDerivationPaths( + uniqueEntries.map(({ derivationPath }) => derivationPath), + ); + const existingAccountsByPath = new Map(); - await this.#repository.insert(newAccount); + uniqueEntries.forEach((entry, index) => { + const account = existingAccounts[index]; + if (account && account.network === entry.req.network) { + existingAccountsByPath.set(entry.pathKey, account); + } + }); - // First notify the event has been created, then schedule full scan. - await this.#snapClient.emitAccountCreatedEvent( - newAccount, - correlationId, - accountName, + 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; + }, + ); + + if (newAccounts.length > 0) { + await this.#repository.insertMany(newAccounts); + } + + const newAccountsByPath = new Map( + entriesToCreate.map((entry, index) => [ + entry.pathKey, + newAccounts[index] as BitcoinAccount, + ]), + ); + + const accountsInOrder = entries.map((entry) => { + const account = + existingAccountsByPath.get(entry.pathKey) ?? + newAccountsByPath.get(entry.pathKey); + + if (!account) { + throw new AssertionError('Failed to create account', { + index: entry.index, + derivationPath: entry.derivationPath, + }); + } + + return account; + }); + + return { + accounts: accountsInOrder, + createdAccountKeys: new Set(newAccountsByPath.keys()), + }; + }, ); - if (synchronize) { - await this.#snapClient.scheduleBackgroundEvent({ - duration: 'PT1S', - method: CronMethod.FullScanAccount, - params: { accountId: newAccount.id }, - }); + const scheduledAccountIds = new Set(); + for (const [index, account] of accounts.entries()) { + const req = reqs[index] as CreateAccountParams; + const pathKey = getDerivationPathKey(getAccountDerivationPath(req)); + if ( + req.synchronize && + createdAccountKeys.has(pathKey) && + !scheduledAccountIds.has(account.id) + ) { + scheduledAccountIds.add(account.id); + await this.#snapClient.scheduleBackgroundEvent({ + duration: 'PT1S', + method: CronMethod.FullScanAccount, + params: { accountId: account.id }, + }); + } } - this.#logger.info( - 'Bitcoin account created successfully: %s. Public address: %s, Request: %o', - newAccount.id, - newAccount.publicAddress, - req, - ); - return newAccount; + return accounts; } async synchronize( @@ -331,15 +502,17 @@ export class AccountUseCases { async delete(id: string): Promise { this.#logger.debug('Deleting account: %s', id); - const account = await this.#repository.get(id); - if (!account) { - throw new NotFoundError('Account not found', { id }); - } + await this.#runAccountMutation(async () => { + const account = await this.#repository.get(id); + if (!account) { + throw new NotFoundError('Account not found', { id }); + } - await this.#snapClient.emitAccountDeletedEvent(id); - await this.#repository.delete(id); + await this.#snapClient.emitAccountDeletedEvent(id); + await this.#repository.delete(id); - this.#logger.info('Account deleted successfully: %s', account.id); + this.#logger.info('Account deleted successfully: %s', account.id); + }); } async fillPsbt( @@ -680,6 +853,24 @@ export class AccountUseCases { return txid; } + async #runAccountMutation( + fn: () => Promise, + ): Promise { + const previousMutation = this.#accountMutationQueue; + let releaseMutation: () => void = () => undefined; + this.#accountMutationQueue = new Promise((resolve) => { + releaseMutation = resolve; + }); + + await previousMutation; + + try { + return await fn(); + } finally { + releaseMutation(); + } + } + #checkCapability( account: BitcoinAccount, capability: AccountCapability, diff --git a/yarn.lock b/yarn.lock index 4b094eb72..e2304b2e6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3076,8 +3076,8 @@ __metadata: "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/bitcoindevkit": "npm:^0.1.13" "@metamask/key-tree": "npm:^10.1.1" - "@metamask/keyring-api": "npm:^21.3.0" - "@metamask/keyring-snap-sdk": "npm:^7.1.1" + "@metamask/keyring-api": "npm:^22.0.0" + "@metamask/keyring-snap-sdk": "npm:^8.0.0" "@metamask/slip44": "npm:^4.2.0" "@metamask/snaps-cli": "npm:^8.3.0" "@metamask/snaps-jest": "npm:^9.8.0" @@ -3100,7 +3100,7 @@ __metadata: version: 0.0.0-use.local resolution: "@metamask/bitcoin-wallet-test-dapp@workspace:packages/site" dependencies: - "@metamask/keyring-api": "npm:^21.3.0" + "@metamask/keyring-api": "npm:^22.0.0" "@metamask/providers": "npm:^22.1.0" "@svgr/webpack": "npm:^8.1.0" "@testing-library/dom": "npm:^8.17.1" @@ -3286,43 +3286,43 @@ __metadata: languageName: node linkType: hard -"@metamask/keyring-api@npm:^21.3.0": - version: 21.3.0 - resolution: "@metamask/keyring-api@npm:21.3.0" +"@metamask/keyring-api@npm:^22.0.0": + version: 22.0.0 + resolution: "@metamask/keyring-api@npm:22.0.0" dependencies: - "@metamask/keyring-utils": "npm:^3.1.0" + "@metamask/keyring-utils": "npm:^3.2.0" "@metamask/superstruct": "npm:^3.1.0" "@metamask/utils": "npm:^11.1.0" bitcoin-address-validation: "npm:^2.2.3" - checksum: 10/5d1f2705d76d8cfc70378f740e054fca163735b7b77b86b092cfd311d1befb27fac16f81b210ef2c719ce32010e1db55916b7b78d1f7a84239e47b771e5d84cc + checksum: 10/ff1e9537c7219fb906b61d6755de28890239ec44f634732bd8571801c662e8b801671e98961a1c5047e079cef314353297499a52f7091a2b5391325d575ec4f9 languageName: node linkType: hard -"@metamask/keyring-snap-sdk@npm:^7.1.1": - version: 7.1.1 - resolution: "@metamask/keyring-snap-sdk@npm:7.1.1" +"@metamask/keyring-snap-sdk@npm:^8.0.0": + version: 8.0.0 + resolution: "@metamask/keyring-snap-sdk@npm:8.0.0" dependencies: - "@metamask/keyring-utils": "npm:^3.1.0" - "@metamask/snaps-sdk": "npm:^9.0.0" + "@metamask/keyring-utils": "npm:^3.2.0" + "@metamask/snaps-sdk": "npm:^11.0.0" "@metamask/superstruct": "npm:^3.1.0" "@metamask/utils": "npm:^11.1.0" webextension-polyfill: "npm:^0.12.0" peerDependencies: - "@metamask/keyring-api": ^21.2.0 + "@metamask/keyring-api": ^22.0.0 "@metamask/providers": ^19.0.0 - checksum: 10/ac4ce050f4647096ef66ebd04d99d1423c002ca0fb05bd83e11caec59754b56d73bb8a95ac3a76f64472713256205e889d6785003dfe2c35f5f1d67c2f2efd12 + checksum: 10/271b4f84d214813c0312ffbbe3fb6201b66172210da4d06a155c3ddb359403ed1fa4c7afa424cd34fa9db04e3c876f30f69afb676bb7a905d392552d871ebd57 languageName: node linkType: hard -"@metamask/keyring-utils@npm:^3.1.0": - version: 3.1.0 - resolution: "@metamask/keyring-utils@npm:3.1.0" +"@metamask/keyring-utils@npm:^3.2.0": + version: 3.2.0 + resolution: "@metamask/keyring-utils@npm:3.2.0" dependencies: "@ethereumjs/tx": "npm:^5.4.0" "@metamask/superstruct": "npm:^3.1.0" "@metamask/utils": "npm:^11.1.0" bitcoin-address-validation: "npm:^2.2.3" - checksum: 10/d7325bb72e47bd3d81b1bce55203d8343408c0d37dd2862203c21bb68c6a1e32a1cfa7ca46a4f6fe1f14e757084bbc45db8db3eedbefc90ce81805ce22d335e8 + checksum: 10/e71aa1b9ec9a24c72ea6d4864a10f11e68e5b77789728067230ec40cee2e85ad69073404d2fa62c760f014fd910fb68b3305a08f906f2534b9119e2a26d06a2b languageName: node linkType: hard