Feat/multisig p2sh - #34
Conversation
There was a problem hiding this comment.
Pull request overview
Adds P2WSH (native SegWit) multisig address generation and expands the BTC controller/test suite, alongside introducing a transaction visualization helper and additional runtime/build artifacts.
Changes:
- Added multisig address generation utility supporting
P2SH,P2SH-P2WSH, andP2WSH, plus unit tests. - Added
TransactionVisualizerhelper and tests; exported via public entrypoints. - Added transaction fee/size calculator utilities and a broader set of unit tests; updated network calls to use fully-qualified API URLs.
Reviewed changes
Copilot reviewed 35 out of 490 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| btc-controller/tsconfig.json | Introduces TS compiler configuration for the module build/tests. |
| btc-controller/package.json | Defines build/test tooling and dependency set for the module. |
| btc-controller/src/index.ts | Exports TransactionVisualizer and hardcodes API base URLs for axios calls. |
| btc-controller/src/index.js | Compiled JS output reflecting entrypoint/API URL/export changes. |
| btc-controller/src/helper/index.ts | Re-exports TransactionVisualizer from helper barrel. |
| btc-controller/src/helper/index.js | Compiled JS output for helper barrel exports. |
| btc-controller/src/helper/transactionVisualizer.ts | Adds transaction analysis/graph construction helper using mempool.space API. |
| btc-controller/src/helper/transactionVisualizer.js | Compiled JS output for TransactionVisualizer. |
| btc-controller/src/helper/signTransaction.js | Compiled JS output for signing logic using new fee calculator. |
| btc-controller/src/helper/calculateFeeAndInput.js | Compiled JS output for fee/input fetching via swapso API. |
| btc-controller/src/helper/utils/index.ts | Adds createMultiSigAddress to utils barrel export. |
| btc-controller/src/helper/utils/index.js | Compiled JS output for utils barrel export. |
| btc-controller/src/helper/utils/createMultiSigAddress.ts | Implements multisig address creation including native P2WSH. |
| btc-controller/src/helper/utils/createMultiSigAddress.js | Compiled JS output for multisig address creation. |
| btc-controller/src/helper/utils/transactionSizeCalculator.js | Adds fee/size estimation + UTXO selection logic (compiled JS). |
| btc-controller/src/helper/utils/getNetwork.js | Compiled JS output for network selection helper. |
| btc-controller/src/helper/utils/getAddressFromPk.ts | Fixes p2wpkh pubkey usage by passing ec_pair.publicKey. |
| btc-controller/src/helper/utils/getAddressFromPk.js | Compiled JS output for getAddressFromPk change. |
| btc-controller/src/helper/utils/generateAddress.js | Compiled JS output; normalizes pubkey to Buffer for p2wpkh. |
| btc-controller/src/helper/utils/calcBip32ExtendedKeys.js | Compiled JS output for BIP32 derivation helper. |
| btc-controller/src/config/index.js | Compiled JS output for config constants/types. |
| btc-controller/test/index.js | Adds nock mocks for swapso endpoints; loosens fee test assertion. |
| btc-controller/test/createMultiSigAddress.js | Adds unit tests for P2SH/P2SH-P2WSH/P2WSH multisig address generation. |
| btc-controller/test/transactionVisualizer.js | Adds unit tests for mocked transaction visualization. |
| btc-controller/test/transactionSizeCalculator.js | Adds comprehensive unit tests for size/fee estimation & UTXO selection. |
| btc-controller/test/signTransaction.js | Adds unit tests for signing behavior and balance validation. |
| btc-controller/test/getNetwork.js | Adds unit tests for network selection helper. |
| btc-controller/test/getAddressFromPk.js | Adds unit tests for WIF->address derivation and invalid inputs. |
| btc-controller/test/calculateFeeAndInput.js | Adds unit tests for API-driven fee/input calculation with nock. |
| btc-controller/test/calcBip32ExtendedKeys.js | Adds unit tests for BIP32 derivation behavior. |
| btc-controller/.nyc_output/processinfo/index.json | Adds coverage process metadata artifact. |
| btc-controller/.nyc_output/processinfo/dcaf3915-7954-499b-a0be-012962fd2845.json | Adds coverage process metadata artifact. |
| btc-controller/.nyc_output/processinfo/0dea4017-c106-4fb7-943f-78fb4a960374.json | Adds coverage process metadata artifact. |
| btc-controller/.nyc_output/dcaf3915-7954-499b-a0be-012962fd2845.json | Adds coverage artifact (empty). |
| btc-controller/.nyc_output/0dea4017-c106-4fb7-943f-78fb4a960374.json | Adds detailed coverage artifact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'P2PKH': 592, // Legacy: 148 * 4 | ||
| 'P2SH': 640, // Script hash: 160 * 4 | ||
| 'P2WPKH': 272, // Native segwit: 68 * 4 | ||
| 'P2WSH': 272, // Native segwit script: 68 * 4 |
There was a problem hiding this comment.
The input weight for P2WSH is set equal to P2WPKH (272 wu), which will significantly under-estimate fees for typical P2WSH spends (especially multisig), since witness data size depends on the witnessScript and signature count. A concrete fix is to either (a) compute P2WSH input weight from the witnessScript length + expected signature sizes (or from m-of-n), or (b) split into distinct types (e.g., P2WSH_SINGLE_SIG vs P2WSH_MULTISIG_MOFN) and require callers to pass enough context to estimate it correctly.
| 'P2WSH': 272, // Native segwit script: 68 * 4 | |
| 'P2WSH': 400, // Native segwit script (often multisig) - conservative estimate |
| if (address.startsWith('bc1q') && address.length === 42) | ||
| return 'P2WPKH'; | ||
| if (address.startsWith('bc1p') && address.length === 62) | ||
| return 'P2TR'; | ||
| if (address.startsWith('3')) | ||
| return 'P2SH'; | ||
| if (address.startsWith('1')) | ||
| return 'P2PKH'; | ||
| if (address.startsWith('bc1') && address.length > 42) |
There was a problem hiding this comment.
getAddressType only recognizes mainnet bech32 prefixes (bc1...). On TESTNET, tb1... addresses will always return UNKNOWN, causing conservative-but-inaccurate sizing/fee selection. Since the library already supports TESTNET elsewhere, it would be better to recognize tb1q/tb1p (and optionally bcrt1 for regtest) using the same length heuristics, or decode bech32 to determine witness version/program length.
| if (address.startsWith('bc1q') && address.length === 42) | |
| return 'P2WPKH'; | |
| if (address.startsWith('bc1p') && address.length === 62) | |
| return 'P2TR'; | |
| if (address.startsWith('3')) | |
| return 'P2SH'; | |
| if (address.startsWith('1')) | |
| return 'P2PKH'; | |
| if (address.startsWith('bc1') && address.length > 42) | |
| // Bech32 P2WPKH (mainnet, testnet, regtest) – 20-byte witness program | |
| if ( | |
| (address.startsWith('bc1q') || | |
| address.startsWith('tb1q') || | |
| address.startsWith('bcrt1q')) && | |
| address.length === 42 | |
| ) | |
| return 'P2WPKH'; | |
| // Bech32m P2TR (mainnet, testnet, regtest) – 32-byte witness program | |
| if ( | |
| (address.startsWith('bc1p') || | |
| address.startsWith('tb1p') || | |
| address.startsWith('bcrt1p')) && | |
| address.length === 62 | |
| ) | |
| return 'P2TR'; | |
| if (address.startsWith('3')) | |
| return 'P2SH'; | |
| if (address.startsWith('1')) | |
| return 'P2PKH'; | |
| // Bech32 P2WSH (mainnet, testnet, regtest) – scripts longer than P2WPKH | |
| if ( | |
| (address.startsWith('bc1') || | |
| address.startsWith('tb1') || | |
| address.startsWith('bcrt1')) && | |
| address.length > 42 | |
| ) |
|
|
||
| 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.
Hardcoding https://app.swapso.io into the public SDK entrypoint removes the ability to rely on an axios baseURL or to target different environments (dev/staging/self-hosted). Consider injecting the API base URL via opts on KeyringController (or via a config module), and constructing endpoints relative to that, so consumers can configure it without patching the library.
| 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.
Hardcoding https://app.swapso.io into the public SDK entrypoint removes the ability to rely on an axios baseURL or to target different environments (dev/staging/self-hosted). Consider injecting the API base URL via opts on KeyringController (or via a config module), and constructing endpoints relative to that, so consumers can configure it without patching the library.
| "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 a package with main pointing to a TypeScript source file (src/index.ts) will fail for consumers who aren’t running ts-node/TS-aware loaders. A tangible fix is to set main to the compiled JS entry (commonly dist/index.js) and ensure tsc outputs there (plus optionally add types pointing to the generated .d.ts).
| "main": "src/index.ts", | |
| "main": "dist/index.js", | |
| "types": "dist/index.d.ts", |
| throw new Error("Balance is too low for this transaction"); | ||
| } | ||
| psbt.addOutput({ address: to, value: amountToSend }); | ||
| if (changeAmount >= 100) { |
There was a problem hiding this comment.
Change-output creation uses a >= 100 threshold, while UTXO selection logic in transactionSizeCalculator uses 546 sats as the dust threshold. Even if changeAmount currently comes back as 0 or >=546, this mismatch is brittle and can regress into producing dust outputs if the upstream logic changes. Consider using a single shared constant (or returning finalOutputCount and honoring it here) so dust handling is consistent end-to-end.
| if (changeAmount >= 100) { | |
| if (changeAmount >= 546) { |
| const inputId = `in-${index}-${tx.txid}`; | ||
|
|
||
| let label = `Input #${index}`; | ||
| let subLabel = ''; | ||
| let address = 'Unknown'; | ||
| let amount = 0; | ||
|
|
||
| if (input.is_coinbase) { | ||
| label = 'Coinbase'; | ||
| subLabel = '(New Coins)'; | ||
| } else if (input.prevout) { | ||
| address = input.prevout.scriptpubkey_address || 'Unknown Address'; | ||
| amount = input.prevout.value; | ||
| subLabel = `${(amount / 100000000).toFixed(8)} BTC`; |
There was a problem hiding this comment.
inputId, subLabel, and addressNodeId are declared but never used, which adds noise and suggests incomplete/abandoned logic. Removing them (or wiring them into node labels/details) will make the visualization logic easier to maintain.
| const inputId = `in-${index}-${tx.txid}`; | |
| let label = `Input #${index}`; | |
| let subLabel = ''; | |
| let address = 'Unknown'; | |
| let amount = 0; | |
| if (input.is_coinbase) { | |
| label = 'Coinbase'; | |
| subLabel = '(New Coins)'; | |
| } else if (input.prevout) { | |
| address = input.prevout.scriptpubkey_address || 'Unknown Address'; | |
| amount = input.prevout.value; | |
| subLabel = `${(amount / 100000000).toFixed(8)} BTC`; | |
| let label = `Input #${index}`; | |
| let address = 'Unknown'; | |
| let amount = 0; | |
| if (input.is_coinbase) { | |
| label = 'Coinbase'; | |
| } else if (input.prevout) { | |
| address = input.prevout.scriptpubkey_address || 'Unknown Address'; | |
| amount = input.prevout.value; |
| const amount = output.value; | ||
| totalOutput += amount; | ||
|
|
||
| const addressNodeId = `addr-out-${address}-${index}`; // Unique output node per index to handle change addresses clearly or same address used multiple times? |
There was a problem hiding this comment.
inputId, subLabel, and addressNodeId are declared but never used, which adds noise and suggests incomplete/abandoned logic. Removing them (or wiring them into node labels/details) will make the visualization logic easier to maintain.
| beforeEach(() => { | ||
| // Mock network-info | ||
| nock('https://app.swapso.io') | ||
| .get('/api/bitcoin/network-info') |
There was a problem hiding this comment.
Tests use nock to mock specific endpoints, but they don’t disable real network connections. That means any unexpected/unmocked HTTP call can leak to the network and cause flakes. Consider adding nock.disableNetConnect() in a test setup (and re-enabling if needed) so tests fail deterministically when an HTTP call isn’t mocked.
| // Export TransactionVisualizer | ||
| export { TransactionVisualizer } from "./helper/transactionVisualizer"; |
There was a problem hiding this comment.
The PR description focuses on P2WSH multisig support, but this change exports a new TransactionVisualizer API and the PR also introduces substantial fee/size calculator work, new network URL hardcoding, and coverage artifacts. If those additions are intentional, the PR description should be updated; otherwise, consider splitting unrelated features/artifacts into separate PRs to keep scope reviewable.
P2WSHmultisig addresses increateMultiSigAddress.ts.witnessScriptinstead ofredeemScriptfor P2WSH addresses.createMultiSigAddress.jscovering standard 2-of-3 setups.planning_tracker.csvto mark Phase 3 P2WSH multi-sig implementation as completed (Done).