diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 6b8303d1..55456f11 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Coalesce concurrent account synchronization runs so stacked triggers (the 30s cronjob, `onActive`, and background events scheduled by `setSelectedAccounts`) share one run instead of duplicating network fetches, state writes, and keyring events ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) - Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) - v2 clients reject v1 lifecycle events, which aborted the deletion before the account was removed from state. Deletion is client-initiated in v2, so no event is needed. +- Reveal and persist the wallet's own output scripts when signing a PSBT, so change from partner-supplied templates is always covered by routine sync ([#225](https://github.com/MetaMask/internal-snaps/pull/225)) - Ensure certain errors are stringified correctly ([#179](https://github.com/MetaMask/internal-snaps/pull/179)) ## [2.0.1] diff --git a/packages/bitcoin-wallet-snap/src/entities/account.ts b/packages/bitcoin-wallet-snap/src/entities/account.ts index 901dfc2c..3d68b4cd 100644 --- a/packages/bitcoin-wallet-snap/src/entities/account.ts +++ b/packages/bitcoin-wallet-snap/src/entities/account.ts @@ -98,6 +98,15 @@ export type BitcoinAccount = { */ revealNextAddress(): AddressInfo; + /** + * Reveals addresses up to and including the derivation index of `script` if it belongs to + * this wallet and lies beyond the revealed set. + * + * @param script - the script to reveal up to. + * @returns true if new addresses were revealed. + */ + revealToScript(script: ScriptBuf): boolean; + /** * Start a full scan. * diff --git a/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.test.ts b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.test.ts index 3b2425f9..26f24406 100644 --- a/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.test.ts @@ -1,7 +1,11 @@ import type { + AddressInfo, ChangeSet, DescriptorPair, + KeychainKind, Network, + ScriptBuf, + SpkIndexed, } from '@metamask/bitcoindevkit'; import { Wallet } from '@metamask/bitcoindevkit'; import { mock } from 'jest-mock-extended'; @@ -187,4 +191,66 @@ describe('BdkAccountAdapter', () => { }); }); }); + + describe('revealToScript', () => { + const mockScript = mock(); + + const indexed = (keychain: KeychainKind, index: number): SpkIndexed => + mock({ 0: keychain, 1: index }); + + const adapter = (): BdkAccountAdapter => + BdkAccountAdapter.create( + mockId, + mockDerivationPath, + mockDescriptors, + mockNetwork, + ); + + it('returns false and reveals nothing when the script is not ours', () => { + mockWallet.derivation_of_spk.mockReturnValue(undefined); + + expect(adapter().revealToScript(mockScript)).toBe(false); + expect(mockWallet.reveal_addresses_to).not.toHaveBeenCalled(); + }); + + it('returns false and reveals nothing when the index is already revealed', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('internal', 4)); + mockWallet.derivation_index.mockReturnValue(4); + + expect(adapter().revealToScript(mockScript)).toBe(false); + expect(mockWallet.reveal_addresses_to).not.toHaveBeenCalled(); + }); + + it('reveals up to the index when the script lies beyond the revealed set', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('internal', 7)); + mockWallet.derivation_index.mockReturnValue(4); + mockWallet.reveal_addresses_to.mockReturnValue([mock()]); + + expect(adapter().revealToScript(mockScript)).toBe(true); + expect(mockWallet.reveal_addresses_to).toHaveBeenCalledWith( + 'internal', + 7, + ); + }); + + it('reveals when the keychain has no revealed index yet', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('external', 0)); + mockWallet.derivation_index.mockReturnValue(undefined); + mockWallet.reveal_addresses_to.mockReturnValue([mock()]); + + expect(adapter().revealToScript(mockScript)).toBe(true); + expect(mockWallet.reveal_addresses_to).toHaveBeenCalledWith( + 'external', + 0, + ); + }); + + it('returns false when the reveal produces no new addresses', () => { + mockWallet.derivation_of_spk.mockReturnValue(indexed('external', 7)); + mockWallet.derivation_index.mockReturnValue(4); + mockWallet.reveal_addresses_to.mockReturnValue([]); + + expect(adapter().revealToScript(mockScript)).toBe(false); + }); + }); }); diff --git a/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts index 89af0a75..7f83c345 100644 --- a/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/BdkAccountAdapter.ts @@ -156,6 +156,20 @@ export class BdkAccountAdapter implements BitcoinAccount { return this.#wallet.reveal_next_address('external'); } + revealToScript(script: ScriptBuf): boolean { + const indexed = this.#wallet.derivation_of_spk(script); + if (!indexed) { + return false; + } + const keychain = indexed[0]; + const index = indexed[1]; + const lastRevealed = this.#wallet.derivation_index(keychain); + if (lastRevealed !== undefined && lastRevealed >= index) { + return false; + } + return this.#wallet.reveal_addresses_to(keychain, index).length > 0; + } + startFullScan(): FullScanRequest { return this.#wallet.start_full_scan(); } diff --git a/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts index 908e97ca..29a45161 100644 --- a/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/StoredAccountAdapter.ts @@ -167,6 +167,10 @@ export class StoredAccountAdapter implements BitcoinAccount { return this.#unsupported(); } + revealToScript(_script: ScriptBuf): boolean { + return this.#unsupported(); + } + sentAndReceived(_tx: Transaction): [Amount, Amount] { return this.#unsupported(); } 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 2a94fa25..fa7e9bc3 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -1092,6 +1092,74 @@ describe('AccountUseCases', () => { expect(txid).toBe(mockTxid); expect(psbt).toBe('mockSignedPsbt'); }); + + it('reveals and persists own output scripts after signing (broadcast: false)', async () => { + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(true); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: false, + }); + + expect(mockAccount.revealToScript).toHaveBeenCalledWith( + mockOutput.script_pubkey, + ); + expect(mockRepository.update).toHaveBeenCalledWith(mockAccount); + }); + + it('reveals and persists own output scripts after signing (broadcast: true)', async () => { + mockAccount.getTransaction.mockReturnValue(mockWalletTx); + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(true); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: true, + }); + + expect(mockAccount.revealToScript).toHaveBeenCalledWith( + mockOutput.script_pubkey, + ); + expect(mockRepository.update).toHaveBeenCalledWith(mockAccount); + }); + + it('does not persist when no output scripts get revealed (broadcast: false)', async () => { + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(false); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: false, + }); + + expect(mockRepository.update).not.toHaveBeenCalled(); + }); + + it('only persists once via #broadcast when no output scripts get revealed (broadcast: true)', async () => { + mockAccount.getTransaction.mockReturnValue(mockWalletTx); + mockAccount.isMine.mockReturnValue(true); + mockAccount.revealToScript.mockReturnValue(false); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: true, + }); + + expect(mockRepository.update).toHaveBeenCalledTimes(1); + expect(mockRepository.update).toHaveBeenCalledWith(mockAccount); + }); + + it('does not call revealToScript for outputs that are not isMine', async () => { + mockAccount.isMine.mockReturnValue(false); + + await useCases.signPsbt('account-id', mockPsbt, 'metamask', { + fill: false, + broadcast: false, + }); + + expect(mockAccount.revealToScript).not.toHaveBeenCalled(); + }); }); describe('fillPsbt', () => { @@ -1185,6 +1253,7 @@ describe('AccountUseCases', () => { expect(mockTxBuilder.drainToByScript).toHaveBeenCalledWith( mockOutput.script_pubkey, ); + expect(mockAccount.revealToScript).not.toHaveBeenCalled(); expect(psbt).toBe(mockFilledPsbt); }); @@ -1503,6 +1572,7 @@ describe('AccountUseCases', () => { mockOutput.script_pubkey, ); expect(mockTxBuilder.addRecipientByScript).not.toHaveBeenCalled(); + expect(mockAccount.revealToScript).not.toHaveBeenCalled(); expect(fee).toBe(mockFee); }); diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index e77521f5..1880bc2c 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -487,6 +487,16 @@ export class AccountUseCases { : psbt; const signedPsbt = account.sign(psbtToSign); + let revealed = false; + for (const txout of psbtToSign.unsigned_tx.output) { + if (account.isMine(txout.script_pubkey)) { + revealed = account.revealToScript(txout.script_pubkey) || revealed; + } + } + if (revealed) { + await this.#repository.update(account); + } + if (options.broadcast) { const psbtString = signedPsbt.toString(); const tx = account.extractTransaction(signedPsbt);