feat(multisig): add UTXO selection to transaction builder - #37
Conversation
There was a problem hiding this comment.
Pull request overview
Adds minimum-input UTXO selection and additional helper utilities/tests to reduce unnecessary inputs in constructed Bitcoin transactions (notably multisig), and expands the module surface with a transaction visualization helper.
Changes:
- Implemented greedy (largest-first) UTXO selection for multisig PSBT building and expanded fee/size calculation utilities.
- Added multiple new test suites (fee/size calculator, signing, multisig builder, network utils, tx visualization) with HTTP mocking via
nock. - Updated exported helpers and switched some API calls to a hard-coded
https://app.swapso.iobase URL.
Reviewed changes
Copilot reviewed 34 out of 489 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| btc-controller/tsconfig.json | Adds project TS configuration for building/tests. |
| btc-controller/package.json | Defines build/test scripts and dependencies used by the new utilities/tests. |
| btc-controller/src/index.ts | Exposes new helpers and hard-codes API endpoints for fees/balance. |
| btc-controller/src/index.js | Compiled output reflecting new exports and API URL changes. |
| btc-controller/src/config/index.js | Adds compiled config output used at runtime. |
| btc-controller/src/helper/index.ts | Re-exports new multisig builder and transaction visualizer. |
| btc-controller/src/helper/index.js | Compiled output for updated helper exports. |
| btc-controller/src/helper/signTransaction.js | Uses the new optimal fee/UTXO selection results when building PSBTs. |
| btc-controller/src/helper/calculateFeeAndInput.js | Compiled output using hard-coded Swapso API URL for UTXO queries. |
| btc-controller/src/helper/buildMultiSigTransaction.ts | Adds multisig PSBT builder with greedy UTXO selection to minimize inputs. |
| btc-controller/src/helper/buildMultiSigTransaction.js | Compiled output for multisig PSBT builder. |
| btc-controller/src/helper/transactionVisualizer.ts | Adds a helper to fetch a tx and map it to nodes/edges for visualization. |
| btc-controller/src/helper/transactionVisualizer.js | Compiled output for transaction visualizer. |
| btc-controller/src/helper/utils/transactionSizeCalculator.js | Adds size/fee estimation + UTXO selection used by signing flows. |
| btc-controller/src/helper/utils/index.js | Compiled utility export barrel. |
| btc-controller/src/helper/utils/getNetwork.js | Compiled utility for selecting mainnet/testnet params. |
| btc-controller/src/helper/utils/getAddressFromPk.ts | Fixes p2wpkh address derivation by providing the pubkey. |
| btc-controller/src/helper/utils/getAddressFromPk.js | Compiled output for pubkey fix. |
| btc-controller/src/helper/utils/generateAddress.js | Compiled output adjusting pubkey Buffer handling. |
| btc-controller/src/helper/utils/calcBip32ExtendedKeys.js | Compiled output for BIP32 derivation helper. |
| btc-controller/test/index.js | Adds nock HTTP mocks and relaxes an assertion around fee fetching errors. |
| btc-controller/test/buildMultiSigTransaction.js | Adds tests for minimal-input UTXO selection + insufficient funds in multisig builder. |
| btc-controller/test/transactionSizeCalculator.js | Adds unit tests for fee/size estimation and UTXO selection edge cases. |
| btc-controller/test/signTransaction.js | Adds signing tests validating outputs, determinism, and insufficient funds behavior. |
| btc-controller/test/transactionVisualizer.js | Adds unit test for tx visualization graph generation with mocked HTTP. |
| btc-controller/test/getNetwork.js | Adds tests for network selection utility. |
| btc-controller/test/getAddressFromPk.js | Adds tests for address derivation and invalid inputs. |
| btc-controller/test/calculateFeeAndInput.js | Adds HTTP-mocked tests for fee/input calculation utility. |
| btc-controller/test/calcBip32ExtendedKeys.js | Adds tests for hardened/non-hardened derivation behavior. |
| btc-controller/.nyc_output/processinfo/index.json | Adds coverage runtime artifact (should not be committed). |
| btc-controller/.nyc_output/processinfo/dcaf3915-7954-499b-a0be-012962fd2845.json | Adds coverage runtime artifact (should not be committed). |
| btc-controller/.nyc_output/processinfo/0dea4017-c106-4fb7-943f-78fb4a960374.json | Adds coverage runtime artifact (should not be committed). |
| btc-controller/.nyc_output/dcaf3915-7954-499b-a0be-012962fd2845.json | Adds coverage runtime artifact (should not be committed). |
| btc-controller/.nyc_output/0dea4017-c106-4fb7-943f-78fb4a960374.json | Adds coverage runtime artifact (should not be committed). |
Comments suppressed due to low confidence (1)
btc-controller/test/index.js:1
- The tests set up
nockmocks but don’t assert they were actually used (so the suite can pass even if requests are never made or are made to a different URL). Consider addingassert.ok(nock.isDone())(or checkingpendingMocks()) inafterEach, and disabling real network connections during tests to prevent accidental live HTTP calls.
var assert = require('assert');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const selection = this.selectOptimalUTXOs(utxos, targetAmount, feeRate, senderAddress); | ||
| // CRITICAL: Use the SAME calculation logic as selectOptimalUTXOs | ||
| const finalSize = this.estimateTransactionSize(selection.selectedUTXOs, selection.finalOutputCount, senderAddress); | ||
| // CRITICAL: Use Math.ceil to ensure we never underpay | ||
| const finalFeeInSats = Math.ceil(finalSize.vBytes * feeRate); | ||
| // Add small buffer to prevent edge cases (optional but recommended) | ||
| const bufferedFeeInSats = finalFeeInSats + 1; // Add 1 sat buffer | ||
| console.log(`🔥 Final Fee Calculation: | ||
| - Transaction size: ${finalSize.vBytes} vBytes | ||
| - Fee rate: ${feeRate} sat/vB | ||
| - Calculated fee: ${finalFeeInSats} sats | ||
| - Buffered fee: ${bufferedFeeInSats} sats | ||
| - Output count: ${selection.finalOutputCount}`); | ||
| return { | ||
| fee: bufferedFeeInSats / 1e8, | ||
| feeInSats: bufferedFeeInSats, | ||
| transactionSize: finalSize.vBytes, | ||
| selectedUTXOs: selection.selectedUTXOs, | ||
| changeAmount: selection.changeAmount | ||
| }; |
There was a problem hiding this comment.
The function increases the fee by 1 sat (bufferedFeeInSats) but returns the original selection.changeAmount (which was computed using an unbuffered fee). This can make selectedUTXOs insufficient by 1 sat in exact-balance cases (and can also overstate change), causing signTransaction to throw even though selection succeeded. Fix by either (a) removing the unconditional buffer, or (b) incorporating the buffer into UTXO selection and adjusting change (e.g., recompute change as totalInput - targetAmount - bufferedFeeInSats when change outputs are used).
| console.log(`💰 UTXO Selection Debug: | ||
| - Selected UTXOs: ${selectedUTXOs.length} | ||
| - Total input: ${totalInput} sats | ||
| - Target amount: ${targetAmount} sats | ||
| - Fee (${finalOutputCount} outputs): ${finalFee} sats | ||
| - Change: ${finalChange} sats | ||
| - vBytes: ${finalOutputCount === 2 ? estimateWithChange.vBytes : estimateWithoutChange.vBytes} | ||
| - Fee rate: ${feeRate} sat/vB`); |
There was a problem hiding this comment.
Unconditional console.log in a core fee/UTXO-selection routine will spam logs in production and can leak transaction amounts/fees. Prefer removing these logs or gating them behind a debug flag (e.g., environment variable) and using a structured logger where appropriate.
| // Fallback estimation | ||
| return { | ||
| size: 140, | ||
| weight: 560, | ||
| vBytes: 140, | ||
| inputCount: 1, | ||
| outputCount: 2, | ||
| inputTypes: ['P2WPKH'] |
There was a problem hiding this comment.
The fallback branch ignores the caller-provided outputCount argument and always returns outputCount: 2. This makes estimateTransactionSize(_, 1, ...) return inconsistent metadata, and can skew fee computation in “no change output” scenarios if this fallback is ever used for outputCount = 1. Use the function parameter for outputCount in the returned object (and ensure the weight/vBytes align with that choice).
| // Fallback estimation | |
| return { | |
| size: 140, | |
| weight: 560, | |
| vBytes: 140, | |
| inputCount: 1, | |
| outputCount: 2, | |
| inputTypes: ['P2WPKH'] | |
| // Fallback estimation assuming: | |
| // - 1 P2WPKH input | |
| // - `outputCount` P2WPKH outputs | |
| const inputCount = 1; | |
| const inputTypes = ['P2WPKH']; | |
| let totalWeight = 0; | |
| // Transaction overhead (version + locktime + input/output counts) | |
| totalWeight += 40; // 10 bytes * 4 | |
| // One P2WPKH input | |
| totalWeight += this.getInputWeight('P2WPKH'); | |
| // Outputs (assume P2WPKH) | |
| totalWeight += outputCount * this.getOutputWeight('P2WPKH'); | |
| const vBytes = Math.ceil(totalWeight / 4); | |
| const size = Math.ceil(totalWeight / 4); | |
| return { | |
| size, | |
| weight: totalWeight, | |
| vBytes, | |
| inputCount, | |
| outputCount, | |
| inputTypes |
| @@ -0,0 +1 @@ | |||
| {"processes":{"0dea4017-c106-4fb7-943f-78fb4a960374":{"parent":null,"children":[]},"dcaf3915-7954-499b-a0be-012962fd2845":{"parent":null,"children":[]}},"files":{"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\index.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"],"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\utils\\index.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"],"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\utils\\generateAddress.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"],"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\utils\\calcBip32ExtendedKeys.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"]},"externalIds":{}} No newline at end of file | |||
There was a problem hiding this comment.
Coverage artifacts under .nyc_output/ are being committed (including machine-specific absolute paths). These files should be removed from the PR and added to .gitignore to avoid noisy diffs and leaking local filesystem structure.
| {"processes":{"0dea4017-c106-4fb7-943f-78fb4a960374":{"parent":null,"children":[]},"dcaf3915-7954-499b-a0be-012962fd2845":{"parent":null,"children":[]}},"files":{"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\index.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"],"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\utils\\index.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"],"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\utils\\generateAddress.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"],"C:\\Users\\RiH\\Desktop\\Open Source\\swapso open source\\BTC-module\\btc-controller\\src\\helper\\utils\\calcBip32ExtendedKeys.ts":["0dea4017-c106-4fb7-943f-78fb4a960374"]},"externalIds":{}} | |
| {"processes":{},"files":{},"externalIds":{}} |
| "name": "btc-module", | ||
| "version": "1.0.0", | ||
| "description": "Enterprise-grade Bitcoin wallet infrastructure", | ||
| "main": "src/index.ts", |
There was a problem hiding this comment.
Publishing/consuming a package with "main": "src/index.ts" typically breaks Node consumers unless they run ts-node/TypeScript at runtime. Prefer pointing "main" to a compiled JS entry (commonly under dist/), and add "types" to the generated .d.ts entry if you intend TypeScript support.
| "main": "src/index.ts", | |
| "main": "dist/index.js", | |
| "types": "dist/index.d.ts", |
|
|
||
| try { | ||
| const response = await axios(`/api/bitcoin/network-info?network=${networkType}`, { | ||
| const response = await axios(`https://app.swapso.io/api/bitcoin/network-info?network=${networkType}`, { |
There was a problem hiding this comment.
This PR changes API behavior by hard-coding https://app.swapso.io (previously relative /api/...), but the PR description focuses on multisig UTXO selection. Either update the PR description to cover this behavioral change, or revert/move the base-URL change to a dedicated PR (ideally making the base URL configurable rather than hard-coded).
| try { | ||
| const balance = await axios( | ||
| `/api/bitcoin/balance?address=${address}&network=${networkType}`, | ||
| `https://app.swapso.io/api/bitcoin/balance?address=${address}&network=${networkType}`, |
There was a problem hiding this comment.
This PR changes API behavior by hard-coding https://app.swapso.io (previously relative /api/...), but the PR description focuses on multisig UTXO selection. Either update the PR description to cover this behavioral change, or revert/move the base-URL change to a dedicated PR (ideally making the base URL configurable rather than hard-coded).
| afterEach(() => { | ||
| nock.cleanAll(); | ||
| }); |
There was a problem hiding this comment.
The tests set up nock mocks but don’t assert they were actually used (so the suite can pass even if requests are never made or are made to a different URL). Consider adding assert.ok(nock.isDone()) (or checking pendingMocks()) in afterEach, and disabling real network connections during tests to prevent accidental live HTTP calls.
| tx.vin.forEach((input, index) => { | ||
| const inputId = `in-${index}-${tx.txid}`; | ||
|
|
||
| let label = `Input #${index}`; | ||
| let subLabel = ''; | ||
| let address = 'Unknown'; | ||
| let amount = 0; |
There was a problem hiding this comment.
inputId, label, and subLabel are computed but never used (no input nodes are added; only address nodes + edges are created). Either remove the unused variables or complete the implementation by emitting input nodes (using the declared group: 'input' | 'output' types) so the code matches its apparent intent.
| // fee (BTC) must equal feeInSats / 1e8 | ||
| assert.strictEqual(result.fee, result.feeInSats / 1e8); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Add a regression test for the “exact funds” boundary case where totalInput === targetAmount + fee to ensure calculateOptimalFee and signTransaction don’t fail due to fee rounding/buffering mismatches (especially given the + 1 sat buffer currently applied).
| it('handles the exact-funds boundary where totalInput equals targetAmount plus fee', async () => { | |
| const targetAmount = 50000; // sats | |
| const feeRate = 1; // sats/vbyte | |
| // First, get a representative fee for this target at the given feerate | |
| const bootstrapResult = await BitcoinTransactionSizeCalculator.calculateOptimalFee( | |
| P2WPKH_ADDR, | |
| targetAmount, | |
| feeRate, | |
| [makeUtxo(1000000)] | |
| ); | |
| assert.ok(bootstrapResult.feeInSats > 0, 'Bootstrap fee should be positive'); | |
| const exactInput = targetAmount + bootstrapResult.feeInSats; | |
| const utxos = [makeUtxo(exactInput)]; | |
| // Now exercise the exact-funds boundary: inputs sum to targetAmount + fee | |
| const exactResult = await BitcoinTransactionSizeCalculator.calculateOptimalFee( | |
| P2WPKH_ADDR, | |
| targetAmount, | |
| feeRate, | |
| utxos | |
| ); | |
| const totalSelected = exactResult.selectedUTXOs.reduce( | |
| (sum, utxo) => sum + utxo.value, | |
| 0 | |
| ); | |
| // Ensure we actually selected the exact-input UTXO set we constructed | |
| assert.strictEqual(totalSelected, exactInput, 'Selected UTXOs should match exact input'); | |
| assert.ok(exactResult.feeInSats >= 0, 'Fee must be non-negative'); | |
| assert.ok(exactResult.changeAmount >= 0, 'Change must be non-negative'); | |
| // Verify basic accounting still holds at the exact-funds boundary: | |
| // totalSelected = targetAmount + fee + change | |
| const accountedTarget = totalSelected - exactResult.changeAmount - exactResult.feeInSats; | |
| assert.strictEqual( | |
| accountedTarget, | |
| targetAmount, | |
| 'Inputs, fee, and change should balance to the target amount at the exact-funds boundary' | |
| ); | |
| }); |
Implement minimum-input UTXO selection for buildMultiSigTransaction so multisig PSBTs only include required inputs for outputs plus fee. Add unit tests for successful selection and insufficient-funds behavior, with build and full test suite passing.