phase 1 utxo improvements #1526
Conversation
📝 WalkthroughWalkthroughAdds a pluggable UTXO selection framework, typed UTXO errors/validators, transaction-size constants, enhanced Bitcoin client flows for prepared/enhanced tx building and send-max, updates keystore/ledger transfer signatures to accept UTXO preferences, extensive tests and e2e updates, and three new developer guides plus a changeset. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (28)
.changeset/violet-suits-spend.md (1)
1-6: Polish the changeset wording and list the new public APIs explicitly."Add enhance tx building" reads awkwardly; also, calling out the new API surface helps consumers scanning release notes.
Apply this diff:
---- '@xchainjs/xchain-bitcoin': patch '@xchainjs/xchain-utxo': patch ---- - -Add enhance tx building & new function sendMax() +Add enhanced tx building and new function `sendMax()`. + +@xchainjs/xchain-bitcoin +- New APIs: `prepareTxEnhanced`, `buildTxEnhanced`, `prepareMaxTx`, `sendMax`. +- `ClientKeystore.transfer` and `ClientLedger.transfer` now use `prepareTxEnhanced` with default UTXO preferences (minimizeFee=true, avoidDust=true, minimizeInputs=false). +- Deprecate `buildTx` and `prepareTx` (backward compatible for now). + +@xchainjs/xchain-utxo +- New modules exported: `UtxoError`, `UtxoErrorCode`, `UtxoTransactionValidator`, `UtxoSelector` (+ related types).packages/xchain-bitcoin/src/clientLedger.ts (1)
61-71: Consider minimizing inputs for Ledger UX.For hardware signing, fewer inputs typically reduce signing time and user prompts. If fee sensitivity isn’t critical in Ledger flows, consider
minimizeInputs: true(or expose it via caller prefs; your merge above enables that).packages/xchain-bitcoin/__e2e__/bitcoin-ledger-client.e2e.ts (2)
88-115: Avoid BigNumber → number conversion for balance comparisons (precision risk).
toNumber()can overflow JS’s safe integer range for larger balances. Use BigNumber arithmetic to keep it lossless.- const currentBalance = balance[0].amount.amount().toNumber() - console.log('Current balance (satoshis):', currentBalance) + const currentBalanceBN = balance[0].amount.amount() // BigNumber + console.log('Current balance (satoshis):', currentBalanceBN.toString()) @@ - expect(maxTxData.maxAmount + maxTxData.fee).toBeLessThanOrEqual(currentBalance) + // Use BN-safe comparison + const totalSpendBN = currentBalanceBN.constructor(maxTxData.maxAmount).plus(maxTxData.fee) + expect(totalSpendBN.lte(currentBalanceBN)).toBeTruthy()Note: If
bignumber.jsis not globally available viaamount().constructor, import it explicitly:+import BigNumber from 'bignumber.js' ... - const totalSpendBN = currentBalanceBN.constructor(maxTxData.maxAmount).plus(maxTxData.fee) + const totalSpendBN = new BigNumber(maxTxData.maxAmount).plus(maxTxData.fee)
95-95: Remove the hard-coded fee comment.
// 10 sat/bytewill drift over time and can mislead during test maintenance.- feeRate: feeRate.rates.average, // 10 sat/byte + feeRate: feeRate.rates.average,claude/xchainjs-expert-development-agent.md (3)
388-391: Replace bare URL with a proper Markdown linkThis avoids markdownlint MD034 and reads cleaner in rendered docs.
- - **Documentation**: https://xchainjs.gitbook.io/xchainjs + - **Documentation**: [XChainJS Docs](https://xchainjs.gitbook.io/xchainjs)
343-345: Verify minimum toolchain versionsPlease confirm Node and Yarn versions align with the monorepo’s engines and CI matrix. If the repo targets Node 18+ now, update here to prevent contributor confusion.
I can scan package.json engines across packages and the CI config and prepare a short matrix for the docs.
47-125: Scope check: listed packages vs. what exists in the monorepoA few entries (e.g., some “Other” chains and “Power Tools”) might not exist in this repo or are out-of-scope. Consider adding “examples/roadmap” qualifiers or removing non-existent packages to avoid misleading readers.
I can diff the listed packages vs. actual directories in the repo and return a suggested table-of-truth you can paste back here.
packages/xchain-utxo/__tests__/edge-cases.test.ts (5)
6-6: Remove unused import to satisfy ESLint
UtxoSelectionPreferencesisn’t used in this test file.-import { UtxoSelector, UtxoSelectionPreferences } from '../src/utxo-selector' +import { UtxoSelector } from '../src/utxo-selector'
181-187: Avoidanyin negative tests without disabling lint rulesYou can preserve intent while keeping strict typing.
- UtxoTransactionValidator.validateUtxoSet(null as any) + UtxoTransactionValidator.validateUtxoSet(null as unknown as UTXO[])- UtxoTransactionValidator.validateUtxoSet('not an array' as any) + UtxoTransactionValidator.validateUtxoSet('not an array' as unknown as UTXO[])Alternatively, add a one-line eslint-disable comment for these explicit negative tests.
256-265: Avoidanyfor non-buffer sentinel in witnessUtxoKeep the test negative while staying type-safe.
- script: 'not a buffer' as any, + script: 'not a buffer' as unknown as Buffer,If you want to assert the validator catches non-buffers, you can also parametrize this case and try a few invalid types.
356-369: “Exact match” fixture doesn’t align with fee calculation constantsWith a P2WPKH model, a 1-in/1-out no-change tx at 10 sat/vB is closer to:
- fee = UtxoSelector.calculateFee(1, 1, 10)
- ideal UTXO = 50_000 + fee
The hard-coded 50_690 assumes a different sizing model and will drift if constants change. Suggest deriving it from the helper to keep tests resilient.
- { hash: 'hash2', index: 0, value: 50690 }, // Perfect for 50000 + fee + // Perfect for 50000 + no-change fee (derived to stay in sync with sizing constants) + { hash: 'hash2', index: 0, value: 50000 + UtxoSelector.calculateFee(1, 1, 10) },
443-473: Performance assertions may be flaky across environmentsThe time-based assertions (1s for 1,000 UTXOs; 5s for 100 pathological UTXOs) can intermittently fail on CI under load. Consider asserting only result characteristics or adding generous buffers with “slow CI” env guards.
I can parameterize the thresholds via env (e.g., EDGE_CASES_MAX_MS) and update the tests accordingly if desired.
claude/utxo-expert-development-agent.md (2)
186-199: UTXO “height” usage conflicts with current type definitionValidator notes that
heightisn’t available in the current UTXO type. Either mark it optional in types, or clarify that certain providers enrich UTXOs withheightand guard for its absence.I can propose a minimal type update:
height?: numberin UTXO, plus doc callouts where it’s provider-dependent.
642-776: Provider examples: sanitize and narrow “any” and implicit JSON shapesIn the CustomUTXOProvider, map response shapes to local interfaces to improve readability and type safety, and avoid pervasive
any. Also consider error handling for non-200 responses.I can supply typed interfaces and minimal fetch guards for these examples, keeping the doc concise.
claude/evm-expert-development-agent.md (2)
83-99: Network names and examples may be staleDouble-check that stagenet/testnet endpoints and networks (e.g., Ropsten/Goerli) reflect the networks supported by xchain-ethereum today. Replace deprecated networks and ensure explorer/provider examples match the current client defaults.
If you’d like, I can scrape the repo configs and open a follow-up PR updating this section.
239-254: Hard-coded token symbols without contracts invite confusionPrefer full token asset strings with contract addresses (as you did for USDT elsewhere) or explain that these are illustrative and not authoritative.
packages/xchain-utxo/src/utxo-selector.ts (3)
346-351: Randomized selection usessort(() => Math.random() - 0.5)This is biased and non-deterministic, which makes debugging and tests flaky. Prefer Fisher–Yates; optionally allow injecting a PRNG/seed for reproducibility.
- const shuffledUtxos = [...utxos].sort(() => Math.random() - 0.5) + const shuffledUtxos = [...utxos] + for (let i = shuffledUtxos.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)) + ;[shuffledUtxos[i], shuffledUtxos[j]] = [shuffledUtxos[j], shuffledUtxos[i]] + }
80-89: Swallowing strategy errors with console.debug may hide actionable failuresTotally fine for “Chill” mode, but consider capturing the last error for diagnostics or exposing an optional “traces” array on the result. This helps explain why all strategies failed.
I can wire a lightweight trace collector and keep it opt-in via a preferences flag.
154-167: Early “insufficient balance” check ignores fees
totalValue < targetValuepermits cases where UTXOs cover the target but not any fee, only to fail later as “selection failed.” Consider a clearer early rejection by checkingtotalValue < targetValue + minPossibleFee.Example approach:
- Estimate a lower bound fee with 1 input/1 output at the provided feeRate.
- If
totalValue < targetValue + lowerBoundFee, throwinsufficientBalancewith a precise required amount.packages/xchain-utxo/src/validators.ts (4)
309-340: Bech32/Bech32m BTC address regexes are brittle; prefer decode or relax lengths.The segwit patterns are strict and may reject valid addresses (lengths vary; bech32 charset subtleties). Lengths like
{58}for Taproot can be wrong for some valid P2TR addresses. Since xchain-utxo is chain-agnostic, consider either:
- delegating strict validation to chain clients (xchain-bitcoin already validates addresses), or
- using a light-weight check (HRP + charset) and deferring full validation downstream.
Minimal safer tweak (relax lengths; keep HRP and charset):
- segwitV0: /^bc1[02-9ac-hj-np-z]{7,87}$/, - segwitV1: /^bc1p[02-9ac-hj-np-z]{58}$/, + segwitV0: /^bc1[02-9ac-hj-np-z]{6,90}$/, + segwitV1: /^bc1p[02-9ac-hj-np-z]{6,90}$/,And testnet:
- segwitV0: /^tb1[02-9ac-hj-np-z]{7,87}$/, - segwitV1: /^tb1p[02-9ac-hj-np-z]{58}$/, + segwitV0: /^tb1[02-9ac-hj-np-z]{6,90}$/, + segwitV1: /^tb1p[02-9ac-hj-np-z]{6,90}$/,If you’d like, I can draft a small helper using bech32/bech32m decoding without pulling in bitcoinjs. Would you prefer that?
352-363: BCH CashAddr pattern is too rigid.Exact length
{42}risks false negatives. CashAddr payload length can vary (different types/lengths). Suggest relaxing the length and only enforcing the right HRP.- ? /^bitcoincash:[qpzry9x8gf2tvdw0s3jn54khce6mua7l]{42}$/ - : /^bchtest:[qpzry9x8gf2tvdw0s3jn54khce6mua7l]{42}$/ + ? /^bitcoincash:[qpzry9x8gf2tvdw0s3jn54khce6mua7l]{20,100}$/ + : /^bchtest:[qpzry9x8gf2tvdw0s3jn54khce6mua7l]{20,100}$/
97-106: Wallet index message is misleading.The limit 2,147,483,647 corresponds to the maximum non-hardened index (2^31−1), not “BIP32 hardened key limit.”
- } else if (params.walletIndex > 2147483647) { - errors.push('Wallet index exceeds BIP32 hardened key limit') + } else if (params.walletIndex > 2147483647) { + errors.push('Wallet index exceeds maximum non-hardened index (2^31-1)') }
387-393: Avoid logging sensitive fields in validation errors.sanitizeParamsForLogging redacts amount, but memo and addresses are still logged verbatim. Consider redacting or truncating memo and partially masking addresses.
private static sanitizeParamsForLogging(params: any): any { // Remove sensitive data from params for safe logging const sanitized = { ...params } if (sanitized.amount) { sanitized.amount = sanitized.amount.amount().toString() } + if (typeof sanitized.memo === 'string') { + sanitized.memo = sanitized.memo.length > 16 ? sanitized.memo.slice(0, 16) + '…' : sanitized.memo + } + if (typeof sanitized.recipient === 'string') { + sanitized.recipient = sanitized.recipient.slice(0, 6) + '…' + sanitized.recipient.slice(-6) + } + if (typeof sanitized.sender === 'string') { + sanitized.sender = sanitized.sender.slice(0, 6) + '…' + sanitized.sender.slice(-6) + } return sanitized }packages/xchain-utxo/src/errors.ts (2)
194-207: Align user-facing fee-rate guidance with validator policy.getUserFriendlyMessage says “between 1 and 1000 sat/byte,” while validateTransferParams currently accepts up to 1,000,000. Once you unify the upper bound (see validators.ts comment), make sure this stays consistent.
10-18: Expose INVALID_UTXO to users with a friendly message (optional).You define INVALID_UTXO but never map it in getUserFriendlyMessage. Consider adding a case to improve UX when UTXO structure checks fail.
getUserFriendlyMessage(): string { switch (this.code) { + case UtxoErrorCode.INVALID_UTXO: + return 'One or more UTXOs are invalid or inconsistent. Please refresh UTXOs and try again.'Also applies to: 183-189
packages/xchain-bitcoin/src/client.ts (3)
169-180: Redundant null-check on required sender parameter.Type signature mandates sender: Address; the conditional “if (params.sender)” is unnecessary.
- if (params.sender) { - UtxoTransactionValidator.validateAddressFormat(params.sender, this.network, 'BTC') - } + UtxoTransactionValidator.validateAddressFormat(params.sender, this.network, 'BTC')
645-664: Library logging: replace console.error with pluggable logger or remove.console.error from library code can pollute consumers’ logs. Consider a logger interface or remove logs; keep details in thrown UtxoError.
Also applies to: 599-623, 571-576
479-507: sendMax: consider validating fee rate upfront for clearer errors.You rely on downstream selection to reject invalid feeRate. For better UX, run validateFeeRate(feeRate) here as well (aligned with your unified bounds).
+ UtxoTransactionValidator.validateFeeRate(feeRate, { minFeeRate: 1, maxFeeRate: 1000 })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
.changeset/violet-suits-spend.md(1 hunks)claude/evm-expert-development-agent.md(1 hunks)claude/utxo-expert-development-agent.md(1 hunks)claude/xchainjs-expert-development-agent.md(1 hunks)packages/xchain-bitcoin/__e2e__/bitcoin-ledger-client.e2e.ts(2 hunks)packages/xchain-bitcoin/__e2e__/blockcypher.bitcoin-client.e2e.ts(2 hunks)packages/xchain-bitcoin/src/client.ts(4 hunks)packages/xchain-bitcoin/src/clientKeystore.ts(1 hunks)packages/xchain-bitcoin/src/clientLedger.ts(1 hunks)packages/xchain-utxo/__tests__/edge-cases.test.ts(1 hunks)packages/xchain-utxo/src/errors.ts(1 hunks)packages/xchain-utxo/src/index.ts(1 hunks)packages/xchain-utxo/src/utxo-selector.ts(1 hunks)packages/xchain-utxo/src/validators.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/xchain-utxo/__tests__/edge-cases.test.ts (4)
packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)packages/xchain-utxo/src/validators.ts (1)
UtxoTransactionValidator(10-394)packages/xchain-util/src/asset.ts (1)
baseAmount(103-123)packages/xchain-utxo/src/utxo-selector.ts (1)
UtxoSelector(37-190)
packages/xchain-utxo/src/utxo-selector.ts (1)
packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
packages/xchain-utxo/src/errors.ts (1)
packages/xchain-utxo/src/index.ts (2)
UtxoErrorCode(12-12)UtxoError(12-12)
packages/xchain-bitcoin/__e2e__/blockcypher.bitcoin-client.e2e.ts (2)
packages/xchain-util/src/asset.ts (2)
assetToBase(146-152)baseAmount(103-123)packages/xchain-bitcoin/src/const.ts (2)
AssetBTC(40-40)BTC_DECIMAL(22-22)
packages/xchain-utxo/src/validators.ts (2)
packages/xchain-util/src/asset.ts (1)
Address(18-18)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
packages/xchain-bitcoin/__e2e__/bitcoin-ledger-client.e2e.ts (1)
packages/xchain-util/src/asset.ts (1)
baseAmount(103-123)
packages/xchain-bitcoin/src/client.ts (5)
packages/xchain-utxo/src/index.ts (8)
UTXO(9-9)UtxoSelectionPreferences(18-18)UtxoSelectionResult(17-17)UtxoSelector(15-15)UtxoError(12-12)TxParams(9-9)UtxoTransactionValidator(13-13)PreparedTx(9-9)packages/xchain-utxo/src/types/types.ts (3)
UTXO(36-36)TxParams(30-32)PreparedTx(25-28)packages/xchain-utxo/src/utxo-selector.ts (3)
UtxoSelectionPreferences(26-32)UtxoSelectionResult(15-21)UtxoSelector(37-190)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)packages/xchain-utxo/src/validators.ts (1)
UtxoTransactionValidator(10-394)
🪛 ESLint
packages/xchain-utxo/__tests__/edge-cases.test.ts
[error] 6-6: 'UtxoSelectionPreferences' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 181-181: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 185-185: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 257-257: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 LanguageTool
claude/xchainjs-expert-development-agent.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...ace across different blockchain networks 2. Modularity: Loosely coupled packages f...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...ed packages for specific functionalities 3. Protocol Agnosticism: Support for mult...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...ltiple blockchain protocols and networks 4. Type Safety: Full TypeScript support w...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...port with comprehensive type definitions 5. Consistency: Standardized patterns acr...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...ries: #### Client Packages (20+ chains) - UTXO-based: Bitcoin, Litecoin, Bitcoin...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...oin, Bitcoin Cash, Dogecoin, Dash, ZCash - EVM-based: Ethereum, Binance Smart Cha...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...e Smart Chain, Avalanche, Arbitrum, Base - Cosmos SDK: Cosmos, THORChain, MAYACha...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...**: Cosmos, THORChain, MAYAChain, Kujira - Other: Solana, Cardano, Binance Chain,...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...ities, asset handling, number formatting - xchain-client: Base client interface and types - `xch...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...client: Base client interface and types - xchain-crypto`: Cryptographic functions, keystore mana...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...tographic functions, keystore management - xchain-utxo: UTXO-specific utilities - xchain-evm...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...- xchain-utxo: UTXO-specific utilities - xchain-evm: EVM-specific utilities - `xchain-cosmo...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...s - xchain-evm: EVM-specific utilities - xchain-cosmos-sdk: Cosmos SDK utilities #### Data Provid...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...oviders: UTXO blockchain data providers - xchain-evm-providers`: EVM blockchain data providers #### Pr...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...n data providers #### Protocol Packages - THORChain: xchain-thornode, `xchain-...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ain-midgard, xchain-thorchain-query, xchain-thorchain-amm- **MAYAChain**:xchain-mayanode, xchain-...
(QB_NEW_EN)
[grammar] ~44-~44: There might be a mistake here.
Context: ...n-wallet: Multi-chain wallet management - xchain-aggregator`: Cross-protocol swap aggregation ## Co...
(QB_NEW_EN)
[grammar] ~243-~243: There might be a mistake here.
Context: ... Best Practices ### 1. Amount Precision - Always use BaseAmount for calculations...
(QB_NEW_EN)
[grammar] ~245-~245: There might be a mistake here.
Context: ...ations - Convert to display format only in UI layer - Use CryptoAmount for asset...
(QB_NEW_EN)
[grammar] ~248-~248: There might be a mistake here.
Context: ...operations ### 2. Network Configuration - Use environment variables for network se...
(QB_NEW_EN)
[grammar] ~253-~253: There might be a mistake here.
Context: ...upported networks ### 3. Error Handling - Implement comprehensive error catching -...
(QB_NEW_EN)
[grammar] ~258-~258: There might be a mistake here.
Context: ...rk timeouts and retries ### 4. Security - Never log private keys or mnemonics - Us...
(QB_NEW_EN)
[grammar] ~263-~263: There might be a mistake here.
Context: ... before transactions ### 5. Performance - Cache frequently accessed data - Use con...
(QB_NEW_EN)
[grammar] ~268-~268: There might be a mistake here.
Context: ...e limiting for API calls ### 6. Testing - Write tests for all chain operations - U...
(QB_NEW_EN)
[grammar] ~366-~366: There might be a mistake here.
Context: ...Issues 1. Gas/Fee Estimation Failures - Check network congestion - Verify suf...
(QB_NEW_EN)
[grammar] ~371-~371: There might be a mistake here.
Context: ...guration 2. Address Validation Errors - Ensure correct network format - Valid...
(QB_NEW_EN)
[grammar] ~376-~376: There might be a mistake here.
Context: ...net addresses 3. Transaction Timeouts - Implement retry mechanisms - Use appr...
(QB_NEW_EN)
[uncategorized] ~381-~381: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...- Monitor network status 4. Provider Rate Limiting - Implement exponential backoff ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~381-~381: There might be a mistake here.
Context: ...work status 4. Provider Rate Limiting - Implement exponential backoff - Use m...
(QB_NEW_EN)
[grammar] ~388-~388: There might be a mistake here.
Context: ...**: https://xchainjs.gitbook.io/xchainjs - Discord: Community discussions and sup...
(QB_NEW_EN)
[grammar] ~389-~389: There might be a mistake here.
Context: ...ord**: Community discussions and support - Telegram: Real-time developer chat - *...
(QB_NEW_EN)
[grammar] ~390-~390: There might be a mistake here.
Context: ...- Telegram: Real-time developer chat - GitHub: Issues, feature requests, cont...
(QB_NEW_EN)
[grammar] ~395-~395: There might be a mistake here.
Context: ...ainJS welcomes contributions. Key areas: - New chain integrations - Protocol implem...
(QB_NEW_EN)
[grammar] ~396-~396: There might be a mistake here.
Context: ...ons. Key areas: - New chain integrations - Protocol implementations - Bug fixes a...
(QB_NEW_EN)
[grammar] ~397-~397: There might be a mistake here.
Context: ... integrations - Protocol implementations - Bug fixes and optimizations - Documentat...
(QB_NEW_EN)
[grammar] ~398-~398: There might be a mistake here.
Context: ...ntations - Bug fixes and optimizations - Documentation improvements - Test covera...
(QB_NEW_EN)
[grammar] ~399-~399: There might be a mistake here.
Context: ...timizations - Documentation improvements - Test coverage expansion Follow the esta...
(QB_NEW_EN)
claude/utxo-expert-development-agent.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...WPKH (Bech32) and P2TR (Taproot) support - Litecoin (xchain-litecoin) - Native ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...itecoin) - Native SegWit implementation - **Bitcoin Cash** (xchain-bitcoincash`) - ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...bitcoincash) - BCH with CashAddr format - **Dogecoin** (xchain-doge`) - DOGE with s...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...chain-doge) - DOGE with standard P2WPKH - **Dash** (xchain-dash`) - DASH with Insta...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...n-dash) - DASH with InstantSend support - **Zcash** (xchain-zcash`) - ZEC with tran...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...#### xchain-utxo - Core UTXO Framework The foundation package providing shared ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...xchain-utxo-providers - Data Providers Multiple data providers for UTXO blockch...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...oviders for UTXO blockchain interaction: - HaskoinProvider: Haskoin API integrati...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...skoinProvider**: Haskoin API integration - SochainProvider: Sochain API with API ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ider**: Sochain API with API key support - BlockcypherProvider: BlockCypher API i...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...rProvider**: BlockCypher API integration - BitgoProvider: BitGo enterprise API - ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ... BitgoProvider: BitGo enterprise API - NownodesProvider: Nownodes infrastruct...
(QB_NEW_EN)
claude/evm-expert-development-agent.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...ethereum) - The original EVM blockchain - **Binance Smart Chain** (xchain-bsc`) - B...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ... (xchain-bsc) - BSC with BEP-20 tokens - Avalanche (xchain-avax) - Avalanche ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...x) - Avalanche C-Chain with native AVAX - **Arbitrum** (xchain-arbitrum`) - Layer 2...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...um) - Layer 2 Ethereum scaling solution - **Base** (xchain-base`) - Coinbase's Laye...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ... #### xchain-evm - Core EVM Utilities The foundation package providing shared ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ... xchain-evm-providers - Data Providers Multiple data providers for blockchain i...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...ta providers for blockchain interaction: - EtherscanProvider: Block explorer data...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...EtherscanProvider**: Block explorer data - CovalentProvider: Multi-chain API data...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...CovalentProvider**: Multi-chain API data - RoutescanProvider: Alternative explore...
(QB_NEW_EN)
[grammar] ~685-~685: There might be a mistake here.
Context: ...on EVM Issues ### 1. Gas-Related Issues - Underpriced Transaction: Increase gas ...
(QB_NEW_EN)
[grammar] ~690-~690: There might be a mistake here.
Context: ...mits with buffer ### 2. Provider Issues - Rate Limiting: Implement exponential b...
(QB_NEW_EN)
[grammar] ~695-~695: There might be a mistake here.
Context: ...s before processing ### 3. Token Issues - Invalid Contract Address: Validate con...
(QB_NEW_EN)
[grammar] ~700-~700: There might be a mistake here.
Context: ...t tokens ### 4. Network-Specific Issues - MEV Protection: Use private mempools f...
(QB_NEW_EN)
[grammar] ~727-~727: There might be a mistake here.
Context: ...``` ### Production Deployment Checklist - [ ] Configure rate-limited, reliable RPC...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
claude/xchainjs-expert-development-agent.md
388-388: Bare URL used
(MD034, no-bare-urls)
🪛 Biome (2.1.2)
packages/xchain-utxo/src/validators.ts
[error] 306-306: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 306-306: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 306-306: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 306-306: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 306-306: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 306-306: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
packages/xchain-bitcoin/src/clientKeystore.ts (1)
125-134: prepareTxEnhanced usage and types verified—no action needed• Reviewed
prepareTxEnhancedsignature in client.ts: it accepts
–utxoSelectionPreferences?: UtxoSelectionPreferenceswith fields
•minimizeFee,minimizeInputs,avoidDust
– and returnsPromise<PreparedTx>whererawUnsignedTxis a base64 string.
• Confirmed in clientKeystore.ts you’re passing exactly those three boolean prefs and destructuring onlyrawUnsignedTx.
• E2E tests and clientLedger usages likewise align with this signature and return type.Everything matches correctly—approving these changes.
packages/xchain-utxo/src/index.ts (1)
11-19: Re-exports look good and type-only exports are correctly marked.This cleanly surfaces errors/validator/selector to consumers. No issues.
packages/xchain-bitcoin/__e2e__/bitcoin-ledger-client.e2e.ts (1)
3-3: Importing baseAmount is appropriate for satoshi-precise max-send broadcasting.No concerns.
packages/xchain-utxo/src/validators.ts (1)
64-73: It looks like both validations use different maximum fee rates. We need to confirm the implementations before unifying:• validateTransferParams appears to allow up to 1 000 000 sat/byte.
• validateFeeRate is documented elsewhere (e.g., in error messages) to cap at 1 000 sat/byte.Once we have the exact code snippets, we can choose a single bound (1–1000 sat/vB) and replace all hard-coded values with a shared constant.
–––
Rewrite plan after verification:
- Import or define a
const MAX_FEE_RATE = 1000(sat/byte) in a shared constants file (e.g.,packages/xchain-utxo/src/constants.ts).- In
validateTransferParamsandvalidateFeeRate, replace any1000000or1000literals withMAX_FEE_RATE.- Update error messages to reference this constant (
Fee rate is unreasonably high (max ${MAX_FEE_RATE} sat/byte)).Pending the exact lines, please review and adjust both functions accordingly.
packages/xchain-bitcoin/__e2e__/blockcypher.bitcoin-client.e2e.ts (1)
203-306: transfer already uses the enhanced UTXO selection path—no mismatch riskI checked the
clientKeystore.transferimplementation and it callsprepareTxEnhancedwith the same defaultutxoSelectionPreferencesused bysendMax/prepareMaxTx. This means your broadcast viabtcClient.transferwill pick the identical inputs and fees that you calculated in the “send max” preparation step. You can safely remove the suggested gating logic.Likely an incorrect or invalid review comment.
packages/xchain-bitcoin/src/client.ts (1)
185-238: Max-send search bounds and behavior look sound.Binary search anchored to dust threshold with graceful insufficient-balance error reads well. Good use of selection preferences and no-change modeling.
| const { pubkey, output } = Bitcoin.payments.p2tr({ | ||
| address: sender, | ||
| }) | ||
| selectionResult.inputs.forEach((utxo: UTXO) => | ||
| psbt.addInput({ | ||
| hash: utxo.hash, | ||
| index: utxo.index, | ||
| witnessUtxo: { value: utxo.value, script: output as Buffer }, | ||
| tapInternalKey: pubkey, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
P2TR input construction: pass network to payments.p2tr for correctness.
bitcoinjs’ address decoding is network aware. Be explicit to avoid subtle mismatches on testnet/stagenet.
- const { pubkey, output } = Bitcoin.payments.p2tr({
- address: sender,
- })
+ const { pubkey, output } = Bitcoin.payments.p2tr({
+ address: sender,
+ network: Utils.btcNetwork(this.network),
+ })Repeat in sendMax where p2tr is used.
Also applies to: 540-551
🤖 Prompt for AI Agents
In packages/xchain-bitcoin/src/client.ts around lines 337 to 347 (and similarly
update the p2tr usage in sendMax at lines ~540-551), the Bitcoin.payments.p2tr
calls currently only pass { address: sender } which relies on default network
inference and can cause address/decoder mismatches on testnet/other networks;
update both calls to include the explicit network option (e.g., { address:
sender, network: this.network }) so the payment/address decoding is
network-aware and consistent with the client’s configured network.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
packages/xchain-bitcoin/src/clientLedger.ts (1)
64-73: Caller UTXO preferences are now respected (merging defaults) — good fixMerges defaults with caller-provided preferences without mutation; addresses the earlier concern.
packages/xchain-bitcoin/src/clientKeystore.ts (1)
124-131: Caller UTXO preferences are now respected (merging defaults) — goodDefaults are sensible and you allow overrides; aligns with ledger path.
packages/xchain-utxo/src/utxo-selector.ts (1)
46-50: Correct P2WPKH vbyte constantsUsing 68 vB/input and 31 vB/output fixes fee inflation noted previously.
packages/xchain-bitcoin/__e2e__/blockcypher.bitcoin-client.e2e.ts (1)
176-193: Gate mainnet broadcast behind an env flag to avoid flaky/expensive CI runsUnconditionally broadcasting costs real fees and can fail nondeterministically.
- // Actually send the prepared enhanced transaction - const txHash = await btcClient.transfer({ - asset: AssetBTC, - recipient: to, - amount, - memo: 'enhanced-test-broadcast', - feeRate: feeRate.rates.fast, - }) - - console.log('Enhanced TX broadcast successful!') - console.log('Transaction hash:', txHash) - - // Verify transaction hash format - expect(txHash).toBeDefined() - expect(typeof txHash).toBe('string') - expect(txHash.length).toBeGreaterThan(0) + if (process.env.ALLOW_MAINNET_TXS === 'true') { + // Actually send the prepared enhanced transaction + const txHash = await btcClient.transfer({ + asset: AssetBTC, + recipient: to, + amount, + memo: 'enhanced-test-broadcast', + feeRate: feeRate.rates.fast, + }) + console.log('Enhanced TX broadcast successful!') + console.log('Transaction hash:', txHash) + // Verify transaction hash format + expect(txHash).toBeDefined() + expect(typeof txHash).toBe('string') + expect(txHash.length).toBeGreaterThan(0) + } else { + console.warn('[E2E] Skipping mainnet broadcast; set ALLOW_MAINNET_TXS=true to enable') + }claude/utxo-expert-development-agent.md (2)
206-216: Good: switched from coinselect to first‑party UtxoSelector.This aligns the guide with the new multi‑strategy selector introduced by the PR.
265-279: Avoid nonWitnessUtxo example (misaligned with repo UTXO type).The repo’s exported UTXO type models witnessUtxo. Showing a nonWitnessUtxo branch (from txHex) will mislead readers.
if (utxo.witnessUtxo) { psbt.addInput({ hash: utxo.hash, index: utxo.index, witnessUtxo: utxo.witnessUtxo }) - } else if (utxo.txHex) { - // Use full transaction hex when witnessUtxo is not available - psbt.addInput({ - hash: utxo.hash, - index: utxo.index, - nonWitnessUtxo: Buffer.from(utxo.txHex, 'hex') - }) - } + } + // Note: To use non-witness inputs you must extend the UTXO type to include `nonWitnessUtxo` + // and adapt imports accordingly; current public types model `witnessUtxo`.
🧹 Nitpick comments (10)
packages/xchain-bitcoin/src/clientKeystore.ts (1)
111-111: Fix Prettier complaint on method signature formattingApply the formatter-friendly trailing comma/newline.
- async transfer(params: TxParams & { feeRate?: FeeRate; utxoSelectionPreferences?: UtxoSelectionPreferences }): Promise<TxHash> { + async transfer( + params: TxParams & { feeRate?: FeeRate; utxoSelectionPreferences?: UtxoSelectionPreferences }, + ): Promise<TxHash> {packages/xchain-utxo/src/utxo-selector.ts (2)
346-349: Use Fisher–Yates for unbiased shuffle (privacy/selection variance)
sort(() => Math.random() - 0.5)is biased and engine-dependent.- const shuffledUtxos = [...utxos].sort(() => Math.random() - 0.5) + const shuffledUtxos = [...utxos] + for (let i = shuffledUtxos.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)) + ;[shuffledUtxos[i], shuffledUtxos[j]] = [shuffledUtxos[j], shuffledUtxos[i]] + }
115-149: Expose weights as tunables (future-proofing preferences)Hardcoded weights make tuning difficult. Consider static defaults with optional overrides in preferences.
claude/utxo-expert-development-agent.md (7)
53-68: Clarify pseudo-code and missing imports.This class snippet references AddressFormat, BTCChain without imports. Add a brief “illustrative-only” note or include imports to prevent copy-paste errors.
export abstract class BitcoinClient extends UTXOClient { protected addressFormat: AddressFormat @@ constructor(params: UtxoClientParams & { addressFormat?: AddressFormat }) { + // Example-only snippet for documentation purposes
206-209: Use ES import in TypeScript instead of require().Keep the example idiomatic and typed.
- const { UtxoSelector } = require('@xchainjs/xchain-utxo') - const selector = new UtxoSelector() + import { UtxoSelector } from '@xchainjs/xchain-utxo' + const selector = new UtxoSelector()
288-294: Dust threshold should match output script type.546 sat is the common P2PKH threshold; P2WPKH is lower (~294 sat at 1 sat/vB). Consider deriving per AddressFormat.
- if (change > 546) { // Dust threshold + const dust = addressFormat === AddressFormat.P2WPKH ? 294 : 546 + if (change > dust) {
320-327: Use “sat/vB” terminology.Bitcoin fee rates are conventionally quoted in satoshis per virtual byte.
- feeRate: 10 // 10 sat/byte + feeRate: 10 // 10 sat/vB
428-447: Transaction size math is off; prefer weight/vbytes per input type.The constants here don’t reflect standard vbytes for P2WPKH/P2TR. Consider using weight-based sizes or well-known approximations (e.g., ~68 vB P2WPKH, ~57–65 vB P2TR) and 31 vB for P2WPKH outputs.
- const TX_OUTPUT_PUBKEYHASH = 25 + const TX_OUTPUT_PUBKEYHASH = 22 // P2WPKH scriptPubKey is 22 bytes @@ - case AddressFormat.P2WPKH: - inputSize = 32 + 4 + 1 + 4 + (107 / 4) // SegWit discount + case AddressFormat.P2WPKH: + inputSize = 68 // approx vbytes per input @@ - case AddressFormat.P2TR: - inputSize = 32 + 4 + 1 + 4 + (64 / 4) // Taproot witness discount + case AddressFormat.P2TR: + inputSize = 58 // approx vbytes per input (keypath spend)
654-667: Environment note: Buffer usage.Buffer.from requires Node or a polyfill; call this out for browser consumers or switch to Uint8Array helpers.
- script: Buffer.from(utxo.witnessUtxo.script, 'hex'), + // Node-only: Buffer.from(...). In browsers, convert hex -> Uint8Array. + script: Buffer.from(utxo.witnessUtxo.script, 'hex'),
779-781: Hardcoded 8 decimals.Derive decimals from the asset/client to support LTC, BCH, etc., even if many are 8 today.
- return this.chain === 'BTC' ? 8 : 8 // Default to 8 decimals + // Prefer: look up per-asset decimals from client or a chain map. + return 8
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
claude/utxo-expert-development-agent.md(1 hunks)packages/xchain-bitcoin/__e2e__/bitcoin-ledger-client.e2e.ts(2 hunks)packages/xchain-bitcoin/__e2e__/blockcypher.bitcoin-client.e2e.ts(1 hunks)packages/xchain-bitcoin/src/clientKeystore.ts(3 hunks)packages/xchain-bitcoin/src/clientLedger.ts(3 hunks)packages/xchain-utxo/src/utxo-selector.ts(1 hunks)packages/xchain-utxo/src/validators.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/xchain-utxo/src/validators.ts
- packages/xchain-bitcoin/e2e/bitcoin-ledger-client.e2e.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/xchain-bitcoin/__e2e__/blockcypher.bitcoin-client.e2e.ts (2)
packages/xchain-util/src/asset.ts (1)
assetToBase(146-152)packages/xchain-bitcoin/src/const.ts (1)
AssetBTC(40-40)
packages/xchain-bitcoin/src/clientKeystore.ts (2)
packages/xchain-client/src/types.ts (1)
TxParams(117-123)packages/xchain-utxo/src/utxo-selector.ts (1)
UtxoSelectionPreferences(26-32)
packages/xchain-utxo/src/utxo-selector.ts (2)
packages/xchain-utxo/src/index.ts (6)
UtxoSelectionStrategy(16-16)UTXO(9-9)UtxoSelectionResult(17-17)UtxoSelectionPreferences(18-18)UtxoSelector(15-15)UtxoError(12-12)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
packages/xchain-bitcoin/src/clientLedger.ts (3)
packages/xchain-utxo/src/types/types.ts (1)
TxParams(30-32)packages/xchain-client/src/types.ts (1)
TxParams(117-123)packages/xchain-utxo/src/utxo-selector.ts (1)
UtxoSelectionPreferences(26-32)
🪛 ESLint
packages/xchain-bitcoin/src/clientKeystore.ts
[error] 111-111: Replace params:·TxParams·&·{·feeRate?:·FeeRate;·utxoSelectionPreferences?:·UtxoSelectionPreferences·} with ⏎····params:·TxParams·&·{·feeRate?:·FeeRate;·utxoSelectionPreferences?:·UtxoSelectionPreferences·},⏎··
(prettier/prettier)
🪛 LanguageTool
claude/utxo-expert-development-agent.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...WPKH (Bech32) and P2TR (Taproot) support - Litecoin (xchain-litecoin) - Native ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...itecoin) - Native SegWit implementation - **Bitcoin Cash** (xchain-bitcoincash`) - ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...bitcoincash) - BCH with CashAddr format - **Dogecoin** (xchain-doge`) - DOGE with s...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...chain-doge) - DOGE with standard P2WPKH - **Dash** (xchain-dash`) - DASH with Insta...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...n-dash) - DASH with InstantSend support - **Zcash** (xchain-zcash`) - ZEC with tran...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...#### xchain-utxo - Core UTXO Framework The foundation package providing shared ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...xchain-utxo-providers - Data Providers Multiple data providers for UTXO blockch...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...oviders for UTXO blockchain interaction: - HaskoinProvider: Haskoin API integrati...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...skoinProvider**: Haskoin API integration - SochainProvider: Sochain API with API ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ider**: Sochain API with API key support - BlockcypherProvider: BlockCypher API i...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...rProvider**: BlockCypher API integration - BitgoProvider: BitGo enterprise API - ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ... BitgoProvider: BitGo enterprise API - NownodesProvider: Nownodes infrastruct...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
packages/xchain-bitcoin/src/clientLedger.ts (2)
75-81: prepareTxEnhanced wired correctly with merged preferencesPassing the merged preferences alongside sender and feeRate looks correct and consistent with keystore.
84-97: Inputs Are Already Restricted to a Single AddressI inspected the enhanced transaction builder (
buildTxEnhanced) inpackages/xchain-bitcoin/src/client.tsand verified:
- All UTXOs are fetched via
getValidatedUtxos(sender, …), which only returns UTXOs for the singlesenderaddress passed in – no other addresses.selectUtxosForTransactiononly ever sees that filtered list.- Consequently,
prepareTxEnhanced(which delegates tobuildTxEnhanced) can never select UTXOs from multiple derivation paths.Since the Ledger client maps each input to the one derivation path for
fromAddressIndex, andsenderis always the corresponding address for that index, there is no risk of mixing key paths. You can safely use the existing code without plumbing per-input key paths.packages/xchain-bitcoin/src/clientKeystore.ts (2)
5-5: Importing UtxoSelectionPreferences is correctMatches usage in transfer signature and merge logic.
133-138: Switched to prepareTxEnhanced — consistent and correctEnhanced builder usage and passing merged preferences look solid.
| - **Bitcoin** (`xchain-bitcoin`) - P2WPKH (Bech32) and P2TR (Taproot) support | ||
| - **Litecoin** (`xchain-litecoin`) - Native SegWit implementation | ||
| - **Bitcoin Cash** (`xchain-bitcoincash`) - BCH with CashAddr format | ||
| - **Dogecoin** (`xchain-doge`) - DOGE with standard P2WPKH | ||
| - **Dash** (`xchain-dash`) - DASH with InstantSend support | ||
| - **Zcash** (`xchain-zcash`) - ZEC with transparent addresses | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inaccurate chain support and address types (DOGE, DASH, ZEC).
- Dogecoin does not use P2WPKH/Bech32; it’s legacy P2PKH/P2SH.
- Please verify whether xchain-dash and xchain-zcash packages actually exist and are supported in this repo; if not, remove or mark as “planned/experimental” to avoid misleading readers.
Apply:
- - **Dogecoin** (`xchain-doge`) - DOGE with standard P2WPKH
- - **Dash** (`xchain-dash`) - DASH with InstantSend support
- - **Zcash** (`xchain-zcash`) - ZEC with transparent addresses
+ - **Dogecoin** (`xchain-doge`) - Legacy P2PKH/P2SH (no native Bech32/SegWit)
+ <!-- If DASH/ZEC are unsupported in this repo, omit them or add a clear note that support is out of scope here. -->📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Bitcoin** (`xchain-bitcoin`) - P2WPKH (Bech32) and P2TR (Taproot) support | |
| - **Litecoin** (`xchain-litecoin`) - Native SegWit implementation | |
| - **Bitcoin Cash** (`xchain-bitcoincash`) - BCH with CashAddr format | |
| - **Dogecoin** (`xchain-doge`) - DOGE with standard P2WPKH | |
| - **Dash** (`xchain-dash`) - DASH with InstantSend support | |
| - **Zcash** (`xchain-zcash`) - ZEC with transparent addresses | |
| - **Bitcoin** (`xchain-bitcoin`) - P2WPKH (Bech32) and P2TR (Taproot) support | |
| - **Litecoin** (`xchain-litecoin`) - Native SegWit implementation | |
| - **Bitcoin Cash** (`xchain-bitcoincash`) - BCH with CashAddr format | |
| - **Dogecoin** (`xchain-doge`) - Legacy P2PKH/P2SH (no native Bech32/SegWit) | |
| <!-- If DASH/ZEC are unsupported in this repo, omit them or add a clear note that support is out of scope here. --> |
🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: There might be a mistake here.
Context: ...WPKH (Bech32) and P2TR (Taproot) support - Litecoin (xchain-litecoin) - Native ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...itecoin) - Native SegWit implementation - **Bitcoin Cash** (xchain-bitcoincash`) - ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...bitcoincash) - BCH with CashAddr format - **Dogecoin** (xchain-doge`) - DOGE with s...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...chain-doge) - DOGE with standard P2WPKH - **Dash** (xchain-dash`) - DASH with Insta...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...n-dash) - DASH with InstantSend support - **Zcash** (xchain-zcash`) - ZEC with tran...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In claude/utxo-expert-development-agent.md around lines 13-19, the listed
chain/address support is inaccurate: Dogecoin does not use P2WPKH/Bech32 but
legacy P2PKH/P2SH, and xchain-dash/xchain-zcash packages may not exist; update
the Dogecoin line to state legacy P2PKH/P2SH (and verify the actual package name
if present), and check the repository for xchain-dash and xchain-zcash—if they
are not implemented remove them or mark them as “planned/experimental” (or point
to correct package names) to avoid misleading readers.
| import { | ||
| Client, | ||
| UTXO, | ||
| UtxoClientParams, | ||
| PreparedTx, | ||
| Witness, | ||
| toBitcoinJS | ||
| } from '@xchainjs/xchain-utxo' | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify import names exist in @xchainjs/xchain-utxo public API.
Double-check that Client, UTXO, UtxoClientParams, PreparedTx, Witness, toBitcoinJS are exported from the package’s index.
Run:
🏁 Script executed:
#!/bin/bash
# Inspect exports from xchain-utxo
fd -t f 'index.ts' packages/xchain-utxo | xargs -I{} rg -nP 'export\s+\{[^}]*\}' {}
rg -nP '\bexport\s+(type|interface|class|const|function)\s+(Client|UTXO|UtxoClientParams|PreparedTx|Witness|toBitcoinJS)\b' packages/xchain-utxoLength of output: 718
Missing exports in @xchainjs/xchain-utxo – imports will fail at compile time
The package’s public entry‐point (packages/xchain-utxo/index.ts) only re-exports:
- Client
- toBitcoinJS
- UtxoError, UtxoErrorCode
- UtxoTransactionValidator
It does not export UTXO, UtxoClientParams, PreparedTx or Witness, so your import in claude/utxo-expert-development-agent.md (lines 26–34) will fail.
Action items:
- Either import these types directly from their source file:
import { Client, toBitcoinJS } from '@xchainjs/xchain-utxo' import type { UtxoClientParams, PreparedTx, UTXO, Witness } from '@xchainjs/xchain-utxo/src/types/types'
- Or, preferably, add them to the package’s public API by updating
packages/xchain-utxo/index.ts:export { Client, toBitcoinJS }
- export type {
- UtxoClientParams,
- PreparedTx,
- UTXO,
- Witness
- } from './src/types/types'
export { UtxoError, UtxoErrorCode } from './errors'
export { UtxoTransactionValidator } from './validators'
This is required so that consumers can import all referenced symbols from `@xchainjs/xchain-utxo`.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In claude/utxo-expert-development-agent.md around lines 26–34, the import pulls
symbols (UTXO, UtxoClientParams, PreparedTx, Witness) that are not exported from
the package entrypoint and will fail at compile time; fix by either importing
only Client and toBitcoinJS from '@xchainjs/xchain-utxo' and adding type-only
imports for the missing types from '@xchainjs/xchain-utxo/src/types/types', or
preferably update packages/xchain-utxo/index.ts to re-export UTXO,
UtxoClientParams, PreparedTx and Witness from their source so consumers can
import them from the package root; after making the change, run the
build/type-check to confirm imports resolve.
</details>
<!-- fingerprinting:phantom:triton:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
| private providers: UtxoOnlineDataProviders[] | ||
| private currentProviderIndex: number = 0 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type name likely singular.
Check whether the correct type is UtxoOnlineDataProvider (singular). Adjust accordingly.
🏁 Script executed:
#!/bin/bash
rg -nP '\binterface\s+UtxoOnlineDataProviders\b|\btype\s+UtxoOnlineDataProviders\b' packages
rg -nP '\bUtxoOnlineDataProvider\b' packagesLength of output: 2889
Use singular UtxoOnlineDataProvider for the providers field
The codebase defines both
– export interface UtxoOnlineDataProvider { … }
and
– export type UtxoOnlineDataProviders = Record<Network, UtxoOnlineDataProvider | undefined>
The former is the interface you want when you store a list of provider instances; the latter is a mapping type alias. You should change the snippet in the agent to use the singular interface as the element type of your array.
Locations to update:
- claude/utxo-expert-development-agent.md, lines 347–349
- private providers: UtxoOnlineDataProviders[]
+ private providers: UtxoOnlineDataProvider[]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private providers: UtxoOnlineDataProviders[] | |
| private currentProviderIndex: number = 0 | |
| private providers: UtxoOnlineDataProvider[] | |
| private currentProviderIndex: number = 0 |
🤖 Prompt for AI Agents
In claude/utxo-expert-development-agent.md around lines 347 to 349, the
providers field is typed as an array of UtxoOnlineDataProviders (a Record
mapping) but should be an array of the singular interface
UtxoOnlineDataProvider; update the declaration to use UtxoOnlineDataProvider as
the array element type (i.e., change providers: UtxoOnlineDataProviders[] to
providers: UtxoOnlineDataProvider[]), and ensure any related usages or imports
align with the singular interface.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/xchain-utxo/src/utxo-selector.ts (1)
222-231: Fix exact-match fee double counting and incorrect change math (pass targetValue, not target+fee).
select()pre-adds a 1-input fee, thenfindExactMatch()adds it again. This misses true exact/no-change candidates and skews change. Also, the “reasonable change” branch subtracts the pre-added fee again when computingfinalChange. Apply:@@ - // Calculate the target including fee with no change output - const noChangeFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) - const exactTarget = targetValue + noChangeFee - - // Try to find exact match first - const exactResult = this.findExactMatch(sortedUtxos, exactTarget, feeRate, extraOutputs) + // Try to find single-input exact/no-change candidates + const exactResult = this.findExactMatch(sortedUtxos, targetValue, feeRate, extraOutputs) @@ - private findExactMatch( - utxos: UTXO[], - target: number, - feeRate: number, - extraOutputs: number, - ): UtxoSelectionResult | null { + private findExactMatch( + utxos: UTXO[], + targetValue: number, + feeRate: number, + extraOutputs: number, + ): UtxoSelectionResult | null { @@ - const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) - const requiredWithFee = target + singleInputFee + const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) + const requiredWithFee = targetValue + singleInputFee @@ - if (change > UtxoSelector.DUST_THRESHOLD && change < target * 0.1) { + if (change > UtxoSelector.DUST_THRESHOLD && change < targetValue * 0.1) { @@ - const finalChange = utxo.value - target - feeWithChange + const finalChange = utxo.value - targetValue - feeWithChangeAlso applies to: 241-271
🧹 Nitpick comments (4)
packages/xchain-utxo/src/utxo-selector.ts (4)
55-60: Dust threshold likely too high for P2WPKH; consider parameterizing by script type.Constants use P2WPKH vbytes (68/31), but dust is hardcoded to 546 (legacy P2PKH policy). For P2WPKH, ~294 sats is typical. Expose a per-script dust threshold or derive from output size and a relay fee baseline.
Would you like a follow-up patch that adds a ScriptType enum and computes dust per type?
365-367: Use Fisher–Yates instead ofsort(() => Math.random() - 0.5).
Array.sortwith a random comparator is biased and engine-dependent. Prefer an unbiased shuffle.- const shuffledUtxos = [...utxos].sort(() => Math.random() - 0.5) + const shuffledUtxos = fisherYatesShuffle([...utxos])Add once in this module:
function fisherYatesShuffle<T>(arr: T[]): T[] { for (let i = arr.length - 1; i > 0; i--) { const j = Math.floor(Math.random() * (i + 1)) ;[arr[i], arr[j]] = [arr[j], arr[i]] } return arr }
124-167: Scoring can saturate >1 due to consolidation bonuses; normalize or cap contributions.
smallUtxoCount * 0.2plus other bonuses frequently pushes raw score >1 then gets clamped, washing out preference weights. Consider scaling by fraction used (e.g., smallUtxoCount / inputs.length) and rebalancing weights to avoid frequent saturation.
172-185: Early insufficient-balance check ignores fees; error UX may mislead.You throw
INSUFFICIENT_BALANCEwhen total < target, but users will still often fail later due to fees. Option: compute a minimal “best-case” required (1 input, 1 output) and fail early with that total for clearer messaging.I can draft a minimal-fee precheck that still defers to strategies when borderline.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/xchain-utxo/__tests__/edge-cases.test.ts(1 hunks)packages/xchain-utxo/src/utxo-selector.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xchain-utxo/tests/edge-cases.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-utxo/src/utxo-selector.ts (2)
packages/xchain-utxo/src/index.ts (6)
UtxoSelectionStrategy(16-16)UTXO(9-9)UtxoSelectionResult(17-17)UtxoSelectionPreferences(18-18)UtxoSelector(15-15)UtxoError(12-12)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
claude/utxo-expert-development-agent.md (3)
264-279: Non-witness example can mislead — add an explicit caveat or removeRepo’s exported UTXO type models witnessUtxo. Keep readers from copying a non-compatible branch verbatim.
} else if (utxo.txHex) { - // Use full transaction hex when witnessUtxo is not available - psbt.addInput({ - hash: utxo.hash, - index: utxo.index, - nonWitnessUtxo: Buffer.from(utxo.txHex, 'hex') - }) + // NOTE: If using legacy/non-witness inputs, ensure your UTXO type includes `nonWitnessUtxo`. + // XChainJS currently exports a UTXO type modeling `witnessUtxo`. Extend types before using: + // psbt.addInput({ + // hash: utxo.hash, + // index: utxo.index, + // nonWitnessUtxo: Buffer.from(utxo.txHex, 'hex') + // }) }
347-351: Use singular UtxoOnlineDataProvider (not the Record alias) for arraysArray element type should be the interface, not the mapping alias.
- private providers: UtxoOnlineDataProviders[] + private providers: UtxoOnlineDataProvider[] - constructor(providers: UtxoOnlineDataProviders[]) { + constructor(providers: UtxoOnlineDataProvider[]) { - private getNextProvider(): UtxoOnlineDataProviders { + private getNextProvider(): UtxoOnlineDataProvider {Also applies to: 386-390
550-556: P2WPKH size constants are wrong — they’re legacy P2PKHUse SegWit vbytes; current values overpay and skew estimations.
private estimateConsolidationSize(inputCount: number, outputCount: number): number { // Simplified size calculation for consolidation const TX_OVERHEAD = 10 - const INPUT_SIZE = 148 // P2WPKH input size - const OUTPUT_SIZE = 34 // P2WPKH output size + const INPUT_SIZE = 68 // P2WPKH input vbytes (approx) + const OUTPUT_SIZE = 31 // P2WPKH output vbytes (approx) return TX_OVERHEAD + (inputCount * INPUT_SIZE) + (outputCount * OUTPUT_SIZE) }packages/xchain-utxo/src/utxo-selector.ts (1)
241-243: Exact-match path double-counts the single-input fee
targetalready includes single-input fee; adding it again misprices and rejects valid single-input matches.// Calculate fee for single input const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) - const requiredWithFee = target + singleInputFee + // `target` already includes the single-input fee from caller. + const requiredWithFee = target @@ - const feeWithChange = UtxoSelector.calculateFee(1, extraOutputs + 1, feeRate) - const finalChange = utxo.value - target - feeWithChange + const feeWithChange = UtxoSelector.calculateFee(1, extraOutputs + 1, feeRate) + // target = targetValue + singleInputFee => targetValue = target - singleInputFee + const finalChange = utxo.value - (target - singleInputFee) - feeWithChangeAlso applies to: 269-271
🧹 Nitpick comments (4)
claude/utxo-expert-development-agent.md (2)
26-34: Import UtxoSelector at module scope; avoid require in TS snippetsUse ESM imports and avoid per-call require.
import { Client, UTXO, UtxoClientParams, PreparedTx, Witness, - toBitcoinJS + toBitcoinJS, + UtxoSelector } from '@xchainjs/xchain-utxo'
206-213: Drop CommonJS require; rely on the top-level ESM importMinor TS hygiene.
- // Implementation using first-party UtxoSelector with multi-strategy approach - const { UtxoSelector } = require('@xchainjs/xchain-utxo') - const selector = new UtxoSelector() + // Implementation using first-party UtxoSelector with multi-strategy approach + const selector = new UtxoSelector()packages/xchain-utxo/src/utxo-selector.ts (2)
320-321: Prettier violations — fix formatting to satisfy lintObserved by static analysis; keep CI green.
- efficiency: this.calculateEfficiency(currentInputs, target, finalFee, change, change <= UtxoSelector.DUST_THRESHOLD), + efficiency: this.calculateEfficiency( + currentInputs, + target, + finalFee, + change, + change <= UtxoSelector.DUST_THRESHOLD, + ),- private calculateEfficiency(inputs: UTXO[], target: number, fee: number, change: number, changeAbsorbed: boolean = false): number { + private calculateEfficiency( + inputs: UTXO[], + target: number, + fee: number, + change: number, + changeAbsorbed: boolean = false, + ): number { const totalValue = inputs.reduce((sum, utxo) => sum + utxo.value, 0) - const wastedValue = changeAbsorbed ? 0 : (change > UtxoSelector.DUST_THRESHOLD ? 0 : change) + const wastedValue = changeAbsorbed ? 0 : change > UtxoSelector.DUST_THRESHOLD ? 0 : change const efficiency = target / (totalValue + fee + wastedValue) return Math.min(1, efficiency) }Also applies to: 350-355
55-60: Optional: parameterize fee sizing by input script type (P2WPKH/P2TR/etc.)Improves accuracy across formats; keep defaults as P2WPKH.
- public static readonly BYTES_PER_INPUT = 68 // approximate vbytes per P2WPKH input - public static readonly BYTES_PER_OUTPUT = 31 // vbytes per P2WPKH output + public static readonly BYTES_PER_INPUT = 68 // P2WPKH default + public static readonly BYTES_PER_OUTPUT = 31 // P2WPKH default + // future: accept AddressFormat to switch constants (e.g., P2TR ~58 in, ~43 out) - static calculateFee(inputCount: number, outputCount: number, feeRate: number): number { + static calculateFee(inputCount: number, outputCount: number, feeRate: number): number { const txSize = UtxoSelector.BASE_TX_SIZE + inputCount * UtxoSelector.BYTES_PER_INPUT + outputCount * UtxoSelector.BYTES_PER_OUTPUT return Math.ceil(txSize * feeRate) }Also applies to: 201-207
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
claude/utxo-expert-development-agent.md(1 hunks)packages/xchain-bitcoin/__tests__/client-old.test.ts(3 hunks)packages/xchain-utxo/src/utxo-selector.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-utxo/src/utxo-selector.ts (2)
packages/xchain-utxo/src/index.ts (6)
UtxoSelectionStrategy(16-16)UTXO(9-9)UtxoSelectionResult(17-17)UtxoSelectionPreferences(18-18)UtxoSelector(15-15)UtxoError(12-12)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
🪛 LanguageTool
claude/utxo-expert-development-agent.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...WPKH (Bech32) and P2TR (Taproot) support - Litecoin (xchain-litecoin) - Native ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...itecoin) - Native SegWit implementation - **Bitcoin Cash** (xchain-bitcoincash`) - ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...bitcoincash) - BCH with CashAddr format - **Dogecoin** (xchain-doge`) - DOGE with l...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ... - DOGE with legacy P2PKH/P2SH addresses - Dash (xchain-dash) - DASH with Insta...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...n-dash) - DASH with InstantSend support - **Zcash** (xchain-zcash`) - ZEC with tran...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...#### xchain-utxo - Core UTXO Framework The foundation package providing shared ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...xchain-utxo-providers - Data Providers Multiple data providers for UTXO blockch...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...oviders for UTXO blockchain interaction: - HaskoinProvider: Haskoin API integrati...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...skoinProvider**: Haskoin API integration - SochainProvider: Sochain API with API ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ider**: Sochain API with API key support - BlockcypherProvider: BlockCypher API i...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...rProvider**: BlockCypher API integration - BitgoProvider: BitGo enterprise API - ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ... BitgoProvider: BitGo enterprise API - NownodesProvider: Nownodes infrastruct...
(QB_NEW_EN)
🪛 ESLint
packages/xchain-utxo/src/utxo-selector.ts
[error] 320-320: Replace currentInputs,·target,·finalFee,·change,·change·<=·UtxoSelector.DUST_THRESHOLD with ⏎············currentInputs,⏎············target,⏎············finalFee,⏎············change,⏎············change·<=·UtxoSelector.DUST_THRESHOLD,⏎··········
(prettier/prettier)
[error] 350-350: Replace inputs:·UTXO[],·target:·number,·fee:·number,·change:·number,·changeAbsorbed:·boolean·=·false with ⏎····inputs:·UTXO[],⏎····target:·number,⏎····fee:·number,⏎····change:·number,⏎····changeAbsorbed:·boolean·=·false,⏎··
(prettier/prettier)
[error] 352-352: Replace (change·>·UtxoSelector.DUST_THRESHOLD·?·0·:·change) with change·>·UtxoSelector.DUST_THRESHOLD·?·0·:·change
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
claude/utxo-expert-development-agent.md (1)
26-34: All referenced exports are correctly exposed from @xchainjs/xchain-utxoI’ve checked
packages/xchain-utxo/src/index.tsand confirmed:
export { Client, toBitcoinJS }export type { UTXO, UtxoClientParams, Witness, PreparedTx, … }Since every symbol you import (Client, UTXO, UtxoClientParams, PreparedTx, Witness, toBitcoinJS) is publicly re-exported by the package entrypoint, the snippet will compile as written.
packages/xchain-bitcoin/__tests__/client-old.test.ts (3)
203-204: LGTM: assertion aligned with centralized error textCovers capitalized “Memo too long”.
253-254: LGTM: updated to “Insufficient balance:” prefixMatches UtxoError.insufficientBalance message.
263-264: LGTM: consistent insufficient-balance expectationKeeps tests in sync with new error strings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/xchain-utxo/src/utxo-selector.ts (1)
231-238: Fix exact-match fee double counting and incorrect change computation
findExactMatch()assumestargetalready includes the single-input fee (it’s passedexactTargetfrom Line 233) but adds the single-input fee again and later subtracts it once more when computingfinalChange. This misses true exact/no-change candidates and underestimatesfinalChange.Apply this minimal, localized fix:
@@ - // Calculate the target including fee with no change output - const noChangeFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) - const exactTarget = targetValue + noChangeFee + // Calculate the target including fee with no change output + const noChangeFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) + const exactTarget = targetValue + noChangeFee @@ private findExactMatch( utxos: UTXO[], target: number, feeRate: number, extraOutputs: number, ): UtxoSelectionResult | null { - // Calculate fee for single input - const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) - const requiredWithFee = target + singleInputFee + // `target` already includes the single-input fee from the caller. + const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) + const baseTarget = target - singleInputFee + const requiredWithFee = target @@ - if (change > UtxoSelector.DUST_THRESHOLD && change < target * 0.1) { + if (change > UtxoSelector.DUST_THRESHOLD && change < baseTarget * 0.1) { // Add change output fee const feeWithChange = UtxoSelector.calculateFee(1, extraOutputs + 1, feeRate) - const finalChange = utxo.value - target - feeWithChange + const finalChange = utxo.value - baseTarget - feeWithChangeAlso applies to: 243-252, 275-281
🧹 Nitpick comments (6)
packages/xchain-utxo/src/utxo-selector.ts (6)
459-463: Remove redundant sort in LargestFirst
AccumulativeStrategyalready sorts descending internally, so this extra sort is wasted work.- // Sort by value descending - const sortedUtxos = [...utxos].sort((a, b) => b.value - a.value) - return new AccumulativeStrategy().select(sortedUtxos, targetValue, feeRate, extraOutputs) + return new AccumulativeStrategy().select(utxos, targetValue, feeRate, extraOutputs)
385-388: Use Fisher–Yates instead of random sort comparator
sort(() => Math.random() - 0.5)is biased and engine-dependent.- // Shuffle UTXOs for randomness - const shuffledUtxos = [...utxos].sort(() => Math.random() - 0.5) + // Shuffle UTXOs (Fisher–Yates) + const shuffledUtxos = [...utxos] + for (let i = shuffledUtxos.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)) + ;[shuffledUtxos[i], shuffledUtxos[j]] = [shuffledUtxos[j], shuffledUtxos[i]] + }
69-70: Clarify extraOutputs doc to avoid confusionDefault of 1 means only recipient outputs; change is added conditionally elsewhere. Current comment suggests both.
- extraOutputs: number = 1, // recipient output + possible change output + extraOutputs: number = 1, // recipient outputs (exclude change; add +1 when change is needed)
55-60: Dust threshold likely mismatched for P2WPKH; consider parameterizingConstants reflect P2WPKH vbytes, but
DUST_THRESHOLDis set to 546 (legacy P2PKH). For P2WPKH, dust threshold is ~294 sats. Misalignment can over-absorb change into fees and skew strategy outcomes.Do you want to expose
dustThresholdinUtxoSelectionPreferences(or parameterize by script type), defaulting to sane per-script values?
79-85: Dust filter equality edge caseUsing
>excludes UTXOs exactly at the threshold; most implementations treat “< threshold” as dust. Consider>=for clarity.- ? utxos.filter((utxo) => utxo.value > UtxoSelector.DUST_THRESHOLD) + ? utxos.filter((utxo) => utxo.value >= UtxoSelector.DUST_THRESHOLD)
296-363: BnB pruning and tie-breaks (optional)
- Add a tie-break on fewer inputs when
feeis equal to prefer smaller txs.- Consider exposing
MAX_TRIESvia preferences for tunability under different UTXO sets.Happy to draft the minimal changes if you want this in Phase 1.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/xchain-utxo/src/utxo-selector.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-utxo/src/utxo-selector.ts (2)
packages/xchain-utxo/src/index.ts (6)
UtxoSelectionStrategy(16-16)UTXO(9-9)UtxoSelectionResult(17-17)UtxoSelectionPreferences(18-18)UtxoSelector(15-15)UtxoError(12-12)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/xchain-utxo/src/utxo-selector.ts (2)
238-241: Optional clarity: rename for readability and consistency.The fee here is the “single-input no-change” fee. Rename to avoid confusion.
- const noChangeFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) - const exactTarget = targetValue + noChangeFee + const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) + const exactTarget = targetValue + singleInputFee
256-259: Fix exact-match path: you’re double-adding the single-input fee (blocks true exact/no-change matches).
targetalready includes the single-input fee from the caller; adding it again requires utxo.value >= target + 2*fee. Apply:- // Calculate fee for single input - const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) - const requiredWithFee = target + singleInputFee + // `target` already includes the single-input fee from caller. + const singleInputFee = UtxoSelector.calculateFee(1, extraOutputs, feeRate) + const requiredWithFee = target
🧹 Nitpick comments (7)
packages/xchain-utxo/src/utxo-selector.ts (7)
55-57: Dust threshold likely legacy; verify and/or parameterize by script type.546 sat is the legacy P2PKH dust threshold; P2WPKH is typically ~294 sat (depends on relay fee and script). Consider parameterizing dust by output script type and relay fee, or at least verify intended behavior.
Do you want me to draft a tiny helper like
dustThreshold(scriptType, relayFeePerKvB)and thread it through strategies?
393-395: Replace non-uniform shuffle with Fisher–Yates; optionally allow seeded RNG for test determinism.
sort(() => Math.random() - 0.5)is biased and engine-dependent.- const shuffledUtxos = [...utxos].sort(() => Math.random() - 0.5) + const shuffledUtxos = [...utxos] + for (let i = shuffledUtxos.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)) + ;[shuffledUtxos[i], shuffledUtxos[j]] = [shuffledUtxos[j], shuffledUtxos[i]] + }Optional: accept an injected RNG for reproducible tests.
309-314: BnB pruning does O(n) work per node; precompute suffix sums.Avoid
slice(...).reduce(...)at every recursion step.let tries = 0 + + // Precompute suffix sums for pruning + const suffixSums = new Array(utxos.length + 1).fill(0) + for (let i = utxos.length - 1; i >= 0; i--) { + suffixSums[i] = suffixSums[i + 1] + utxos[i].value + } @@ - const remainingValue = utxos.slice(index).reduce((sum, utxo) => sum + utxo.value, 0) + const remainingValue = suffixSums[index]Also applies to: 356-358
69-70: Comment contradicts usage ofextraOutputs.You use
extraOutputsas “recipient outputs (excluding change)” and add+ 1when assuming change. Update the doc to avoid confusion.- extraOutputs: number = 1, // recipient output + possible change output + extraOutputs: number = 1, // recipient outputs (exclude change; add +1 when change is created)
96-98: Library logging: gateconsole.debugbehind an optional logger or debug flag.Prevents noisy logs for consumers and eases redirection to app-level logging.
I can wire a lightweight
Loggerinterface intoUtxoSelectorand strategies if you’d like.
188-195: Input validation: add finite/integer checks forfeeRateandtargetValue.Guard against NaN/Infinity and non-integer sat amounts.
- if (targetValue <= 0) { + if (!Number.isFinite(targetValue) || targetValue <= 0) { throw UtxoError.invalidAmount(targetValue, 'Target value must be positive') } @@ - if (feeRate <= 0) { + if (!Number.isFinite(feeRate) || feeRate <= 0) { throw UtxoError.invalidFeeRate(feeRate, 'Fee rate must be positive') }
101-104: Surface strategy in failure details when possible.If you can identify the “closest” strategy/attempt, include it in
utxoSelectionFailedto aid UX and telemetry.Happy to add a “best partial” tracker and plumb it into the error.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/xchain-utxo/src/utxo-selector.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-utxo/src/utxo-selector.ts (2)
packages/xchain-utxo/src/index.ts (6)
UtxoSelectionStrategy(16-16)UTXO(9-9)UtxoSelectionResult(17-17)UtxoSelectionPreferences(18-18)UtxoSelector(15-15)UtxoError(12-12)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
packages/xchain-utxo/src/utxo-selector.ts (3)
55-60: P2WPKH vbytes constants look correct.Good correction to 68/31 vB; base size 10 vB is acceptable as an approximation.
372-383: Efficiency calc avoids dust double-count now.Passing
changeAbsorbedand dropping extra “wasted” value double-count is correct.
476-512: SmallFirst now truly accumulates ascending — nice.This resolves the previous “re-sort” issue and aligns behavior with the strategy name.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
claude/utxo-expert-development-agent.md (2)
347-351: Type mismatch: use singular provider interface in arrays
This should be UtxoOnlineDataProvider[], not UtxoOnlineDataProviders[] (a mapping type).- private providers: UtxoOnlineDataProviders[] + private providers: UtxoOnlineDataProvider[] - constructor(providers: UtxoOnlineDataProviders[]) { + constructor(providers: UtxoOnlineDataProvider[]) {Also update getNextProvider’s return type (Lines 386–390):
- private getNextProvider(): UtxoOnlineDataProviders { + private getNextProvider(): UtxoOnlineDataProvider {
549-556: P2WPKH vbytes are wrong (148 is legacy P2PKH) — fix estimator
Using 148vB for SegWit will overpay fees substantially.private estimateConsolidationSize(inputCount: number, outputCount: number): number { // Simplified size calculation for consolidation const TX_OVERHEAD = 10 - const INPUT_SIZE = 148 // P2WPKH input size - const OUTPUT_SIZE = 34 // P2WPKH output size + const INPUT_SIZE = 68 // ~ P2WPKH input vbytes + const OUTPUT_SIZE = 31 // ~ P2WPKH output vbytes return TX_OVERHEAD + (inputCount * INPUT_SIZE) + (outputCount * OUTPUT_SIZE) }
🧹 Nitpick comments (5)
claude/utxo-expert-development-agent.md (5)
206-216: Use ESM import for UtxoSelector; avoid require in TS snippetsConsistency with other examples and typical TS builds.
- const { UtxoSelector } = require('@xchainjs/xchain-utxo') - const selector = new UtxoSelector() + const selector = new UtxoSelector()(Declare once at the top of the code block:)
+import { UtxoSelector } from '@xchainjs/xchain-utxo'
263-279: Non-witness input example can mislead — align with exported UTXO typeIf txHex/nonWitnessUtxo aren’t part of the exported UTXO type, this sample will fail when copy-pasted. Consider showing only witnessUtxo or add an explicit note.
- } else if (utxo.txHex) { - // Use full transaction hex when witnessUtxo is not available - psbt.addInput({ - hash: utxo.hash, - index: utxo.index, - nonWitnessUtxo: Buffer.from(utxo.txHex, 'hex') - }) - } + // If your UTXO model includes non-witness inputs, adapt accordingly by + // providing `nonWitnessUtxo: Buffer.from(txHex, 'hex')` here.
147-170: Validate utils import paths and API names
Confirm validateAddress and any btcNetwork/getPrefix exports are public and stable in @xchainjs/xchain-bitcoin/utils. If not, switch to the package’s public helpers (or re-export).
288-294: Dust threshold should be dynamic
546 sat is legacy P2PKH-oriented. For SegWit, compute dust from output size and feeRate to avoid unnecessary change removal.- if (change > 546) { // Dust threshold + const p2wpkhDust = Math.ceil(31 /*out vB*/ * feeRate) + if (change > p2wpkhDust) {
570-576: Ensure listed clients exist/are supported
Verify DogecoinClient and DashClient class names and packages are available in this repo; otherwise, mark as planned or provide alternate examples.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
claude/utxo-expert-development-agent.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
claude/utxo-expert-development-agent.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...WPKH (Bech32) and P2TR (Taproot) support - Litecoin (xchain-litecoin) - Native ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...itecoin) - Native SegWit implementation - **Bitcoin Cash** (xchain-bitcoincash`) - ...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...bitcoincash) - BCH with CashAddr format - **Dogecoin** (xchain-doge`) - DOGE with l...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ... - DOGE with legacy P2PKH/P2SH addresses - Dash (xchain-dash) - DASH with stand...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...) - DASH with standard UTXO transactions - Zcash (xchain-zcash) - ZEC with tran...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...#### xchain-utxo - Core UTXO Framework The foundation package providing shared ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...xchain-utxo-providers - Data Providers Multiple data providers for UTXO blockch...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ...oviders for UTXO blockchain interaction: - HaskoinProvider: Haskoin API integrati...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...skoinProvider**: Haskoin API integration - SochainProvider: Sochain API with API ...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...ider**: Sochain API with API key support - BlockcypherProvider: BlockCypher API i...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...rProvider**: BlockCypher API integration - BitgoProvider: BitGo enterprise API - ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ... BitgoProvider: BitGo enterprise API - NownodesProvider: Nownodes infrastruct...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
claude/utxo-expert-development-agent.md (2)
200-221: Nice: switched to first‑party UtxoSelector
This aligns the guide with the PR’s selector.
26-34: No changes required
UTXO, UtxoClientParams, PreparedTx, and Witness are already re-exported from the package’s root (src/index.ts), so importing them directly from@xchainjs/xchain-utxois valid.Likely an incorrect or invalid review comment.
| /** | ||
| * Branch and Bound strategy - optimal for minimizing fees and change | ||
| */ | ||
| export class BranchAndBoundStrategy implements UtxoSelectionStrategy { |
There was a problem hiding this comment.
Split one strategy per file to improve readability
| /** | ||
| * Validate address format for specific network | ||
| */ | ||
| static validateAddressFormat(address: string, network: Network, chain: string, allowedFormats?: string[]): void { |
There was a problem hiding this comment.
We already have an address validation function per client. Should we use them ?. Maybe address validation should be at specifica package level (bitcoin, litecoin, etc..) because it's different validation per chain.
| } | ||
|
|
||
| // Basic length check | ||
| if (address.length < 10 || address.length > 100) { |
There was a problem hiding this comment.
This seems like an arbitrary check that does not provide much value
| throw UtxoError.invalidAddress('', network) | ||
| } | ||
|
|
||
| if (typeof address !== 'string') { |
There was a problem hiding this comment.
Should we check this when we are using Typescript ?
| * | ||
| * @param {TxParams&Address&FeeRate&boolean} params The transfer options. | ||
| * @returns {PreparedTx} The raw unsigned transaction. | ||
| * @deprecated Use prepareTxEnhanced for better performance and validation |
There was a problem hiding this comment.
The new method respects the signature of the old one, so why not release a major version, remove the old method, and thus clean up the codebase and reduce dependencies, instead of just marking it as deprecated?
| // Address validation | ||
| if (!params.recipient?.trim()) { | ||
| errors.push('Recipient address is required') | ||
| } else if (params.recipient.length > 100) { |
There was a problem hiding this comment.
Why length > 100 ?
| } | ||
|
|
||
| // Check for dust amount (546 satoshis is standard dust threshold) | ||
| if (amount.lt(546)) { |
There was a problem hiding this comment.
I think dust threshole it's defiend as constant in other place. Maybe reuse constant instead of use magic number. Also wondering. Dush threshold can be equal for all UTXO changes ?
|
|
||
| // Validate decimal precision | ||
| const decimals = params.amount.decimal | ||
| if (decimals < 0 || decimals > 18) { |
There was a problem hiding this comment.
At the moment, all integrated UTXOs have 8 decimal places, why between 0 and 18 ?
| if (params.feeRate !== undefined) { | ||
| if (!Number.isFinite(params.feeRate)) { | ||
| errors.push('Fee rate must be a finite number') | ||
| } else if (params.feeRate < 1) { | ||
| errors.push('Fee rate must be at least 1 sat/byte') | ||
| } else if (params.feeRate > 1000000) { | ||
| errors.push('Fee rate is unreasonably high (max 1,000,000 sat/byte)') | ||
| } | ||
| } |
There was a problem hiding this comment.
Would it make sense to use the fee bounds here instead hardcoded values ?
| if (typeof params.memo !== 'string') { | ||
| errors.push('Memo must be a string') | ||
| } else { |
There was a problem hiding this comment.
Is the string check necessary? I also think this validation is being done several times
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/xchain-utxo/src/client.ts (1)
172-192: Fix always-truthy Protocol.THORCHAIN conditional
Replace theif (!protocol || Protocol.THORCHAIN)branch so it only fires whenprotocol === Protocol.THORCHAIN, not on every call.- if (!protocol || Protocol.THORCHAIN) { + if (!protocol || protocol === Protocol.THORCHAIN) { try { const feeRate = await this.getFeeRateFromThorchain() return standardFeeRates(feeRate) } catch { console.warn(`Can not retrieve fee rates from Thorchain`) } }packages/xchain-bitcoin/src/clientKeystore.ts (1)
120-125: Guard against missing phrase before key derivation.Avoid passing an undefined phrase into
getBtcKeys.const fromAddressIndex = params?.walletIndex || 0 - // Get the Bitcoin keys - const btcKeys = this.getBtcKeys(this.phrase, fromAddressIndex) + // Ensure phrase is present before deriving keys + if (!this.phrase) { + throw new Error('Phrase must be provided') + } + // Get the Bitcoin keys + const btcKeys = this.getBtcKeys(this.phrase, fromAddressIndex)
♻️ Duplicate comments (4)
packages/xchain-bitcoin/src/clientKeystore.ts (1)
126-132: LGTM: merged UTXO prefs honor caller overrides.Defaults are set and then overridden by caller-provided prefs. Matches earlier feedback.
packages/xchain-utxo/src/validators.ts (1)
230-233: Address length heuristic is brittle at UTXO-common layer.Length varies across chains and formats (e.g., bech32/bech32m). Prefer client-level validators; keep only emptiness/trim checks here.
- // Basic length check - most crypto addresses are between 25-100 characters - if (address.length < 10 || address.length > 100) { - throw UtxoError.validationError('Address length is invalid') - } + // Length checks are chain-/format-specific; defer to chain clientspackages/xchain-bitcoin/src/client.ts (2)
312-316: Fee underestimation: budget for potential change inextraOutputs.Selection must account for recipient + potential change (+ optional memo). Current value omits change, risking underfunded selections.
- const extraOutputs = 1 + (compiledMemo ? 1 : 0) // recipient + optional memo (change calculated separately) + // recipient + potential change (+ optional memo) + const extraOutputs = 2 + (compiledMemo ? 1 : 0)
337-340: Taproot input construction: passnetworktopayments.p2tr.Decoding is network-aware; relying on defaults risks testnet/mainnet mismatches.
- const { pubkey, output } = Bitcoin.payments.p2tr({ - address: sender, - }) + const { pubkey, output } = Bitcoin.payments.p2tr({ + address: sender, + network: Utils.btcNetwork(this.network), + })Repeat in both locations above.
Also applies to: 468-471
🧹 Nitpick comments (25)
packages/xchain-bitcoin/src/utils.ts (3)
41-48: Add an explicit default to guard futureNetworkadditions.Today all enum members are handled, but future additions would return
undefinedat runtime while the signature promisesBitcoin.Network.Apply:
export const btcNetwork = (network: Network): Bitcoin.Network => { switch (network) { case Network.Mainnet: case Network.Stagenet: return Bitcoin.networks.bitcoin case Network.Testnet: return Bitcoin.networks.testnet + default: + throw new Error(`Unsupported network: ${network as unknown as string}`) } } @@ export const getPrefix = (network: Network): string => { switch (network) { case Network.Mainnet: case Network.Stagenet: return 'bc1' case Network.Testnet: return 'tb1' + default: + throw new Error(`Unsupported network: ${network as unknown as string}`) } }Also applies to: 71-78
28-32: Handle empty arrays inarrayAverageto avoidNaN.Return 0 (or throw) on empty input.
export const arrayAverage = (array: number[]): number => { - let sum = 0 - array.forEach((value) => (sum += value)) - return sum / array.length + if (array.length === 0) return 0 + let sum = 0 + array.forEach((value) => (sum += value)) + return sum / array.length }
7-13: Potential duplication with new UTXO size constants.This file defines TX size constants while
@xchainjs/xchain-utxonow exportsTX_SIZE_CONSTANTS. Divergence will cause inconsistent fee/size estimates.
- Either import and use
TX_SIZE_CONSTANTShere, or clearly document why Bitcoin-specific constants differ.Would you like me to draft a follow-up patch unifying these?
packages/xchain-utxo/src/constants.ts (3)
6-16: Fix dust-threshold rationale in the comment.The current math is inaccurate for the standard 546-sat P2PKH threshold. Suggest a concise correction.
- * This is the minimum value for a UTXO to be considered economically spendable. - * The value comes from Bitcoin Core's calculation: - * - P2PKH output size: 34 bytes - * - Cost to spend: 34 bytes × 3 sat/byte = 102 satoshis - * - Dust threshold: 102 × 3 = 306 satoshis (theoretical) - * - Bitcoin Core uses 546 for safety margin and worst-case scenarios + * Minimum value for a UTXO to be considered economically spendable under default relay fees. + * Historically, at 3 sat/vB relay fee: + * - Spending a P2PKH input ~148 vB and creating a 34-byte output implies a break-even around 546 sats. + * - Bitcoin Core rounds to 546 sats for P2PKH; other script types can differ.
22-29: Clarify script types / add Taproot sizes.These are P2WPKH estimates. Consider documenting that explicitly and (optionally) exporting P2TR and P2PKH variants for better fidelity.
I can add a typed map like
{ P2WPKH: {...}, P2TR: {...}, P2PKH: {...} }if desired.
35-35: Prefer bigint for max-amount sentinel.Using a string forces downstream parsing.
bigintis safer and faster for bounds checks.-export const MAX_REASONABLE_AMOUNT = '2100000000000000' +export const MAX_REASONABLE_AMOUNT = 2_100_000_000_000_000nIf consumers rely on string, we can dual-export a string alias to avoid breaking changes.
packages/xchain-utxo/src/client.ts (1)
114-116: SilencingconfirmedOnlyis fine for now; mark it clearly as deprecated.Keeps interface compatibility with
XChainClient./** * Gets balance of a given address. * * @param {Address} address The address to get balances from - * @param {undefined} Needed for legacy only to be in common with `XChainClient` interface - will be removed by a next version - * @param {confirmedOnly} Flag to get balances of confirmed txs only + * @param _assets Not used (legacy compatibility with `XChainClient`) + * @param confirmedOnly Deprecated — reserved for future filtering. Not used yet. * * @returns {Balance[]} BTC balances */packages/xchain-utxo/src/strategies/largest-first.ts (1)
12-14: Remove redundant sort for O(n log n) -> O(n) delegation.
AccumulativeStrategysorts internally; pre-sorting here wastes work and allocates another array.The diff above already removes the extra sort.
packages/xchain-bitcoin/src/clientKeystore.ts (5)
37-41: Align error message with the actual check.Index 0 is allowed; the message says “greater than zero”.
- throw new Error('index must be greater than zero') + throw new Error('index must be greater than or equal to zero')
79-81: Type the async getter consistently with Address.Return type should be
Promise<Address>.- async getAddressAsync(index = 0): Promise<string> { + async getAddressAsync(index = 0): Promise<Address> {
107-110: Update JSDoc to reflect the actual params type.Document
TxParamswith optionalfeeRateandutxoSelectionPreferences.- * @param {TxParams&FeeRate} params The transfer options including the fee rate. + * @param {TxParams & { feeRate?: FeeRate; utxoSelectionPreferences?: UtxoSelectionPreferences }} params Transfer options.
155-160: Avoid double extraction of the transaction.Extract once and reuse to reduce work.
- // Extract the transaction hex - const txHex = psbt.extractTransaction().toHex() - - // Extract the transaction hash - const txHash = psbt.extractTransaction().getId() + // Extract the transaction once + const tx = psbt.extractTransaction() + const txHex = tx.toHex() + const txHash = tx.getId()
161-171: Preserve the original error object in catch.Bare
catch {}drops context; captureerrfor logging/telemetry.- } catch { + } catch (err) { + // TODO: add logger here if available, e.g., this.logger?.error('broadcast failed', err) // If broadcasting fails, return an error message with a link to the explorer const error = `Server error, please check explorer for tx confirmation ${this.explorerProviders[ this.network ].getExplorerTxUrl(txHash)}` return error }If you have a logger in this package, wire it up here to ensure failures are visible.
packages/xchain-utxo/src/strategies/types.ts (2)
8-9: Clarify units in the strategy contract.Document that
targetValueis in satoshis andfeeRateis in sat/vByte to prevent mismatches across strategies.- select(utxos: UTXO[], targetValue: number, feeRate: number, extraOutputs?: number): UtxoSelectionResult | null + select( + utxos: UTXO[], + targetValue: number, // satoshis + feeRate: number, // sat/vByte + extraOutputs?: number + ): UtxoSelectionResult | null
18-19: Make efficiency definition precise.State the formula (e.g.,
targetValue / (targetValue + feePaid)), and expected range [0, 1].- efficiency: number // 0-1 score indicating how good this selection is + efficiency: number // targetValue / (targetValue + feePaid), in [0, 1]packages/xchain-utxo/src/strategies/accumulative.ts (1)
16-19: Optional: consider smallest-first for true “accumulative.”Many accumulative coin selection algorithms add smallest UTXOs first to minimize change and fragmentation. If intentional, consider renaming to “LargestFirst” or document the choice.
Would you like a second variant (“SmallestFirstAccumulative”) to compare outcomes in the selector?
.changeset/violet-suits-spend.md (1)
10-12: Tighten wording and fix hyphenation; mention caller overrides.Reflect that defaults are merged and caller prefs take precedence; fix “backward compatible” hyphenation.
-- `ClientKeystore.transfer` and `ClientLedger.transfer` now use `prepareTxEnhanced` with default UTXO preferences (minimizeFee=true, avoidDust=true, minimizeInputs=false). - - Deprecate `buildTx` and `prepareTx` (backward compatible for now). +- `ClientKeystore.transfer` and `ClientLedger.transfer` now use `prepareTxEnhanced` with default UTXO preferences (minimizeFee=true, avoidDust=true, minimizeInputs=false), merged with caller-provided preferences. +- Deprecate `buildTx` and `prepareTx` (backward-compatible for now).packages/xchain-utxo/src/strategies/single-random-draw.ts (1)
17-19: Use Fisher–Yates instead ofsort(() => Math.random() - 0.5)for unbiased shuffling.The current pattern is biased and engine-dependent. Fisher–Yates is simple and stable.
- const shuffledUtxos = [...utxos].sort(() => Math.random() - 0.5) + const shuffledUtxos = [...utxos] + for (let i = shuffledUtxos.length - 1; i > 0; i--) { + const j = Math.floor(Math.random() * (i + 1)) + ;[shuffledUtxos[i], shuffledUtxos[j]] = [shuffledUtxos[j], shuffledUtxos[i]] + }packages/xchain-utxo/src/strategies/branch-and-bound.ts (1)
139-141: Avoid O(n) recomputation ofremainingValueat each node; precompute suffix sums.This is on the hot path and degrades DFS performance.
Outline:
- Precompute
suffixSum[i] = sum(utxos[i..end])once.- Replace
utxos.slice(index).reduce(...)withsuffixSum[index].I can provide a concrete patch if you’d like.
packages/xchain-utxo/src/validators.ts (3)
91-105: Avoid Node-onlyBuffer.byteLengthin shared lib; useTextEncoderfallback.Prevents bundler/node polyfill issues in browsers.
- const memoBytes = Buffer.byteLength(params.memo, 'utf8') + const memoBytes = + typeof TextEncoder !== 'undefined' + ? new TextEncoder().encode(params.memo).length + : Buffer.byteLength(params.memo, 'utf8')
181-184: Use the shared max-amount constant for consistency.This avoids the magic number duplication and keeps a single source of truth.
- } else if (utxo.value > 2100000000000000) { + } else if (utxo.value > Number(MAX_REASONABLE_AMOUNT)) { // 21M BTC in satoshis errors.push(`${prefix}: UTXO value exceeds maximum`) }
263-270: Library-levelconsole.warnnoise.Consider gating via a logger/verbosity flag to avoid noisy stdout in consuming apps.
packages/xchain-utxo/src/utxo-selector.ts (3)
45-47: ClarifyextraOutputssemantics in comment.It represents base outputs (recipient + optional memo), excluding change; strategies add change when needed. Current comment is misleading.
- extraOutputs: number = 1, // recipient output + possible change output + extraOutputs: number = 1, // base outputs (recipient [+ memo]); change is added internally if needed
55-57: Dust filter should include boundary.546 sats is generally the minimum non-dust for P2PKH; include equality to avoid dropping exact-threshold UTXOs.
- ? utxos.filter((utxo) => utxo.value > UtxoSelector.DUST_THRESHOLD) + ? utxos.filter((utxo) => utxo.value >= UtxoSelector.DUST_THRESHOLD)
165-176: Early “insufficient balance” check ignores fees.Not wrong, but message can be misleading when total >= target yet < target+fee. Consider removing this precheck and letting strategies emit precise
utxoSelectionFailedwith available total.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
.changeset/violet-suits-spend.md(1 hunks)packages/xchain-bitcoin/__e2e__/bitcoin-ledger-client.e2e.ts(2 hunks)packages/xchain-bitcoin/__tests__/client-old.test.ts(4 hunks)packages/xchain-bitcoin/src/client.ts(5 hunks)packages/xchain-bitcoin/src/clientKeystore.ts(4 hunks)packages/xchain-bitcoin/src/utils.ts(1 hunks)packages/xchain-utxo/__tests__/edge-cases.test.ts(1 hunks)packages/xchain-utxo/src/client.ts(3 hunks)packages/xchain-utxo/src/constants.ts(1 hunks)packages/xchain-utxo/src/index.ts(1 hunks)packages/xchain-utxo/src/strategies/accumulative.ts(1 hunks)packages/xchain-utxo/src/strategies/branch-and-bound.ts(1 hunks)packages/xchain-utxo/src/strategies/index.ts(1 hunks)packages/xchain-utxo/src/strategies/largest-first.ts(1 hunks)packages/xchain-utxo/src/strategies/single-random-draw.ts(1 hunks)packages/xchain-utxo/src/strategies/small-first.ts(1 hunks)packages/xchain-utxo/src/strategies/types.ts(1 hunks)packages/xchain-utxo/src/utxo-selector.ts(1 hunks)packages/xchain-utxo/src/validators.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/xchain-utxo/tests/edge-cases.test.ts
- packages/xchain-bitcoin/tests/client-old.test.ts
- packages/xchain-bitcoin/e2e/bitcoin-ledger-client.e2e.ts
- packages/xchain-utxo/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (10)
packages/xchain-utxo/src/strategies/largest-first.ts (2)
packages/xchain-utxo/src/strategies/types.ts (2)
UtxoSelectionStrategy(6-9)UtxoSelectionResult(14-20)packages/xchain-utxo/src/strategies/accumulative.ts (1)
AccumulativeStrategy(8-60)
packages/xchain-utxo/src/strategies/types.ts (1)
packages/xchain-utxo/src/index.ts (1)
UTXO(9-9)
packages/xchain-utxo/src/strategies/single-random-draw.ts (2)
packages/xchain-utxo/src/strategies/types.ts (2)
UtxoSelectionStrategy(6-9)UtxoSelectionResult(14-20)packages/xchain-utxo/src/constants.ts (2)
DUST_THRESHOLD(17-17)TX_SIZE_CONSTANTS(22-29)
packages/xchain-utxo/src/strategies/small-first.ts (3)
packages/xchain-utxo/src/strategies/types.ts (2)
UtxoSelectionStrategy(6-9)UtxoSelectionResult(14-20)packages/xchain-utxo/src/constants.ts (2)
DUST_THRESHOLD(17-17)TX_SIZE_CONSTANTS(22-29)packages/xchain-utxo/src/index.ts (1)
UTXO(9-9)
packages/xchain-utxo/src/strategies/branch-and-bound.ts (2)
packages/xchain-utxo/src/strategies/types.ts (2)
UtxoSelectionStrategy(6-9)UtxoSelectionResult(14-20)packages/xchain-utxo/src/constants.ts (2)
DUST_THRESHOLD(17-17)TX_SIZE_CONSTANTS(22-29)
packages/xchain-bitcoin/src/client.ts (6)
packages/xchain-utxo/src/types/types.ts (3)
UTXO(36-36)TxParams(30-32)PreparedTx(25-28)packages/xchain-bitcoincash/src/types/client-types.ts (2)
UTXO(22-29)FeeRate(5-5)packages/xchain-utxo/src/strategies/types.ts (2)
UtxoSelectionPreferences(25-31)UtxoSelectionResult(14-20)packages/xchain-utxo/src/utxo-selector.ts (1)
UtxoSelector(18-200)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)packages/xchain-utxo/src/validators.ts (1)
UtxoTransactionValidator(11-322)
packages/xchain-utxo/src/strategies/accumulative.ts (2)
packages/xchain-utxo/src/strategies/types.ts (2)
UtxoSelectionStrategy(6-9)UtxoSelectionResult(14-20)packages/xchain-utxo/src/constants.ts (2)
DUST_THRESHOLD(17-17)TX_SIZE_CONSTANTS(22-29)
packages/xchain-utxo/src/validators.ts (4)
packages/xchain-util/src/asset.ts (1)
Address(18-18)packages/xchain-client/src/types.ts (1)
FeeBounds(180-180)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)packages/xchain-utxo/src/constants.ts (3)
MAX_REASONABLE_AMOUNT(35-35)DUST_THRESHOLD(17-17)MAX_UTXO_DECIMALS(47-47)
packages/xchain-bitcoin/src/clientKeystore.ts (2)
packages/xchain-client/src/types.ts (3)
TxParams(117-123)FeeRate(145-145)TxHash(63-63)packages/xchain-utxo/src/strategies/types.ts (1)
UtxoSelectionPreferences(25-31)
packages/xchain-utxo/src/utxo-selector.ts (3)
packages/xchain-utxo/src/strategies/types.ts (3)
UtxoSelectionStrategy(6-9)UtxoSelectionPreferences(25-31)UtxoSelectionResult(14-20)packages/xchain-utxo/src/constants.ts (2)
DUST_THRESHOLD(17-17)TX_SIZE_CONSTANTS(22-29)packages/xchain-utxo/src/errors.ts (1)
UtxoError(23-234)
🪛 LanguageTool
.changeset/violet-suits-spend.md
[grammar] ~10-~10: There might be a mistake here.
Context: ..., avoidDust=true, minimizeInputs=false). - Deprecate buildTx and prepareTx (bac...
(QB_NEW_EN)
🔇 Additional comments (3)
packages/xchain-bitcoin/src/utils.ts (1)
60-62: Consistent with modern TS: barecatchis fine here.No behavior change; avoids an unused var.
packages/xchain-utxo/src/strategies/index.ts (1)
1-9: Barrel re-exports look good.Centralizes the strategy API surface without side effects.
packages/xchain-utxo/src/validators.ts (1)
49-69: Shorthand BigNumber comparison methods are valid aliases – BigNumber.js supports.lt,.lte,.gt,.gteas equivalent toisLessThan,isLessThanOrEqualTo,isGreaterThan,isGreaterThanOrEqualTorespectively, so the existing checks in validators.ts are correct and require no changes. (jsdocs.io, mikemcl.github.io)Likely an incorrect or invalid review comment.
| utxos: UTXO[], | ||
| feeRate: number, | ||
| hasMemo: boolean, | ||
| preferences?: UtxoSelectionPreferences, | ||
| ): { amount: number; fee: number; inputs: UTXO[] } { | ||
| const selector = new UtxoSelector() | ||
| const extraOutputs = hasMemo ? 2 : 1 // recipient + optional memo (no change for max send) | ||
|
|
||
| const totalBalance = utxos.reduce((sum, utxo) => sum + utxo.value, 0) | ||
|
|
||
| // Binary search for maximum sendable amount | ||
| let low = UtxoSelector.DUST_THRESHOLD | ||
| let high = totalBalance | ||
| let bestResult: UtxoSelectionResult | null = null | ||
|
|
||
| while (high - low > 1) { | ||
| const mid = Math.floor((low + high) / 2) | ||
|
|
||
| try { | ||
| const result = selector.selectOptimal(utxos, mid, feeRate, { ...preferences, minimizeFee: true }, extraOutputs) | ||
|
|
||
| // For max send, we don't want change - the goal is to send everything | ||
| const totalInput = result.inputs.reduce((sum, utxo) => sum + utxo.value, 0) | ||
| const maxSendable = totalInput - result.fee | ||
|
|
||
| if (maxSendable >= mid) { | ||
| bestResult = { | ||
| ...result, | ||
| changeAmount: 0, // No change for max send | ||
| } | ||
| low = mid | ||
| } else { | ||
| high = mid - 1 | ||
| } | ||
| } catch { | ||
| high = mid - 1 | ||
| } | ||
| } | ||
|
|
||
| if (!bestResult) { | ||
| throw UtxoError.insufficientBalance('max', totalBalance.toString(), 'BTC') | ||
| } | ||
|
|
||
| // Calculate the actual maximum we can send | ||
| const totalInput = bestResult.inputs.reduce((sum, utxo) => sum + utxo.value, 0) | ||
| const maxAmount = totalInput - bestResult.fee | ||
|
|
||
| return { | ||
| amount: maxAmount, | ||
| fee: bestResult.fee, | ||
| inputs: bestResult.inputs, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Max-send accuracy: recompute fee for “no-change” before comparing/returning.
selectOptimal may compute result.fee including a change output that sendMax later omits. This underestimates maxAmount and can skew the binary search. Recompute a no-change fee and use it for the decision and the returned fee.
- const extraOutputs = hasMemo ? 2 : 1 // recipient + optional memo (no change for max send)
+ const extraOutputs = hasMemo ? 2 : 1 // base outputs: recipient + optional memo (no change)
@@
- try {
- const result = selector.selectOptimal(utxos, mid, feeRate, { ...preferences, minimizeFee: true }, extraOutputs)
+ try {
+ const result = selector.selectOptimal(utxos, mid, feeRate, { ...preferences, minimizeFee: true }, extraOutputs)
@@
- const totalInput = result.inputs.reduce((sum, utxo) => sum + utxo.value, 0)
- const maxSendable = totalInput - result.fee
-
- if (maxSendable >= mid) {
+ const totalInput = result.inputs.reduce((sum, utxo) => sum + utxo.value, 0)
+ // No-change fee (outputs exclude change)
+ const feeNoChange = UtxoSelector.calculateFee(result.inputs.length, extraOutputs, feeRate)
+ const maxSendableNoChange = totalInput - feeNoChange
+
+ if (maxSendableNoChange >= mid) {
bestResult = {
...result,
- changeAmount: 0, // No change for max send
+ changeAmount: 0,
+ fee: feeNoChange,
}
low = mid
} else {
high = mid - 1
}And when returning, maxAmount is already totalInput - bestResult.fee.
🤖 Prompt for AI Agents
In packages/xchain-bitcoin/src/client.ts around lines 186 to 238, the code uses
result.fee from selectOptimal which may include a change output, causing
underestimation when computing a "no-change" sendMax; recompute the fee assuming
no change (i.e., recalc fee for result.inputs with extraOutputs reduced by one /
change output omitted) and use that no-change fee for the binary search
decision, for bestResult.fee assignment, and for the returned fee/amount
calculation so comparisons and the final returned values reflect the true
no-change transaction cost.
| try { | ||
| // Basic validation (skip amount validation since we're calculating max) | ||
| if (!recipient?.trim()) { | ||
| throw UtxoError.invalidAddress(recipient, this.network) | ||
| } | ||
|
|
||
| if (!this.validateAddress(recipient)) { | ||
| throw UtxoError.invalidAddress(recipient, this.network) | ||
| } | ||
| if (!this.validateAddress(sender)) { | ||
| throw UtxoError.invalidAddress(sender, this.network) | ||
| } | ||
|
|
||
| // Memo validation is handled by validateTransactionInputs |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate memo in sendMax (size/control chars) to avoid non-standard OP_RETURN.
sendMax skips validateTransactionInputs, so oversize or invalid memos can slip through and cause broadcast failures.
if (!this.validateAddress(sender)) {
throw UtxoError.invalidAddress(sender, this.network)
}
- // Memo validation is handled by validateTransactionInputs
+ // Memo validation (80B limit, no control/null bytes)
+ if (memo !== undefined) {
+ const memoBytes =
+ typeof TextEncoder !== 'undefined' ? new TextEncoder().encode(memo).length : Buffer.byteLength(memo, 'utf8')
+ if (memoBytes > 80) throw UtxoError.invalidMemo(memo, `Memo too long: ${memoBytes} bytes (max 80)`)
+ for (let i = 0; i < memo.length; i++) {
+ const c = memo.charCodeAt(i)
+ if ((c <= 8) || c === 11 || c === 12 || (c >= 14 && c <= 31) || c === 127) {
+ throw UtxoError.invalidMemo(memo, 'Memo contains invalid control characters')
+ }
+ }
+ if (memo.includes('\0')) throw UtxoError.invalidMemo(memo, 'Memo cannot contain null bytes')
+ }Follow-up (optional): extract this into UtxoTransactionValidator.validateMemo() and reuse in both paths.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // Basic validation (skip amount validation since we're calculating max) | |
| if (!recipient?.trim()) { | |
| throw UtxoError.invalidAddress(recipient, this.network) | |
| } | |
| if (!this.validateAddress(recipient)) { | |
| throw UtxoError.invalidAddress(recipient, this.network) | |
| } | |
| if (!this.validateAddress(sender)) { | |
| throw UtxoError.invalidAddress(sender, this.network) | |
| } | |
| // Memo validation is handled by validateTransactionInputs | |
| try { | |
| // Basic validation (skip amount validation since we're calculating max) | |
| if (!recipient?.trim()) { | |
| throw UtxoError.invalidAddress(recipient, this.network) | |
| } | |
| if (!this.validateAddress(recipient)) { | |
| throw UtxoError.invalidAddress(recipient, this.network) | |
| } | |
| if (!this.validateAddress(sender)) { | |
| throw UtxoError.invalidAddress(sender, this.network) | |
| } | |
| // Memo validation (80B limit, no control/null bytes) | |
| if (memo !== undefined) { | |
| const memoBytes = | |
| typeof TextEncoder !== 'undefined' | |
| ? new TextEncoder().encode(memo).length | |
| : Buffer.byteLength(memo, 'utf8') | |
| if (memoBytes > 80) { | |
| throw UtxoError.invalidMemo( | |
| memo, | |
| `Memo too long: ${memoBytes} bytes (max 80)` | |
| ) | |
| } | |
| for (let i = 0; i < memo.length; i++) { | |
| const c = memo.charCodeAt(i) | |
| if ((c <= 8) || c === 11 || c === 12 || (c >= 14 && c <= 31) || c === 127) { | |
| throw UtxoError.invalidMemo( | |
| memo, | |
| 'Memo contains invalid control characters' | |
| ) | |
| } | |
| } | |
| if (memo.includes('\0')) { | |
| throw UtxoError.invalidMemo(memo, 'Memo cannot contain null bytes') | |
| } | |
| } | |
| // ... rest of sendMax logic ... | |
| } catch (error) { | |
| // error handling | |
| } |
🤖 Prompt for AI Agents
In packages/xchain-bitcoin/src/client.ts around lines 433 to 446, sendMax
currently skips validateTransactionInputs so memo fields can exceed allowed
OP_RETURN size or contain control characters and cause non-standard/broadcast
failures; update sendMax to validate the memo the same way
validateTransactionInputs does by either calling a new
UtxoTransactionValidator.validateMemo(memo) or extracting the memo checks into
such a method and invoking it here, ensuring it enforces max byte length and
rejects control/non-printable chars (and trim/normalize as needed) so
oversized/invalid memos are rejected before building the transaction.
| import { FeeOption, FeeRate, TxHash, checkFeeBounds } from '@xchainjs/xchain-client' | ||
| import { getSeed } from '@xchainjs/xchain-crypto' | ||
| import { Address } from '@xchainjs/xchain-util' | ||
| import { TxParams, UtxoClientParams } from '@xchainjs/xchain-utxo' | ||
| import { TxParams, UtxoClientParams, UtxoSelectionPreferences } from '@xchainjs/xchain-utxo' | ||
| import { HDKey } from '@scure/bip32' |
There was a problem hiding this comment.
Fix incorrect import source for TxParams.
TxParams should come from @xchainjs/xchain-client, not @xchainjs/xchain-utxo.
Apply:
-import { FeeOption, FeeRate, TxHash, checkFeeBounds } from '@xchainjs/xchain-client'
+import { FeeOption, FeeRate, TxHash, checkFeeBounds, TxParams } from '@xchainjs/xchain-client'
@@
-import { TxParams, UtxoClientParams, UtxoSelectionPreferences } from '@xchainjs/xchain-utxo'
+import { UtxoClientParams, UtxoSelectionPreferences } from '@xchainjs/xchain-utxo'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { FeeOption, FeeRate, TxHash, checkFeeBounds } from '@xchainjs/xchain-client' | |
| import { getSeed } from '@xchainjs/xchain-crypto' | |
| import { Address } from '@xchainjs/xchain-util' | |
| import { TxParams, UtxoClientParams } from '@xchainjs/xchain-utxo' | |
| import { TxParams, UtxoClientParams, UtxoSelectionPreferences } from '@xchainjs/xchain-utxo' | |
| import { HDKey } from '@scure/bip32' | |
| import { FeeOption, FeeRate, TxHash, checkFeeBounds, TxParams } from '@xchainjs/xchain-client' | |
| import { getSeed } from '@xchainjs/xchain-crypto' | |
| import { Address } from '@xchainjs/xchain-util' | |
| import { UtxoClientParams, UtxoSelectionPreferences } from '@xchainjs/xchain-utxo' | |
| import { HDKey } from '@scure/bip32' |
🤖 Prompt for AI Agents
In packages/xchain-bitcoin/src/clientKeystore.ts around lines 2 to 6, the file
incorrectly imports TxParams from '@xchainjs/xchain-utxo'; update the imports so
that TxParams is imported from '@xchainjs/xchain-client' instead (remove
TxParams from the '@xchainjs/xchain-utxo' import and add it to the import list
from '@xchainjs/xchain-client'), keeping the other imports unchanged.
| return { | ||
| inputs: [...selectedInputs], | ||
| changeAmount: finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalChange : 0, | ||
| fee: finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange, | ||
| efficiency: targetValue / (currentValue + finalFee), | ||
| strategy: this.name, | ||
| } |
There was a problem hiding this comment.
Fix efficiency and fee when no change output is created.
Current efficiency uses currentValue + finalFee (double counts fee) and fee omits dust handling only in one branch. Compute feeToPay once and base efficiency on targetValue + feeToPay.
- return {
- inputs: [...selectedInputs],
- changeAmount: finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalChange : 0,
- fee: finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange,
- efficiency: targetValue / (currentValue + finalFee),
- strategy: this.name,
- }
+ const changeAmount =
+ finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalChange : 0
+ const feeToPay = finalFee + (changeAmount === 0 ? finalChange : 0)
+ return {
+ inputs: [...selectedInputs],
+ changeAmount,
+ fee: feeToPay,
+ efficiency: targetValue / (targetValue + feeToPay),
+ strategy: this.name,
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| inputs: [...selectedInputs], | |
| changeAmount: finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalChange : 0, | |
| fee: finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange, | |
| efficiency: targetValue / (currentValue + finalFee), | |
| strategy: this.name, | |
| } | |
| const changeAmount = | |
| finalChange > AccumulativeStrategy.DUST_THRESHOLD ? finalChange : 0 | |
| const feeToPay = finalFee + (changeAmount === 0 ? finalChange : 0) | |
| return { | |
| inputs: [...selectedInputs], | |
| changeAmount, | |
| fee: feeToPay, | |
| efficiency: targetValue / (targetValue + feeToPay), | |
| strategy: this.name, | |
| } |
🤖 Prompt for AI Agents
In packages/xchain-utxo/src/strategies/accumulative.ts around lines 37 to 43,
the result builds fee, changeAmount and efficiency inconsistently: fee is
adjusted for dust only in one branch and efficiency uses currentValue + finalFee
which double-counts fee. Compute a single feeToPay = finalChange >
AccumulativeStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange, set
changeAmount = finalChange > DUST ? finalChange : 0, set fee = feeToPay, and
compute efficiency as targetValue / (targetValue + feeToPay).
| // Calculate the target including fee with no change output | ||
| const noChangeFee = this.calculateFee(1, extraOutputs, feeRate) | ||
| const exactTarget = targetValue + noChangeFee | ||
|
|
||
| // Try to find exact match first | ||
| const exactResult = this.findExactMatch(sortedUtxos, exactTarget, feeRate, extraOutputs) | ||
| if (exactResult) return exactResult | ||
|
|
||
| // Run branch and bound algorithm | ||
| return this.branchAndBound(sortedUtxos, targetValue, feeRate, extraOutputs) | ||
| } |
There was a problem hiding this comment.
Exact-match path adds the fee twice; pass targetValue into findExactMatch.
exactTarget = targetValue + noChangeFee and then requiredWithFee = target + singleInputFee double-counts the fee, breaking exact-match detection.
- const noChangeFee = this.calculateFee(1, extraOutputs, feeRate)
- const exactTarget = targetValue + noChangeFee
-
- // Try to find exact match first
- const exactResult = this.findExactMatch(sortedUtxos, exactTarget, feeRate, extraOutputs)
+ // Try to find exact match first (compute fee inside the helper)
+ const exactResult = this.findExactMatch(sortedUtxos, targetValue, feeRate, extraOutputs)
if (exactResult) return exactResultAnd inside findExactMatch the existing
const requiredWithFee = target + singleInputFeeis then correct and needs no change.
| // Calculate current fee and requirements | ||
| const inputCount = currentInputs.length | ||
| const outputCount = | ||
| extraOutputs + | ||
| (currentValue > | ||
| target + this.calculateFee(inputCount, extraOutputs, feeRate) + BranchAndBoundStrategy.DUST_THRESHOLD | ||
| ? 1 | ||
| : 0) | ||
| const fee = this.calculateFee(inputCount, outputCount, feeRate) | ||
| const required = target + fee | ||
|
|
||
| // Check if we have a solution | ||
| if (currentValue >= required) { | ||
| const change = currentValue - required | ||
| const finalFee = change > BranchAndBoundStrategy.DUST_THRESHOLD ? fee : fee + change | ||
|
|
||
| const result: UtxoSelectionResult = { | ||
| inputs: [...currentInputs], | ||
| changeAmount: change > BranchAndBoundStrategy.DUST_THRESHOLD ? change : 0, | ||
| fee: finalFee, | ||
| efficiency: this.calculateEfficiency( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Recalculate fee/output count at solution time; current logic can overestimate fee when final change is dust.
outputCount is pre-guessed. If the final change ends up dust, you add it to fee without removing the change output’s bytes, overpaying by one output’s fee.
- if (currentValue >= required) {
- const change = currentValue - required
- const finalFee = change > BranchAndBoundStrategy.DUST_THRESHOLD ? fee : fee + change
-
- const result: UtxoSelectionResult = {
- inputs: [...currentInputs],
- changeAmount: change > BranchAndBoundStrategy.DUST_THRESHOLD ? change : 0,
- fee: finalFee,
- efficiency: this.calculateEfficiency(
- currentInputs,
- target,
- finalFee,
- change,
- change <= BranchAndBoundStrategy.DUST_THRESHOLD,
- ),
- strategy: this.name,
- }
+ if (currentValue >= required) {
+ // Decide final outputs based on actual change and recompute fee/change consistently
+ const outputsIfChange = extraOutputs + 1
+ const feeIfChange = this.calculateFee(inputCount, outputsIfChange, feeRate)
+ const changeIfChange = currentValue - target - feeIfChange
+ let finalOutputs = extraOutputs
+ let finalFee = this.calculateFee(inputCount, finalOutputs, feeRate)
+ let finalChange = currentValue - target - finalFee
+ if (changeIfChange > BranchAndBoundStrategy.DUST_THRESHOLD) {
+ finalOutputs = outputsIfChange
+ finalFee = feeIfChange
+ finalChange = changeIfChange
+ } else if (finalChange <= BranchAndBoundStrategy.DUST_THRESHOLD) {
+ finalFee = finalFee + Math.max(0, finalChange)
+ finalChange = 0
+ }
+
+ const result: UtxoSelectionResult = {
+ inputs: [...currentInputs],
+ changeAmount: finalChange,
+ fee: finalFee,
+ efficiency: this.calculateEfficiency(
+ currentInputs,
+ target,
+ finalFee,
+ finalChange,
+ finalChange === 0, // dust absorbed
+ ),
+ strategy: this.name,
+ }Committable suggestion skipped: line range outside the PR's diff.
| select(utxos: UTXO[], targetValue: number, feeRate: number, extraOutputs: number = 1): UtxoSelectionResult | null { | ||
| // Sort by value descending | ||
| const sortedUtxos = [...utxos].sort((a, b) => b.value - a.value) | ||
| return new AccumulativeStrategy().select(sortedUtxos, targetValue, feeRate, extraOutputs) | ||
| } |
There was a problem hiding this comment.
Strategy name leak: result currently reports Accumulative.
Since you delegate to AccumulativeStrategy, its strategy field bubbles up. Override it to LargestFirst.
- select(utxos: UTXO[], targetValue: number, feeRate: number, extraOutputs: number = 1): UtxoSelectionResult | null {
- // Sort by value descending
- const sortedUtxos = [...utxos].sort((a, b) => b.value - a.value)
- return new AccumulativeStrategy().select(sortedUtxos, targetValue, feeRate, extraOutputs)
- }
+ select(utxos: UTXO[], targetValue: number, feeRate: number, extraOutputs: number = 1): UtxoSelectionResult | null {
+ // Avoid double-sorting; AccumulativeStrategy already sorts internally.
+ const res = new AccumulativeStrategy().select(utxos, targetValue, feeRate, extraOutputs)
+ return res ? { ...res, strategy: this.name } : null
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/xchain-utxo/src/strategies/largest-first.ts around lines 11 to 15,
the selection result currently reports the delegated AccumulativeStrategy name;
after calling AccumulativeStrategy().select(...), override the returned
UtxoSelectionResult.strategy property to "LargestFirst" (or replace the strategy
field value) before returning so the selection result correctly identifies the
LargestFirst strategy.
| const fee = this.calculateFee(1, extraOutputs, feeRate) | ||
| const required = targetValue + fee | ||
|
|
||
| if (utxo.value >= required) { | ||
| const change = utxo.value - required | ||
| const hasChange = change > SingleRandomDrawStrategy.DUST_THRESHOLD | ||
| const finalFee = hasChange ? fee : fee + change | ||
|
|
||
| return { | ||
| inputs: [utxo], | ||
| changeAmount: hasChange ? change : 0, | ||
| fee: finalFee, | ||
| efficiency: targetValue / (utxo.value + finalFee), | ||
| strategy: this.name, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix fee underestimation when a change output is needed.
fee is computed without accounting for a potential change output, but when hasChange is true, the transaction will have one more output and thus a higher fee. This underestimates required, risks underpaying fees, and skews efficiency.
Apply a two-path check (no-change vs. with-change) and only return when the fee matches the chosen output count:
- const fee = this.calculateFee(1, extraOutputs, feeRate)
- const required = targetValue + fee
-
- if (utxo.value >= required) {
- const change = utxo.value - required
- const hasChange = change > SingleRandomDrawStrategy.DUST_THRESHOLD
- const finalFee = hasChange ? fee : fee + change
-
- return {
- inputs: [utxo],
- changeAmount: hasChange ? change : 0,
- fee: finalFee,
- efficiency: targetValue / (utxo.value + finalFee),
- strategy: this.name,
- }
- }
+ // Path 1: no change output
+ const feeNoChange = this.calculateFee(1, extraOutputs, feeRate)
+ const requiredNoChange = targetValue + feeNoChange
+ if (utxo.value >= requiredNoChange) {
+ const changeNoChange = utxo.value - requiredNoChange
+ if (changeNoChange <= SingleRandomDrawStrategy.DUST_THRESHOLD) {
+ const finalFee = feeNoChange + changeNoChange
+ return {
+ inputs: [utxo],
+ changeAmount: 0,
+ fee: finalFee,
+ efficiency: targetValue / (utxo.value + finalFee),
+ strategy: this.name,
+ }
+ }
+ }
+
+ // Path 2: with change output
+ const feeWithChange = this.calculateFee(1, extraOutputs + 1, feeRate)
+ const requiredWithChange = targetValue + feeWithChange
+ if (utxo.value >= requiredWithChange) {
+ const finalChange = utxo.value - requiredWithChange
+ if (finalChange > SingleRandomDrawStrategy.DUST_THRESHOLD) {
+ const finalFee = feeWithChange
+ return {
+ inputs: [utxo],
+ changeAmount: finalChange,
+ fee: finalFee,
+ efficiency: targetValue / (utxo.value + finalFee),
+ strategy: this.name,
+ }
+ }
+ }🤖 Prompt for AI Agents
In packages/xchain-utxo/src/strategies/single-random-draw.ts around lines 21 to
35, the code computes fee once assuming no change output then decides hasChange
which can require an extra output and thus a larger fee, causing
underestimation; fix by performing a two-path check: compute feeNoChange =
calculateFee(1, extraOutputs, feeRate) and check if utxo.value >= targetValue +
feeNoChange (no-change path) returning that result, otherwise compute
feeWithChange = calculateFee(1, extraOutputs + 1, feeRate) and check if
utxo.value >= targetValue + feeWithChange (with-change path), set changeAmount =
utxo.value - targetValue - feeWithChange when applicable, set fee to the
corresponding final fee, compute efficiency using targetValue / (utxo.value +
fee) and only return when the fee matches the chosen output count.
| return { | ||
| inputs: [...selectedInputs], | ||
| changeAmount: finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalChange : 0, | ||
| fee: finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange, | ||
| efficiency: targetValue / (currentValue + finalFee), | ||
| strategy: this.name, | ||
| } |
There was a problem hiding this comment.
Efficiency is computed with finalFee but returned fee may include dust; use the returned fee for consistency.
When finalChange <= DUST_THRESHOLD, you return finalFee + finalChange as fee, yet efficiency divides by currentValue + finalFee, overstating efficiency.
- return {
+ const returnedFee =
+ finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange
+ return {
inputs: [...selectedInputs],
- changeAmount: finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalChange : 0,
- fee: finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange,
- efficiency: targetValue / (currentValue + finalFee),
+ changeAmount:
+ finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalChange : 0,
+ fee: returnedFee,
+ efficiency: targetValue / (currentValue + returnedFee),
strategy: this.name,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| inputs: [...selectedInputs], | |
| changeAmount: finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalChange : 0, | |
| fee: finalChange > SmallFirstStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange, | |
| efficiency: targetValue / (currentValue + finalFee), | |
| strategy: this.name, | |
| } | |
| const returnedFee = | |
| finalChange > SmallFirstStrategy.DUST_THRESHOLD | |
| ? finalFee | |
| : finalFee + finalChange | |
| return { | |
| inputs: [...selectedInputs], | |
| changeAmount: | |
| finalChange > SmallFirstStrategy.DUST_THRESHOLD | |
| ? finalChange | |
| : 0, | |
| fee: returnedFee, | |
| efficiency: targetValue / (currentValue + returnedFee), | |
| strategy: this.name, | |
| } |
🤖 Prompt for AI Agents
In packages/xchain-utxo/src/strategies/small-first.ts around lines 37 to 43, the
efficiency calculation uses finalFee but the returned fee may include dust
(finalFee + finalChange) when finalChange <= DUST_THRESHOLD; update the code to
compute the actual returned fee once (e.g., actualFee = finalChange >
SmallFirstStrategy.DUST_THRESHOLD ? finalFee : finalFee + finalChange), use that
actualFee for the fee field and for the efficiency calculation, and keep
changeAmount computation the same (0 when dust) so the returned fee and
efficiency are consistent.
Summary by CodeRabbit
New Features
API
Documentation
Tests
Chores