From 8081592b66606a8ec383c4cb9a69230b5d79d257 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 13 Aug 2026 11:12:06 +0200 Subject: [PATCH 01/18] perf(tron-wallet-snap): reduce createAccounts extension RPC round trips --- packages/tron-wallet-snap/CHANGELOG.md | 3 + packages/tron-wallet-snap/snap.manifest.json | 2 +- .../accounts/AccountsRepository.test.ts | 28 +++++++++ .../services/accounts/AccountsRepository.ts | 25 +++++++- .../services/accounts/AccountsService.test.ts | 57 +++++++++++++++-- .../src/services/accounts/AccountsService.ts | 62 ++++++++++++++----- 6 files changed, 151 insertions(+), 26 deletions(-) diff --git a/packages/tron-wallet-snap/CHANGELOG.md b/packages/tron-wallet-snap/CHANGELOG.md index a091cac27..f78aba543 100644 --- a/packages/tron-wallet-snap/CHANGELOG.md +++ b/packages/tron-wallet-snap/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Reduce extension RPC round trips in `keyring_createAccounts` from 5 to at most 4 ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) + - `mergeKeyringAccounts` now returns the merge result instead of requiring a post-merge state re-read, and the existing-accounts read runs in parallel with the BIP-32 entropy fetch. + - `snap_getBip32Entropy` is now called even when all requested indices already exist (this path only occurs on idempotent retries); no new permissions are required. - Extract shared asset util functions and inject `SnapAssetsAdapter` from `context` into `AssetsService` ([#143](https://github.com/MetaMask/internal-snaps/pull/143)) - Rename `getByKeyringAccountId` to `getAccountAssets` (with essential-asset synthesis) and update keyring callers ([#143](https://github.com/MetaMask/internal-snaps/pull/143)) diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index 0f47ebb3f..b4f24d6cd 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "NpxOo6DkB0sBh8xpisBu3o+7G6MJriDNsADcHVb/Qp8=", + "shasum": "b0zTF4I78txypck9p+FhyttFm/3cDVM0jqOpFQl/u9E=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.test.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.test.ts index a972dda70..b53cc0b0e 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.test.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.test.ts @@ -144,6 +144,34 @@ describe('AccountsRepository', () => { ]); }); + it('returns the merged state and added accounts from mergeKeyringAccounts', async () => { + const existing = createTestAccount({ id: 'existing-0' }); + const repository = new AccountsRepository( + createEmptyState({ [existing.id]: existing }), + ); + const newIndexAccount = createTestAccount({ + id: 'new-index', + index: 1, + derivationPath: "m/44'/195'/0'/0/1", + address: 'TAddress1', + }); + + const result = await repository.mergeKeyringAccounts({ + 'duplicate-index': { + ...existing, + id: 'duplicate-index', + }, + [newIndexAccount.id]: newIndexAccount, + }); + + // The conflict loser is omitted from `added`; the winner is in `merged`. + expect(Object.keys(result.added)).toStrictEqual(['new-index']); + expect(result.merged).toStrictEqual({ + 'existing-0': existing, + 'new-index': newIndexAccount, + }); + }); + it('skips duplicate indices within the same merge batch', async () => { const base = createTestAccount({ id: 'first' }); const repository = new AccountsRepository(createEmptyState()); diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.ts index 0436768d5..c50e7b86e 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsRepository.ts @@ -17,6 +17,18 @@ type AccountCreationRange = { type KeyringAccountsState = Record; +/** + * Result of merging accounts into `keyringAccounts`. + * + * @param merged - The full post-merge keyring accounts state. + * @param added - The subset of incoming accounts that was actually persisted; + * conflict losers are omitted (their winners are present in `merged`). + */ +export type KeyringAccountsMergeResult = { + merged: Record; + added: Record; +}; + /** * Tron accounts use a fixed BIP-44 path template; uniqueness is entropy + index. * @@ -165,17 +177,24 @@ export class AccountsRepository { * Merges multiple keyring accounts into `keyringAccounts` in a single atomic state update. * * @param newAccounts - The new accounts to merge. + * @returns The post-merge state and the subset of accounts actually added, + * so callers can resolve persisted accounts (including conflict winners) + * without re-reading state. */ async mergeKeyringAccounts( newAccounts: Record, - ): Promise { + ): Promise { + let result: KeyringAccountsMergeResult = { merged: {}, added: {} }; + await this.#state.setKeyWith( this.#storageKey, (current) => { - const existing = current ?? {}; - return mergeAccountsWithoutIndexConflicts(existing, newAccounts).merged; + result = mergeAccountsWithoutIndexConflicts(current ?? {}, newAccounts); + return result.merged; }, ); + + return result; } async delete(id: string): Promise { diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts index b75f006e0..822835744 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts @@ -213,15 +213,24 @@ async function withAccountsService( .mockImplementation( async (newAccounts: Record) => { const occupied = new Set(keyringAccounts.map(getAccountIndexKey)); + const added: Record = {}; - for (const account of Object.values(newAccounts)) { + for (const [id, account] of Object.entries(newAccounts)) { const indexKey = getAccountIndexKey(account); if (!occupied.has(indexKey)) { keyringAccounts.push(account); occupied.add(indexKey); + added[id] = account; } } + + return { + merged: Object.fromEntries( + keyringAccounts.map((account) => [account.id, account]), + ), + added, + }; }, ), delete: jest.fn().mockImplementation(async (id: string) => { @@ -419,9 +428,10 @@ describe('AccountsService', () => { expect( mockAccountsRepository.findByEntropySourceAndRange, ).toHaveBeenCalledWith('test-entropy', { from: 0, to: 1 }); + // No post-merge re-read: the merge result is used instead. expect( mockAccountsRepository.findByEntropySourceAndRange, - ).toHaveBeenCalledTimes(2); + ).toHaveBeenCalledTimes(1); expect(mockAccountsRepository.getAll).not.toHaveBeenCalled(); expect( @@ -489,9 +499,15 @@ describe('AccountsService', () => { await withAccountsService( async ({ accountsService, mockAccountsRepository }) => { - mockAccountsRepository.findByEntropySourceAndRange - .mockResolvedValueOnce([]) - .mockResolvedValueOnce([concurrentAccount]); + // The first read sees nothing; a concurrent writer wins the merge, + // so the winner only appears in the merge result. + mockAccountsRepository.findByEntropySourceAndRange.mockResolvedValue( + [], + ); + mockAccountsRepository.mergeKeyringAccounts.mockResolvedValue({ + merged: { [concurrentAccount.id]: concurrentAccount }, + added: {}, + }); const result = await accountsService.createAccounts({ type: AccountCreationType.Bip44DeriveIndex, @@ -501,6 +517,9 @@ describe('AccountsService', () => { expect(result).toHaveLength(1); expect(result[0]?.id).toBe('concurrent-0'); + expect( + mockAccountsRepository.findByEntropySourceAndRange, + ).toHaveBeenCalledTimes(1); }, coinJson, ); @@ -550,12 +569,38 @@ describe('AccountsService', () => { expect( mockAccountsRepository.mergeKeyringAccounts, ).not.toHaveBeenCalled(); - expect(mockSnapClient.getBip32Entropy).not.toHaveBeenCalled(); + // The coin-type entropy fetch runs in parallel with the state read, + // so it happens (speculatively) even when the range already exists. + expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledTimes(1); + expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledWith({ + entropySource: 'test-entropy', + path: ['m', "44'", "195'"], + curve: 'secp256k1', + }); }, coinJson, ); }); + it('logs phase timings for a batch creation', async () => { + const coinJson = await getTronTestCoinTypeJson(); + + await withAccountsService(async ({ accountsService }) => { + await accountsService.createAccounts({ + type: AccountCreationType.Bip44DeriveIndexRange, + entropySource: 'test-entropy', + range: { from: 0, to: 1 }, + }); + + expect(mockLogger.log).toHaveBeenCalledWith( + '[🔑 AccountsService]', + expect.stringMatching( + /^\[createAccounts\] Phase timings \{.*"created":2.*"readAndEntropyMs":\d+.*"deriveMs":\d+.*"mergeMs":\d+.*"totalMs":\d+.*\}$/u, + ), + ); + }, coinJson); + }); + it('throws before storage or entropy access when the range is invalid', async () => { await withAccountsService( async ({ accountsService, mockAccountsRepository, mockSnapClient }) => { diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts index 3f2674fe8..266c6b5cd 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts @@ -384,12 +384,20 @@ export class AccountsService { } validateAccountCreationRange(range); - // Get existing accounts for the same entropy source/range to avoid duplicate state writes. - const existingAccounts = - await this.#accountsRepository.findByEntropySourceAndRange( + const startMs = Date.now(); + + // The existing-accounts read and the coin-type entropy fetch are + // independent RPCs, so overlap them. This makes the entropy fetch + // speculative when every requested index already exists, but that only + // happens on idempotent retries. + const [existingAccounts, tronAddressDeriver] = await Promise.all([ + this.#accountsRepository.findByEntropySourceAndRange( entropySource, range, - ); + ), + this.#createTronAddressDeriver(entropySource), + ]); + const readAndEntropyMs = Date.now() - startMs; const allAccounts = new Map(); for (const account of existingAccounts) { @@ -404,10 +412,11 @@ export class AccountsService { } const newAccounts: Record = {}; + let deriveMs = 0; + let mergeMs = 0; if (missingIndices.length > 0) { - const tronAddressDeriver = - await this.#createTronAddressDeriver(entropySource); + const deriveStartMs = Date.now(); for (const groupIndex of missingIndices) { const id = globalThis.crypto.randomUUID(); @@ -439,19 +448,40 @@ export class AccountsService { newAccounts[id] = tronKeyringAccount; } - await this.#accountsRepository.mergeKeyringAccounts(newAccounts); - - const persistedAccounts = - await this.#accountsRepository.findByEntropySourceAndRange( - entropySource, - range, - ); - - for (const account of persistedAccounts) { - allAccounts.set(account.index, account); + deriveMs = Date.now() - deriveStartMs; + + const mergeStartMs = Date.now(); + const { merged } = + await this.#accountsRepository.mergeKeyringAccounts(newAccounts); + mergeMs = Date.now() - mergeStartMs; + + // Resolve the persisted account for each requested index from the merge + // result: for indices lost to a concurrent writer, `merged` holds the + // winner's account rather than the one derived above. + for (const account of Object.values(merged)) { + if ( + account.entropySource === entropySource && + account.index >= range.from && + account.index <= range.to + ) { + allAccounts.set(account.index, account); + } } } + // Stringified so the values survive in the console after the snap's + // execution environment is torn down (live objects become unexpandable). + this.#logger.log( + `[createAccounts] Phase timings ${JSON.stringify({ + range, + created: missingIndices.length, + readAndEntropyMs, + deriveMs, + mergeMs, + totalMs: Date.now() - startMs, + })}`, + ); + const result: KeyringAccount[] = []; for (let groupIndex = range.from; groupIndex <= range.to; groupIndex += 1) { const account = allAccounts.get(groupIndex); From 8730c7071d77f8a975c3fb0909a9e8bbe225a20d Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 13 Aug 2026 11:17:47 +0200 Subject: [PATCH 02/18] perf(tron-wallet-snap): fetch entropy once during BIP-44 account discovery --- packages/tron-wallet-snap/CHANGELOG.md | 1 + packages/tron-wallet-snap/snap.manifest.json | 2 +- .../services/accounts/AccountsService.test.ts | 64 +++++++++++++++++++ .../src/services/accounts/AccountsService.ts | 16 ++--- 4 files changed, 73 insertions(+), 10 deletions(-) diff --git a/packages/tron-wallet-snap/CHANGELOG.md b/packages/tron-wallet-snap/CHANGELOG.md index f78aba543..dea1b3e25 100644 --- a/packages/tron-wallet-snap/CHANGELOG.md +++ b/packages/tron-wallet-snap/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Reduce BIP-44 account discovery to a single entropy fetch by reusing the coin-type deriver for the on-chain activity check ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) - Reduce extension RPC round trips in `keyring_createAccounts` from 5 to at most 4 ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) - `mergeKeyringAccounts` now returns the merge result instead of requiring a post-merge state re-read, and the existing-accounts read runs in parallel with the BIP-32 entropy fetch. - `snap_getBip32Entropy` is now called even when all requested indices already exist (this path only occurs on idempotent retries); no new permissions are required. diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index b4f24d6cd..afc1b2982 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "b0zTF4I78txypck9p+FhyttFm/3cDVM0jqOpFQl/u9E=", + "shasum": "+gro5SDzUm1zy/pTkNUPXyBQeFVQTv4jJiiXs9quzrg=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts index 822835744..91078f0d8 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts @@ -702,6 +702,70 @@ describe('AccountsService', () => { coinJson, ); }); + + it('fetches entropy once for bip44:discover, reusing the coin-type deriver for the activity check', async () => { + const coinJson = await getTronTestCoinTypeJson(); + + await withAccountsService( + async ({ + accountsService, + mockSnapClient, + mockTransactionsService, + }) => { + mockTransactionsService.checkAddressActivity.mockResolvedValueOnce( + true, + ); + + const result = await accountsService.createAccounts({ + type: AccountCreationType.Bip44Discover, + entropySource: 'test-entropy', + groupIndex: 2, + }); + + expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledTimes(1); + expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledWith({ + entropySource: 'test-entropy', + path: ['m', "44'", "195'"], + curve: 'secp256k1', + }); + + // The address probed for activity is the one persisted. + const checkedAddress = + mockTransactionsService.checkAddressActivity.mock.calls[0]?.[1]; + expect(result[0]?.address).toBe(checkedAddress); + }, + coinJson, + ); + }); + + it('fetches entropy once for bip44:discover even when no activity is found', async () => { + const coinJson = await getTronTestCoinTypeJson(); + + await withAccountsService( + async ({ + accountsService, + mockSnapClient, + mockTransactionsService, + }) => { + mockTransactionsService.checkAddressActivity.mockResolvedValue(false); + + const result = await accountsService.createAccounts({ + type: AccountCreationType.Bip44Discover, + entropySource: 'test-entropy', + groupIndex: 0, + }); + + expect(result).toStrictEqual([]); + expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledTimes(1); + expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledWith({ + entropySource: 'test-entropy', + path: ['m', "44'", "195'"], + curve: 'secp256k1', + }); + }, + coinJson, + ); + }); }); describe('create', () => { diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts index 266c6b5cd..a192a2306 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts @@ -354,18 +354,16 @@ export class AccountsService { // For discovery, only proceed if the account at groupIndex has on-chain // activity. No activity signals end-of-discovery; return [] to the client. + // The deriver created here doubles as the entropy fetch for the derivation + // below, so discovery costs a single `snap_getBip32Entropy` call. + let discoverDeriver: TronAddressDeriver | undefined; if (options.type === AccountCreationType.Bip44Discover) { const { groupIndex } = options; - const derivedAccount = await this.deriveAccount({ - entropySource, - index: groupIndex, - }); + discoverDeriver = await this.#createTronAddressDeriver(entropySource); + const { address } = await discoverDeriver(groupIndex); const activityChecks = await Promise.all( SUPPORTED_SCOPES.map((scope) => - this.#transactionsService.checkAddressActivity( - scope, - derivedAccount.address, - ), + this.#transactionsService.checkAddressActivity(scope, address), ), ); if (!activityChecks.some(Boolean)) { @@ -395,7 +393,7 @@ export class AccountsService { entropySource, range, ), - this.#createTronAddressDeriver(entropySource), + discoverDeriver ?? this.#createTronAddressDeriver(entropySource), ]); const readAndEntropyMs = Date.now() - startMs; From f729c3800ed88ee8436b520eb5afc824c2fa62ac Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 13 Aug 2026 13:14:25 +0200 Subject: [PATCH 03/18] fix(tron-wallet-snap): coalesce concurrent account synchronization runs --- packages/snap-networks-utils/CHANGELOG.md | 4 + packages/snap-networks-utils/package.json | 10 ++ .../src/dedupe/InFlightCoalescer.test.ts | 77 +++++++++++++++ .../src/dedupe/InFlightCoalescer.ts | 24 +++++ .../snap-networks-utils/src/dedupe/index.ts | 1 + packages/tron-wallet-snap/CHANGELOG.md | 6 +- packages/tron-wallet-snap/snap.manifest.json | 2 +- .../services/accounts/AccountsService.test.ts | 97 +++++++++++++++++++ .../src/services/accounts/AccountsService.ts | 22 ++++- 9 files changed, 237 insertions(+), 6 deletions(-) create mode 100644 packages/snap-networks-utils/src/dedupe/InFlightCoalescer.test.ts create mode 100644 packages/snap-networks-utils/src/dedupe/InFlightCoalescer.ts create mode 100644 packages/snap-networks-utils/src/dedupe/index.ts diff --git a/packages/snap-networks-utils/CHANGELOG.md b/packages/snap-networks-utils/CHANGELOG.md index 8c3f233d3..feb84d2a5 100644 --- a/packages/snap-networks-utils/CHANGELOG.md +++ b/packages/snap-networks-utils/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `InFlightCoalescer`, exported from a new `./dedupe` entry point, which coalesces concurrent async operations by key so callers share one in-flight run ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) + ### Changed - **BREAKING** Replace the logger utilities with a configurable `Logger` class that requires a log level and supports level filtering, per-instance prefixes, and method decorators. diff --git a/packages/snap-networks-utils/package.json b/packages/snap-networks-utils/package.json index d713604dd..edbebc452 100644 --- a/packages/snap-networks-utils/package.json +++ b/packages/snap-networks-utils/package.json @@ -32,6 +32,16 @@ "default": "./dist/index.cjs" } }, + "./dedupe": { + "import": { + "types": "./dist/dedupe/index.d.mts", + "default": "./dist/dedupe/index.mjs" + }, + "require": { + "types": "./dist/dedupe/index.d.cts", + "default": "./dist/dedupe/index.cjs" + } + }, "./logger": { "import": { "types": "./dist/logger/index.d.mts", diff --git a/packages/snap-networks-utils/src/dedupe/InFlightCoalescer.test.ts b/packages/snap-networks-utils/src/dedupe/InFlightCoalescer.test.ts new file mode 100644 index 000000000..722fb1acb --- /dev/null +++ b/packages/snap-networks-utils/src/dedupe/InFlightCoalescer.test.ts @@ -0,0 +1,77 @@ +import { InFlightCoalescer } from './InFlightCoalescer'; + +describe('InFlightCoalescer', () => { + it('returns the result of the wrapped function', async () => { + const coalescer = new InFlightCoalescer(); + + const result = await coalescer.run('key', async () => 'value'); + + expect(result).toBe('value'); + }); + + it('shares one in-flight run between concurrent callers with the same key', async () => { + const coalescer = new InFlightCoalescer(); + let resolveRun: (value: string) => void = () => undefined; + const fn = jest.fn( + async () => + new Promise((resolve) => { + resolveRun = resolve; + }), + ); + + const first = coalescer.run('key', fn); + const second = coalescer.run('key', fn); + resolveRun('shared'); + + expect(await first).toBe('shared'); + expect(await second).toBe('shared'); + expect(fn).toHaveBeenCalledTimes(1); + }); + + it('runs again once the previous run for the key has settled', async () => { + const coalescer = new InFlightCoalescer(); + const fn = jest.fn(async () => 'value'); + + await coalescer.run('key', fn); + await coalescer.run('key', fn); + + expect(fn).toHaveBeenCalledTimes(2); + }); + + it('runs concurrent callers with different keys independently', async () => { + const coalescer = new InFlightCoalescer(); + const fnA = jest.fn(async () => 'a'); + const fnB = jest.fn(async () => 'b'); + + const [resultA, resultB] = await Promise.all([ + coalescer.run('a', fnA), + coalescer.run('b', fnB), + ]); + + expect(resultA).toBe('a'); + expect(resultB).toBe('b'); + expect(fnA).toHaveBeenCalledTimes(1); + expect(fnB).toHaveBeenCalledTimes(1); + }); + + it('propagates rejections to coalesced callers and clears the entry', async () => { + const coalescer = new InFlightCoalescer(); + let rejectRun: (error: Error) => void = () => undefined; + const failing = jest.fn( + async () => + new Promise((_resolve, reject) => { + rejectRun = reject; + }), + ); + + const first = coalescer.run('key', failing); + const second = coalescer.run('key', failing); + rejectRun(new Error('boom')); + + await expect(first).rejects.toThrow('boom'); + await expect(second).rejects.toThrow('boom'); + expect(failing).toHaveBeenCalledTimes(1); + + expect(await coalescer.run('key', async () => 'ok')).toBe('ok'); + }); +}); diff --git a/packages/snap-networks-utils/src/dedupe/InFlightCoalescer.ts b/packages/snap-networks-utils/src/dedupe/InFlightCoalescer.ts new file mode 100644 index 000000000..231dcbbab --- /dev/null +++ b/packages/snap-networks-utils/src/dedupe/InFlightCoalescer.ts @@ -0,0 +1,24 @@ +/** + * Coalesces concurrent async operations by key: while a call for a key is in + * flight, subsequent calls with the same key await the same promise instead of + * starting duplicate work. Once a run settles, the next call starts a fresh one. + * + * Note that coalesced callers share the run's outcome, including rejections. + */ +export class InFlightCoalescer { + readonly #inFlight = new Map>(); + + async run(key: string, fn: () => Promise): Promise { + const pending = this.#inFlight.get(key); + if (pending) { + return pending as Promise; + } + + const task = fn().finally(() => { + this.#inFlight.delete(key); + }); + this.#inFlight.set(key, task); + + return task; + } +} diff --git a/packages/snap-networks-utils/src/dedupe/index.ts b/packages/snap-networks-utils/src/dedupe/index.ts new file mode 100644 index 000000000..600b16218 --- /dev/null +++ b/packages/snap-networks-utils/src/dedupe/index.ts @@ -0,0 +1 @@ +export { InFlightCoalescer } from './InFlightCoalescer'; diff --git a/packages/tron-wallet-snap/CHANGELOG.md b/packages/tron-wallet-snap/CHANGELOG.md index dea1b3e25..f9e9e6056 100644 --- a/packages/tron-wallet-snap/CHANGELOG.md +++ b/packages/tron-wallet-snap/CHANGELOG.md @@ -9,13 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Reduce BIP-44 account discovery to a single entropy fetch by reusing the coin-type deriver for the on-chain activity check ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) +- Reduce BIP-44 account discovery to a single entropy fetch by reusing the coin-type deriver for the on-chain activity check ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) - Reduce extension RPC round trips in `keyring_createAccounts` from 5 to at most 4 ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) - `mergeKeyringAccounts` now returns the merge result instead of requiring a post-merge state re-read, and the existing-accounts read runs in parallel with the BIP-32 entropy fetch. - `snap_getBip32Entropy` is now called even when all requested indices already exist (this path only occurs on idempotent retries); no new permissions are required. - Extract shared asset util functions and inject `SnapAssetsAdapter` from `context` into `AssetsService` ([#143](https://github.com/MetaMask/internal-snaps/pull/143)) - Rename `getByKeyringAccountId` to `getAccountAssets` (with essential-asset synthesis) and update keyring callers ([#143](https://github.com/MetaMask/internal-snaps/pull/143)) +### Fixed + +- Coalesce concurrent account synchronization runs for the same accounts so stacked triggers (cronjob and background events) share one run instead of duplicating network fetches, state writes, and keyring events ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) + ## [3.1.0] ### Added diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index afc1b2982..1c1d450ea 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "+gro5SDzUm1zy/pTkNUPXyBQeFVQTv4jJiiXs9quzrg=", + "shasum": "EAiF9pFDki9e+I9CynRfMC3MM1Q55KARtLzMy7QhpHE=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts index 91078f0d8..efda8d848 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts @@ -1480,5 +1480,102 @@ describe('AccountsService', () => { }, ); }); + + const makeSyncAccount = ( + id: string, + index: number, + ): TronKeyringAccount => ({ + id, + address: `TCoalesce${index}2345678901234567890`, + type: TrxAccountType.Eoa, + options: {}, + methods: [], + scopes: [], + entropySource: 'e1', + derivationPath: `m/44'/195'/0'/0/${index}`, + index, + }); + + it('coalesces concurrent synchronize calls for the same accounts into one run', async () => { + const account = makeSyncAccount('coalesce-id', 0); + + await withAccountsService( + async ({ + accountsService, + mockConfigProvider, + mockAssetsService, + mockTransactionsService, + }) => { + mockConfigProvider.get.mockReturnValue({ + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet], + }); + + await Promise.all([ + accountsService.synchronize([account]), + accountsService.synchronize([account]), + accountsService.synchronize([account]), + ]); + + expect( + mockAssetsService.fetchAssetsAndBalancesForAccount, + ).toHaveBeenCalledTimes(1); + expect( + mockTransactionsService.fetchNewTransactionsForAccount, + ).toHaveBeenCalledTimes(1); + expect(mockAssetsService.saveMany).toHaveBeenCalledTimes(1); + expect(mockTransactionsService.saveMany).toHaveBeenCalledTimes(1); + }, + ); + }); + + it('runs synchronize again once the previous run has finished', async () => { + const account = makeSyncAccount('sequential-id', 0); + + await withAccountsService( + async ({ accountsService, mockConfigProvider, mockAssetsService }) => { + mockConfigProvider.get.mockReturnValue({ + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet], + }); + + await accountsService.synchronize([account]); + await accountsService.synchronize([account]); + + expect( + mockAssetsService.fetchAssetsAndBalancesForAccount, + ).toHaveBeenCalledTimes(2); + }, + ); + }); + + it('does not coalesce concurrent synchronize calls for different accounts', async () => { + const accountA = makeSyncAccount('different-a', 0); + const accountB = makeSyncAccount('different-b', 1); + + await withAccountsService( + async ({ accountsService, mockConfigProvider, mockAssetsService }) => { + mockConfigProvider.get.mockReturnValue({ + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet], + }); + + await Promise.all([ + accountsService.synchronize([accountA]), + accountsService.synchronize([accountB]), + ]); + + expect( + mockAssetsService.fetchAssetsAndBalancesForAccount, + ).toHaveBeenCalledTimes(2); + expect( + mockAssetsService.fetchAssetsAndBalancesForAccount, + ).toHaveBeenCalledWith(Network.Mainnet, accountA); + expect( + mockAssetsService.fetchAssetsAndBalancesForAccount, + ).toHaveBeenCalledWith(Network.Mainnet, accountB); + }, + ); + }); }); }); diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts index a192a2306..2e0fd2ae2 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts @@ -14,6 +14,7 @@ import { emitSnapKeyringEvent, getSelectedAccounts, } from '@metamask/keyring-snap-sdk'; +import { InFlightCoalescer } from '@metamask/snap-networks-utils/dedupe'; import type { Json } from '@metamask/snaps-sdk'; import { assert } from '@metamask/superstruct'; import { hexToBytes } from '@metamask/utils'; @@ -114,6 +115,8 @@ export class AccountsService { readonly #snapClient: SnapClient; + readonly #syncCoalescer = new InFlightCoalescer(); + constructor({ accountsRepository, configProvider, @@ -597,10 +600,21 @@ export class AccountsService { } async synchronize(accounts: TronKeyringAccount[]): Promise { - await Promise.allSettled([ - this.synchronizeAssets(accounts), - this.synchronizeTransactions(accounts), - ]); + // Sync triggers stack up (60s cronjob, a background event scheduled by + // every `setSelectedAccounts` call, post-transaction refreshes), so + // concurrent invocations for the same accounts share one run instead of + // duplicating network fetches, state writes, and keyring events. + const key = accounts + .map(({ id }) => id) + .sort() + .join(','); + + await this.#syncCoalescer.run(key, async () => { + await Promise.allSettled([ + this.synchronizeAssets(accounts), + this.synchronizeTransactions(accounts), + ]); + }); } async #createTronAddressDeriver( From 345193fe7d5aef27c05adcc781d2f818f3bfa696 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 13 Aug 2026 13:54:26 +0200 Subject: [PATCH 04/18] chore(tron-wallet-snap): update shasum --- packages/tron-wallet-snap/snap.manifest.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index 1c1d450ea..43d94e6e8 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "EAiF9pFDki9e+I9CynRfMC3MM1Q55KARtLzMy7QhpHE=", + "shasum": "UqNjKwhzbfBmIKjP1Hlx/jxUnEJz61HxvTP0FJBThQ4=", "location": { "npm": { "filePath": "dist/bundle.js", From 4f00b99767c8fdb4ec2c398502805c5a54970bb1 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 12:54:12 +0200 Subject: [PATCH 05/18] refactor(tron-wallet-snap): remove unreachable v1 account-creation path --- packages/tron-wallet-snap/snap.manifest.json | 2 +- .../src/handlers/keyring/keyring.test.ts | 2 - .../services/accounts/AccountsService.test.ts | 366 +----------------- .../src/services/accounts/AccountsService.ts | 163 +------- .../src/services/accounts/types.ts | 8 - .../src/utils/getLowestUnusedIndex.ts | 44 --- 6 files changed, 4 insertions(+), 581 deletions(-) delete mode 100644 packages/tron-wallet-snap/src/services/accounts/types.ts delete mode 100644 packages/tron-wallet-snap/src/utils/getLowestUnusedIndex.ts diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index 43d94e6e8..127de4db9 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "UqNjKwhzbfBmIKjP1Hlx/jxUnEJz61HxvTP0FJBThQ4=", + "shasum": "CkbJj+1wNlwEe63xVizdwEQ2FAEwaho+WoX/gsmP9V0=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts b/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts index de310fec6..ac90d8db0 100644 --- a/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts +++ b/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts @@ -73,8 +73,6 @@ describe('KeyringHandler', () => { mockAccountsService = { findById: jest.fn().mockResolvedValue(mockAccount), findByIdOrThrow: jest.fn().mockResolvedValue(mockAccount), - deriveAccount: jest.fn(), - create: jest.fn(), createAccounts: jest.fn(), getAll: jest.fn().mockResolvedValue([mockAccount]), deriveTronKeypair: jest.fn().mockResolvedValue({ diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts index efda8d848..8282f6c29 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts @@ -8,15 +8,8 @@ import type { CreateAccountOptions as KeyringBatchCreateAccountOptions, Transaction, } from '@metamask/keyring-api'; -import { - AccountCreationType, - KeyringEvent, - TrxAccountType, -} from '@metamask/keyring-api'; -import { - emitSnapKeyringEvent, - getSelectedAccounts, -} from '@metamask/keyring-snap-sdk'; +import { AccountCreationType, TrxAccountType } from '@metamask/keyring-api'; +import { getSelectedAccounts } from '@metamask/keyring-snap-sdk'; import type { SnapClient } from '../../clients/snap/SnapClient'; import { Network } from '../../constants'; @@ -32,13 +25,9 @@ import type { AccountsRepository } from './AccountsRepository'; import { AccountsService, SUPPORTED_SCOPES } from './AccountsService'; jest.mock('@metamask/keyring-snap-sdk', () => ({ - emitSnapKeyringEvent: jest.fn(), getSelectedAccounts: jest.fn().mockResolvedValue([]), })); -const mockedEmitSnapKeyringEvent = emitSnapKeyringEvent as jest.MockedFunction< - typeof emitSnapKeyringEvent ->; const mockedGetSelectedAccounts = getSelectedAccounts as jest.MockedFunction< typeof getSelectedAccounts >; @@ -328,58 +317,6 @@ describe('AccountsService', () => { }); }); - describe('deriveAccount', () => { - it('returns TronKeyringAccount with correct structure for index 0', async () => { - await withAccountsService(async ({ accountsService, mockSnapClient }) => { - const result = await accountsService.deriveAccount({ - entropySource: 'test-entropy', - index: 0, - }); - - expect(result).toMatchObject({ - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - scopes: SUPPORTED_SCOPES, - methods: ['signMessage', 'signTransaction'], - }); - expect(result.id).toBeDefined(); - expect(typeof result.id).toBe('string'); - expect(result.address).toBeDefined(); - expect(result.address.length).toBeGreaterThan(0); - expect(result.options.entropy).toMatchObject({ - type: 'mnemonic', - id: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - groupIndex: 0, - }); - - expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledWith({ - entropySource: 'test-entropy', - path: ['m', "44'", "195'", "0'", '0', '0'], - curve: 'secp256k1', - }); - }); - }); - - it('returns correct derivation path for index 5', async () => { - await withAccountsService(async ({ accountsService, mockSnapClient }) => { - const result = await accountsService.deriveAccount({ - entropySource: 'test-entropy', - index: 5, - }); - - expect(result.derivationPath).toBe("m/44'/195'/0'/0/5"); - expect(mockSnapClient.getBip32Entropy).toHaveBeenCalledWith( - expect.objectContaining({ - path: ['m', "44'", "195'", "0'", '0', '5'], - }), - ); - }); - }); - }); - describe('deriveTronKeypair', () => { it('throws when getBip32Entropy returns missing key material', async () => { await withAccountsService(async ({ accountsService, mockSnapClient }) => { @@ -768,305 +705,6 @@ describe('AccountsService', () => { }); }); - describe('create', () => { - it('creates and persists a new account', async () => { - mockedEmitSnapKeyringEvent.mockResolvedValue(); - - await withAccountsService( - async ({ accountsService, mockAccountsRepository }) => { - jest.spyOn(accountsService, 'deriveAccount').mockResolvedValue({ - id: 'test-uuid-123', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - address: 'TTestAddress1234567890123456789', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: { - entropy: { - type: 'mnemonic', - id: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - groupIndex: 0, - }, - exportable: true, - }, - methods: ['signMessage', 'signTransaction'], - }); - - const result = await accountsService.create({ - entropySource: 'test-entropy', - index: 0, - }); - - expect(result.id).toBe('test-uuid-123'); - expect(mockAccountsRepository.create).toHaveBeenCalledWith( - expect.objectContaining({ id: 'test-uuid-123' }), - ); - expect(mockedEmitSnapKeyringEvent).toHaveBeenCalledWith( - expect.anything(), - KeyringEvent.AccountCreated, - expect.objectContaining({ - account: expect.objectContaining({ id: 'test-uuid-123' }), - }), - ); - }, - ); - }); - - it('uses default entropy source and lowest unused index when options are omitted', async () => { - mockedEmitSnapKeyringEvent.mockResolvedValue(); - - const existingAccount: TronKeyringAccount = { - id: 'existing-default-0', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - address: 'TExistingDefault0', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: {}, - methods: ['signMessage', 'signTransaction'], - }; - - await withAccountsService( - async ({ accountsService, mockAccountsRepository, mockSnapClient }) => { - mockAccountsRepository.getAll.mockResolvedValue([existingAccount]); - const deriveAccount = jest - .spyOn(accountsService, 'deriveAccount') - .mockResolvedValue({ - id: 'default-create-id', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/1", - index: 1, - type: TrxAccountType.Eoa, - address: 'TDefaultCreate1', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: { - entropy: { - type: 'mnemonic', - id: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/1", - groupIndex: 1, - }, - exportable: true, - }, - methods: ['signMessage', 'signTransaction'], - }); - - const result = await accountsService.create(); - - expect(result.id).toBe('default-create-id'); - expect(mockSnapClient.listEntropySources).toHaveBeenCalledTimes(1); - expect(deriveAccount).toHaveBeenCalledWith({ - entropySource: 'test-entropy', - index: 1, - }); - expect(mockAccountsRepository.create).toHaveBeenCalledWith( - expect.objectContaining({ id: 'default-create-id', index: 1 }), - ); - }, - ); - }); - - it('returns existing account when same derivation path exists', async () => { - const existingAccount: TronKeyringAccount = { - id: 'existing-id', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - address: 'TExisting123456789012345678901', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: {}, - methods: ['signMessage', 'signTransaction'], - }; - - await withAccountsService( - async ({ accountsService, mockAccountsRepository }) => { - mockAccountsRepository.getAll.mockResolvedValue([existingAccount]); - - const result = await accountsService.create({ - entropySource: 'test-entropy', - index: 0, - }); - - expect(result.id).toBe('existing-id'); - expect(mockAccountsRepository.create).not.toHaveBeenCalled(); - expect(mockLogger.warn).toHaveBeenCalled(); - }, - ); - }); - - it('rolls back persisted account when event emission fails', async () => { - mockedEmitSnapKeyringEvent.mockRejectedValue( - new Error('Event emission failed'), - ); - - await withAccountsService( - async ({ accountsService, mockAccountsRepository }) => { - jest.spyOn(accountsService, 'deriveAccount').mockResolvedValue({ - id: 'rollback-test-id', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - address: 'TRollback12345678901234567890', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: { - entropy: { - type: 'mnemonic', - id: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - groupIndex: 0, - }, - exportable: true, - }, - methods: ['signMessage', 'signTransaction'], - }); - - await expect( - accountsService.create({ - entropySource: 'test-entropy', - index: 0, - }), - ).rejects.toThrow('Event emission failed'); - - expect(mockAccountsRepository.create).toHaveBeenCalled(); - expect(mockAccountsRepository.delete).toHaveBeenCalledWith( - 'rollback-test-id', - ); - }, - ); - }); - - it('preserves the original error when rollback delete also fails', async () => { - mockedEmitSnapKeyringEvent.mockRejectedValue( - new Error('Event emission failed'), - ); - - await withAccountsService( - async ({ accountsService, mockAccountsRepository }) => { - mockAccountsRepository.delete.mockRejectedValue( - new Error('Delete failed'), - ); - jest.spyOn(accountsService, 'deriveAccount').mockResolvedValue({ - id: 'rollback-fail-id', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - address: 'TRollback12345678901234567890', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: { - entropy: { - type: 'mnemonic', - id: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - groupIndex: 0, - }, - exportable: true, - }, - methods: ['signMessage', 'signTransaction'], - }); - - await expect( - accountsService.create({ - entropySource: 'test-entropy', - index: 0, - }), - ).rejects.toThrow('Event emission failed'); - - expect(mockAccountsRepository.delete).toHaveBeenCalledWith( - 'rollback-fail-id', - ); - expect(mockLogger.error).toHaveBeenCalledWith( - expect.any(String), - expect.objectContaining({ accountId: 'rollback-fail-id' }), - 'Failed to rollback account creation', - ); - }, - ); - }); - - it('passes metamask options through to emit', async () => { - mockedEmitSnapKeyringEvent.mockResolvedValue(); - - await withAccountsService(async ({ accountsService }) => { - jest.spyOn(accountsService, 'deriveAccount').mockResolvedValue({ - id: 'meta-id', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - address: 'TMeta1234567890123456789012', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: {}, - methods: ['signMessage', 'signTransaction'], - }); - - await accountsService.create({ - entropySource: 'test-entropy', - index: 0, - metamask: { correlationId: 'corr-123' }, - }); - - expect(mockedEmitSnapKeyringEvent).toHaveBeenCalledWith( - expect.anything(), - KeyringEvent.AccountCreated, - expect.objectContaining({ - metamask: { correlationId: 'corr-123' }, - }), - ); - }); - }); - - it('returns the persisted account and warns when repository create returns a conflicting account', async () => { - const conflictingAccount: TronKeyringAccount = { - id: 'pre-existing-conflict-id', - entropySource: 'test-entropy', - derivationPath: "m/44'/195'/0'/0/0", - index: 0, - type: TrxAccountType.Eoa, - address: 'TConflict12345678901234567890', - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: {}, - methods: ['signMessage', 'signTransaction'], - }; - - await withAccountsService( - async ({ accountsService, mockAccountsRepository }) => { - mockAccountsRepository.create.mockResolvedValue(conflictingAccount); - - const result = await accountsService.create({ - entropySource: 'test-entropy', - index: 0, - }); - - expect(result.id).toBe('pre-existing-conflict-id'); - expect(mockLogger.warn).toHaveBeenCalled(); - }, - ); - }); - - it('throws when no primary entropy source is available', async () => { - await withAccountsService(async ({ accountsService, mockSnapClient }) => { - mockSnapClient.listEntropySources.mockResolvedValue([ - { - id: 'non-primary', - primary: false, - type: 'mnemonic', - name: 'Non-Primary', - }, - ]); - - await expect(accountsService.create()).rejects.toThrow( - 'No default entropy source found', - ); - }); - }); - }); - describe('getAll', () => { it('delegates to repository and returns result', async () => { const accounts: TronKeyringAccount[] = [ diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts index 2e0fd2ae2..c664b2e9b 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts @@ -7,15 +7,10 @@ import type { import { AccountCreationType, assertCreateAccountOptionIsSupported, - KeyringEvent, TrxAccountType, } from '@metamask/keyring-api'; -import { - emitSnapKeyringEvent, - getSelectedAccounts, -} from '@metamask/keyring-snap-sdk'; +import { getSelectedAccounts } from '@metamask/keyring-snap-sdk'; import { InFlightCoalescer } from '@metamask/snap-networks-utils/dedupe'; -import type { Json } from '@metamask/snaps-sdk'; import { assert } from '@metamask/superstruct'; import { hexToBytes } from '@metamask/utils'; import { computeAddress } from 'ethers'; @@ -28,7 +23,6 @@ import { asStrictKeyringAccount } from '../../entities/keyring-account'; import type { TronKeyringAccount } from '../../entities/keyring-account'; import { createTronBip44AddressDeriver } from '../../utils/deriveTronFromCoinTypeNode'; import { sanitizeSensitiveError } from '../../utils/errors'; -import { getLowestUnusedIndex } from '../../utils/getLowestUnusedIndex'; import { createPrefixedLogger } from '../../utils/logger'; import type { ILogger } from '../../utils/logger'; import { DerivationPathStruct } from '../../validation/structs'; @@ -36,7 +30,6 @@ import type { AssetsService } from '../assets/AssetsService'; import type { ConfigProvider } from '../config'; import type { TransactionsService } from '../transactions/TransactionsService'; import type { AccountsRepository } from './AccountsRepository'; -import type { CreateAccountOptions } from './types'; /** * Elliptic curve for TRON (same as Ethereum) @@ -204,136 +197,6 @@ export class AccountsService { } } - async deriveAccount({ - entropySource, - index, - }: { - entropySource: EntropySourceId; - index: number; - }): Promise { - const derivationPath = AccountsService.getDefaultDerivationPath(index); - const { address } = await this.deriveTronKeypair({ - entropySource, - derivationPath, - }); - - return { - id: globalThis.crypto.randomUUID(), - entropySource, - derivationPath, - index, - type: TrxAccountType.Eoa, - address, - scopes: SUPPORTED_SCOPES as unknown as Network[], - options: { - entropy: { - type: 'mnemonic', - id: entropySource, - derivationPath, - groupIndex: index, - }, - exportable: true, - }, - methods: ['signMessage', 'signTransaction'], - }; - } - - async create(options?: CreateAccountOptions): Promise { - const accounts = await this.#accountsRepository.getAll(); - - const entropySource = - options?.entropySource ?? (await this.#getDefaultEntropySource()); - const index = - options?.index ?? - this.#getLowestUnusedKeyringAccountIndex(accounts, entropySource); - - /** - * Now that we have the `entropySource` and `index` ready, - * we need to make sure that they do not correspond to an existing account already. - */ - const sameAccount = accounts.find( - (account) => - account.index === index && account.entropySource === entropySource, - ); - - if (sameAccount) { - this.#logger.warn( - '[🔑 Keyring] An account already exists with the same derivation path and entropy source. Skipping account creation.', - ); - return asStrictKeyringAccount(sameAccount); - } - - const derivedAccount = await this.deriveAccount({ - entropySource, - index, - }); - - const { metamask: metamaskOptions, ...remainingOptions } = options ?? {}; - - const tronKeyringAccount: TronKeyringAccount = { - ...derivedAccount, - options: { - ...derivedAccount.options, - ...(Object.fromEntries( - Object.entries(remainingOptions).filter( - ([, value]) => value !== undefined, - ), - ) as Record), - groupIndex: index, - }, - }; - - const persistedAccount = - await this.#accountsRepository.create(tronKeyringAccount); - - if (persistedAccount.id !== tronKeyringAccount.id) { - this.#logger.warn( - '[🔑 Keyring] An account already exists with the same derivation path and entropy source. Skipping account creation.', - ); - return asStrictKeyringAccount(persistedAccount); - } - - try { - const keyringAccount = asStrictKeyringAccount(tronKeyringAccount); - - await emitSnapKeyringEvent(snap, KeyringEvent.AccountCreated, { - /** - * We can't pass the `keyringAccount` object because it contains the index - * and the snaps sdk does not allow extra properties. - */ - account: keyringAccount, - /** - * Skip account creation confirmation dialogs to make it look like a native - * account creation flow. - */ - displayConfirmation: false, - /** - * Internal options to MetaMask that includes a correlation ID. We need - * to also emit this ID to the Snap keyring. - */ - ...(metamaskOptions - ? { - metamask: metamaskOptions, - } - : {}), - }); - - return keyringAccount; - } catch (error) { - // Rollback: if the event emission fails after the account was persisted, - // remove it from state so we don't end up with an orphaned record. - try { - await this.#accountsRepository.delete(tronKeyringAccount.id); - } catch (deleteError) { - this.#logger.error( - { deleteError, accountId: tronKeyringAccount.id }, - 'Failed to rollback account creation', - ); - } - throw error; - } - } - /** * Batch-creates Tron accounts for a BIP-44 index or index range. Existing accounts for the * same entropy source and index are returned without duplicate state writes. @@ -629,31 +492,7 @@ export class AccountsService { return createTronBip44AddressDeriver(bip44Node); } - #getLowestUnusedKeyringAccountIndex( - accounts: TronKeyringAccount[], - entropySource: EntropySourceId, - ): number { - const accountsFilteredByEntropySourceId = accounts.filter( - (account) => account.entropySource === entropySource, - ); - - return getLowestUnusedIndex(accountsFilteredByEntropySourceId); - } - static getDefaultDerivationPath(index: number): `m/${string}` { return `m/44'/195'/0'/0/${index}`; } - - async #getDefaultEntropySource(): Promise { - const entropySources = await this.#snapClient.listEntropySources(); - const defaultEntropySource = entropySources.find(({ primary }) => primary); - - if (!defaultEntropySource) { - throw new Error( - 'No default entropy source found - this can never happen', - ); - } - - return defaultEntropySource.id; - } } diff --git a/packages/tron-wallet-snap/src/services/accounts/types.ts b/packages/tron-wallet-snap/src/services/accounts/types.ts deleted file mode 100644 index af4ed23d2..000000000 --- a/packages/tron-wallet-snap/src/services/accounts/types.ts +++ /dev/null @@ -1,8 +0,0 @@ -import type { EntropySourceId, MetaMaskOptions } from '@metamask/keyring-api'; -import type { Json } from '@metamask/snaps-sdk'; - -export type CreateAccountOptions = { - entropySource?: EntropySourceId; - index?: number; - [key: string]: Json | undefined; -} & MetaMaskOptions; diff --git a/packages/tron-wallet-snap/src/utils/getLowestUnusedIndex.ts b/packages/tron-wallet-snap/src/utils/getLowestUnusedIndex.ts deleted file mode 100644 index fc15d695a..000000000 --- a/packages/tron-wallet-snap/src/utils/getLowestUnusedIndex.ts +++ /dev/null @@ -1,44 +0,0 @@ -export type WithIndex = { - index: number; -}; - -/** - * Generating a new index for the KeyringAccount is not as straightforward as one might think. - * We cannot assume that this number will continuosly increase because one can delete an account with - * an index in the middle of the list. The right way to do it is to loop through the keyringAccounts - * and get the lowest index that is not yet used. - * - * This function does precisely that, in a generic way, as it can work with any array of items that - * have a field `index`. - * - * Eg: - * Used Indices: [] -> Lowest is 0. - * Used Indices: [0, 1, 2, 4] -> Lowest is 3. - * - * @param items - The items to check. - * @returns The lowest unused index. - */ -export function getLowestUnusedIndex(items: WithIndex[]): number { - if (items.length === 0) { - return 0; - } - - const usedIndices = items - .map((item) => item.index) - .sort((first, second) => first - second); - - let lowestUnusedIndex = 0; - - for (const usedIndex of usedIndices) { - /** - * From lower to higher, the moment we find a gap, we can use it - */ - if (usedIndex !== lowestUnusedIndex) { - break; - } - - lowestUnusedIndex += 1; - } - - return lowestUnusedIndex; -} From 183bc251de0e67889cba8dcfb61fbe909d5d784a Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 12:54:12 +0200 Subject: [PATCH 06/18] chore(tron-wallet-snap): update shasum --- packages/tron-wallet-snap/snap.manifest.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index 127de4db9..1ec3d2bc2 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "CkbJj+1wNlwEe63xVizdwEQ2FAEwaho+WoX/gsmP9V0=", + "shasum": "b8h4FBAtY+QB079E+s6C+5/ZWAGA18CNAHwwvotJy3k=", "location": { "npm": { "filePath": "dist/bundle.js", From 90cc05d303084469c0295caa2d4129cb433d6ca8 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 13:08:48 +0200 Subject: [PATCH 07/18] fix(tron-wallet-snap): remove AccountDeleted emission that broke v2 account deletion --- packages/tron-wallet-snap/CHANGELOG.md | 2 ++ packages/tron-wallet-snap/snap.manifest.json | 2 +- .../src/handlers/keyring/keyring.test.ts | 27 +++++++++++++++++++ .../src/handlers/keyring/keyring.ts | 15 ++++------- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/packages/tron-wallet-snap/CHANGELOG.md b/packages/tron-wallet-snap/CHANGELOG.md index f9e9e6056..d13e1e0c3 100644 --- a/packages/tron-wallet-snap/CHANGELOG.md +++ b/packages/tron-wallet-snap/CHANGELOG.md @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from `keyring_deleteAccount` ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) + - 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. - Coalesce concurrent account synchronization runs for the same accounts so stacked triggers (cronjob and background events) share one run instead of duplicating network fetches, state writes, and keyring events ([#149](https://github.com/MetaMask/internal-snaps/pull/149)) ## [3.1.0] diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index 1ec3d2bc2..fcea9f3e5 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "b8h4FBAtY+QB079E+s6C+5/ZWAGA18CNAHwwvotJy3k=", + "shasum": "KQqXlTpbyKV2yXqWLYKmcmFRQhM5U0us9/1QkZQJOZM=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts b/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts index ac90d8db0..7817f8e2c 100644 --- a/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts +++ b/packages/tron-wallet-snap/src/handlers/keyring/keyring.test.ts @@ -74,6 +74,7 @@ describe('KeyringHandler', () => { findById: jest.fn().mockResolvedValue(mockAccount), findByIdOrThrow: jest.fn().mockResolvedValue(mockAccount), createAccounts: jest.fn(), + delete: jest.fn().mockResolvedValue(undefined), getAll: jest.fn().mockResolvedValue([mockAccount]), deriveTronKeypair: jest.fn().mockResolvedValue({ privateKeyHex: 'a'.repeat(64), @@ -559,6 +560,32 @@ describe('KeyringHandler', () => { }); }); + describe('deleteAccount', () => { + it('deletes the account without emitting keyring events', async () => { + await keyringHandler.deleteAccount(mockAccount.id); + + expect(mockAccountsService.delete).toHaveBeenCalledWith(mockAccount.id); + }); + + it('throws for an invalid account id', async () => { + await expect(keyringHandler.deleteAccount('not-a-uuid')).rejects.toThrow( + expect.anything(), + ); + + expect(mockAccountsService.delete).not.toHaveBeenCalled(); + }); + + it('throws when the account does not exist', async () => { + mockAccountsService.findById.mockResolvedValue(null); + + await expect( + keyringHandler.deleteAccount(mockAccount.id), + ).rejects.toThrow(`Account "${mockAccount.id}" not found`); + + expect(mockAccountsService.delete).not.toHaveBeenCalled(); + }); + }); + describe('createAccounts', () => { it('delegates to accountsService.createAccounts and returns the result', async () => { const createdAccounts = [ diff --git a/packages/tron-wallet-snap/src/handlers/keyring/keyring.ts b/packages/tron-wallet-snap/src/handlers/keyring/keyring.ts index 710278d18..02c1c27eb 100644 --- a/packages/tron-wallet-snap/src/handlers/keyring/keyring.ts +++ b/packages/tron-wallet-snap/src/handlers/keyring/keyring.ts @@ -1,7 +1,4 @@ -import { - KeyringEvent, - ListAccountAssetsResponseStruct, -} from '@metamask/keyring-api'; +import { ListAccountAssetsResponseStruct } from '@metamask/keyring-api'; import type { Balance, CreateAccountOptions as KeyringBatchCreateAccountOptions, @@ -16,7 +13,6 @@ import type { ExportedAccount, KeyringSnapRpc, } from '@metamask/keyring-api/v2'; -import { emitSnapKeyringEvent } from '@metamask/keyring-snap-sdk'; import { handleKeyringRequest } from '@metamask/keyring-snap-sdk/v2'; import { InvalidParamsError, @@ -381,12 +377,11 @@ export class KeyringHandler implements KeyringSnapRpc { try { validateRequest({ accountId }, DeleteAccountStruct); - const account = await this.#getAccountOrThrow(accountId); - - await emitSnapKeyringEvent(snap, KeyringEvent.AccountDeleted, { - id: account.id, - }); + await this.#getAccountOrThrow(accountId); + // 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.#accountsService.delete(accountId); } catch (error: unknown) { this.#logger.error({ error }, 'Error deleting account'); From 534a6b23608f01fb6e64426dd332ef6df9687a8a Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 13:25:19 +0200 Subject: [PATCH 08/18] chore(tron-wallet-snap): update shasum --- packages/tron-wallet-snap/snap.manifest.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tron-wallet-snap/snap.manifest.json b/packages/tron-wallet-snap/snap.manifest.json index fcea9f3e5..5630cb017 100644 --- a/packages/tron-wallet-snap/snap.manifest.json +++ b/packages/tron-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "KQqXlTpbyKV2yXqWLYKmcmFRQhM5U0us9/1QkZQJOZM=", + "shasum": "FFwqtuyYlJM+7NFbnb+sYAhnjlQOQUB2Rbhfx0IZkmM=", "location": { "npm": { "filePath": "dist/bundle.js", From 47373ac0b37e506ad5761897163513d6c36f8468 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 14:50:34 +0200 Subject: [PATCH 09/18] fix(bitcoin-wallet-snap): remove AccountDeleted emission that broke v2 account deletion --- packages/bitcoin-wallet-snap/CHANGELOG.md | 5 +++++ .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../src/use-cases/AccountUseCases.test.ts | 22 ++----------------- .../src/use-cases/AccountUseCases.ts | 4 +++- 4 files changed, 11 insertions(+), 22 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index b4baf62a3..c24ab6d10 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) + - 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. + ## [2.0.1] ### Fixed diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index f594f203d..46131e691 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": "jV649WZbbfbj3FpOMD5U/xDPuRD0t4F+pxCoy08a/O0=", + "shasum": "J2jGMtTEEMTrCO+l8Unhq67K6KGtQoBQ31KrASKVq3Q=", "location": { "npm": { "filePath": "dist/bundle.js", 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 694d55691..d80930dda 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -936,7 +936,7 @@ describe('AccountUseCases', () => { 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'; @@ -945,25 +945,8 @@ 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(); + expect(mockSnapClient.emitAccountDeletedEvent).not.toHaveBeenCalled(); }); it('propagates an error if the repository fails', async () => { @@ -977,7 +960,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 85c8b34d9..d7054fc8f 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -516,7 +516,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); From 9bd9de0e41226db18b630dc90ca05e714acff8c3 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 14:53:49 +0200 Subject: [PATCH 10/18] refactor(bitcoin-wallet-snap): remove unreachable v1 account-creation path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AccountUseCases.create had no production caller (KeyringHandler routes keyring_createAccounts through createMany/discover) and emitted the v1 AccountCreated lifecycle event after persisting, with no rollback — a latent orphaned-account bug against keyring v2 clients. Removes create and the now-unused emitAccountCreatedEvent/emitAccountDeletedEvent from SnapClientAdapter and the SnapClient interface. --- .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../bitcoin-wallet-snap/src/entities/snap.ts | 19 -- .../src/infra/SnapClientAdapter.ts | 22 +-- .../src/use-cases/AccountUseCases.test.ts | 182 ------------------ .../src/use-cases/AccountUseCases.ts | 56 ------ 5 files changed, 2 insertions(+), 279 deletions(-) diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 46131e691..6499a95c8 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": "J2jGMtTEEMTrCO+l8Unhq67K6KGtQoBQ31KrASKVq3Q=", + "shasum": "G9peFWO/hWfyozM0KoXS8LuMQd6caw4xxJIbtFM5Nz0=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/bitcoin-wallet-snap/src/entities/snap.ts b/packages/bitcoin-wallet-snap/src/entities/snap.ts index e3a545ad2..1cdeaf354 100644 --- a/packages/bitcoin-wallet-snap/src/entities/snap.ts +++ b/packages/bitcoin-wallet-snap/src/entities/snap.ts @@ -89,25 +89,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/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/use-cases/AccountUseCases.test.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts index d80930dda..5d15126d5 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', @@ -335,7 +160,6 @@ describe('AccountUseCases', () => { ); expect(newAccount.revealNextAddress).toHaveBeenCalled(); expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ duration: 'PT1S', method: CronMethod.FullScanAccount, @@ -355,7 +179,6 @@ describe('AccountUseCases', () => { ]); expect(mockRepository.create).toHaveBeenCalledTimes(1); expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); expect(result).toStrictEqual([newAccount, newAccount]); }); @@ -366,7 +189,6 @@ describe('AccountUseCases', () => { expect(mockRepository.create).not.toHaveBeenCalled(); expect(mockRepository.insertMany).not.toHaveBeenCalled(); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); expect(result).toStrictEqual([existingAccount]); }); @@ -379,7 +201,6 @@ describe('AccountUseCases', () => { 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 () => { @@ -427,7 +248,6 @@ describe('AccountUseCases', () => { await settlementObserver; expect(callOrder).toStrictEqual(['create-1', 'create-2', 'resolve-2']); expect(mockRepository.insertMany).not.toHaveBeenCalled(); - expect(mockSnapClient.emitAccountCreatedEvent).not.toHaveBeenCalled(); }); }); @@ -932,7 +752,6 @@ describe('AccountUseCases', () => { ); expect(mockRepository.get).toHaveBeenCalledWith('non-existent-id'); - expect(mockSnapClient.emitAccountDeletedEvent).not.toHaveBeenCalled(); expect(mockRepository.delete).not.toHaveBeenCalled(); }); @@ -946,7 +765,6 @@ describe('AccountUseCases', () => { expect(mockRepository.get).toHaveBeenCalledWith(mockAccount.id); expect(mockRepository.delete).toHaveBeenCalledWith(mockAccount.id); - expect(mockSnapClient.emitAccountDeletedEvent).not.toHaveBeenCalled(); }); it('propagates an error if the repository fails', async () => { diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index d7054fc8f..1a2bd622f 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -239,62 +239,6 @@ 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 []; From d32e4e38d9d3003c7d81efad56d4888b5d571919 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 14:56:43 +0200 Subject: [PATCH 11/18] fix(bitcoin-wallet-snap): coalesce concurrent account synchronization runs --- packages/bitcoin-wallet-snap/CHANGELOG.md | 1 + .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../src/handlers/CronHandler.test.ts | 63 ++++++++ .../src/handlers/CronHandler.ts | 143 ++++++++++-------- 4 files changed, 147 insertions(+), 62 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index c24ab6d10..5b6c0a0ac 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) - Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) - 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. diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 6499a95c8..0aa47f27f 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": "G9peFWO/hWfyozM0KoXS8LuMQd6caw4xxJIbtFM5Nz0=", + "shasum": "h3BgnAqj0gKCVGEDN4Nk8qbuYXJxG8pFSRBgnIH6ydg=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index 224e9bf12..7e1d004d0 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -288,6 +288,69 @@ 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('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 f034158ff..1c15fd323 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'; @@ -34,6 +35,8 @@ export class CronHandler { readonly #snap: SnapsProvider; + readonly #syncCoalescer = new InFlightCoalescer(); + constructor( accounts: AccountUseCases, sendFlow: SendFlowUseCases, @@ -76,83 +79,101 @@ 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 key = `syncSelectedAccounts:${[...accountIds].sort().join(',')}`; + + await this.#syncCoalescer.run(key, async () => { + const accountIdSet = new Set(accountIds); + 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); + }); } /** From a696e89a0bb903eeac904a5e78465d8321f6c51a Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 15:03:15 +0200 Subject: [PATCH 12/18] perf(bitcoin-wallet-snap): fetch entropy once per parent path in batch account creation --- packages/bitcoin-wallet-snap/CHANGELOG.md | 6 ++ .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../src/entities/account.ts | 15 ++++ .../src/store/BdkAccountRepository.test.ts | 80 +++++++++++++++++ .../src/store/BdkAccountRepository.ts | 73 ++++++++++++++++ .../src/use-cases/AccountUseCases.test.ts | 76 +++++----------- .../src/use-cases/AccountUseCases.ts | 87 ++++--------------- 7 files changed, 217 insertions(+), 122 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 5b6c0a0ac..1692e7aab 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) + - 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. + ### 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 0aa47f27f..d07a55406 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": "h3BgnAqj0gKCVGEDN4Nk8qbuYXJxG8pFSRBgnIH6ydg=", + "shasum": "cHjMfkPDgwL2RIrBHjqHoO14Q2fHNKHACiQ6gC5EuyQ=", "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 f8d27eee2..c3c002b8f 100644 --- a/packages/bitcoin-wallet-snap/src/entities/account.ts +++ b/packages/bitcoin-wallet-snap/src/entities/account.ts @@ -293,6 +293,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. * diff --git a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts index e909e902a..70848a624 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 { + mnemonicPhraseToBytes, + SLIP10Node as RealSlip10Node, +} from '@metamask/key-tree'; import { mock } from 'jest-mock-extended'; import type { @@ -360,6 +365,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 { + return RealSlip10Node.fromDerivationPath({ + derivationPath: [ + mnemonicPhraseToBytes(mnemonic), + ...segments.map((segment) => `bip32:${segment}` as const), + ], + 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( diff --git a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts index 44d1232db..ae5070947 100644 --- a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts +++ b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts @@ -8,6 +8,7 @@ import { xpriv_to_descriptor, xpub_to_descriptor, } from '@metamask/bitcoindevkit'; +import { SLIP10Node } from '@metamask/key-tree'; import { v4 } from 'uuid'; import { StorageError } from '../entities'; @@ -218,6 +219,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, 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 5d15126d5..891abf65e 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -142,7 +142,7 @@ describe('AccountUseCases', () => { existingAccount, null, ]); - mockRepository.create.mockResolvedValue(newAccount); + mockRepository.createMany.mockResolvedValue([newAccount]); const result = await useCases.createMany([ createParams, @@ -153,11 +153,13 @@ 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.scheduleBackgroundEvent).toHaveBeenCalledWith({ @@ -170,14 +172,21 @@ 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.createMany.mockResolvedValue([newAccount]); const result = await useCases.createMany([createParams, createParams]); expect(mockRepository.getByDerivationPaths).toHaveBeenCalledWith([ firstDerivationPath, ]); - expect(mockRepository.create).toHaveBeenCalledTimes(1); + expect(mockRepository.createMany).toHaveBeenCalledTimes(1); + expect(mockRepository.createMany).toHaveBeenCalledWith([ + { + derivationPath: firstDerivationPath, + network: createParams.network, + addressType: createParams.addressType, + }, + ]); expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); expect(result).toStrictEqual([newAccount, newAccount]); }); @@ -187,7 +196,7 @@ describe('AccountUseCases', () => { const result = await useCases.createMany([createParams]); - expect(mockRepository.create).not.toHaveBeenCalled(); + expect(mockRepository.createMany).not.toHaveBeenCalled(); expect(mockRepository.insertMany).not.toHaveBeenCalled(); expect(result).toStrictEqual([existingAccount]); }); @@ -195,7 +204,7 @@ describe('AccountUseCases', () => { 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.createMany.mockResolvedValue([newAccount]); mockRepository.insertMany.mockRejectedValue(error); await expect(useCases.createMany([createParams])).rejects.toBe(error); @@ -203,50 +212,13 @@ describe('AccountUseCases', () => { expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); }); - 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(); + it('propagates createMany errors without inserting accounts', async () => { + const error = new Error('createMany failed'); + mockRepository.getByDerivationPaths.mockResolvedValue([null]); + 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(); }); }); diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index 1a2bd622f..f32c16d33 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. * @@ -279,19 +223,24 @@ 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; - }, - ); + + // 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 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(); + } if (newAccounts.length > 0) { await this.#repository.insertMany(newAccounts); From 86b6ca02a721d040ccdefcef1c44f53789f0023c Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 15:10:53 +0200 Subject: [PATCH 13/18] perf(bitcoin-wallet-snap): reuse lookup state snapshot for batch inserts --- packages/bitcoin-wallet-snap/CHANGELOG.md | 1 + .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../src/entities/account.ts | 20 +++-- .../bitcoin-wallet-snap/src/entities/snap.ts | 9 ++ .../src/store/BdkAccountRepository.test.ts | 80 +++++++++++++++++- .../src/store/BdkAccountRepository.ts | 83 ++++++++++++------- .../src/use-cases/AccountUseCases.test.ts | 47 ++++++++--- .../src/use-cases/AccountUseCases.ts | 11 ++- 8 files changed, 200 insertions(+), 53 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 1692e7aab..377f86c0d 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) - 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) ### Fixed diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index d07a55406..500180380 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": "cHjMfkPDgwL2RIrBHjqHoO14Q2fHNKHACiQ6gC5EuyQ=", + "shasum": "vpB/+U/doryeMNdiK+g9SQ2o+ZTR7eMQ9X8sc2WncFE=", "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 c3c002b8f..dd0908715 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'; /** @@ -273,11 +274,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 state 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. @@ -319,8 +323,14 @@ export type BitcoinAccountRepository = { * Insert accounts. * * @param accounts - Bitcoin accounts. + * @param snapshot - Optional state snapshot (from `getByDerivationPaths`) + * to merge into, avoiding a redundant state read. Only safe when the + * snapshot was taken within the same account mutation. */ - insertMany(accounts: BitcoinAccount[]): Promise; + 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 1cdeaf354..45f545aca 100644 --- a/packages/bitcoin-wallet-snap/src/entities/snap.ts +++ b/packages/bitcoin-wallet-snap/src/entities/snap.ts @@ -17,6 +17,15 @@ export type SnapState = { derivationPaths: Record; }; +/** + * In-memory snapshot of the account maps, as loaded from state. Lets a lookup + * and a subsequent insert within the same account mutation share one read. + */ +export type AccountStateSnapshot = { + accounts: SnapState['accounts'] | null; + derivationPaths: SnapState['derivationPaths'] | null; +}; + export type AccountState = { // Split derivation path. derivationPath: string[]; diff --git a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts index 70848a624..0e703d5bd 100644 --- a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts +++ b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts @@ -245,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(); }); @@ -270,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...'); @@ -301,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 () => { @@ -316,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', }); @@ -610,6 +625,63 @@ describe('BdkAccountRepository', () => { }, ); }); + + it('merges into a provided snapshot without re-reading state', async () => { + const existingAccountState: AccountState = { + wallet: mockWalletData, + inscriptions: [], + derivationPath: mockDerivationPath, + }; + 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'", + ]); + + await repo.insertMany([account1, account2], { + accounts: { 'existing-id': existingAccountState }, + derivationPaths: { "m/84'/0'/0'": 'existing-id' }, + }); + + expect(mockSnapClient.getState).not.toHaveBeenCalled(); + expect(mockSnapClient.setState).toHaveBeenCalledWith( + 'accounts', + expect.objectContaining({ + 'existing-id': existingAccountState, + '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 ae5070947..83ff51d40 100644 --- a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts +++ b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.ts @@ -13,6 +13,7 @@ import { v4 } from 'uuid'; import { StorageError } from '../entities'; import type { + AccountStateSnapshot, BitcoinAccountRepository, BitcoinAccount, SnapClient, @@ -118,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([ @@ -167,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 { @@ -330,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 []; } @@ -363,24 +377,37 @@ export class BdkAccountRepository implements BitcoinAccountRepository { 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), - }); + // Reuse the caller's snapshot when provided (safe within the same account + // mutation) instead of re-reading state that was just loaded. + let existingAccounts: SnapState['accounts'] | null; + let existingDerivationPaths: SnapState['derivationPaths'] | null; + if (snapshot) { + ({ + accounts: existingAccounts, + derivationPaths: existingDerivationPaths, + } = snapshot); + } else { + [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('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 891abf65e..a2edfa973 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -136,12 +136,16 @@ 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.getByDerivationPaths.mockResolvedValue({ + accounts: [existingAccount, null], + snapshot: mockSnapshot, + }); mockRepository.createMany.mockResolvedValue([newAccount]); const result = await useCases.createMany([ @@ -161,7 +165,10 @@ describe('AccountUseCases', () => { }, ]); expect(newAccount.revealNextAddress).toHaveBeenCalled(); - expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); + expect(mockRepository.insertMany).toHaveBeenCalledWith( + [newAccount], + mockSnapshot, + ); expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({ duration: 'PT1S', method: CronMethod.FullScanAccount, @@ -171,7 +178,10 @@ describe('AccountUseCases', () => { }); it('creates only one account for duplicate derivation paths in the same batch', async () => { - mockRepository.getByDerivationPaths.mockResolvedValue([null]); + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [null], + snapshot: mockSnapshot, + }); mockRepository.createMany.mockResolvedValue([newAccount]); const result = await useCases.createMany([createParams, createParams]); @@ -187,12 +197,18 @@ describe('AccountUseCases', () => { addressType: createParams.addressType, }, ]); - expect(mockRepository.insertMany).toHaveBeenCalledWith([newAccount]); + 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]); @@ -203,18 +219,27 @@ describe('AccountUseCases', () => { it('propagates insertMany errors without emitting account-created events', async () => { const error = new Error('insertMany failed'); - mockRepository.getByDerivationPaths.mockResolvedValue([null]); + 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(mockRepository.insertMany).toHaveBeenCalledWith( + [newAccount], + mockSnapshot, + ); }); it('propagates createMany errors without inserting accounts', async () => { const error = new Error('createMany failed'); - mockRepository.getByDerivationPaths.mockResolvedValue([null]); + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [null], + snapshot: mockSnapshot, + }); mockRepository.createMany.mockRejectedValue(error); await expect(useCases.createMany([createParams])).rejects.toBe(error); diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index f32c16d33..003e1db98 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -208,9 +208,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) => { @@ -243,7 +244,9 @@ export class AccountUseCases { } if (newAccounts.length > 0) { - await this.#repository.insertMany(newAccounts); + // Reuse the lookup's state snapshot: we're inside the account + // mutation, so it cannot have been changed by another creation. + await this.#repository.insertMany(newAccounts, snapshot); } const newAccountsByPath = new Map( From 56156d556717695e212126017b8152255bdfe8a1 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 15:15:06 +0200 Subject: [PATCH 14/18] perf(bitcoin-wallet-snap): create requested account ranges in a single batch --- packages/bitcoin-wallet-snap/CHANGELOG.md | 1 + .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../src/handlers/KeyringHandler.test.ts | 12 ++---- .../src/handlers/KeyringHandler.ts | 40 +++++++------------ 4 files changed, 21 insertions(+), 34 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 377f86c0d..9e7210c76 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) +- 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) ### Fixed diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 500180380..272a02503 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": "vpB/+U/doryeMNdiK+g9SQ2o+ZTR7eMQ9X8sc2WncFE=", + "shasum": "UzUpj/8XwS1xZjmLAKEtQsU2x2ncsB+i6yqxMMBZoOQ=", "location": { "npm": { "filePath": "dist/bundle.js", 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; From fc32de4c4ebaca232a057283f03ab9fc649f3f15 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Fri, 14 Aug 2026 15:19:06 +0200 Subject: [PATCH 15/18] chore(bitcoin-wallet-snap): log phase timings for batch account creation --- .../bitcoin-wallet-snap/snap.manifest.json | 2 +- .../src/use-cases/AccountUseCases.test.ts | 16 ++++++++++++++ .../src/use-cases/AccountUseCases.ts | 21 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 272a02503..6979633ed 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": "UzUpj/8XwS1xZjmLAKEtQsU2x2ncsB+i6yqxMMBZoOQ=", + "shasum": "DMPoQCJAx2c68fD+TPI9BjxZT0xE7aYvaFVzpGgOQZw=", "location": { "npm": { "filePath": "dist/bundle.js", 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 a2edfa973..957adbbc7 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -234,6 +234,22 @@ describe('AccountUseCases', () => { ); }); + it('logs phase timings for a batch creation', async () => { + mockRepository.getByDerivationPaths.mockResolvedValue({ + accounts: [null], + snapshot: mockSnapshot, + }); + mockRepository.createMany.mockResolvedValue([newAccount]); + + await useCases.createMany([createParams]); + + expect(mockLogger.info).toHaveBeenCalledWith( + expect.stringMatching( + /^\[createMany\] Phase timings \{.*"requested":1.*"created":1.*"lookupMs":\d+.*"deriveMs":\d+.*"persistMs":\d+.*"totalMs":\d+.*\}$/u, + ), + ); + }); + it('propagates createMany errors without inserting accounts', async () => { const error = new Error('createMany failed'); mockRepository.getByDerivationPaths.mockResolvedValue({ diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index 003e1db98..04629beba 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -188,8 +188,11 @@ export class AccountUseCases { 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 { @@ -224,10 +227,12 @@ export class AccountUseCases { const entriesToCreate = uniqueEntries.filter( ({ pathKey }) => !existingAccountsByPath.has(pathKey), ); + 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( @@ -242,12 +247,28 @@ export class AccountUseCases { for (const newAccount of newAccounts) { newAccount.revealNextAddress(); } + const deriveMs = Date.now() - deriveStartMs; + const persistStartMs = Date.now(); if (newAccounts.length > 0) { // Reuse the lookup's state snapshot: we're inside the account // mutation, so it cannot have been changed by another creation. 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) => [ From e9ed5fdb2a3669445bfa6d691f0d18737b5ef7ef Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 27 Aug 2026 12:01:47 +0200 Subject: [PATCH 16/18] chore(bitcoin-wallet-snap): update shasum --- packages/bitcoin-wallet-snap/snap.manifest.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index d59b88aac..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": "sYefpN30aR0fb7v2DtdJ+jNFSnJJX5jtqvdsDof4RHQ=", + "shasum": "ZEMZtj4BZq+pj3bzb81K1Vk5WQuqMA0NArfIhrIppds=", "location": { "npm": { "filePath": "dist/bundle.js", From a4745260508ec71b1f98b61820f25c4a6ce94f01 Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 27 Aug 2026 12:03:15 +0200 Subject: [PATCH 17/18] fix(bitcoin-wallet-snap): lint fix --- .../src/store/BdkAccountRepository.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts index 0e703d5bd..fc9851b12 100644 --- a/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts +++ b/packages/bitcoin-wallet-snap/src/store/BdkAccountRepository.test.ts @@ -435,9 +435,7 @@ describe('BdkAccountRepository', () => { // 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 expected = (await deriveFixtureNode(["84'", "0'", "1'"])).neuter(); const passedNode = (slip10_to_extended as jest.Mock).mock .calls[0]?.[0] as RealSlip10Node; From 4877b2f23b8788a9afe6379a1f7c0aa5d771f20f Mon Sep 17 00:00:00 2001 From: Hassan Malik Date: Thu, 27 Aug 2026 12:05:34 +0200 Subject: [PATCH 18/18] fix(bitcoin-wallet-snap): fix changelog --- packages/bitcoin-wallet-snap/CHANGELOG.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index e23ef2e71..c8a2b11f1 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -9,16 +9,16 @@ 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) +- 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) -- 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) +- 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)) ### 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 ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) -- Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#150](https://github.com/MetaMask/internal-snaps/pull/150)) +- 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))