diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 9bceacff1..09f146094 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. ([#543](https://github.com/MetaMask/internal-snaps/pull/543)) + ## [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..8be761380 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: [] }); }); @@ -307,6 +306,221 @@ describe('CronHandler', () => { mockAccount2.listTransactions.mockReturnValue([]); }); + it('schedules a single synchronization job for normal sync and repair', () => { + expect( + snapManifest.initialPermissions['endowment:cronjob'].jobs, + ).toStrictEqual([ + { duration: 'PT30S', request: { method: 'synchronizeAccounts' } }, + ]); + }); + + 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; + }); + let notifyRepairStarted!: () => void; + const repairStarted = new Promise((resolve) => { + notifyRepairStarted = resolve; + }); + mockAccountUseCases.fullScan.mockImplementation(async () => { + notifyRepairStarted(); + return repairResult; + }); + + const firstRun = handler.synchronizeAccounts(); + let overlappingRun: Promise | undefined; + try { + await Promise.race([repairStarted, firstRun]); + expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(1); + 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: [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 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; + 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 synchronization = handler.synchronizeAccounts(); + try { + await syncStarted; + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); + } finally { + releaseSynchronization({ + account: mockAccount2, + transactionsToNotify: [], + }); + await synchronization; + } + 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 during normal synchronization', 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 synchronization = handler.synchronizeAccounts(); + mockSnapClient.getClientStatus.mockResolvedValue({ + active: true, + locked: true, + clientVersion: '1.0.0', + platformVersion: '1.0.0', + }); + releaseSynchronization({ + account: mockAccount2, + transactionsToNotify: [], + }); + await synchronization; + + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); + expect(mockState.getKey).not.toHaveBeenCalled(); + }); + + 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(request); + expect(mockState.setKey).not.toHaveBeenCalled(); + + await handler.route(request); + + expect(mockAccountUseCases.fullScan).toHaveBeenCalledTimes(2); + expect(mockState.setKey).toHaveBeenCalledWith('rescanV1', { + pending: [], + }); + }); + + 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'), + ); + await expect(handler.synchronizeAccounts()).rejects.toThrow( + 'list failed', + ); + expect(mockAccountUseCases.fullScan).not.toHaveBeenCalled(); + + 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 }, + ])('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(request); + 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 () => { const state: Pick = {}; mockState.getKey.mockResolvedValue(state.rescanV1); @@ -353,7 +567,6 @@ describe('CronHandler', () => { expect(initialPersistOrder).toBeLessThan(scanOrder as number); expect(scanOrder).toBeLessThan(shortenedPersistOrder as number); - // The normal sync flow still runs afterwards. expect(mockAccountUseCases.synchronize).toHaveBeenCalled(); }); @@ -419,13 +632,10 @@ describe('CronHandler', () => { 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); }); - 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', async () => { mockState.getKey.mockResolvedValue({ pending: ['account-1', 'account-2'], }); @@ -440,8 +650,7 @@ 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 synchronization run. expect(mockState.setKey).not.toHaveBeenCalledWith('rescanV1', { pending: ['account-2'], }); diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index dce8a6cc0..df5c59275 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -143,20 +143,9 @@ 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. + // 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 () => { - 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 +169,21 @@ export class CronHandler { results, accounts.map(({ id }) => id), ); + + // Publish normal balance and activity updates before the slower repair scan. + try { + // Normal synchronization may have taken long enough for the client to lock. + const { active, locked } = await this.#snapClient.getClientStatus(); + if (!active || locked) { + return; + } + + 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..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'); }); }); @@ -656,6 +758,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 +926,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..8638898db 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'; @@ -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 @@ -277,9 +288,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/use-cases/AccountUseCases.test.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts index 211a21e8b..e1e9effb3 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,18 @@ describe('AccountUseCases', () => { beforeEach(() => { mockRepository.create.mockResolvedValue(mockAccount); + mockAccount.listTransactions.mockReturnValue([]); + }); + + it('returns the scanned candidate without persisting it', async () => { + mockAccount.listTransactions.mockReturnValue([mock()]); + + const discovered = await useCases.discover(discoverParams); + + expect(discovered).toBe(mockAccount); + expect(mockChain.fullScan).toHaveBeenCalledWith(discovered, 'discovery'); + expect(mockRepository.insert).not.toHaveBeenCalled(); + expect(mockRepository.insertMany).not.toHaveBeenCalled(); }); it.each([ @@ -411,6 +423,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..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); 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..497828e16 --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/utils/format-btc.test.ts @@ -0,0 +1,56 @@ +/** + * @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'], + [10_000_001n, '0.10000001'], + [10_100_000n, '0.101'], + [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..98e999ed4 --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/utils/format-btc.ts @@ -0,0 +1,24 @@ +/** + * 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'); + + // 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(); +}