From 07159596ed9bb79261365279df20f153136d24ae Mon Sep 17 00:00:00 2001 From: Andrew Taran Date: Sun, 1 Nov 2026 12:30:22 +0100 Subject: [PATCH 1/5] fix(bitcoin): avoid scientific notation in amounts --- packages/bitcoin-wallet-snap/CHANGELOG.md | 4 + .../src/handlers/CronHandler.test.ts | 73 ++++++++++++++++++- .../src/handlers/CronHandler.ts | 17 +++-- .../src/handlers/KeyringHandler.test.ts | 23 ++++++ .../src/handlers/KeyringHandler.ts | 6 +- .../src/handlers/RpcHandler.test.ts | 1 + .../src/handlers/mappings.test.ts | 59 ++++++++++++++- .../src/handlers/mappings.ts | 3 +- .../src/infra/SnapClientAdapter.test.ts | 52 ++++++++++--- .../src/infra/SnapClientAdapter.ts | 6 +- .../src/infra/jsx/format.test.ts | 16 +++- .../src/infra/jsx/format.ts | 7 +- .../src/utils/format-btc.test.ts | 54 ++++++++++++++ .../src/utils/format-btc.ts | 18 +++++ 14 files changed, 301 insertions(+), 38 deletions(-) create mode 100644 packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts create mode 100644 packages/bitcoin-wallet-snap/src/utils/format-btc.ts diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 9bceacff1..625b3f123 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Retry transient Bitcoin indexer, price API, and signed transaction broadcast failures with the shared fetch policy, and treat duplicate broadcasts as successful. ([#530](https://github.com/MetaMask/internal-snaps/pull/530)) +### Fixed + +- Fix formatting of small Bitcoin balances to avoid scientific notation. + ## [3.2.0] ### Added diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index 68d1b63a2..04f7b6c0c 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -307,6 +307,75 @@ describe('CronHandler', () => { mockAccount2.listTransactions.mockReturnValue([]); }); + it('emits normal sync events before a pending repair and coalesces overlapping runs', async () => { + (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-2']); + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + const transaction = buildTx('selected-account-transaction'); + mockAccountUseCases.synchronize.mockResolvedValue({ + account: mockAccount2, + transactionsToNotify: [transaction], + }); + let releaseRepair!: (result: SyncResult) => void; + const repairResult = new Promise((resolve) => { + releaseRepair = resolve; + }); + let notifyRepairStarted!: () => void; + const repairStarted = new Promise((resolve) => { + notifyRepairStarted = resolve; + }); + mockAccountUseCases.fullScan.mockImplementation(async () => { + notifyRepairStarted(); + return repairResult; + }); + + const firstRun = handler.synchronizeAccounts(); + const secondRun = handler.synchronizeAccounts(); + + try { + await repairStarted; + + expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(1); + expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent, + ).toHaveBeenCalledTimes(1); + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent, + ).toHaveBeenCalledWith([mockAccount2]); + expect( + mockSnapClient.emitAccountTransactionsUpdatedEvent, + ).toHaveBeenCalledWith(mockAccount2, [transaction]); + expect(mockState.setKey).not.toHaveBeenCalled(); + } finally { + releaseRepair({ account: mockAccount1, transactionsToNotify: [] }); + await Promise.all([firstRun, secondRun]); + } + + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent, + ).toHaveBeenLastCalledWith([mockAccount1]); + expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { + pending: [], + }); + }); + + it('continues repairing accounts after normal account synchronization fails', async () => { + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + mockAccountUseCases.synchronize.mockRejectedValue( + new Error('sync failed'), + ); + + await handler.route(request); + + expect(mockAccountUseCases.fullScan).toHaveBeenCalledWith(mockAccount1); + expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { + pending: [], + }); + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent, + ).toHaveBeenCalledWith([mockAccount1]); + }); + it('on first run, persists the full pending list, scans only the first account, and emits for missed transactions', async () => { const state: Pick = {}; mockState.getKey.mockResolvedValue(state.rescanV1); @@ -353,7 +422,7 @@ describe('CronHandler', () => { expect(initialPersistOrder).toBeLessThan(scanOrder as number); expect(scanOrder).toBeLessThan(shortenedPersistOrder as number); - // The normal sync flow still runs afterwards. + // The normal sync flow runs before the repair. expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); }); @@ -425,7 +494,7 @@ describe('CronHandler', () => { expect(mockAccountUseCases.list).toHaveBeenCalledTimes(1); }); - it('reports a scan failure without shortening the pending list, and still runs the regular sync', async () => { + it('reports a repair failure without shortening the pending list after normal sync', async () => { mockState.getKey.mockResolvedValue({ pending: ['account-1', 'account-2'], }); diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index dce8a6cc0..f38a23114 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -149,14 +149,6 @@ export class CronHandler { // that coalesced callers share the run's outcome, including the reported // synchronization failures. await this.#syncCoalescer.run('synchronizeAccounts', async () => { - try { - await this.#repairNextAccount(); - } catch (error) { - await this.#snapClient.emitTrackingError( - new Error('Account repair scan failed', { cause: error }), - ); - } - const selectedAccounts: Set = new Set( await getSelectedAccounts(this.#snap), ); @@ -180,6 +172,15 @@ export class CronHandler { results, accounts.map(({ id }) => id), ); + + // Publish normal balance and activity updates before a slower repair scan. + try { + await this.#repairNextAccount(); + } catch (error) { + await this.#snapClient.emitTrackingError( + new Error('Account repair scan failed', { cause: error }), + ); + } }); } diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts index c27159d9e..539cdb1bb 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts @@ -656,6 +656,28 @@ describe('KeyringHandler', () => { }); describe('getAccountBalances', () => { + it.each([ + [1n, '0.00000001'], + [77n, '0.00000077'], + ])( + 'returns a decimal balance for %s satoshis', + async (satoshis, amount) => { + const account = mock({ + id: 'small-balance', + network: 'bitcoin', + balance: { + trusted_spendable: { to_sat: () => satoshis }, + }, + listUnspent: () => [], + }); + mockAccounts.get.mockResolvedValue(account); + + expect(await handler.getAccountBalances(account.id)).toStrictEqual({ + [Caip19Asset.Bitcoin]: { amount, unit: 'BTC' }, + }); + }, + ); + it('gets the account balance', async () => { mockAccounts.get.mockResolvedValue(mockAccount); const expectedResponse = { @@ -802,6 +824,7 @@ describe('KeyringHandler', () => { const pagination = { limit: 10, next: null }; const mockAmount = mock({ + to_sat: () => 2_100_000_000n, to_btc: () => 21, }); const mockScriptPubkey = mock({ diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts index 084f80ecc..156104c76 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts @@ -1,5 +1,4 @@ import type { AddressType } from '@metamask/bitcoindevkit'; -import { Amount } from '@metamask/bitcoindevkit'; import { AccountCreationType, assertCreateAccountOptionIsSupported, @@ -39,6 +38,7 @@ import type { AccountUseCases, CreateAccountParams, } from '../use-cases/AccountUseCases'; +import { formatBtcAmount } from '../utils/format-btc'; import { NetworkStruct, networkToCaip19, scopeToNetwork } from './caip'; import { CronMethod } from './CronHandler'; import type { KeyringRequestHandler } from './KeyringRequestHandler'; @@ -277,9 +277,7 @@ export class KeyringHandler implements KeyringSnapRpc { id: string, ): Promise> { const account = await this.#accountsUseCases.get(id); - const balance = Amount.from_sat(computeDisplayBalanceSats(account)) - .to_btc() - .toString(); + const balance = formatBtcAmount(computeDisplayBalanceSats(account)); return { [networkToCaip19[account.network]]: { diff --git a/packages/bitcoin-wallet-snap/src/handlers/RpcHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/RpcHandler.test.ts index ab437a33b..3205f499d 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/RpcHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/RpcHandler.test.ts @@ -524,6 +524,7 @@ describe('RpcHandler', () => { it('executes computeFee', async () => { const mockAmount = mock({ + to_sat: () => 1000n, to_btc: jest.fn().mockReturnValue('0.00001'), }); mockAccountsUseCases.computeFee.mockResolvedValue(mockAmount); diff --git a/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts b/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts index b1e6490da..a53f9c629 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts @@ -7,12 +7,21 @@ import type { WalletTx, } from '@metamask/bitcoindevkit'; import { Address } from '@metamask/bitcoindevkit'; -import { TransactionStatus, FeeType } from '@metamask/keyring-api'; +import { + TransactionStatus, + TransactionStruct, + FeeType, +} from '@metamask/keyring-api'; +import { assert } from '@metamask/superstruct'; import { mock } from 'jest-mock-extended'; import type { BitcoinAccount } from '../entities'; import { Caip19Asset } from './caip'; -import { mapPsbtToTransaction, mapToTransaction } from './mappings'; +import { + mapPsbtToTransaction, + mapToTransaction, + mapToTransactionFees, +} from './mappings'; // Mock the entire bitcoindevkit module /* eslint-disable @typescript-eslint/naming-convention */ @@ -22,6 +31,27 @@ jest.mock('@metamask/bitcoindevkit', () => ({ }, })); +describe('mapToTransactionFees', () => { + it.each([ + [0n, '0'], + [1n, '0.00000001'], + [64n, '0.00000064'], + [77n, '0.00000077'], + [99n, '0.00000099'], + [100n, '0.000001'], + [100_000_000n, '1'], + [123_456_789n, '1.23456789'], + [18_446_744_073_709_551_615n, '184467440737.09551615'], + ])('formats %s satoshis as the exact decimal %s', (satoshis, expected) => { + const amount = mock({ + to_sat: () => satoshis, + to_btc: () => Number(satoshis) / 100_000_000, + }); + + expect(mapToTransactionFees(amount, 'bitcoin').asset.amount).toBe(expected); + }); +}); + describe('mapPsbtToTransaction', () => { const ACCOUNT_ID = '724ac464-6572-4d9c-a8e2-4075c8846d65'; const TIMESTAMP = 1234567890; @@ -60,6 +90,7 @@ describe('mapPsbtToTransaction', () => { */ function createMockOutput(satoshis: number) { const mockAmount = mock(); + jest.spyOn(mockAmount, 'to_sat').mockReturnValue(BigInt(satoshis)); jest.spyOn(mockAmount, 'to_btc').mockReturnValue(satoshis / 100_000_000); const mockScript = mock(); @@ -85,6 +116,7 @@ describe('mapPsbtToTransaction', () => { addressType: 'p2pkh' | 'p2sh' | 'p2wpkh' | 'p2wsh' | 'p2tr' = 'p2wpkh', ) { const mockFeeAmount = mock(); + jest.spyOn(mockFeeAmount, 'to_sat').mockReturnValue(BigInt(feeSatoshis)); jest .spyOn(mockFeeAmount, 'to_btc') .mockReturnValue(feeSatoshis / 100_000_000); @@ -415,6 +447,7 @@ describe('mapToTransaction', () => { */ function createMockOutput(satoshis: number): TxOut { const mockAmount = mock(); + jest.spyOn(mockAmount, 'to_sat').mockReturnValue(BigInt(satoshis)); jest.spyOn(mockAmount, 'to_btc').mockReturnValue(satoshis / 100_000_000); const mockScript = mock(); @@ -443,6 +476,7 @@ describe('mapToTransaction', () => { .mockReturnValue(sentSatoshis / 100_000_000); const feeAmount = mock(); + jest.spyOn(feeAmount, 'to_sat').mockReturnValue(BigInt(feeSatoshis)); jest.spyOn(feeAmount, 'to_btc').mockReturnValue(feeSatoshis / 100_000_000); const account = mock(); @@ -459,6 +493,27 @@ describe('mapToTransaction', () => { return account; } + it.each([64, 77])( + 'emits a valid transaction with a %s-satoshi fee', + (fee) => { + const account = createMockAccount(100000, fee); + const output = createMockOutput(1); + const transaction = createMockTransaction('small-fee', [output]); + jest.mocked(Address.from_script).mockReturnValue({ + toString: () => 'bc1qrecipient', + } as Address); + + const result = mapToTransaction(account, { + tx: transaction, + txid: transaction.compute_txid(), + chain_position: { anchor: undefined, last_seen: undefined }, + } as unknown as WalletTx); + + expect(() => assert(result, TransactionStruct)).not.toThrow(); + expect(result.to[0]).toMatchObject({ asset: { amount: '0.00000001' } }); + }, + ); + it('keeps a self-send recipient output that is owned by the account', () => { const account = createMockAccount(); // Self-send: recipient (external keychain) + change (internal keychain). diff --git a/packages/bitcoin-wallet-snap/src/handlers/mappings.ts b/packages/bitcoin-wallet-snap/src/handlers/mappings.ts index 87d1b895d..55344090f 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/mappings.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/mappings.ts @@ -21,6 +21,7 @@ import { import { canAccountTxidBeMalleated, networkToCurrencyUnit } from '../entities'; import type { BitcoinAccount } from '../entities'; import { mapToTransactionType } from '../entities/transaction'; +import { formatBtcAmount } from '../utils/format-btc'; import type { Caip19Asset } from './caip'; import { addressTypeToCaip, networkToCaip19, networkToScope } from './caip'; @@ -87,7 +88,7 @@ export function mapToKeyringAccount(account: BitcoinAccount): KeyringAccount { const mapToAmount = (amount: Amount, network: Network): TransactionAmount => { return { - amount: amount.to_btc().toString(), + amount: formatBtcAmount(amount.to_sat()), fungible: true, unit: networkToCurrencyUnit[network], type: networkToCaip19[network], diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts index 0ae4ce311..52d2bf165 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts @@ -1,5 +1,9 @@ import type { Amount, WalletTx } from '@metamask/bitcoindevkit'; -import { TransactionStatus, TransactionType } from '@metamask/keyring-api'; +import { + KeyringEvent, + TransactionStatus, + TransactionType, +} from '@metamask/keyring-api'; import { getJsonError } from '@metamask/snaps-sdk'; import { mock } from 'jest-mock-extended'; @@ -8,15 +12,7 @@ import { TrackingSnapEvent } from '../entities'; import logger from '../utils/logger'; import { SnapClientAdapter } from './SnapClientAdapter'; -jest.mock('@metamask/bitcoindevkit', () => ({ - Amount: { - from_sat: jest.fn(() => ({ - to_btc: jest.fn(() => ({ - toString: jest.fn(() => '0'), - })), - })), - }, -})); +jest.mock('@metamask/bitcoindevkit', () => ({})); jest.mock('../utils/logger', () => ({ __esModule: true, @@ -525,6 +521,42 @@ describe('SnapClientAdapter', () => { }); }); + describe('emitAccountBalancesUpdatedEvent', () => { + it.each([ + [1n, '0.00000001'], + [77n, '0.00000077'], + ])('emits a decimal balance for %s satoshis', async (satoshis, amount) => { + const { snapClient, mockRequest } = setupTest(); + const account = mock({ + id: 'small-balance', + network: 'bitcoin', + balance: { + trusted_spendable: { to_sat: jest.fn().mockReturnValue(satoshis) }, + }, + listUnspent: () => [], + }); + + await snapClient.emitAccountBalancesUpdatedEvent([account]); + + expect(mockRequest).toHaveBeenCalledWith({ + method: 'snap_manageAccounts', + params: { + method: KeyringEvent.AccountBalancesUpdated, + params: { + balances: { + [account.id]: { + 'bip122:000000000019d6689c085ae165831e93/slip44:0': { + amount, + unit: 'BTC', + }, + }, + }, + }, + }, + }); + }); + }); + describe('emitAccountTransactionsUpdatedEvent', () => { const createWalletTx = (txid: string): WalletTx => mock({ diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts index 159cec711..1f20928a9 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts @@ -1,5 +1,4 @@ import type { WalletTx } from '@metamask/bitcoindevkit'; -import { Amount } from '@metamask/bitcoindevkit'; import type { JsonSLIP10Node } from '@metamask/key-tree'; import { SLIP10Node } from '@metamask/key-tree'; import { @@ -38,6 +37,7 @@ import { } from '../handlers'; import { mapToTransaction } from '../handlers/mappings'; import { trackError } from '../utils/errors'; +import { formatBtcAmount } from '../utils/format-btc'; export class SnapClientAdapter implements SnapClient { readonly #logger: Logger; @@ -75,9 +75,7 @@ export class SnapClientAdapter implements SnapClient { ...acc, [account.id]: { [networkToCaip19[account.network]]: { - amount: Amount.from_sat(computeDisplayBalanceSats(account)) - .to_btc() - .toString(), + amount: formatBtcAmount(computeDisplayBalanceSats(account)), unit: networkToCurrencyUnit[account.network], }, }, diff --git a/packages/bitcoin-wallet-snap/src/infra/jsx/format.test.ts b/packages/bitcoin-wallet-snap/src/infra/jsx/format.test.ts index 6a8c225c2..013d9bb13 100644 --- a/packages/bitcoin-wallet-snap/src/infra/jsx/format.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/jsx/format.test.ts @@ -1,12 +1,20 @@ -import { displayOrigin } from './format'; +import { CurrencyUnit } from '../../entities'; +import { displayAmount, displayOrigin } from './format'; jest.mock('@metamask/bitcoindevkit', () => ({ - Amount: { - from_sat: jest.fn(), - }, BdkErrorCode: {}, })); +describe('displayAmount', () => { + it('displays one satoshi as a plain decimal', () => { + expect(displayAmount(1n)).toBe('0.00000001'); + }); + + it('preserves the currency suffix', () => { + expect(displayAmount(77n, CurrencyUnit.Bitcoin)).toBe('0.00000077 BTC'); + }); +}); + describe('displayOrigin', () => { it('returns the known label for the internal "metamask" origin', () => { expect(displayOrigin('metamask')).toBe('MetaMask'); diff --git a/packages/bitcoin-wallet-snap/src/infra/jsx/format.ts b/packages/bitcoin-wallet-snap/src/infra/jsx/format.ts index 8e54922fb..da34a0298 100644 --- a/packages/bitcoin-wallet-snap/src/infra/jsx/format.ts +++ b/packages/bitcoin-wallet-snap/src/infra/jsx/format.ts @@ -1,20 +1,21 @@ import type { Network } from '@metamask/bitcoindevkit'; -import { Amount, BdkErrorCode } from '@metamask/bitcoindevkit'; +import { BdkErrorCode } from '@metamask/bitcoindevkit'; import type { CaipAccountId } from '@metamask/snaps-sdk'; import type { CurrencyRate, CurrencyUnit, Messages } from '../../entities'; import { networkToScope } from '../../handlers'; +import { formatBtcAmount } from '../../utils/format-btc'; export const displayAmount = ( amountSats: bigint, currency?: CurrencyUnit, ): string => { - const amount = Amount.from_sat(amountSats).to_btc(); + const amount = formatBtcAmount(amountSats); if (currency) { return `${amount} ${currency}`; } - return amount.toString(); + return amount; }; export const exchangeAmount = ( diff --git a/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts b/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts new file mode 100644 index 000000000..4b4b94df3 --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts @@ -0,0 +1,54 @@ +/** + * @jest-environment node + */ + +import { BalanceStruct } from '@metamask/keyring-api'; +import { assert } from '@metamask/superstruct'; + +import { formatBtcAmount } from './format-btc'; + +describe('formatBtcAmount', () => { + it.each([ + [0n, '0'], + [1n, '0.00000001'], + [9n, '0.00000009'], + [10n, '0.0000001'], + [11n, '0.00000011'], + [64n, '0.00000064'], + [77n, '0.00000077'], + [99n, '0.00000099'], + [100n, '0.000001'], + [101n, '0.00000101'], + [1_000n, '0.00001'], + [10_000n, '0.0001'], + [100_000n, '0.001'], + [1_000_000n, '0.01'], + [10_000_000n, '0.1'], + [99_999_999n, '0.99999999'], + [100_000_000n, '1'], + [100_000_001n, '1.00000001'], + [100_000_010n, '1.0000001'], + [100_010_000n, '1.0001'], + [123_456_789n, '1.23456789'], + [1_000_000_000n, '10'], + [1_230_000_000n, '12.3'], + [2_100_000_000_000_000n, '21000000'], + [9_007_199_254_740_991n, '90071992.54740991'], + [9_007_199_254_740_992n, '90071992.54740992'], + [9_007_199_254_740_993n, '90071992.54740993'], + [18_446_744_073_709_551_615n, '184467440737.09551615'], + ])('formats %s satoshis as %s BTC', (satoshis, expected) => { + expect(formatBtcAmount(satoshis)).toBe(expected); + }); + + it('produces valid keyring balances for every amount below 100 satoshis', () => { + // Number.toString() uses scientific notation for these BTC values. + for (let satoshis = 1n; satoshis < 100n; satoshis += 1n) { + const amount = formatBtcAmount(satoshis); + + expect(() => + assert({ amount, unit: 'BTC' }, BalanceStruct), + ).not.toThrow(); + } + }); +}); diff --git a/packages/bitcoin-wallet-snap/src/utils/format-btc.ts b/packages/bitcoin-wallet-snap/src/utils/format-btc.ts new file mode 100644 index 000000000..4fd67c3f1 --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/utils/format-btc.ts @@ -0,0 +1,18 @@ +/** + * Formats a non-negative satoshi amount as an exact BTC decimal string. + * + * Integer arithmetic avoids scientific notation and floating-point precision + * loss in keyring amounts and displayed balances. + * + * @param satoshis - The amount in satoshis. + * @returns The BTC amount without unnecessary trailing decimal zeros. + */ +export function formatBtcAmount(satoshis: bigint): string { + const whole = satoshis / 100_000_000n; + const fraction = (satoshis % 100_000_000n) + .toString() + .padStart(8, '0') + .replace(/0+$/u, ''); + + return fraction ? `${whole}.${fraction}` : whole.toString(); +} From 2a9c1e83e10e115632fd5d019d47e6848db88d24 Mon Sep 17 00:00:00 2001 From: Andrew Taran Date: Sun, 1 Nov 2026 13:05:42 +0100 Subject: [PATCH 2/5] fix(bitcoin): run account repairs in a separate cron job --- packages/bitcoin-wallet-snap/CHANGELOG.md | 3 +- .../bitcoin-wallet-snap/snap.manifest.json | 6 + .../src/handlers/CronHandler.test.ts | 205 +++++++++++++----- .../src/handlers/CronHandler.ts | 78 +++++-- 4 files changed, 218 insertions(+), 74 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 625b3f123..d0a3ceb40 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -10,10 +10,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Retry transient Bitcoin indexer, price API, and signed transaction broadcast failures with the shared fetch policy, and treat duplicate broadcasts as successful. ([#530](https://github.com/MetaMask/internal-snaps/pull/530)) +- Run Bitcoin account repairs separately from regular balance and activity updates. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) ### Fixed -- Fix formatting of small Bitcoin balances to avoid scientific notation. +- Fix formatting of small Bitcoin balances to avoid scientific notation. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) ## [3.2.0] diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index a1b0b420c..5e085df2f 100644 --- a/packages/bitcoin-wallet-snap/snap.manifest.json +++ b/packages/bitcoin-wallet-snap/snap.manifest.json @@ -85,6 +85,12 @@ "request": { "method": "synchronizeAccounts" } + }, + { + "duration": "PT45S", + "request": { + "method": "repairAccounts" + } } ] }, diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index 04f7b6c0c..61b981918 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -6,6 +6,7 @@ import type { IStateManager } from '@metamask/snap-networks-utils'; import type { Json, SnapsProvider, JsonRpcRequest } from '@metamask/snaps-sdk'; import { mock } from 'jest-mock-extended'; +import snapManifest from '../../snap.manifest.json'; import type { BitcoinAccount, Logger, @@ -45,8 +46,6 @@ describe('CronHandler', () => { clientVersion: '1.0.0', platformVersion: '1.0.0', }); - // Default the repair rescan to nothing pending so existing tests don't - // trigger a repair scan. mockState.getKey.mockResolvedValue({ pending: [] }); }); @@ -283,7 +282,8 @@ describe('CronHandler', () => { ); }); - describe('repair rescan', () => { + describe('repairAccounts', () => { + const repairRequest = { method: 'repairAccounts' } as JsonRpcRequest; const buildTx = (txid: string): WalletTx => mock({ txid: mock({ toString: () => txid }), @@ -307,14 +307,28 @@ describe('CronHandler', () => { mockAccount2.listTransactions.mockReturnValue([]); }); - it('emits normal sync events before a pending repair and coalesces overlapping runs', async () => { + it('schedules repairs independently from regular synchronization', () => { + expect( + snapManifest.initialPermissions['endowment:cronjob'].jobs, + ).toStrictEqual([ + { duration: 'PT30S', request: { method: 'synchronizeAccounts' } }, + { duration: 'PT45S', request: { method: 'repairAccounts' } }, + ]); + }); + + it('does not repair accounts during regular synchronization', async () => { + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + + await handler.route(request); + + expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); + expect(mockState.getKey).not.toHaveBeenCalled(); + }); + + it('coalesces repairs without blocking subsequent regular sync runs', async () => { (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-2']); mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); - const transaction = buildTx('selected-account-transaction'); - mockAccountUseCases.synchronize.mockResolvedValue({ - account: mockAccount2, - transactionsToNotify: [transaction], - }); let releaseRepair!: (result: SyncResult) => void; const repairResult = new Promise((resolve) => { releaseRepair = resolve; @@ -328,52 +342,144 @@ describe('CronHandler', () => { return repairResult; }); - const firstRun = handler.synchronizeAccounts(); - const secondRun = handler.synchronizeAccounts(); + const firstRepair = handler.route(repairRequest); + const secondRepair = handler.route(repairRequest); try { - await repairStarted; + await Promise.race([repairStarted, firstRepair]); + await handler.synchronizeAccounts(); + await handler.synchronizeAccounts(); - expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(1); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); - expect( - mockSnapClient.emitAccountBalancesUpdatedEvent, - ).toHaveBeenCalledTimes(1); - expect( - mockSnapClient.emitAccountBalancesUpdatedEvent, - ).toHaveBeenCalledWith([mockAccount2]); - expect( - mockSnapClient.emitAccountTransactionsUpdatedEvent, - ).toHaveBeenCalledWith(mockAccount2, [transaction]); + expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(2); expect(mockState.setKey).not.toHaveBeenCalled(); } finally { releaseRepair({ account: mockAccount1, transactionsToNotify: [] }); - await Promise.all([firstRun, secondRun]); + await Promise.all([firstRepair, secondRepair]); } - expect( - mockSnapClient.emitAccountBalancesUpdatedEvent, - ).toHaveBeenLastCalledWith([mockAccount1]); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { pending: [], }); }); - it('continues repairing accounts after normal account synchronization fails', async () => { + it('waits for an existing regular sync before starting repair', async () => { + (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-2']); mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); - mockAccountUseCases.synchronize.mockRejectedValue( - new Error('sync failed'), - ); + let releaseSynchronization!: (result: SyncResult) => void; + const syncResult = new Promise((resolve) => { + releaseSynchronization = resolve; + }); + let notifySyncStarted!: () => void; + const syncStarted = new Promise((resolve) => { + notifySyncStarted = resolve; + }); + mockAccountUseCases.synchronize.mockImplementation(async () => { + notifySyncStarted(); + return syncResult; + }); + const regularSynchronization = handler.synchronizeAccounts(); + await syncStarted; + const repair = handler.route(repairRequest); - await handler.route(request); + try { + // Let the repair pass its status check and reach the in-flight sync. + await Promise.resolve(); + await Promise.resolve(); + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); + } finally { + releaseSynchronization({ + account: mockAccount2, + transactionsToNotify: [], + }); + await Promise.all([regularSynchronization, repair]); + } expect(mockAccountUseCases.fullScan).toHaveBeenCalledWith(mockAccount1); + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent.mock + .invocationCallOrder[0], + ).toBeLessThan( + mockAccountUseCases.fullScan.mock.invocationCallOrder[0] as number, + ); + }); + + it('does not start repair if the client locks while waiting for regular sync', async () => { + (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-2']); + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + let releaseSynchronization!: (result: SyncResult) => void; + const syncResult = new Promise((resolve) => { + releaseSynchronization = resolve; + }); + mockAccountUseCases.synchronize.mockReturnValue(syncResult); + + const regularSynchronization = handler.synchronizeAccounts(); + const repair = handler.repairAccounts(); + mockSnapClient.getClientStatus.mockResolvedValue({ + active: true, + locked: true, + clientVersion: '1.0.0', + platformVersion: '1.0.0', + }); + + releaseSynchronization({ + account: mockAccount2, + transactionsToNotify: [], + }); + await Promise.all([regularSynchronization, repair]); + + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); + expect(mockState.getKey).not.toHaveBeenCalled(); + }); + + it('retries a failed repair on the next repair job', async () => { + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + mockAccountUseCases.fullScan.mockRejectedValueOnce( + new Error('scan failed'), + ); + + await handler.route(repairRequest); + expect(mockState.setKey).not.toHaveBeenCalled(); + + await handler.route(repairRequest); + + expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(2); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { pending: [], }); - expect( - mockSnapClient.emitAccountBalancesUpdatedEvent, - ).toHaveBeenCalledWith([mockAccount1]); + }); + + it('can repair after a failed regular sync and keeps future sync runs working', async () => { + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + mockAccountUseCases.list.mockRejectedValueOnce( + new Error('list failed'), + ); + const regularSynchronization = handler.synchronizeAccounts(); + const repair = handler.route(repairRequest); + + await expect(regularSynchronization).rejects.toThrow('list failed'); + await repair; + await handler.synchronizeAccounts(); + + expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); + expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(2); + }); + + it.each([ + { active: false, locked: false }, + { active: true, locked: true }, + ])('does not repair when the client is %o', async (status) => { + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + mockSnapClient.getClientStatus.mockResolvedValue({ + ...status, + clientVersion: '1.0.0', + platformVersion: '1.0.0', + }); + + await handler.route(repairRequest); + + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); + expect(mockState.getKey).not.toHaveBeenCalled(); }); it('on first run, persists the full pending list, scans only the first account, and emits for missed transactions', async () => { @@ -389,7 +495,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(request); + await handler.route(repairRequest); expect(mockState.getKey).toHaveBeenCalledWith('rescanV1'); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); @@ -422,8 +528,7 @@ describe('CronHandler', () => { expect(initialPersistOrder).toBeLessThan(scanOrder as number); expect(scanOrder).toBeLessThan(shortenedPersistOrder as number); - // The normal sync flow runs before the repair. - expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); + expect(mockAccountUseCases.synchronize).not.toHaveBeenCalled(); }); it('on a subsequent run, scans only the next pending account', async () => { @@ -436,7 +541,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(request); + await handler.route(repairRequest); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); expect(mockAccountUseCases.fullScan).toHaveBeenCalledWith(mockAccount2); @@ -454,7 +559,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(request); + await handler.route(repairRequest); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { pending: ['account-2'], @@ -471,7 +576,7 @@ describe('CronHandler', () => { it('does not scan when every pending account has been deleted', async () => { mockState.getKey.mockResolvedValue({ pending: ['gone'] }); - await handler.route(request); + await handler.route(repairRequest); // The pending list is pruned to nothing, and the repair pass ends // without scanning anything. @@ -484,24 +589,21 @@ describe('CronHandler', () => { it('does nothing when nothing is pending, short-circuiting before list()', async () => { mockState.getKey.mockResolvedValue({ pending: [] }); - await handler.route(request); + await handler.route(repairRequest); expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); expect(mockState.setKey).not.toHaveBeenCalled(); - // The regular sync flow is the only caller of `list()` here — the - // repair path short-circuits on the empty pending list before - // reaching it. - expect(mockAccountUseCases.list).toHaveBeenCalledTimes(1); + expect(mockAccountUseCases.list).not.toHaveBeenCalled(); }); - it('reports a repair failure without shortening the pending list after normal sync', async () => { + it('reports a repair failure without shortening the pending list', async () => { mockState.getKey.mockResolvedValue({ pending: ['account-1', 'account-2'], }); const scanError = new Error('scan failed'); mockAccountUseCases.fullScan.mockRejectedValue(scanError); - await handler.route(request); + await handler.route(repairRequest); expect(mockSnapClient.emitTrackingError).toHaveBeenCalledWith( expect.objectContaining({ @@ -509,12 +611,11 @@ describe('CronHandler', () => { cause: scanError, }), ); - // The failure is contained: the pending list is not shortened and - // the regular sync still runs. + // A failed repair remains pending for the next repair cron job. expect(mockState.setKey).not.toHaveBeenCalledWith('rescanV1', { pending: ['account-2'], }); - expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); + expect(mockAccountUseCases.synchronize).not.toHaveBeenCalled(); }); it('treats malformed stored state as unset and reinitializes pending from the account list', async () => { @@ -524,7 +625,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(request); + await handler.route(repairRequest); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { pending: ['account-1', 'account-2'], @@ -575,7 +676,7 @@ describe('CronHandler', () => { ); await Promise.all([ - stateHandler.route(request), + stateHandler.route(repairRequest), state.setKeyWith( 'derivationPaths', (current) => ({ ...current, concurrent: 'account-2' }), diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index f38a23114..016c96162 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -26,6 +26,7 @@ import type { SendFlowUseCases, AccountUseCases } from '../use-cases'; export const CronMethod = { SynchronizeAccounts: 'synchronizeAccounts', + RepairAccounts: 'repairAccounts', RefreshRates: 'refreshRates', SyncSelectedAccounts: 'syncSelectedAccounts', FullScanAccount: 'fullScanAccount', @@ -64,6 +65,8 @@ export class CronHandler { readonly #syncCoalescer = new InFlightCoalescer(); + #synchronization: Promise | undefined; + constructor( accounts: AccountUseCases, sendFlow: SendFlowUseCases, @@ -125,6 +128,9 @@ export class CronHandler { case CronMethod.SynchronizeAccounts: { return this.synchronizeAccounts(); } + case CronMethod.RepairAccounts: { + return this.repairAccounts(); + } case CronMethod.RefreshRates: { assert(params, SendFormRefreshRatesRequest); return this.#sendFlowUseCases.refresh(params.interfaceId); @@ -148,33 +154,63 @@ export class CronHandler { // duplicating network fetches, state writes, and keyring events. Note // that coalesced callers share the run's outcome, including the reported // synchronization failures. - await this.#syncCoalescer.run('synchronizeAccounts', async () => { - const selectedAccounts: Set = new Set( - await getSelectedAccounts(this.#snap), - ); + const synchronization = 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'), + ), + ); - await this.#finishSync(results); + await this.#finishSync(results); - await this.#reportSyncFailures( - 'synchronizeAccounts: Account synchronization failures', - results, - accounts.map(({ id }) => id), - ); + await this.#reportSyncFailures( + 'synchronizeAccounts: Account synchronization failures', + results, + accounts.map(({ id }) => id), + ); + }, + ); + + this.#synchronization = synchronization; + try { + await synchronization; + } finally { + if (this.#synchronization === synchronization) { + this.#synchronization = undefined; + } + } + } - // Publish normal balance and activity updates before a slower repair scan. + /** + * Runs one queued repair independently of regular synchronization. Overlapping + * repair jobs share one run. An existing regular sync finishes first, while + * subsequent regular sync calls never wait for the repair. + */ + async repairAccounts(): Promise { + await this.#syncCoalescer.run('repairAccounts', async () => { try { + // A failed regular sync must not prevent the independently scheduled repair. + await this.#synchronization?.catch(() => undefined); + + // The client may have locked or become inactive while waiting for sync. + const { active, locked } = await this.#snapClient.getClientStatus(); + + if (!active || locked) { + return; + } + await this.#repairNextAccount(); } catch (error) { await this.#snapClient.emitTrackingError( From 54f75124cfa61c86950cf9ed6c24abfac206cf9b Mon Sep 17 00:00:00 2001 From: Andrew Taran Date: Mon, 2 Nov 2026 15:19:05 +0100 Subject: [PATCH 3/5] fix: move repair to account sync cronjob and remove regexp --- packages/bitcoin-wallet-snap/CHANGELOG.md | 1 - .../bitcoin-wallet-snap/snap.manifest.json | 6 - .../src/handlers/CronHandler.test.ts | 171 +++++++++++------- .../src/handlers/CronHandler.ts | 81 +++------ .../src/utils/format-btc.test.ts | 2 + .../src/utils/format-btc.ts | 16 +- 6 files changed, 142 insertions(+), 135 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index d0a3ceb40..09f146094 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -10,7 +10,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Retry transient Bitcoin indexer, price API, and signed transaction broadcast failures with the shared fetch policy, and treat duplicate broadcasts as successful. ([#530](https://github.com/MetaMask/internal-snaps/pull/530)) -- Run Bitcoin account repairs separately from regular balance and activity updates. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) ### Fixed diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 5e085df2f..a1b0b420c 100644 --- a/packages/bitcoin-wallet-snap/snap.manifest.json +++ b/packages/bitcoin-wallet-snap/snap.manifest.json @@ -85,12 +85,6 @@ "request": { "method": "synchronizeAccounts" } - }, - { - "duration": "PT45S", - "request": { - "method": "repairAccounts" - } } ] }, diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index 61b981918..8be761380 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -282,8 +282,7 @@ describe('CronHandler', () => { ); }); - describe('repairAccounts', () => { - const repairRequest = { method: 'repairAccounts' } as JsonRpcRequest; + describe('repair rescan', () => { const buildTx = (txid: string): WalletTx => mock({ txid: mock({ toString: () => txid }), @@ -307,28 +306,35 @@ describe('CronHandler', () => { mockAccount2.listTransactions.mockReturnValue([]); }); - it('schedules repairs independently from regular synchronization', () => { + it('schedules a single synchronization job for normal sync and repair', () => { expect( snapManifest.initialPermissions['endowment:cronjob'].jobs, ).toStrictEqual([ { duration: 'PT30S', request: { method: 'synchronizeAccounts' } }, - { duration: 'PT45S', request: { method: 'repairAccounts' } }, ]); }); - it('does not repair accounts during regular synchronization', async () => { - mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); - - await handler.route(request); - - expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); - expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); - expect(mockState.getKey).not.toHaveBeenCalled(); - }); - - it('coalesces repairs without blocking subsequent regular sync runs', async () => { - (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-2']); + it('publishes normal updates first and prevents later same-account sync from overtaking repair', async () => { + (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-1']); mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + const normalTransaction = buildTx('normal-tx'); + const pendingTransaction = mock({ + txid: mock({ toString: () => 'repair-tx' }), + chain_position: mock({ + is_confirmed: false, + }), + }); + const confirmedTransaction = mock({ + txid: mock({ toString: () => 'repair-tx' }), + chain_position: mock({ + is_confirmed: true, + }), + }); + const latestAccount = mock({ id: mockAccount1.id }); + mockAccountUseCases.synchronize.mockResolvedValue({ + account: mockAccount1, + transactionsToNotify: [normalTransaction], + }); let releaseRepair!: (result: SyncResult) => void; const repairResult = new Promise((resolve) => { releaseRepair = resolve; @@ -342,28 +348,59 @@ describe('CronHandler', () => { return repairResult; }); - const firstRepair = handler.route(repairRequest); - const secondRepair = handler.route(repairRequest); - + const firstRun = handler.synchronizeAccounts(); + let overlappingRun: Promise | undefined; try { - await Promise.race([repairStarted, firstRepair]); - await handler.synchronizeAccounts(); - await handler.synchronizeAccounts(); - + await Promise.race([repairStarted, firstRun]); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); - expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(2); + overlappingRun = handler.synchronizeAccounts(); + await Promise.resolve(); + + expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(1); + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent, + ).toHaveBeenCalledTimes(1); + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent, + ).toHaveBeenCalledWith([mockAccount1]); + expect( + mockSnapClient.emitAccountTransactionsUpdatedEvent, + ).toHaveBeenCalledWith(mockAccount1, [normalTransaction]); expect(mockState.setKey).not.toHaveBeenCalled(); } finally { - releaseRepair({ account: mockAccount1, transactionsToNotify: [] }); - await Promise.all([firstRepair, secondRepair]); + releaseRepair({ + account: mockAccount1, + transactionsToNotify: [pendingTransaction], + }); + await Promise.all([firstRun, overlappingRun]); } + mockState.getKey.mockResolvedValue({ pending: [] }); + mockAccountUseCases.list.mockResolvedValue([latestAccount]); + mockAccountUseCases.synchronize.mockResolvedValue({ + account: latestAccount, + transactionsToNotify: [confirmedTransaction], + }); + await handler.synchronizeAccounts(); + + expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(2); + expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); + expect( + mockSnapClient.emitAccountBalancesUpdatedEvent, + ).toHaveBeenLastCalledWith([latestAccount]); + expect( + mockSnapClient.emitAccountTransactionsUpdatedEvent.mock.calls, + ).toStrictEqual([ + [mockAccount1, [normalTransaction]], + [mockAccount1, [pendingTransaction]], + [latestAccount, [confirmedTransaction]], + ]); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { pending: [], }); }); - it('waits for an existing regular sync before starting repair', async () => { + it('waits for normal synchronization and event publication before starting repair', async () => { (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-2']); mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); let releaseSynchronization!: (result: SyncResult) => void; @@ -378,23 +415,17 @@ describe('CronHandler', () => { notifySyncStarted(); return syncResult; }); - const regularSynchronization = handler.synchronizeAccounts(); - await syncStarted; - const repair = handler.route(repairRequest); - + const synchronization = handler.synchronizeAccounts(); try { - // Let the repair pass its status check and reach the in-flight sync. - await Promise.resolve(); - await Promise.resolve(); + await syncStarted; expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); } finally { releaseSynchronization({ account: mockAccount2, transactionsToNotify: [], }); - await Promise.all([regularSynchronization, repair]); + await synchronization; } - expect(mockAccountUseCases.fullScan).toHaveBeenCalledWith(mockAccount1); expect( mockSnapClient.emitAccountBalancesUpdatedEvent.mock @@ -404,7 +435,7 @@ describe('CronHandler', () => { ); }); - it('does not start repair if the client locks while waiting for regular sync', async () => { + it('does not start repair if the client locks during normal synchronization', async () => { (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-2']); mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); let releaseSynchronization!: (result: SyncResult) => void; @@ -412,36 +443,32 @@ describe('CronHandler', () => { releaseSynchronization = resolve; }); mockAccountUseCases.synchronize.mockReturnValue(syncResult); - - const regularSynchronization = handler.synchronizeAccounts(); - const repair = handler.repairAccounts(); + const synchronization = handler.synchronizeAccounts(); mockSnapClient.getClientStatus.mockResolvedValue({ active: true, locked: true, clientVersion: '1.0.0', platformVersion: '1.0.0', }); - releaseSynchronization({ account: mockAccount2, transactionsToNotify: [], }); - await Promise.all([regularSynchronization, repair]); + await synchronization; expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); expect(mockState.getKey).not.toHaveBeenCalled(); }); - it('retries a failed repair on the next repair job', async () => { + it('retries a failed repair during the next synchronization run', async () => { mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); mockAccountUseCases.fullScan.mockRejectedValueOnce( new Error('scan failed'), ); - - await handler.route(repairRequest); + await handler.route(request); expect(mockState.setKey).not.toHaveBeenCalled(); - await handler.route(repairRequest); + await handler.route(request); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(2); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { @@ -449,22 +476,36 @@ describe('CronHandler', () => { }); }); - it('can repair after a failed regular sync and keeps future sync runs working', async () => { + it('can retry synchronization after a failure and then repair pending accounts', async () => { mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); mockAccountUseCases.list.mockRejectedValueOnce( new Error('list failed'), ); - const regularSynchronization = handler.synchronizeAccounts(); - const repair = handler.route(repairRequest); + await expect(handler.synchronizeAccounts()).rejects.toThrow( + 'list failed', + ); + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); - await expect(regularSynchronization).rejects.toThrow('list failed'); - await repair; await handler.synchronizeAccounts(); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); expect(mockAccountUseCases.synchronize).toHaveBeenCalledTimes(2); }); + it('still repairs accounts when individual normal account synchronizations fail', async () => { + mockState.getKey.mockResolvedValue({ pending: ['account-1'] }); + mockAccountUseCases.synchronize.mockRejectedValue( + new Error('sync failed'), + ); + + await handler.route(request); + + expect(mockAccountUseCases.fullScan).toHaveBeenCalledWith(mockAccount1); + expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { + pending: [], + }); + }); + it.each([ { active: false, locked: false }, { active: true, locked: true }, @@ -475,9 +516,7 @@ describe('CronHandler', () => { clientVersion: '1.0.0', platformVersion: '1.0.0', }); - - await handler.route(repairRequest); - + await handler.route(request); expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); expect(mockState.getKey).not.toHaveBeenCalled(); }); @@ -495,7 +534,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(repairRequest); + await handler.route(request); expect(mockState.getKey).toHaveBeenCalledWith('rescanV1'); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); @@ -528,7 +567,7 @@ describe('CronHandler', () => { expect(initialPersistOrder).toBeLessThan(scanOrder as number); expect(scanOrder).toBeLessThan(shortenedPersistOrder as number); - expect(mockAccountUseCases.synchronize).not.toHaveBeenCalled(); + expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); }); it('on a subsequent run, scans only the next pending account', async () => { @@ -541,7 +580,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(repairRequest); + await handler.route(request); expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); expect(mockAccountUseCases.fullScan).toHaveBeenCalledWith(mockAccount2); @@ -559,7 +598,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(repairRequest); + await handler.route(request); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { pending: ['account-2'], @@ -576,7 +615,7 @@ describe('CronHandler', () => { it('does not scan when every pending account has been deleted', async () => { mockState.getKey.mockResolvedValue({ pending: ['gone'] }); - await handler.route(repairRequest); + await handler.route(request); // The pending list is pruned to nothing, and the repair pass ends // without scanning anything. @@ -589,11 +628,11 @@ describe('CronHandler', () => { it('does nothing when nothing is pending, short-circuiting before list()', async () => { mockState.getKey.mockResolvedValue({ pending: [] }); - await handler.route(repairRequest); + await handler.route(request); expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); expect(mockState.setKey).not.toHaveBeenCalled(); - expect(mockAccountUseCases.list).not.toHaveBeenCalled(); + expect(mockAccountUseCases.list).toHaveBeenCalledTimes(1); }); it('reports a repair failure without shortening the pending list', async () => { @@ -603,7 +642,7 @@ describe('CronHandler', () => { const scanError = new Error('scan failed'); mockAccountUseCases.fullScan.mockRejectedValue(scanError); - await handler.route(repairRequest); + await handler.route(request); expect(mockSnapClient.emitTrackingError).toHaveBeenCalledWith( expect.objectContaining({ @@ -611,11 +650,11 @@ describe('CronHandler', () => { cause: scanError, }), ); - // A failed repair remains pending for the next repair cron job. + // A failed repair remains pending for the next synchronization run. expect(mockState.setKey).not.toHaveBeenCalledWith('rescanV1', { pending: ['account-2'], }); - expect(mockAccountUseCases.synchronize).not.toHaveBeenCalled(); + expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); }); it('treats malformed stored state as unset and reinitializes pending from the account list', async () => { @@ -625,7 +664,7 @@ describe('CronHandler', () => { transactionsToNotify: [], }); - await handler.route(repairRequest); + await handler.route(request); expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { pending: ['account-1', 'account-2'], @@ -676,7 +715,7 @@ describe('CronHandler', () => { ); await Promise.all([ - stateHandler.route(repairRequest), + stateHandler.route(request), state.setKeyWith( 'derivationPaths', (current) => ({ ...current, concurrent: 'account-2' }), diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index 016c96162..df5c59275 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -26,7 +26,6 @@ import type { SendFlowUseCases, AccountUseCases } from '../use-cases'; export const CronMethod = { SynchronizeAccounts: 'synchronizeAccounts', - RepairAccounts: 'repairAccounts', RefreshRates: 'refreshRates', SyncSelectedAccounts: 'syncSelectedAccounts', FullScanAccount: 'fullScanAccount', @@ -65,8 +64,6 @@ export class CronHandler { readonly #syncCoalescer = new InFlightCoalescer(); - #synchronization: Promise | undefined; - constructor( accounts: AccountUseCases, sendFlow: SendFlowUseCases, @@ -128,9 +125,6 @@ export class CronHandler { case CronMethod.SynchronizeAccounts: { return this.synchronizeAccounts(); } - case CronMethod.RepairAccounts: { - return this.repairAccounts(); - } case CronMethod.RefreshRates: { assert(params, SendFormRefreshRatesRequest); return this.#sendFlowUseCases.refresh(params.interfaceId); @@ -149,64 +143,37 @@ export class CronHandler { } async synchronizeAccounts(): Promise { - // 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 the reported - // synchronization failures. - const synchronization = 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); - }, - ); + // Keep regular synchronization and repair in one coalesced run so a later + // synchronization cannot publish newer data before this run's repair finishes. + await this.#syncCoalescer.run('synchronizeAccounts', async () => { + const selectedAccounts: Set = new Set( + await getSelectedAccounts(this.#snap), + ); - const results = await Promise.allSettled( - accounts.map(async (account) => - this.#accountsUseCases.synchronize(account, 'cron'), - ), - ); + const accounts = (await this.#accountsUseCases.list()).filter( + (account) => { + return selectedAccounts.has(account.id); + }, + ); - await this.#finishSync(results); + const results = await Promise.allSettled( + accounts.map(async (account) => + this.#accountsUseCases.synchronize(account, 'cron'), + ), + ); - await this.#reportSyncFailures( - 'synchronizeAccounts: Account synchronization failures', - results, - accounts.map(({ id }) => id), - ); - }, - ); + await this.#finishSync(results); - this.#synchronization = synchronization; - try { - await synchronization; - } finally { - if (this.#synchronization === synchronization) { - this.#synchronization = undefined; - } - } - } + await this.#reportSyncFailures( + 'synchronizeAccounts: Account synchronization failures', + results, + accounts.map(({ id }) => id), + ); - /** - * Runs one queued repair independently of regular synchronization. Overlapping - * repair jobs share one run. An existing regular sync finishes first, while - * subsequent regular sync calls never wait for the repair. - */ - async repairAccounts(): Promise { - await this.#syncCoalescer.run('repairAccounts', async () => { + // Publish normal balance and activity updates before the slower repair scan. try { - // A failed regular sync must not prevent the independently scheduled repair. - await this.#synchronization?.catch(() => undefined); - - // The client may have locked or become inactive while waiting for sync. + // Normal synchronization may have taken long enough for the client to lock. const { active, locked } = await this.#snapClient.getClientStatus(); - if (!active || locked) { return; } diff --git a/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts b/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts index 4b4b94df3..497828e16 100644 --- a/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts +++ b/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts @@ -24,6 +24,8 @@ describe('formatBtcAmount', () => { [100_000n, '0.001'], [1_000_000n, '0.01'], [10_000_000n, '0.1'], + [10_000_001n, '0.10000001'], + [10_100_000n, '0.101'], [99_999_999n, '0.99999999'], [100_000_000n, '1'], [100_000_001n, '1.00000001'], diff --git a/packages/bitcoin-wallet-snap/src/utils/format-btc.ts b/packages/bitcoin-wallet-snap/src/utils/format-btc.ts index 4fd67c3f1..98e999ed4 100644 --- a/packages/bitcoin-wallet-snap/src/utils/format-btc.ts +++ b/packages/bitcoin-wallet-snap/src/utils/format-btc.ts @@ -9,10 +9,16 @@ */ export function formatBtcAmount(satoshis: bigint): string { const whole = satoshis / 100_000_000n; - const fraction = (satoshis % 100_000_000n) - .toString() - .padStart(8, '0') - .replace(/0+$/u, ''); + const fraction = (satoshis % 100_000_000n).toString().padStart(8, '0'); - return fraction ? `${whole}.${fraction}` : whole.toString(); + // Trim at most eight digits directly to avoid the regex backtracking pattern + // flagged. + let fractionEnd = fraction.length; + while (fractionEnd > 0 && fraction[fractionEnd - 1] === '0') { + fractionEnd -= 1; + } + + return fractionEnd > 0 + ? `${whole}.${fraction.slice(0, fractionEnd)}` + : whole.toString(); } From 2a031d53196f27519eab337699ecaa9ea93430ea Mon Sep 17 00:00:00 2001 From: Andrew Taran Date: Mon, 2 Nov 2026 16:49:01 +0100 Subject: [PATCH 4/5] fix(bitcoin): persist active discovered accounts --- packages/bitcoin-wallet-snap/CHANGELOG.md | 1 + .../src/use-cases/AccountUseCases.test.ts | 37 +++++++++++++++++++ .../src/use-cases/AccountUseCases.ts | 6 +++ 3 files changed, 44 insertions(+) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 09f146094..59516d7be 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 ### Fixed +- Persist accounts found by discovery before returning them to MetaMask, so later balance and transaction requests can load the accounts. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) - Fix formatting of small Bitcoin balances to avoid scientific notation. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) ## [3.2.0] 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 211a21e8b..815965006 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -296,6 +296,42 @@ describe('AccountUseCases', () => { beforeEach(() => { mockRepository.create.mockResolvedValue(mockAccount); + mockAccount.listTransactions.mockReturnValue([]); + }); + + it('keeps an active discovered account available for later lookups', async () => { + let storedAccount: BitcoinAccount | null = null; + mockAccount.id = 'discovered-account'; + mockAccount.listTransactions.mockReturnValue([mock()]); + mockRepository.get.mockImplementation(async () => storedAccount); + mockRepository.getByDerivationPath.mockImplementation( + async () => storedAccount, + ); + mockRepository.insert.mockImplementation(async (account) => { + storedAccount = account; + return account; + }); + + const discovered = await useCases.discover(discoverParams); + + expect(await useCases.get(discovered.id)).toBe(discovered); + expect(await useCases.discover(discoverParams)).toBe(discovered); + expect(mockRepository.create).toHaveBeenCalledTimes(1); + expect(mockChain.fullScan).toHaveBeenCalledWith(discovered, 'discovery'); + }); + + it('does not persist an inactive discovery candidate', async () => { + await useCases.discover(discoverParams); + + expect(mockRepository.insert).not.toHaveBeenCalled(); + }); + + it('does not return an active account when persistence fails', async () => { + const error = new Error('Storage unavailable'); + mockAccount.listTransactions.mockReturnValue([mock()]); + mockRepository.insert.mockRejectedValue(error); + + await expect(useCases.discover(discoverParams)).rejects.toBe(error); }); it.each([ @@ -411,6 +447,7 @@ describe('AccountUseCases', () => { expect(mockRepository.getByDerivationPath).toHaveBeenCalled(); expect(mockRepository.create).toHaveBeenCalled(); expect(mockChain.fullScan).toHaveBeenCalled(); + expect(mockRepository.insert).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 9c801f3f2..59826d2cc 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -329,6 +329,12 @@ export class AccountUseCases { // accounts with no tx history await this.#chain.fullScan(newAccount, 'discovery'); + // MetaMask keeps active discovery results, so save the scanned wallet before + // returning it. Inactive candidates remain in memory and are discarded. + if (newAccount.listTransactions().length > 0) { + await this.#repository.insert(newAccount); + } + this.#logger.info( 'Bitcoin account discovered successfully. Request: %o', req, From e1da18746e3974489d06db66fb1a57cefdf01260 Mon Sep 17 00:00:00 2001 From: Andrew Taran Date: Tue, 3 Nov 2026 13:12:46 +0100 Subject: [PATCH 5/5] fix: use createMany instead of insert for discovered accounts --- packages/bitcoin-wallet-snap/CHANGELOG.md | 1 - .../src/handlers/KeyringHandler.test.ts | 136 +++++++++++++++--- .../src/handlers/KeyringHandler.ts | 21 ++- .../src/use-cases/AccountUseCases.test.ts | 30 +--- .../src/use-cases/AccountUseCases.ts | 8 +- 5 files changed, 139 insertions(+), 57 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 59516d7be..09f146094 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -13,7 +13,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Persist accounts found by discovery before returning them to MetaMask, so later balance and transaction requests can load the accounts. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) - Fix formatting of small Bitcoin balances to avoid scientific notation. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) ## [3.2.0] diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts index 539cdb1bb..51d33dab0 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts @@ -23,6 +23,7 @@ import { import { mock } from 'jest-mock-extended'; import { assert } from 'superstruct'; +import snapManifest from '../../snap.manifest.json'; import type { BitcoinAccount, Logger, SnapClient } from '../entities'; import { AccountCapability, CurrencyUnit } from '../entities'; import type { AccountUseCases } from '../use-cases/AccountUseCases'; @@ -442,25 +443,42 @@ describe('KeyringHandler', () => { listTransactions: jest.fn().mockReturnValue(hasTxs ? [{}] : []), }); - it('discovers then returns the account when it has on-chain history', async () => { - mockAccounts.discover.mockResolvedValue(buildDiscoveredAccount(true)); + it.each([0, 5])( + 'creates active accounts and returns their persisted IDs at index %s', + async (groupIndex) => { + mockAccounts.discover.mockResolvedValue(buildDiscoveredAccount(true)); + mockAccounts.createMany.mockResolvedValue([ + buildMockAccount(groupIndex), + ]); - const result = await handler.createAccounts({ - type: AccountCreationType.Bip44Discover, - groupIndex: 0, - entropySource, - }); + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44Discover, + groupIndex, + entropySource, + }); - expect(mockAccounts.discover).toHaveBeenCalledWith({ - network: 'bitcoin', - entropySource, - index: 0, - addressType: 'p2wpkh', - }); - expect(mockAccounts.delete).not.toHaveBeenCalled(); - expect(result).toHaveLength(1); - expect(result[0]?.id).toBe('discovered-id'); - }); + expect(mockAccounts.discover).toHaveBeenCalledWith({ + network: 'bitcoin', + entropySource, + index: groupIndex, + addressType: 'p2wpkh', + }); + expect(mockAccounts.createMany).toHaveBeenCalledTimes(1); + expect(mockAccounts.createMany).toHaveBeenCalledWith([ + { + network: 'bitcoin', + entropySource, + index: groupIndex, + addressType: 'p2wpkh', + synchronize: false, + }, + ]); + expect(mockAccounts.delete).not.toHaveBeenCalled(); + expect(result).toHaveLength(1); + expect(result[0]?.id).toBe(`id-${groupIndex}`); + expect(mnemonicGroupIndex(expectDefined(result[0]))).toBe(groupIndex); + }, + ); it('returns empty array when no on-chain history', async () => { mockAccounts.discover.mockResolvedValue(buildDiscoveredAccount(false)); @@ -472,10 +490,78 @@ describe('KeyringHandler', () => { }); expect(mockAccounts.discover).toHaveBeenCalled(); + expect(mockAccounts.createMany).not.toHaveBeenCalled(); expect(mockAccounts.delete).not.toHaveBeenCalled(); expect(result).toHaveLength(0); }); + describe('multiple scopes', () => { + const { scopes } = + snapManifest.initialPermissions['endowment:keyring'].capabilities; + const originalScopes = [...scopes]; + + beforeEach(() => { + scopes.push(BtcScope.Testnet, BtcScope.Regtest); + }); + + afterEach(() => { + scopes.splice(0, scopes.length, ...originalScopes); + }); + + it.each([true, false])( + 'creates active scopes in a single batch (testnet active: %s)', + async (testnetActive) => { + mockAccounts.discover + .mockResolvedValueOnce(buildDiscoveredAccount(true)) + .mockResolvedValueOnce(buildDiscoveredAccount(testnetActive)) + .mockResolvedValueOnce(buildDiscoveredAccount(false)); + + const mainnetAccount = buildMockAccount(0); + const testnetAccount = buildMockAccount(0); + testnetAccount.id = 'testnet-id'; + testnetAccount.network = 'testnet'; + testnetAccount.derivationPath = ['myEntropy', "84'", "1'", "0'"]; + const persistedAccounts = testnetActive + ? [mainnetAccount, testnetAccount] + : [mainnetAccount]; + mockAccounts.createMany.mockResolvedValue(persistedAccounts); + + const result = await handler.createAccounts({ + type: AccountCreationType.Bip44Discover, + groupIndex: 0, + entropySource, + }); + + expect(mockAccounts.discover).toHaveBeenCalledTimes(3); + for (const [index, network] of [ + 'bitcoin', + 'testnet', + 'regtest', + ].entries()) { + expect(mockAccounts.discover).toHaveBeenNthCalledWith(index + 1, { + network, + entropySource, + index: 0, + addressType: 'p2wpkh', + }); + } + expect(mockAccounts.createMany).toHaveBeenCalledTimes(1); + expect(mockAccounts.createMany).toHaveBeenCalledWith( + persistedAccounts.map(({ network }) => ({ + network, + entropySource, + index: 0, + addressType: 'p2wpkh', + synchronize: false, + })), + ); + expect(result.map(({ id }) => id)).toStrictEqual( + persistedAccounts.map(({ id }) => id), + ); + }, + ); + }); + it('propagates errors from discover as SnapError', async () => { const error = new Error('discover error'); mockAccounts.discover.mockRejectedValue(error); @@ -487,6 +573,22 @@ describe('KeyringHandler', () => { entropySource, }), ).rejects.toThrow('discover error'); + expect(mockAccounts.createMany).not.toHaveBeenCalled(); + }); + + it('propagates errors from persisting active accounts as SnapError', async () => { + mockAccounts.discover.mockResolvedValue(buildDiscoveredAccount(true)); + mockAccounts.createMany.mockRejectedValue( + new Error('Storage unavailable'), + ); + + await expect( + handler.createAccounts({ + type: AccountCreationType.Bip44Discover, + groupIndex: 0, + entropySource, + }), + ).rejects.toThrow('Storage unavailable'); }); }); diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts index 156104c76..8638898db 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts @@ -128,10 +128,10 @@ export class KeyringHandler implements KeyringSnapRpc { } if (options.type === AccountCreationType.Bip44Discover) { - // For each supported scope, discover at this index. Accounts with - // on-chain activity are kept; those without are discarded in-memory. + // Discover at this index for every supported scope, then persist active + // accounts in one batch through the idempotent creation flow. // Returning [] signals end-of-discovery to the client. - const discovered: KeyringAccount[] = []; + const requests: CreateAccountParams[] = []; for (const scope of SUPPORTED_SCOPES) { const network = scopeToNetwork[scope]; @@ -143,11 +143,22 @@ export class KeyringHandler implements KeyringSnapRpc { }); if (account.listTransactions().length > 0) { - discovered.push(mapToKeyringAccount(account)); + requests.push({ + network, + entropySource, + index: options.groupIndex, + addressType, + synchronize: false, + }); } } - return discovered; + if (requests.length === 0) { + return []; + } + + const created = await this.#accountsUseCases.createMany(requests); + return created.map(mapToKeyringAccount); } // Build the index range. For Bip44DeriveIndex the range is a single 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 815965006..e1e9effb3 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -299,39 +299,15 @@ describe('AccountUseCases', () => { mockAccount.listTransactions.mockReturnValue([]); }); - it('keeps an active discovered account available for later lookups', async () => { - let storedAccount: BitcoinAccount | null = null; - mockAccount.id = 'discovered-account'; + it('returns the scanned candidate without persisting it', async () => { mockAccount.listTransactions.mockReturnValue([mock()]); - mockRepository.get.mockImplementation(async () => storedAccount); - mockRepository.getByDerivationPath.mockImplementation( - async () => storedAccount, - ); - mockRepository.insert.mockImplementation(async (account) => { - storedAccount = account; - return account; - }); const discovered = await useCases.discover(discoverParams); - expect(await useCases.get(discovered.id)).toBe(discovered); - expect(await useCases.discover(discoverParams)).toBe(discovered); - expect(mockRepository.create).toHaveBeenCalledTimes(1); + expect(discovered).toBe(mockAccount); expect(mockChain.fullScan).toHaveBeenCalledWith(discovered, 'discovery'); - }); - - it('does not persist an inactive discovery candidate', async () => { - await useCases.discover(discoverParams); - expect(mockRepository.insert).not.toHaveBeenCalled(); - }); - - it('does not return an active account when persistence fails', async () => { - const error = new Error('Storage unavailable'); - mockAccount.listTransactions.mockReturnValue([mock()]); - mockRepository.insert.mockRejectedValue(error); - - await expect(useCases.discover(discoverParams)).rejects.toBe(error); + expect(mockRepository.insertMany).not.toHaveBeenCalled(); }); it.each([ diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index 59826d2cc..17a250a12 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -311,7 +311,7 @@ export class AccountUseCases { const { addressType, network } = req; const derivationPath = getAccountDerivationPath(req); - // Idempotent account creation + ensures only one account per derivation path + // Reuse a stored account for this derivation path and network. const account = await this.#repository.getByDerivationPath(derivationPath); if (account?.network === network) { this.#logger.debug('Account already exists: %s,', account.id); @@ -329,12 +329,6 @@ export class AccountUseCases { // accounts with no tx history await this.#chain.fullScan(newAccount, 'discovery'); - // MetaMask keeps active discovery results, so save the scanned wallet before - // returning it. Inactive candidates remain in memory and are discarded. - if (newAccount.listTransactions().length > 0) { - await this.#repository.insert(newAccount); - } - this.#logger.info( 'Bitcoin account discovered successfully. Request: %o', req,