From 36f884db91f412150bf92686cf6b524fcdf84cc5 Mon Sep 17 00:00:00 2001 From: jeremytsng Date: Mon, 17 Aug 2026 23:54:16 +0700 Subject: [PATCH 1/3] fix(bitcoin-wallet-snap): preserve template output order when filling a PSBT Only treat a wallet-owned template output as the drain output when it is the last output. BDK appends the drain output, so a wallet-owned output placed anywhere earlier was silently moved to the end of the transaction, reordering templates that put change before another output. Verify the built transaction against the template before returning it: every template output must appear at its original index with its original script and value, except the drain output, which takes the excess. Beyond the template, only a single appended output is tolerated, and it has to belong to the wallet. The previous check compared only the number of outputs, so a transaction whose outputs diverged from the template could still be signed and broadcast. --- packages/bitcoin-wallet-snap/CHANGELOG.md | 3 + .../src/use-cases/AccountUseCases.test.ts | 186 ++++++++++++++++++ .../src/use-cases/AccountUseCases.ts | 52 +++-- 3 files changed, 230 insertions(+), 11 deletions(-) diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 6b8303d1..ba4cc763 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -33,6 +33,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix account deletion failing against keyring v2 clients by removing the `AccountDeleted` event emission from the delete flow ([#221](https://github.com/MetaMask/internal-snaps/pull/221)) - v2 clients reject v1 lifecycle events, which aborted the deletion before the account was removed from state. Deletion is client-initiated in v2, so no event is needed. - Ensure certain errors are stringified correctly ([#179](https://github.com/MetaMask/internal-snaps/pull/179)) +- Keep the template output order when filling a PSBT ([#157](https://github.com/MetaMask/internal-snaps/pull/157)) + - A template output belonging to the wallet is now only used as the drain output when it is the last output. Previously any such output was moved to the end of the transaction, silently reordering templates that place change before another output. + - Filling a PSBT now fails with a `ValidationError` when the built transaction does not reproduce every template output, at its original index, with its original value. The drain output is exempt from the value check, since it absorbs the remaining balance by design. Only a single appended output is tolerated, and it has to belong to the wallet. Previously only the output count was compared, so a divergent transaction could be signed and broadcast. ## [2.0.1] 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..56ae31a2 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -1414,6 +1414,192 @@ describe('AccountUseCases', () => { // Result should be the rebuilt PSBT with all outputs preserved expect(result).toBe(rebuiltPsbt); }); + + const identifiableOutput = (scriptHex: string, sats: bigint): TxOut => { + const scriptPubkey = mock(); + scriptPubkey.to_hex_string.mockReturnValue(scriptHex); + const value = mock(); + value.to_sat.mockReturnValue(sats); + + return mock({ script_pubkey: scriptPubkey, value }); + }; + + const accountOwning = (owned: ScriptBuf[]): BitcoinAccount => { + const account = mock({ + id: 'account-id', + network: 'bitcoin', + isMine: (script: ScriptBuf) => owned.includes(script), + capabilities: [AccountCapability.FillPsbt], + }); + account.buildTx.mockReturnValue(mockTxBuilder); + return account; + }; + + it('adds every template output as a fixed recipient when the wallet-owned output is not last', async () => { + const changeOutput = identifiableOutput('0014aaaa', 2548n); + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const template = mock({ + unsigned_tx: { output: [changeOutput, depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { output: [changeOutput, depositOutput] }, + }), + ); + mockRepository.get.mockResolvedValueOnce( + accountOwning([changeOutput.script_pubkey]), + ); + + await useCases.fillPsbt('account-id', template); + + expect(mockTxBuilder.drainToByScript).not.toHaveBeenCalled(); + expect(mockTxBuilder.addRecipientByScript).toHaveBeenCalledTimes(2); + expect(mockTxBuilder.addRecipientByScript).toHaveBeenNthCalledWith( + 1, + changeOutput.value, + changeOutput.script_pubkey, + ); + expect(mockTxBuilder.addRecipientByScript).toHaveBeenNthCalledWith( + 2, + depositOutput.value, + depositOutput.script_pubkey, + ); + }); + + it('throws when the built outputs are reordered against the template', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const opReturnOutput = identifiableOutput('6a3ecccc', 0n); + const template = mock({ + unsigned_tx: { output: [depositOutput, opReturnOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { + output: [ + opReturnOutput, + identifiableOutput('0014aaaa', 2548n), + depositOutput, + ], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce(accountOwning([])); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + it('throws when a built output value diverges from the template', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const template = mock({ + unsigned_tx: { output: [depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { output: [identifiableOutput('5120bbbb', 1n)] }, + }), + ); + mockRepository.get.mockResolvedValueOnce(accountOwning([])); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + it('accepts a built PSBT that appends a change output after the template outputs', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const opReturnOutput = identifiableOutput('6a3ecccc', 0n); + const appendedChange = identifiableOutput('0014aaaa', 2548n); + const template = mock({ + unsigned_tx: { output: [depositOutput, opReturnOutput] }, + toString: () => 'templateBase64', + }); + const builtPsbt = mock({ + unsigned_tx: { + output: [depositOutput, opReturnOutput, appendedChange], + }, + }); + mockTxBuilder.finish.mockReturnValue(builtPsbt); + mockRepository.get.mockResolvedValueOnce( + accountOwning([appendedChange.script_pubkey]), + ); + + expect(await useCases.fillPsbt('account-id', template)).toBe(builtPsbt); + }); + + it('throws when the built PSBT appends an output that is not ours', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const template = mock({ + unsigned_tx: { output: [depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { + output: [depositOutput, identifiableOutput('5120dddd', 1000n)], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce(accountOwning([])); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + it('throws when the built PSBT appends more than one output', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const firstAppended = identifiableOutput('0014aaaa', 1000n); + const secondAppended = identifiableOutput('0014eeee', 1000n); + const template = mock({ + unsigned_tx: { output: [depositOutput] }, + toString: () => 'templateBase64', + }); + mockTxBuilder.finish.mockReturnValue( + mock({ + unsigned_tx: { + output: [depositOutput, firstAppended, secondAppended], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce( + accountOwning([ + firstAppended.script_pubkey, + secondAppended.script_pubkey, + ]), + ); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); + + it('accepts the drained output taking a value the template did not specify', async () => { + const depositOutput = identifiableOutput('5120bbbb', 496774n); + const changeOutput = identifiableOutput('0014aaaa', 1000n); + const template = mock({ + unsigned_tx: { output: [depositOutput, changeOutput] }, + toString: () => 'templateBase64', + }); + const builtPsbt = mock({ + unsigned_tx: { + output: [depositOutput, identifiableOutput('0014aaaa', 2548n)], + }, + }); + mockTxBuilder.finish.mockReturnValue(builtPsbt); + mockRepository.get.mockResolvedValueOnce( + accountOwning([changeOutput.script_pubkey]), + ); + + expect(await useCases.fillPsbt('account-id', template)).toBe(builtPsbt); + expect(mockTxBuilder.drainToByScript).toHaveBeenCalledWith( + changeOutput.script_pubkey, + ); + }); }); describe('computeFee', () => { diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index e77521f5..d1dd90b2 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -699,6 +699,15 @@ export class AccountUseCases { const frozenUTXOs = await this.#repository.getFrozenUTXOs(account.id); const feeRateToUse = feeRate ?? (await this.getFallbackFeeRate(account)); + const templateOutputs = templatePsbt.unsigned_tx.output; + const lastOutput = templateOutputs[templateOutputs.length - 1]; + // the drain output is appended last, so only a trailing output of ours keeps its position. If the template has no output of ours, a change output is added automatically. + const drainOutput = + lastOutput && account.isMine(lastOutput.script_pubkey) + ? lastOutput + : undefined; + + let builtPsbt: Psbt; try { let builder = account .buildTx() @@ -706,9 +715,8 @@ export class AccountUseCases { .unspendable(frozenUTXOs) .untouchedOrdering(); // we need to strictly adhere to the template output order. Many protocols use the order (e.g: 1: deposit, 2: OP_RETURN, 3: change) - for (const txout of templatePsbt.unsigned_tx.output) { - // if the PSBT contains an output that is sending to ourselves, we change its value. If the PSBT contains no change outputs, one will automatically be added. - if (account.isMine(txout.script_pubkey)) { + for (const txout of templateOutputs) { + if (txout === drainOutput) { builder = builder.drainToByScript(txout.script_pubkey); } else { builder = builder.addRecipientByScript( @@ -717,12 +725,9 @@ export class AccountUseCases { ); } } - let builtPsbt = builder.finish(); + builtPsbt = builder.finish(); - if ( - builtPsbt.unsigned_tx.output.length < - templatePsbt.unsigned_tx.output.length - ) { + if (builtPsbt.unsigned_tx.output.length < templateOutputs.length) { // Second attempt: use fixed recipients for all outputs builder = account .buildTx() @@ -730,7 +735,7 @@ export class AccountUseCases { .unspendable(frozenUTXOs) .untouchedOrdering(); - for (const txout of templatePsbt.unsigned_tx.output) { + for (const txout of templateOutputs) { builder = builder.addRecipientByScript( txout.value, txout.script_pubkey, @@ -738,8 +743,6 @@ export class AccountUseCases { } builtPsbt = builder.finish(); } - - return builtPsbt; } catch (error) { const causeMessage = (error as Error)?.message ?? 'unknown cause'; throw new ValidationError( @@ -752,6 +755,33 @@ export class AccountUseCases { error, ); } + + const builtOutputs = builtPsbt.unsigned_tx.output; + // BDK may append a single change output of ours after the template outputs, and nothing else. + const appended = builtOutputs.slice(templateOutputs.length); + const preserved = + appended.length <= 1 && + appended.every((txout) => account.isMine(txout.script_pubkey)) && + templateOutputs.every( + (txout, index) => + builtOutputs[index]?.script_pubkey.to_hex_string() === + txout.script_pubkey.to_hex_string() && + (txout === drainOutput || + builtOutputs[index]?.value.to_sat() === txout.value.to_sat()), + ); + if (!preserved) { + throw new ValidationError( + 'Built PSBT does not preserve the template outputs', + { + id: account.id, + templatePsbt: templatePsbt.toString(), + builtPsbt: builtPsbt.toString(), + feeRate: feeRateToUse, + }, + ); + } + + return builtPsbt; } async #broadcast( From 9079bcf2442bf064ffd6d25bbcfcbf40c6886cfb Mon Sep 17 00:00:00 2001 From: jeremytsng Date: Thu, 27 Aug 2026 03:30:59 +0700 Subject: [PATCH 2/3] fix(bitcoin-wallet-snap): check the drained output value on the rebuild path The rebuild adds every template output as a fixed recipient, so no drain is configured on that attempt and the wallet-owned output's value is fully caller-specified. The verification still exempted it from the value comparison, so a rebuild that changed that value was accepted. Track whether a drain was configured on the attempt that produced the PSBT, and only exempt the drained output when it was. --- .../src/use-cases/AccountUseCases.test.ts | 29 +++++++++++++++++++ .../src/use-cases/AccountUseCases.ts | 6 ++-- 2 files changed, 33 insertions(+), 2 deletions(-) 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 56ae31a2..639704c9 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -1600,6 +1600,35 @@ describe('AccountUseCases', () => { changeOutput.script_pubkey, ); }); + + it('throws when the rebuild changes the value of the wallet-owned output', async () => { + const depositOutput = identifiableOutput('5120bbbb', 100000n); + const changeOutput = identifiableOutput('0014aaaa', 5000n); + const template = mock({ + unsigned_tx: { output: [depositOutput, changeOutput] }, + toString: () => 'templateBase64', + }); + // first attempt drops the sub-dust drain, so the rebuild adds every + // template output as a fixed recipient and no drain is configured + mockTxBuilder.finish + .mockReturnValueOnce( + mock({ unsigned_tx: { output: [depositOutput] } }), + ) + .mockReturnValueOnce( + mock({ + unsigned_tx: { + output: [depositOutput, identifiableOutput('0014aaaa', 1n)], + }, + }), + ); + mockRepository.get.mockResolvedValueOnce( + accountOwning([changeOutput.script_pubkey]), + ); + + await expect(useCases.fillPsbt('account-id', template)).rejects.toThrow( + 'Built PSBT does not preserve the template outputs', + ); + }); }); describe('computeFee', () => { diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index d1dd90b2..c080a74b 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -707,6 +707,7 @@ export class AccountUseCases { ? lastOutput : undefined; + let drainConfigured = drainOutput !== undefined; let builtPsbt: Psbt; try { let builder = account @@ -728,7 +729,8 @@ export class AccountUseCases { builtPsbt = builder.finish(); if (builtPsbt.unsigned_tx.output.length < templateOutputs.length) { - // Second attempt: use fixed recipients for all outputs + // Second attempt: use fixed recipients for all outputs, so no drain is configured + drainConfigured = false; builder = account .buildTx() .feeRate(feeRateToUse) @@ -766,7 +768,7 @@ export class AccountUseCases { (txout, index) => builtOutputs[index]?.script_pubkey.to_hex_string() === txout.script_pubkey.to_hex_string() && - (txout === drainOutput || + ((drainConfigured && txout === drainOutput) || builtOutputs[index]?.value.to_sat() === txout.value.to_sat()), ); if (!preserved) { From 50065ff24b6cb909338f9837435230a5fc63b97b Mon Sep 17 00:00:00 2001 From: jeremytsng Date: Tue, 18 Aug 2026 01:25:10 +0700 Subject: [PATCH 3/3] test(bitcoin-wallet-snap): repair the integration suite and cover template output order The integration suite has not run since the migration into this monorepo. run-integration.sh passed jest.integration.config.js while the file is jest.integration.config.mjs, and the config declared no transform, so it fell back to the preset's babel-jest and required @babel/preset-env, which is not installed. Point the script at the real filename and use the same ts-jest transform the unit config uses. With the suite running for the first time, 53 of 60 tests failed. The assertions still expected the v1 keyring envelope, { pending: false, result }, which the snap stopped returning in 2.0.1, and one assertion required the funding transaction to be the only transaction on a regtest chain that the other suites broadcast to. Add a regression test for template output order: a template whose middle output belongs to the wallet must keep both its position and its value. Before the ordering fix that output was moved to the end of the transaction and given the excess. Assert output order on the existing fillPsbt test too, which only checked that a string came back. --- .../integration-test/keyring-request.test.ts | 167 +++++++++++------- .../integration-test/keyring.test.ts | 26 +-- .../integration-test/psbt-utils.ts | 123 +++++++++++++ .../integration-test/run-integration.sh | 2 +- .../jest.integration.config.mjs | 3 + 5 files changed, 242 insertions(+), 79 deletions(-) create mode 100644 packages/bitcoin-wallet-snap/integration-test/psbt-utils.ts diff --git a/packages/bitcoin-wallet-snap/integration-test/keyring-request.test.ts b/packages/bitcoin-wallet-snap/integration-test/keyring-request.test.ts index b698dbe5..bddccc11 100644 --- a/packages/bitcoin-wallet-snap/integration-test/keyring-request.test.ts +++ b/packages/bitcoin-wallet-snap/integration-test/keyring-request.test.ts @@ -8,6 +8,11 @@ import { Caip19Asset } from '../src/handlers/caip'; import type { FillPsbtResponse } from '../src/handlers/KeyringRequestHandler'; import { BlockchainTestUtils } from './blockchain-utils'; import { MNEMONIC, ORIGIN } from './constants'; +import { buildTemplatePsbt, readOutputs } from './psbt-utils'; + +const DEPOSIT_SCRIPT = + '5120e44fd4d762ab7db99520bf8cc1b44658404c7626bae50b7d78041d4337bb98b8'; +const OP_RETURN_SCRIPT = '6a0568656c6c6f'; const ACCOUNT_INDEX = 3; const submitRequestMethod = 'keyring_submitRequest'; @@ -132,24 +137,20 @@ describe('KeyringRequestHandler', () => { } as KeyringRequest, }); - expect(response).toRespondWith({ - pending: false, - result: [ - { - address: 'bcrt1qs2fj7czz0amfm74j73yujx6dn6223md56gkkuy', - derivationIndex: 0, - outpoint: expect.any(String), - scriptPubkey: - 'OP_0 OP_PUSHBYTES_20 82932f60427f769dfab2f449c91b4d9e94a8edb4', - scriptPubkeyHex: '001482932f60427f769dfab2f449c91b4d9e94a8edb4', - value: '1000000000', - }, - ], - }); + expect(response).toRespondWith([ + { + address: 'bcrt1qs2fj7czz0amfm74j73yujx6dn6223md56gkkuy', + derivationIndex: 0, + outpoint: expect.any(String), + scriptPubkey: + 'OP_0 OP_PUSHBYTES_20 82932f60427f769dfab2f449c91b4d9e94a8edb4', + scriptPubkeyHex: '001482932f60427f769dfab2f449c91b4d9e94a8edb4', + value: '1000000000', + }, + ]); - const utxos = ( - response.response as { result: { result: { outpoint: string }[] } } - ).result.result; + const utxos = (response.response as { result: { outpoint: string }[] }) + .result; response = await snap.onKeyringRequest({ origin: ORIGIN, @@ -169,10 +170,7 @@ describe('KeyringRequestHandler', () => { } as KeyringRequest, }); - expect(response).toRespondWith({ - pending: false, - result: utxos[0], - }); + expect(response).toRespondWith(utxos[0]); }); it('publicDescriptor', async () => { @@ -190,11 +188,9 @@ describe('KeyringRequestHandler', () => { } as KeyringRequest, }); - expect(response).toRespondWith({ - pending: false, - result: - "wpkh([27f9035f/84'/1'/0']tpubDCkv2fHDfPg5ok9EPv6CDozH72rvY2jgEPm79szMeBwCBwUf2T6n5nLrWFfhuuD48SgzrELezoiyDM9KbZaVen4wuuGwrqQANDhzB7E8yDh/0/*)#sx899xk6", - }); + expect(response).toRespondWith( + "wpkh([27f9035f/84'/1'/0']tpubDCkv2fHDfPg5ok9EPv6CDozH72rvY2jgEPm79szMeBwCBwUf2T6n5nLrWFfhuuD48SgzrELezoiyDM9KbZaVen4wuuGwrqQANDhzB7E8yDh/0/*)#sx899xk6", + ); }); }); @@ -236,11 +232,8 @@ describe('KeyringRequestHandler', () => { const result = await response; expect(result).toRespondWith({ - pending: false, - result: { - psbt: SIGNED_PSBT, - txid: null, - }, + psbt: SIGNED_PSBT, + txid: null, }); }); @@ -275,11 +268,8 @@ describe('KeyringRequestHandler', () => { const result = await response; expect(result).toRespondWith({ - pending: false, - result: { - psbt: expect.any(String), // non deterministic - txid: null, - }, + psbt: expect.any(String), // non deterministic + txid: null, }); }); @@ -314,12 +304,9 @@ describe('KeyringRequestHandler', () => { const result = await response; expect(result).toRespondWith({ - pending: false, - result: { - psbt: expect.any(String), // non deterministic - txid: expect.any(String), - canBeMalleable: false, - }, + psbt: expect.any(String), // non deterministic + txid: expect.any(String), + canBeMalleable: false, }); // Regression for issue #597: after broadcasting a partial-spend tx @@ -466,11 +453,69 @@ describe('KeyringRequestHandler', () => { }); expect(response).toRespondWith({ - pending: false, - result: { - psbt: expect.any(String), // non deterministic - }, + psbt: expect.any(String), // the change amount is not deterministic + }); + + const { psbt } = (response.response as { result: FillPsbtResponse }) + .result; + const templateOutputs = readOutputs(TEMPLATE_PSBT); + + // the last template output belongs to the wallet, so it becomes the drain + // output and takes the excess: assert the order, not its value + expect( + readOutputs(psbt) + .slice(0, templateOutputs.length) + .map((output) => output.scriptHex), + ).toStrictEqual(templateOutputs.map((output) => output.scriptHex)); + }); + + it('keeps a wallet-owned output in its template position', async () => { + const utxosResponse = await snap.onKeyringRequest({ + origin: ORIGIN, + method: submitRequestMethod, + params: { + id: account.id, + origin, + scope: BtcScope.Regtest, + account: account.id, + request: { method: AccountCapability.ListUtxos }, + } as KeyringRequest, }); + const ourScriptHex = ( + utxosResponse.response as { result: { scriptPubkeyHex: string }[] } + ).result[0]?.scriptPubkeyHex as string; + + const templateOutputs = [ + { scriptHex: DEPOSIT_SCRIPT, value: 20000 }, + { scriptHex: ourScriptHex, value: 1000 }, + { scriptHex: OP_RETURN_SCRIPT, value: 0 }, + ]; + + const response = await snap.onKeyringRequest({ + origin: ORIGIN, + method: submitRequestMethod, + params: { + id: account.id, + origin, + scope: BtcScope.Regtest, + account: account.id, + request: { + method: AccountCapability.FillPsbt, + params: { + account: { address: account.address }, + psbt: buildTemplatePsbt(templateOutputs), + feeRate: 3, + }, + }, + } as KeyringRequest, + }); + + const { psbt } = (response.response as { result: FillPsbtResponse }) + .result; + const builtOutputs = readOutputs(psbt); + + expect(builtOutputs.slice(0, 3)).toStrictEqual(templateOutputs); + expect(builtOutputs.length).toBeGreaterThan(3); }); it('fails if invalid PSBT', async () => { @@ -526,10 +571,7 @@ describe('KeyringRequestHandler', () => { }); expect(response).toRespondWith({ - pending: false, - result: { - fee: '632', - }, + fee: '632', }); }); @@ -596,9 +638,7 @@ describe('KeyringRequestHandler', () => { const signResult = await signResponse; - const { result } = ( - signResult.response as { result: { result: FillPsbtResponse } } - ).result; + const { result } = signResult.response as { result: FillPsbtResponse }; const response = await snap.onKeyringRequest({ origin: ORIGIN, @@ -619,11 +659,8 @@ describe('KeyringRequestHandler', () => { }); expect(response).toRespondWith({ - pending: false, - result: { - txid: expect.any(String), - canBeMalleable: false, - }, + txid: expect.any(String), + canBeMalleable: false, }); }); @@ -687,11 +724,8 @@ describe('KeyringRequestHandler', () => { const result = await response; expect(result).toRespondWith({ - pending: false, - result: { - txid: expect.any(String), - canBeMalleable: false, - }, + txid: expect.any(String), + canBeMalleable: false, }); }); @@ -749,11 +783,8 @@ describe('KeyringRequestHandler', () => { const result = await response; expect(result).toRespondWith({ - pending: false, - result: { - signature: - 'AkcwRAIgZxodJQ60t9Rr/hABEHZ1zPUJ4m5hdM5QLpysH8fDSzgCIENOEuZtYf9/Nn/ZW15PcImkknol403dmZrgoOQ+6K+TASECwDKypXm/ElmVTxTLJ7nao6X5mB/iGbU2Q2qtot0QRL4=', - }, + signature: + 'AkcwRAIgZxodJQ60t9Rr/hABEHZ1zPUJ4m5hdM5QLpysH8fDSzgCIENOEuZtYf9/Nn/ZW15PcImkknol403dmZrgoOQ+6K+TASECwDKypXm/ElmVTxTLJ7nao6X5mB/iGbU2Q2qtot0QRL4=', }); }); }); diff --git a/packages/bitcoin-wallet-snap/integration-test/keyring.test.ts b/packages/bitcoin-wallet-snap/integration-test/keyring.test.ts index ff7e3745..4f6f18db 100644 --- a/packages/bitcoin-wallet-snap/integration-test/keyring.test.ts +++ b/packages/bitcoin-wallet-snap/integration-test/keyring.test.ts @@ -353,10 +353,14 @@ describe('Keyring', () => { }, }); - expect(response).toRespondWith({ - data: [{ ...FUNDING_TX, account: accoundId }], - next: null, - }); + const { data, next } = ( + response.response as { + result: { data: unknown[]; next: string | null }; + } + ).result; + + expect(data).toContainEqual({ ...FUNDING_TX, account: accoundId }); + expect(next).toBeNull(); }); it('gets an account balance', async () => { @@ -369,12 +373,14 @@ describe('Keyring', () => { }, }); - expect(response).toRespondWith({ - [Caip19Asset.Regtest]: { - amount: '500', - unit: CurrencyUnit.Regtest, - }, - }); + const balance = ( + response.response as { + result: Record; + } + ).result[Caip19Asset.Regtest]; + + expect(balance?.unit).toBe(CurrencyUnit.Regtest); + expect(Number(balance?.amount)).toBeGreaterThanOrEqual(500); }); it.each([ diff --git a/packages/bitcoin-wallet-snap/integration-test/psbt-utils.ts b/packages/bitcoin-wallet-snap/integration-test/psbt-utils.ts new file mode 100644 index 00000000..6d5b5acc --- /dev/null +++ b/packages/bitcoin-wallet-snap/integration-test/psbt-utils.ts @@ -0,0 +1,123 @@ +/* eslint-disable no-restricted-globals */ + +export type TemplateOutput = { scriptHex: string; value: number }; + +const PSBT_MAGIC = '70736274ff'; +const GLOBAL_UNSIGNED_TX = '0100'; + +const varInt = (value: number): Buffer => { + if (value < 0xfd) { + return Buffer.from([value]); + } + const buffer = Buffer.alloc(3); + buffer.writeUInt8(0xfd, 0); + buffer.writeUInt16LE(value, 1); + return buffer; +}; + +const uInt32 = (value: number): Buffer => { + const buffer = Buffer.alloc(4); + buffer.writeUInt32LE(value, 0); + return buffer; +}; + +const uInt64 = (value: number): Buffer => { + const buffer = Buffer.alloc(8); + buffer.writeBigUInt64LE(BigInt(value), 0); + return buffer; +}; + +/** + * Builds a base64 PSBT holding only outputs, the shape bridge and swap providers + * return: no inputs, no change, for the wallet to fill. + * + * @param outputs - The outputs to place, in order. + * @returns The base64 encoded PSBT. + */ +export const buildTemplatePsbt = (outputs: TemplateOutput[]): string => { + const unsignedTx = Buffer.concat([ + uInt32(2), + varInt(0), + varInt(outputs.length), + ...outputs.flatMap((output) => { + const script = Buffer.from(output.scriptHex, 'hex'); + return [uInt64(output.value), varInt(script.length), script]; + }), + uInt32(0), + ]); + + return Buffer.concat([ + Buffer.from(PSBT_MAGIC, 'hex'), + Buffer.from(GLOBAL_UNSIGNED_TX, 'hex'), + varInt(unsignedTx.length), + unsignedTx, + Buffer.from([0x00]), + ...outputs.map(() => Buffer.from([0x00])), + ]).toString('base64'); +}; + +/** + * Reads the outputs of a PSBT's unsigned transaction, in transaction order. + * + * @param psbtBase64 - The base64 encoded PSBT. + * @returns The outputs, in the order they appear in the transaction. + */ +export const readOutputs = (psbtBase64: string): TemplateOutput[] => { + const psbt = Buffer.from(psbtBase64, 'base64'); + let offset = PSBT_MAGIC.length / 2; + + const readVarInt = (): number => { + const first = psbt.readUInt8(offset); + offset += 1; + if (first < 0xfd) { + return first; + } + if (first === 0xfd) { + const value = psbt.readUInt16LE(offset); + offset += 2; + return value; + } + const value = psbt.readUInt32LE(offset); + offset += 4; + return value; + }; + + // global map: find the unsigned transaction record + for (;;) { + const keyLength = readVarInt(); + if (keyLength === 0) { + throw new Error('PSBT has no unsigned transaction'); + } + const keyType = psbt.readUInt8(offset); + offset += keyLength; + const valueLength = readVarInt(); + if (keyType === 0x00) { + break; + } + offset += valueLength; + } + + offset += 4; // version + const inputCount = readVarInt(); + for (let index = 0; index < inputCount; index++) { + offset += 36; // previous outpoint + const scriptSigLength = readVarInt(); + offset += scriptSigLength; + offset += 4; // sequence + } + + const outputCount = readVarInt(); + const outputs: TemplateOutput[] = []; + for (let index = 0; index < outputCount; index++) { + const value = Number(psbt.readBigUInt64LE(offset)); + offset += 8; + const scriptLength = readVarInt(); + outputs.push({ + scriptHex: psbt.subarray(offset, offset + scriptLength).toString('hex'), + value, + }); + offset += scriptLength; + } + + return outputs; +}; diff --git a/packages/bitcoin-wallet-snap/integration-test/run-integration.sh b/packages/bitcoin-wallet-snap/integration-test/run-integration.sh index 1fcd07f3..629bd26d 100755 --- a/packages/bitcoin-wallet-snap/integration-test/run-integration.sh +++ b/packages/bitcoin-wallet-snap/integration-test/run-integration.sh @@ -30,7 +30,7 @@ docker exec esplora bash /init-esplora.sh echo "Running integration tests..." set +e -jest --config jest.integration.config.js +jest --config jest.integration.config.mjs TEST_EXIT_CODE=$? set -e exit $TEST_EXIT_CODE diff --git a/packages/bitcoin-wallet-snap/jest.integration.config.mjs b/packages/bitcoin-wallet-snap/jest.integration.config.mjs index c36c839e..e42012c1 100644 --- a/packages/bitcoin-wallet-snap/jest.integration.config.mjs +++ b/packages/bitcoin-wallet-snap/jest.integration.config.mjs @@ -4,6 +4,9 @@ */ const config = { preset: '@metamask/snaps-jest', + transform: { + '^.+\\.(t|j)sx?$': 'ts-jest', + }, testMatch: ['**/integration-test/**/*.test.ts'], };