-
Notifications
You must be signed in to change notification settings - Fork 5
feat: update UTXO selection logic to require specific UTXOs and remove skipUtxoSelection option #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e skipUtxoSelection option
WalkthroughThis PR replaces the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
witness.js (1)
3-12: Breaking API change: skipUtxoSelection parameter removed.This removes
skipUtxoSelectionfrom the function signature. Any existing callers passing this parameter will experience a breaking change. Ensure this is documented in release notes or a migration guide.test/fixtures/witness.js (1)
369-511: Test fixture is correct but needs edge case coverage.The math checks out:
- Inputs: 5000 + 3000 = 8000
- Payment: 1000
- Fee: 2090
- Change: 4910 ✓
- totalSpent: 3090 ✓
The test correctly validates that only required UTXOs are selected, ignoring larger non-required ones.
Add test fixtures for these edge cases:
- Insufficient required UTXOs (required UTXOs can't cover payment + fees)
- All UTXOs marked as required
- No UTXOs marked as required (backward compatibility check)
- Required UTXOs exceed payment amount significantly
Example fixture to add:
{ description: 'required utxo - insufficient required UTXOs', feeRate: 10, inputs: [ { txId: 'large_optional', vout: 0, value: 100000, amount: '100000', confirmations: 100, own: true, coinbase: false, address: 'tb1qul5mzh5phe7xqyqek0nl42hflfrn7ugxck59jd', path: "m/84'/1'/0'/0/0" }, { txId: 'small_required', vout: 0, value: 1000, amount: '1000', confirmations: 100, own: true, coinbase: false, address: 'tb1qul5mzh5phe7xqyqek0nl42hflfrn7ugxck59jd', path: "m/84'/1'/0'/0/1", required: true } ], outputs: [ { type: 'payment', address: 'tb1quawu6eyfuechu3qhdeejnrzne9y7shr08u8zzt', value: 50000, amount: '50000' } ], network: testnet, changeAddress: { address: 'tb1qul5mzh5phe7xqyqek0nl42hflfrn7ugxck59jd', path: "m/84'/1'/0'/0/0" }, txType: 'p2wpkh', expected: 'Insufficient required UTXOs', shouldThrow: true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
package.json(1 hunks)test/fixtures/witness.js(2 hunks)test/witness.js(1 hunks)witness.d.ts(1 hunks)witness.js(2 hunks)
🔇 Additional comments (3)
test/witness.js (1)
28-28: LGTM!Correctly removes
skipUtxoSelectionfrom the coinSelect call, aligning with the updated API.witness.d.ts (1)
9-9: I'll help you verify whether thecomposeTxfunction inthird-party/utxo-libactually implements therequiredflag logic. Let me search the codebase for this.
<function_calls>
#!/bin/bashFirst, check if third-party/utxo-lib directory exists
ls -la third-party/ 2>/dev/null | head -20
Find all relevant files in third-party/utxo-lib
echo "--- Finding files in third-party/utxo-lib ---"
find third-party/utxo-lib -type f ( -name ".js" -o -name ".ts" -o -name "*.d.ts" ) 2>/dev/null | head -30
</function_calls><function_calls>
#!/bin/bashSearch for composeTx function definition
echo "--- Searching for composeTx function ---"
rg -n "composeTx" --type=ts --type=js third-party/utxo-lib -A 5 -B 2 2>/dev/null | head -100
</function_calls><function_calls>
#!/bin/bashSearch for 'required' property usage in third-party/utxo-lib
echo "--- Searching for 'required' in third-party/utxo-lib ---"
rg -n "required" third-party/utxo-lib -A 3 -B 3 2>/dev/null | head -150
</function_calls><function_calls>
#!/bin/bashAlso check witness.d.ts to understand the context
echo "--- Checking witness.d.ts context ---"
cat -n witness.d.ts 2>/dev/null | head -50
</function_calls>witness.js (1)
13-23: Verified:composeTxproperly implements handling of therequiredproperty on UTXOs.The implementation respects the
requiredflag through multiple layers:
- In
tryconfirmed.js, UTXOs marked as required are included regardless of confirmation count- In
accumulative.js, required UTXOs are separated and checked first before attempting optional selection- In
branchAndBound.js, the algorithm defers when required UTXOs are present- In
coinselectUtils.js, the coinbase filter excludes coinbase UTXOs unless they're marked as requiredThe property is preserved through the request transformation via
Object.assignand properly utilized throughout the coin selection algorithms. The feature will work as intended.
Summary by CodeRabbit
New Features
Chores
skipUtxoSelectionoption from coin selection logic, replaced by the required UTXO mechanism.✏️ Tip: You can customize this high-level summary in your review settings.