diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 26244d58b..a9bbf2bc9 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add `signProofOfOwnershipBatch` for signing multiple proof-of-ownership messages in one request. ([#266](https://github.com/MetaMask/internal-snaps/pull/266)) - Emit `Transaction Added`, `Transaction Approved`, and `Transaction Rejected` tracking events from Bitcoin transaction confirmations ([#328](https://github.com/MetaMask/internal-snaps/pull/328), [#329](https://github.com/MetaMask/internal-snaps/pull/329)) +### Fixed + +- Populate the `from` address of receive transactions by resolving the addresses that funded them through the chain indexer. Bitcoin inputs only reference a previous outpoint, so the sender was previously left empty. ([#372](https://github.com/MetaMask/internal-snaps/pull/372)) + ## [3.0.0] ### Added diff --git a/packages/bitcoin-wallet-snap/integration-test/constants.ts b/packages/bitcoin-wallet-snap/integration-test/constants.ts index e19d50f10..6fdc1d4c0 100644 --- a/packages/bitcoin-wallet-snap/integration-test/constants.ts +++ b/packages/bitcoin-wallet-snap/integration-test/constants.ts @@ -18,7 +18,20 @@ export const FUNDING_TX = { { status: 'confirmed', timestamp: expect.any(Number) }, ], fees: [], - from: [], + // The receive counterparty is resolved from the chain indexer, so the sender + // address depends on the funding wallet used by the regtest fixture. A tx + // can be funded by more than one address, so match a subset. + from: expect.arrayContaining([ + { + address: expect.any(String), + asset: { + amount: '0', + fungible: true, + type: Caip19Asset.Regtest, + unit: CurrencyUnit.Regtest, + }, + }, + ]), id: expect.any(String), status: 'confirmed', timestamp: expect.any(Number), diff --git a/packages/bitcoin-wallet-snap/jest.config.js b/packages/bitcoin-wallet-snap/jest.config.js index 7dc78cfb5..a32eb6d48 100644 --- a/packages/bitcoin-wallet-snap/jest.config.js +++ b/packages/bitcoin-wallet-snap/jest.config.js @@ -15,10 +15,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 74.89, - functions: 62.34, - lines: 82.62, - statements: 82.4, + branches: 75.74, + functions: 64.28, + lines: 83.25, + statements: 83.03, }, }, }; diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 65633c1f6..9f3f32674 100644 --- a/packages/bitcoin-wallet-snap/snap.manifest.json +++ b/packages/bitcoin-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "ym8AM1UALP/J8Al0hkltshJPyaZppBN8f0WpLj0JIJc=", + "shasum": "TVsL0SsAKXSyycYFuS00hOQzsg91kkRB8PpFDopVrOM=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/bitcoin-wallet-snap/src/entities/chain.ts b/packages/bitcoin-wallet-snap/src/entities/chain.ts index 77c29eb93..c6a9e63ca 100644 --- a/packages/bitcoin-wallet-snap/src/entities/chain.ts +++ b/packages/bitcoin-wallet-snap/src/entities/chain.ts @@ -56,4 +56,18 @@ export type BlockchainClient = { * @returns the base url of the explorer */ getExplorerUrl(network: Network): string; + + /** + * Resolve the distinct addresses that funded a transaction (the addresses of + * the outputs its inputs spend). + * + * Bitcoin inputs only reference the previous outpoint, so this requires the + * chain indexer and cannot be derived from the transaction alone. Used to + * populate the counterparty of receive transactions. + * + * @param network - Network the transaction belongs to. + * @param txid - Transaction id. + * @returns The funding addresses, deduped, in input order. + */ + getTransactionSenders(network: Network, txid: string): Promise; }; diff --git a/packages/bitcoin-wallet-snap/src/entities/snap.ts b/packages/bitcoin-wallet-snap/src/entities/snap.ts index 1435290fb..f7a1cf1aa 100644 --- a/packages/bitcoin-wallet-snap/src/entities/snap.ts +++ b/packages/bitcoin-wallet-snap/src/entities/snap.ts @@ -55,6 +55,9 @@ export type SyncResult = { account: BitcoinAccount; // Transactions that changed and should be notified. transactionsToNotify: WalletTx[]; + // Funding addresses per txid, resolved by the chain indexer. Only populated + // for receives; used to display the counterparty. + transactionSenders?: Map; }; export const TrackingSnapEvent = { @@ -140,10 +143,13 @@ export type SnapClient = { * * @param account - The Bitcoin account. * @param txs - The transactions included in the event. + * @param sendersByTxid - Optional funding addresses per txid, used to + * populate the counterparty of receive transactions. */ emitAccountTransactionsUpdatedEvent( account: BitcoinAccount, txs: WalletTx[], + sendersByTxid?: Map, ): Promise; /** diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index 55c2fa4f0..7ae8a3e5a 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -105,6 +105,25 @@ describe('CronHandler', () => { ).toHaveBeenCalledTimes(1); }); + it('forwards resolved senders when emitting transaction events', async () => { + const mockTx = mock(); + const senders = new Map([['txid-1', ['bc1qsender']]]); + const mockResult1: SyncResult = { + account: mockAccount1, + transactionsToNotify: [mockTx], + transactionSenders: senders, + }; + (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-1']); + mockAccountUseCases.list.mockResolvedValue([mockAccount1]); + mockAccountUseCases.synchronize.mockResolvedValueOnce(mockResult1); + + await handler.route(request); + + expect( + mockSnapClient.emitAccountTransactionsUpdatedEvent, + ).toHaveBeenCalledWith(mockAccount1, [mockTx], senders); + }); + it('propagates errors from list', async () => { const error = new Error(); (getSelectedAccounts as jest.Mock).mockResolvedValue(['account-1']); diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index 35fd8c574..5fa6c466f 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -228,11 +228,16 @@ export class CronHandler { ); // Emit transaction events per account - for (const { account, transactionsToNotify } of results) { + for (const { + account, + transactionsToNotify, + transactionSenders, + } of results) { if (transactionsToNotify.length > 0) { await this.#snapClient.emitAccountTransactionsUpdatedEvent( account, transactionsToNotify, + ...(transactionSenders ? [transactionSenders] : []), ); } } diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts index ba98bf292..c27159d9e 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.test.ts @@ -875,6 +875,7 @@ describe('KeyringHandler', () => { mockAccounts.get.mockResolvedValue(mockAccount); mockAccount.sentAndReceived.mockReturnValue([mockAmount, mockAmount]); mockAccount.listTransactions.mockReturnValue([mockWalletTx]); + mockAccounts.resolveTransactionSenders.mockResolvedValue(new Map()); (Address.from_script as jest.Mock).mockReturnValue(mockAddress); }); @@ -927,6 +928,37 @@ describe('KeyringHandler', () => { ]); }); + it('populates the receive counterparty from resolved senders', async () => { + const id = 'some-id'; + + mockAccount.sentAndReceived.mockReturnValueOnce([ + { ...mockAmount, to_btc: (): number => 0 }, + mockAmount, + ]); + mockAccount.isMine.mockReturnValueOnce(true); + mockAccounts.resolveTransactionSenders.mockResolvedValue( + new Map([['txid', ['bc1qsender']]]), + ); + + const result = await handler.getAccountTransactions(id, pagination); + + expect(mockAccounts.resolveTransactionSenders).toHaveBeenCalledWith( + mockAccount, + [mockWalletTx], + ); + expect(result.data[0]?.from).toStrictEqual([ + { + address: 'bc1qsender', + asset: { + amount: '0', + fungible: true, + type: Caip19Asset.Bitcoin, + unit: CurrencyUnit.Bitcoin, + }, + }, + ]); + }); + it('respects limit and sets next to last txid', async () => { const id = 'some-id'; const mockTransactions = Array.from({ length: 12 }, (_, index) => ({ diff --git a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts index aad24bba1..084f80ecc 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/KeyringHandler.ts @@ -320,8 +320,19 @@ export class KeyringHandler implements KeyringSnapRpc { ? (paginatedTxs[paginatedTxs.length - 1]?.txid.toString() ?? null) : null; + // Resolve the counterparty for receives from external senders. Only done + // for the returned page so the cost stays bounded, and best-effort so a + // transient indexer failure cannot break transaction listing. + const sendersByTxid = + await this.#accountsUseCases.resolveTransactionSenders( + account, + paginatedTxs, + ); + return { - data: paginatedTxs.map((tx) => mapToTransaction(account, tx)), + data: paginatedTxs.map((tx) => + mapToTransaction(account, tx, sendersByTxid?.get(tx.txid.toString())), + ), next: nextCursor, }; } diff --git a/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts b/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts index 613b54ddc..b1e6490da 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/mappings.test.ts @@ -403,6 +403,7 @@ describe('mapToTransaction', () => { return mock({ compute_txid: () => mockTxid, output: outputs, + input: [], }); } @@ -531,4 +532,89 @@ describe('mapToTransaction', () => { 'bc1qstku2y3pfh9av50lxj55arm8r5gj8tf2yv5nxz', ); }); + + it('populates from for a receive using the indexer-provided senders', () => { + const account = createMockAccount(0); // received: sent amount is 0 + const output = createMockOutput(5000); + const transaction = createMockTransaction('receive111', [output]); + + jest.spyOn(account, 'isMine').mockReturnValue(true); + jest.mocked(Address.from_script).mockImplementationOnce( + () => + ({ + toString: () => 'bc1qreceiveoutput', + }) as unknown as Address, + ); + + const result = mapToTransaction( + account, + { + tx: transaction, + txid: transaction.compute_txid(), + chain_position: { anchor: undefined, last_seen: undefined }, + } as unknown as WalletTx, + ['bc1qsender1', 'bc1qsender2'], + ); + + expect(result.type).toBe('receive'); + expect(result.from.map((movement) => movement.address)).toStrictEqual([ + 'bc1qsender1', + 'bc1qsender2', + ]); + expect(result.from[0]?.asset).toStrictEqual({ + amount: '0', + fungible: true, + unit: 'BTC', + type: Caip19Asset.Bitcoin, + }); + }); + + it('deduplicates and ignores empty senders for a receive', () => { + const account = createMockAccount(0); + const output = createMockOutput(5000); + const transaction = createMockTransaction('receive222', [output]); + + jest.spyOn(account, 'isMine').mockReturnValue(true); + jest + .mocked(Address.from_script) + .mockImplementationOnce( + () => ({ toString: () => 'bc1qreceiveoutput' }) as unknown as Address, + ); + + const result = mapToTransaction( + account, + { + tx: transaction, + txid: transaction.compute_txid(), + chain_position: { anchor: undefined, last_seen: undefined }, + } as unknown as WalletTx, + ['bc1qsender1', 'bc1qsender1', 'bc1qsender2'], + ); + + expect(result.from.map((movement) => movement.address)).toStrictEqual([ + 'bc1qsender1', + 'bc1qsender2', + ]); + }); + + it('leaves from empty for a receive when no senders were resolved', () => { + const account = createMockAccount(0); + const output = createMockOutput(5000); + const transaction = createMockTransaction('receive333', [output]); + + jest.spyOn(account, 'isMine').mockReturnValue(true); + jest + .mocked(Address.from_script) + .mockImplementationOnce( + () => ({ toString: () => 'bc1qreceiveoutput' }) as unknown as Address, + ); + + const result = mapToTransaction(account, { + tx: transaction, + txid: transaction.compute_txid(), + chain_position: { anchor: undefined, last_seen: undefined }, + } as unknown as WalletTx); + + expect(result.from).toStrictEqual([]); + }); }); diff --git a/packages/bitcoin-wallet-snap/src/handlers/mappings.ts b/packages/bitcoin-wallet-snap/src/handlers/mappings.ts index 01f49389d..42ae9d92d 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/mappings.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/mappings.ts @@ -155,11 +155,15 @@ export function mapToTransactionFees( * * @param account - The account account. * @param walletTx - The Bitcoin transaction managed by this account. + * @param senders - Funding addresses of the transaction, resolved by the chain + * indexer. Only used for receives: Bitcoin inputs reference a previous outpoint + * rather than an address, so the counterparty cannot be derived locally. * @returns The Keyring transaction. */ export function mapToTransaction( account: BitcoinAccount, walletTx: WalletTx, + senders: string[] = [], ): KeyringTransaction { const { tx, chain_position: chainPosition, txid } = walletTx; const { network } = account; @@ -188,7 +192,9 @@ export function mapToTransaction( // - from: empty as irrelevant because we might be sending from multiple addresses. Sufficient to say "Sent from Bitcoin Account". // If it's a Receive transaction: // - to: all the outputs spending to addresses we own. - // - from: empty as irrevelant because we might have hundreds of inputs in a tx. Point to explorer for details. + // - from: the addresses that funded the transaction, so the counterparty can + // be displayed instead of "Unavailable"/blank. Sourced from the chain + // indexer (`senders`), since inputs only reference a previous outpoint. if (isSend) { for (const txout of tx.output) { // Only the change output is filtered out. Outputs to an address we own @@ -209,6 +215,20 @@ export function mapToTransaction( } } } + + transaction.from = [...new Set(senders)].map((address) => ({ + address, + // Bitcoin has a single fee paid by the sender; the received amount is + // already surfaced through `to`. The counterparty only needs its + // address, so a zero amount keeps the movement shape valid without + // implying the sender spent nothing. + asset: { + amount: '0', + fungible: true, + unit: networkToCurrencyUnit[network], + type: networkToCaip19[network], + }, + })); } return transaction; diff --git a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts index 8c73769ea..3c31994fa 100644 --- a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.test.ts @@ -3,7 +3,10 @@ import { EsploraClient } from '@metamask/bitcoindevkit'; import { mock } from 'jest-mock-extended'; import type { BitcoinAccount, ChainConfig } from '../entities'; -import { EsploraClientAdapter } from './EsploraClientAdapter'; +import { + EsploraClientAdapter, + SENDER_REQUEST_TIMEOUT_MS, +} from './EsploraClientAdapter'; jest.mock('@metamask/bitcoindevkit', () => ({ EsploraClient: jest.fn(), @@ -65,4 +68,150 @@ describe('EsploraClientAdapter', () => { ); }); }); + + describe('getTransactionSenders', () => { + const mockFetch = jest.fn(); + let abortController: AbortController; + + beforeEach(() => { + global.fetch = mockFetch; + abortController = new AbortController(); + // Drive the abort path deterministically instead of leaving a real + // 10-second timer pending on each call. + jest + .spyOn(AbortSignal, 'timeout') + .mockReturnValue(abortController.signal); + }); + + afterEach(() => { + mockFetch.mockReset(); + }); + + const okResponse = ( + body: unknown, + ): { ok: boolean; json: () => Promise } => ({ + ok: true, + json: async (): Promise => body, + }); + + const txCallCount = (): number => + mockFetch.mock.calls.filter(([url]) => String(url).includes('/tx/')) + .length; + + it('returns the deduped prevout addresses of every input', async () => { + const { adapter } = setupTest(); + mockFetch.mockResolvedValue( + okResponse({ + vin: [ + { prevout: { scriptpubkey_address: 'bc1qsender1' } }, + { prevout: { scriptpubkey_address: 'bc1qsender2' } }, + { prevout: { scriptpubkey_address: 'bc1qsender1' } }, + ], + }), + ); + + const senders = await adapter.getTransactionSenders('bitcoin', 'txid'); + + expect(senders).toStrictEqual(['bc1qsender1', 'bc1qsender2']); + expect(mockFetch).toHaveBeenCalledWith( + 'https://bitcoin.example/tx/txid', + { + signal: abortController.signal, + }, + ); + expect(AbortSignal.timeout).toHaveBeenCalledWith( + SENDER_REQUEST_TIMEOUT_MS, + ); + }); + + it('rejects and evicts the cache when the request times out', async () => { + const { adapter } = setupTest(); + mockFetch.mockImplementation( + (_url: unknown, init?: { signal?: AbortSignal }) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => + reject(new Error('TimeoutError')), + ); + }), + ); + + const pending = adapter.getTransactionSenders('bitcoin', 'txid'); + abortController.abort(); + + await expect(pending).rejects.toThrow('TimeoutError'); + + // The failure is not cached, so the next lookup retries. + mockFetch.mockRejectedValue(new Error('TimeoutError')); + await expect( + adapter.getTransactionSenders('bitcoin', 'txid'), + ).rejects.toThrow('TimeoutError'); + expect(txCallCount()).toBe(2); + }); + + it('skips inputs without a resolved prevout (e.g. coinbase)', async () => { + const { adapter } = setupTest(); + mockFetch.mockResolvedValue( + okResponse({ + vin: [ + { prevout: null }, + { prevout: { scriptpubkey_address: 'bc1qsender' } }, + {}, + ], + }), + ); + + const senders = await adapter.getTransactionSenders('bitcoin', 'txid'); + + expect(senders).toStrictEqual(['bc1qsender']); + }); + + it('strips a mempool.space /v1 suffix from the configured url', async () => { + const mockEsploraClient = mock(); + jest.mocked(EsploraClient).mockReturnValue(mockEsploraClient); + const adapter = new EsploraClientAdapter( + mock({ + url: { + bitcoin: 'https://mempool.space/api/v1', + testnet: 'https://testnet.example', + testnet4: 'https://testnet4.example', + signet: 'https://signet.example', + regtest: 'https://regtest.example', + }, + }), + ); + mockFetch.mockResolvedValue(okResponse({ vin: [] })); + + await adapter.getTransactionSenders('bitcoin', 'txid'); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://mempool.space/api/tx/txid', + { signal: abortController.signal }, + ); + }); + + it('caches results per network and txid', async () => { + const { adapter } = setupTest(); + mockFetch.mockResolvedValue( + okResponse({ vin: [{ prevout: { scriptpubkey_address: 'bc1q' } }] }), + ); + + await adapter.getTransactionSenders('bitcoin', 'txid'); + await adapter.getTransactionSenders('bitcoin', 'txid'); + + expect(txCallCount()).toBe(1); + }); + + it('throws and does not cache the failure for a non-2xx response', async () => { + const { adapter } = setupTest(); + mockFetch.mockResolvedValue({ ok: false, status: 429 }); + + await expect( + adapter.getTransactionSenders('bitcoin', 'txid'), + ).rejects.toThrow('Failed to fetch transaction'); + await expect( + adapter.getTransactionSenders('bitcoin', 'txid'), + ).rejects.toThrow('Failed to fetch transaction'); + expect(txCallCount()).toBe(2); + }); + }); }); diff --git a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts index 8710ecdc7..3ae7b6ad4 100644 --- a/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/EsploraClientAdapter.ts @@ -12,12 +12,58 @@ import type { BlockchainClient, } from '../entities'; +/** + * Minimal subset of the Esplora/Blockstream REST `/tx/:txid` response needed to + * resolve the addresses that funded a transaction. Each input carries a + * `prevout` describing the output it spends, including the address that + * received it, which the WASM client does not surface. + */ +type EsploraTxVin = { + /* eslint-disable @typescript-eslint/naming-convention -- Mirrors the Esplora REST API response. */ + prevout?: { + scriptpubkey_address?: string; + } | null; + /* eslint-enable @typescript-eslint/naming-convention */ +}; + +type EsploraTx = { + vin: EsploraTxVin[]; +}; + +/** + * Strips trailing slashes and mempool.space's `/v1` API prefix so the raw + * Esplora REST paths (`/tx/:txid`, `/blocks/tip/height`, ...) can be appended. + * The WASM client accepts the `/v1` base for some of its own endpoints, but the + * transaction endpoint lives at the root. + * + * @param url - Configured Esplora base URL. + * @returns The base URL without a trailing slash or `/v1` suffix. + */ +function toEsploraRestUrl(url: string): string { + return url.replace(/\/+$/u, '').replace(/\/v1$/u, ''); +} + +/** + * Upper bound on a single sender lookup request. A stalled connection would + * otherwise never settle, blocking the sync that awaits it and suppressing the + * transaction event; the WASM client applies its own retry budget, so the raw + * REST lookup needs an equivalent bound. + */ +export const SENDER_REQUEST_TIMEOUT_MS = 10_000; + export class EsploraClientAdapter implements BlockchainClient { // Should be a Repository but we don't support custom networks so we can save in memory from config values readonly #clients: Record; readonly #config: ChainConfig; + readonly #restUrls: Record; + + // Funding addresses are resolved while mapping transactions, which runs on + // every sync/event emission. Cache by network + txid so a receive is only + // looked up once instead of on each notification. + readonly #sendersCache = new Map>(); + constructor(config: ChainConfig) { this.#clients = { bitcoin: new EsploraClient(config.url.bitcoin, config.maxRetries), @@ -27,6 +73,14 @@ export class EsploraClientAdapter implements BlockchainClient { regtest: new EsploraClient(config.url.regtest, config.maxRetries), }; + this.#restUrls = { + bitcoin: toEsploraRestUrl(config.url.bitcoin), + testnet: toEsploraRestUrl(config.url.testnet), + testnet4: toEsploraRestUrl(config.url.testnet4), + signet: toEsploraRestUrl(config.url.signet), + regtest: toEsploraRestUrl(config.url.regtest), + }; + this.#config = config; } @@ -99,4 +153,68 @@ export class EsploraClientAdapter implements BlockchainClient { getExplorerUrl(network: Network): string { return this.#config.explorerUrl[network]; } + + async getTransactionSenders( + network: Network, + txid: string, + ): Promise { + const cacheKey = `${network}:${txid}`; + const cached = this.#sendersCache.get(cacheKey); + if (cached) { + return cached; + } + + const request = this.#fetchTransactionSenders(network, txid); + this.#sendersCache.set(cacheKey, request); + + try { + return await request; + } catch (error) { + // Do not keep failures cached: a rate limit or transient outage should + // not permanently suppress a counterparty. + this.#sendersCache.delete(cacheKey); + throw error; + } + } + + /** + * Fetches the funding addresses of a transaction from the Esplora REST API. + * + * Unlike the WASM client's `get_tx`, the REST endpoint resolves each input's + * `prevout`, so all senders are returned in a single request. The request is + * bounded by {@link SENDER_REQUEST_TIMEOUT_MS} so a stalled indexer surfaces + * as a best-effort failure instead of hanging the caller. + * + * @param network - Network the transaction belongs to. + * @param txid - Transaction id. + * @returns The funding addresses, deduped, in input order. + */ + async #fetchTransactionSenders( + network: Network, + txid: string, + ): Promise { + const response = await fetch(`${this.#restUrls[network]}/tx/${txid}`, { + signal: AbortSignal.timeout(SENDER_REQUEST_TIMEOUT_MS), + }); + + if (!response.ok) { + throw new ExternalServiceError(`Failed to fetch transaction`, { + network, + txid, + status: response.status, + }); + } + + const transaction = (await response.json()) as EsploraTx; + // Self-transfers and consolidations can repeat the same funding address; + // dedupe while preserving input order. + const senders = [ + ...new Set( + transaction.vin + .map((input) => input.prevout?.scriptpubkey_address) + .filter((address): address is string => Boolean(address)), + ), + ]; + return senders; + } } diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts index 65803cfa8..c414a2084 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts @@ -1,4 +1,4 @@ -import type { WalletTx } from '@metamask/bitcoindevkit'; +import type { Amount, WalletTx } from '@metamask/bitcoindevkit'; import { getJsonError } from '@metamask/snaps-sdk'; import { mock } from 'jest-mock-extended'; @@ -426,4 +426,85 @@ describe('SnapClientAdapter', () => { ); }); }); + + describe('emitAccountTransactionsUpdatedEvent', () => { + const createWalletTx = (txid: string): WalletTx => + mock({ + txid: { toString: () => txid }, + tx: { output: [] }, + chain_position: { is_confirmed: false }, + }); + + /** + * Creates an account whose transactions are treated as receives, so the + * mapper takes the counterparty path under test. + * + * @returns A mocked Bitcoin account receiving funds. + */ + const createReceiveAccount = (): BitcoinAccount => { + const account = mock({ + id: 'account-1', + network: 'bitcoin', + addressType: 'p2wpkh', + }); + const receivedAmount = mock(); + jest.spyOn(receivedAmount, 'to_btc').mockReturnValue(0); + account.sentAndReceived.mockReturnValue([receivedAmount, mock()]); + account.isMine.mockReturnValue(true); + return account; + }; + + it('maps senders onto the emitted transactions', async () => { + const { snapClient, mockRequest } = setupTest(); + mockRequest.mockResolvedValue(undefined); + + await snapClient.emitAccountTransactionsUpdatedEvent( + createReceiveAccount(), + [createWalletTx('txid-receive')], + new Map([['txid-receive', ['bc1qsender']]]), + ); + + const emitted = mockRequest.mock.calls[0]?.[0] as { + params: { + params: { + transactions: Record; + }; + }; + }; + expect( + emitted.params.params.transactions['account-1']?.[0]?.from, + ).toStrictEqual([ + { + address: 'bc1qsender', + asset: { + amount: '0', + fungible: true, + unit: 'BTC', + type: 'bip122:000000000019d6689c085ae165831e93/slip44:0', + }, + }, + ]); + }); + + it('emits transactions without a counterparty when no senders are given', async () => { + const { snapClient, mockRequest } = setupTest(); + mockRequest.mockResolvedValue(undefined); + + await snapClient.emitAccountTransactionsUpdatedEvent( + createReceiveAccount(), + [createWalletTx('txid-receive')], + ); + + const emitted = mockRequest.mock.calls[0]?.[0] as { + params: { + params: { + transactions: Record; + }; + }; + }; + expect( + emitted.params.params.transactions['account-1']?.[0]?.from, + ).toStrictEqual([]); + }); + }); }); diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts index c3e067f12..56817e1d3 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts @@ -121,10 +121,17 @@ export class SnapClientAdapter implements SnapClient { async emitAccountTransactionsUpdatedEvent( account: BitcoinAccount, txs: WalletTx[], + sendersByTxid?: Map, ): Promise { return emitSnapKeyringEvent(snap, KeyringEvent.AccountTransactionsUpdated, { transactions: { - [account.id]: txs.map((tx) => mapToTransaction(account, tx)), + [account.id]: txs.map((tx) => + mapToTransaction( + account, + tx, + sendersByTxid?.get(tx.txid.toString()) ?? [], + ), + ), }, }); } 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 22c77adf0..ed3985af5 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -40,7 +40,11 @@ import type { CreateAccountParams, DiscoverAccountParams, } from './AccountUseCases'; -import { AccountUseCases } from './AccountUseCases'; +import { + AccountUseCases, + SENDER_LOOKUP_CONCURRENCY, + SENDER_RESOLUTION_LIMIT, +} from './AccountUseCases'; describe('AccountUseCases', () => { const mockLogger = mock(); @@ -425,11 +429,19 @@ describe('AccountUseCases', () => { describe('synchronize', () => { const mockAccount = mock({ id: 'some-id', + network: 'bitcoin', listTransactions: jest.fn(), }); beforeEach(() => { + const receivedAmount = mock(); + jest.spyOn(receivedAmount, 'to_btc').mockReturnValue(0); mockAccount.listTransactions.mockReturnValue([]); + mockAccount.sentAndReceived.mockReturnValue([ + receivedAmount, + receivedAmount, + ]); + mockChain.getTransactionSenders.mockResolvedValue([]); }); it('synchronizes', async () => { @@ -443,6 +455,7 @@ describe('AccountUseCases', () => { expect(result).toStrictEqual({ account: mockAccount, transactionsToNotify: [], + transactionSenders: undefined, }); }); @@ -475,6 +488,29 @@ describe('AccountUseCases', () => { expect(result).toStrictEqual({ account: mockAccount, transactionsToNotify: [mockTransaction], + transactionSenders: undefined, + }); + }); + + it('resolves senders for newly received transactions', async () => { + const mockTransaction2 = mock({ + txid: { toString: () => 'txid-receive' }, + }); + mockAccount.listTransactions + .mockReturnValueOnce([]) + .mockReturnValueOnce([mockTransaction2]); + mockChain.getTransactionSenders.mockResolvedValue(['bc1qsender']); + + const result = await useCases.synchronize(mockAccount, 'test'); + + expect(mockChain.getTransactionSenders).toHaveBeenCalledWith( + 'bitcoin', + 'txid-receive', + ); + expect(result).toStrictEqual({ + account: mockAccount, + transactionsToNotify: [mockTransaction2], + transactionSenders: new Map([['txid-receive', ['bc1qsender']]]), }); }); @@ -508,6 +544,7 @@ describe('AccountUseCases', () => { expect(result).toStrictEqual({ account: mockAccount, transactionsToNotify: [mockTxConfirmed], + transactionSenders: undefined, }); }); @@ -586,6 +623,7 @@ describe('AccountUseCases', () => { expect(result).toStrictEqual({ account: mockAccount, transactionsToNotify: [mockTxConfirmed, mockTxNew, mockTxReorged], + transactionSenders: undefined, }); }); @@ -613,6 +651,7 @@ describe('AccountUseCases', () => { expect(result).toStrictEqual({ account: mockAccount, transactionsToNotify: [mockTxReorged], + transactionSenders: undefined, }); }); @@ -707,6 +746,7 @@ describe('AccountUseCases', () => { expect(result).toStrictEqual({ account: mockAccount, transactionsToNotify: [mockTransaction], + transactionSenders: undefined, }); }); }); @@ -716,11 +756,22 @@ describe('AccountUseCases', () => { id: 'some-id', }); const mockInscriptions = mock(); - const mockTransactions = mock(); + const mockTransactions: WalletTx[] = [mock()]; + + beforeEach(() => { + const receivedAmount = mock(); + jest.spyOn(receivedAmount, 'to_btc').mockReturnValue(0); + mockAccount.sentAndReceived.mockReturnValue([ + receivedAmount, + receivedAmount, + ]); + mockAccount.listTransactions.mockReturnValue([]); + }); it('performs a full scan', async () => { mockAccount.listTransactions.mockReturnValue(mockTransactions); mockMetaProtocols.fetchInscriptions.mockResolvedValue(mockInscriptions); + mockChain.getTransactionSenders.mockResolvedValue([]); const result = await useCases.fullScan(mockAccount); @@ -735,6 +786,7 @@ describe('AccountUseCases', () => { expect(result).toStrictEqual({ account: mockAccount, transactionsToNotify: mockTransactions, + transactionSenders: undefined, }); }); @@ -787,6 +839,241 @@ describe('AccountUseCases', () => { expect(mockMetaProtocols.fetchInscriptions).not.toHaveBeenCalled(); }); + + it('resolves senders for the scanned transactions', async () => { + const scannedTx = mock({ + txid: { toString: () => 'txid-receive' }, + }); + mockAccount.listTransactions.mockReturnValue([scannedTx]); + mockMetaProtocols.fetchInscriptions.mockResolvedValue(mockInscriptions); + mockChain.getTransactionSenders.mockResolvedValue(['bc1qsender']); + + const result = await useCases.fullScan(mockAccount); + + expect(result.transactionSenders).toStrictEqual( + new Map([['txid-receive', ['bc1qsender']]]), + ); + }); + + it('caps the sender lookups so the scan does not resolve the whole history', async () => { + const scannedTxs = Array.from( + { length: SENDER_RESOLUTION_LIMIT + 5 }, + (_, index) => + mock({ txid: { toString: () => `txid-${index}` } }), + ); + mockAccount.listTransactions.mockReturnValue(scannedTxs); + mockChain.getTransactionSenders.mockResolvedValue(['bc1qsender']); + + await useCases.fullScan(mockAccount); + + expect(mockChain.getTransactionSenders).toHaveBeenCalledTimes( + SENDER_RESOLUTION_LIMIT, + ); + }); + }); + + describe('resolveTransactionSenders', () => { + const createTx = (txid: string): WalletTx => + mock({ txid: { toString: () => txid } }); + + const mockAccount2 = mock({ + id: 'some-id', + network: 'bitcoin', + }); + + const mockSentAmount = mock(); + const mockReceivedAmount = mock(); + + beforeEach(() => { + mockAccount2.sentAndReceived.mockReturnValue([ + mockSentAmount, + mockReceivedAmount, + ]); + }); + + it('resolves the funding addresses of receives', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + mockChain.getTransactionSenders.mockResolvedValue(['bc1qsender']); + + const result = await useCases.resolveTransactionSenders(mockAccount2, [ + createTx('txid-receive'), + ]); + + expect(mockChain.getTransactionSenders).toHaveBeenCalledWith( + 'bitcoin', + 'txid-receive', + ); + expect(result).toStrictEqual(new Map([['txid-receive', ['bc1qsender']]])); + }); + + it('does not query the indexer for sends', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0.5); + + const result = await useCases.resolveTransactionSenders(mockAccount2, [ + createTx('txid-send'), + ]); + + expect(mockChain.getTransactionSenders).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + }); + + it('returns undefined when receives have no senders', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + mockChain.getTransactionSenders.mockResolvedValue([]); + + const result = await useCases.resolveTransactionSenders(mockAccount2, [ + createTx('txid-receive'), + ]); + + expect(result).toBeUndefined(); + }); + + it('skips a failed lookup without failing the others', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + mockChain.getTransactionSenders + .mockRejectedValueOnce(new Error('indexer unavailable')) + .mockResolvedValueOnce(['bc1qsender']); + + const result = await useCases.resolveTransactionSenders(mockAccount2, [ + createTx('txid-fail'), + createTx('txid-ok'), + ]); + + expect(result).toStrictEqual(new Map([['txid-ok', ['bc1qsender']]])); + expect(mockLogger.debug).toHaveBeenCalledWith( + 'Failed to resolve transaction senders: %o', + expect.any(Error), + ); + }); + + it('bounds concurrent indexer lookups', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + + let inFlight = 0; + let peak = 0; + mockChain.getTransactionSenders.mockImplementation(async () => { + inFlight += 1; + peak = Math.max(peak, inFlight); + await Promise.resolve(); + inFlight -= 1; + return ['bc1qsender']; + }); + + const txs = Array.from( + { length: SENDER_LOOKUP_CONCURRENCY * 2 + 1 }, + (_, index) => createTx(`txid-${index}`), + ); + + await useCases.resolveTransactionSenders(mockAccount2, txs); + + expect(mockChain.getTransactionSenders).toHaveBeenCalledTimes(txs.length); + expect(peak).toBeLessThanOrEqual(SENDER_LOOKUP_CONCURRENCY); + expect(peak).toBeGreaterThan(1); + }); + + it('shares the concurrency budget across accounts', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + + let inFlight = 0; + let peak = 0; + mockChain.getTransactionSenders.mockImplementation(async () => { + inFlight += 1; + peak = Math.max(peak, inFlight); + await Promise.resolve(); + inFlight -= 1; + return ['bc1qsender']; + }); + + const mockOtherAccount = mock({ + id: 'other-id', + network: 'bitcoin', + }); + mockOtherAccount.sentAndReceived.mockReturnValue([ + mockSentAmount, + mockReceivedAmount, + ]); + + const txs = Array.from( + { length: SENDER_LOOKUP_CONCURRENCY * 2 }, + (_, index) => createTx(`txid-${index}`), + ); + + // `synchronize` runs concurrently for every selected account, so two + // accounts resolving at once must not reach 2x the shared budget. + await Promise.all([ + useCases.resolveTransactionSenders(mockAccount2, txs), + useCases.resolveTransactionSenders(mockOtherAccount, txs), + ]); + + expect(peak).toBeLessThanOrEqual(SENDER_LOOKUP_CONCURRENCY); + }); + + it('releases the concurrency slot when a lookup fails', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + mockChain.getTransactionSenders.mockRejectedValue( + new Error('indexer unavailable'), + ); + + const failingTxs = Array.from( + { length: SENDER_LOOKUP_CONCURRENCY }, + (_, index) => createTx(`txid-fail-${index}`), + ); + + await useCases.resolveTransactionSenders(mockAccount2, failingTxs); + + // A limiter that leaks a slot on rejection would drain the budget and + // hang every later lookup. + mockChain.getTransactionSenders.mockResolvedValue(['bc1qsender']); + const result = await useCases.resolveTransactionSenders(mockAccount2, [ + createTx('txid-after-failure'), + ]); + + expect(result).toStrictEqual( + new Map([['txid-after-failure', ['bc1qsender']]]), + ); + }); + + it('caps the lookups at the given limit', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + mockChain.getTransactionSenders.mockResolvedValue(['bc1qsender']); + + const txs = [createTx('txid-1'), createTx('txid-2'), createTx('txid-3')]; + + const result = await useCases.resolveTransactionSenders( + mockAccount2, + txs, + 2, + ); + + expect(mockChain.getTransactionSenders).toHaveBeenCalledTimes(2); + expect(mockChain.getTransactionSenders).toHaveBeenCalledWith( + 'bitcoin', + 'txid-1', + ); + expect(mockChain.getTransactionSenders).not.toHaveBeenCalledWith( + 'bitcoin', + 'txid-3', + ); + expect(result).toStrictEqual( + new Map([ + ['txid-1', ['bc1qsender']], + ['txid-2', ['bc1qsender']], + ]), + ); + }); + + it('resolves every receive when no limit is given', async () => { + jest.spyOn(mockSentAmount, 'to_btc').mockReturnValue(0); + mockChain.getTransactionSenders.mockResolvedValue(['bc1qsender']); + + const txs = Array.from({ length: SENDER_RESOLUTION_LIMIT + 5 }, (_, i) => + createTx(`txid-${i}`), + ); + + await useCases.resolveTransactionSenders(mockAccount2, txs); + + expect(mockChain.getTransactionSenders).toHaveBeenCalledTimes(txs.length); + }); }); describe('delete', () => { diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index cadb47185..d49f2b8b0 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -10,7 +10,10 @@ import type { import type { BIP32Node } from '@metamask/key-tree'; import { SLIP10Node } from '@metamask/key-tree'; import { getCurrentUnixTimestamp } from '@metamask/keyring-snap-sdk'; -import { normalizeError } from '@metamask/snap-networks-utils'; +import { + batchesAllSettled, + normalizeError, +} from '@metamask/snap-networks-utils'; import { Signer } from 'bip322-js'; import { encode } from 'wif'; @@ -157,6 +160,68 @@ export type BroadcastResult = { canBeMalleable: boolean; }; +/** + * Maximum number of indexer lookups in flight at once, shared across every + * account. Each lookup is a separate HTTP request, so this bounds both latency + * and pressure on the indexer regardless of how many accounts sync together. + */ +export const SENDER_LOOKUP_CONCURRENCY = 5; + +/** + * Maximum number of receives enriched with a counterparty while synchronizing + * or scanning an account. + * + * Those paths notify the whole account history at once, so resolving every + * receive would issue one indexer request per receive before the event can be + * emitted — on a long history that risks the Snap execution deadline. Only a + * bounded prefix of the receives is enriched, as a best-effort pre-warm; + * clients resolve whatever is left lazily through `getAccountTransactions`, + * which is already bounded by the requested page. + */ +export const SENDER_RESOLUTION_LIMIT = 25; + +/** + * Caps the number of indexer lookups in flight across every account. + * + * `synchronize` runs for all selected accounts concurrently, and each account + * resolves its own receives in waves. A limit applied per call therefore + * multiplies by the number of accounts syncing at once; sharing one budget for + * the lifetime of the instance keeps the total request count bounded no matter + * how many accounts are active. + */ +class SenderLookupLimiter { + #available: number; + + readonly #waiting: (() => void)[] = []; + + constructor(limit: number) { + this.#available = limit; + } + + async run(task: () => Promise): Promise { + if (this.#available === 0) { + await new Promise((resolve) => { + this.#waiting.push(resolve); + }); + } else { + this.#available -= 1; + } + + try { + return await task(); + } finally { + // Hand the slot straight to the next waiter rather than releasing it, so + // a queued lookup cannot be overtaken by a newly arriving one. + const next = this.#waiting.shift(); + if (next) { + next(); + } else { + this.#available += 1; + } + } + } +} + export class AccountUseCases { readonly #logger: Logger; @@ -174,6 +239,8 @@ export class AccountUseCases { readonly #targetBlocksConfirmation: number; + readonly #senderLookups = new SenderLookupLimiter(SENDER_LOOKUP_CONCURRENCY); + #accountMutationQueue: Promise = Promise.resolve(); constructor( @@ -484,6 +551,11 @@ export class AccountUseCases { return { account, transactionsToNotify: txsToNotify, + transactionSenders: await this.resolveTransactionSenders( + account, + txsToNotify, + SENDER_RESOLUTION_LIMIT, + ), }; } @@ -497,6 +569,8 @@ export class AccountUseCases { : []; await this.#repository.update(account, inscriptions); + const transactionsToNotify = account.listTransactions(); + this.#logger.info( 'initial full scan performed successfully: %s', account.id, @@ -504,7 +578,12 @@ export class AccountUseCases { return { account, - transactionsToNotify: account.listTransactions(), + transactionsToNotify, + transactionSenders: await this.resolveTransactionSenders( + account, + transactionsToNotify, + SENDER_RESOLUTION_LIMIT, + ), }; } @@ -1044,6 +1123,84 @@ export class AccountUseCases { }; } + /** + * Resolves the funding addresses for the given transactions, one indexer + * lookup per receive. + * + * Bitcoin inputs only carry a previous outpoint, so a receive from an + * external sender requires the chain indexer to resolve the counterparty. + * Resolution is best-effort: a failure (rate limit, outage) leaves the + * transaction without a counterparty rather than failing the caller. + * + * @param account - The Bitcoin account the transactions belong to. + * @param txs - The transactions to resolve senders for. + * @param limit - Maximum number of receives to look up. Omit to resolve every + * receive, which is safe for a paginated caller such as transaction listing; + * callers that pass a whole history should bound the cost with + * {@link SENDER_RESOLUTION_LIMIT}. The remainder is left unresolved so a + * client can still resolve it later through the paginated listing path. + * @returns A map of txid to funding addresses, only for resolved receives. + */ + async resolveTransactionSenders( + account: BitcoinAccount, + txs: WalletTx[], + limit?: number, + ): Promise | undefined> { + const sendersByTxid = new Map(); + + // Sends are displayed as "Sent from Bitcoin Account", so only receives + // need a counterparty. + const receives = txs.filter((tx) => { + const [sent] = account.sentAndReceived(tx.tx); + return sent.to_btc() <= 0; + }); + if (receives.length === 0) { + return undefined; + } + + // Callers that pass a whole history cap the number of lookups so emitting + // the event cannot wait on one request per historical receive. + const requested = limit === undefined ? receives : receives.slice(0, limit); + + if (requested.length < receives.length) { + this.#logger.debug( + 'Resolving senders for %d of %d receives', + requested.length, + receives.length, + ); + } + + // Each lookup is one indexer request (~1s). The limiter is shared across + // accounts, so a long history cannot serialize into a multi-minute scan and + // a multi-account sync cannot burst past the indexer's rate limit. + const settled = await batchesAllSettled( + requested, + SENDER_LOOKUP_CONCURRENCY, + async (tx) => { + const txid = tx.txid.toString(); + const senders = await this.#senderLookups.run(() => + this.#chain.getTransactionSenders(account.network, txid), + ); + return { txid, senders }; + }, + ); + + for (const result of settled) { + if (result.status === 'rejected') { + this.#logger.debug( + 'Failed to resolve transaction senders: %o', + result.reason, + ); + continue; + } + if (result.value.senders.length > 0) { + sendersByTxid.set(result.value.txid, result.value.senders); + } + } + + return sendersByTxid.size > 0 ? sendersByTxid : undefined; + } + async #runAccountMutation( fn: () => Promise, ): Promise {