feat(key-wallet): support for more account types + strategy to consume all utxo available#823
feat(key-wallet): support for more account types + strategy to consume all utxo available#823ZocoLini wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR adds account-preference-based managed wallet transaction building, a new all-UTXO selection strategy, builder limits and errors, updated manager/FFI wiring, and tests including a drain transaction from one account into another. ChangesManaged wallet drain flow
Sequence Diagram(s)sequenceDiagram
participant Test as dash-spv test_drain_account_into_another
participant Manager as WalletManager<ManagedWalletInfo>
participant ManagedWalletInfo
participant CoinSelector
participant TransactionBuilder
participant Dashd as dashd regtest
Test->>Manager: build_and_sign_transaction(..., SelectionStrategy::All)
Manager->>ManagedWalletInfo: resolve source account and build transaction
ManagedWalletInfo->>CoinSelector: select_coins_with_size(All, ...)
ManagedWalletInfo->>TransactionBuilder: assemble_unsigned(...)
TransactionBuilder-->>ManagedWalletInfo: unsigned transaction
Test->>Dashd: broadcast, mine, and resync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
ZocoLini
force-pushed
the
feat/tx-builder-update
branch
from
bd92c00 to
748cf33
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #823 +/- ##
==========================================
- Coverage 73.04% 72.84% -0.20%
==========================================
Files 323 323
Lines 72046 72187 +141
==========================================
- Hits 52623 52583 -40
- Misses 19423 19604 +181
|
…e all utxo available
ZocoLini
force-pushed
the
feat/tx-builder-update
branch
from
748cf33 to
9649b93
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (3)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (2)
83-86: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSkip change-address derivation for drain transactions.
set_fundingderives a change address beforeassemble_unsignedclears it forSelectionStrategy::All, so a sweep can still advance/mark change-address state even though no change output is emitted.As per coding guidelines,
key-wallet/**/*.rs: "Use gap limit and staged address generation instead of unbounded address derivation in Rust address pool implementations."Proposed fix
pub fn set_funding(mut self, funds_acc: &mut ManagedCoreFundsAccount, acc: &Account) -> Self { self.inputs = funds_acc.utxos.values().cloned().collect(); - self.change_addr = funds_acc.next_change_address(Some(&acc.account_xpub), true).ok(); + self.change_addr = if self.selection_strategy == SelectionStrategy::All { + None + } else { + funds_acc.next_change_address(Some(&acc.account_xpub), true).ok() + }; self }Also applies to: 256-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around lines 83 - 86, `TransactionBuilder::set_funding` is deriving and consuming a change address even for drain/sweep flows, which can advance `ManagedCoreFundsAccount` state unnecessarily. Update the funding setup so change-address derivation is skipped when the transaction will use `SelectionStrategy::All` (or equivalent drain mode), and keep `assemble_unsigned` as the place that clears/omits change handling for those cases. Use the existing `set_funding` and `assemble_unsigned` flow, plus `ManagedCoreFundsAccount::next_change_address`, to ensure no change address is generated or marked for sweeps.Source: Coding guidelines
294-319: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not validate drains against the caller’s placeholder output amount.
For
SelectionStrategy::All, line 319 overwrites the only output withtotal_input - fee, but line 294 can still reject the transaction if the caller supplied a non-zero placeholder amount larger than the account balance.Proposed fix
- if total_input < total_output + selection.estimated_fee { + let spend_amount_for_checks = + if self.selection_strategy == SelectionStrategy::All { 0 } else { total_output }; + + if total_input < spend_amount_for_checks + selection.estimated_fee { return Err(BuilderError::InsufficientFunds { available: total_input, - required: total_output + selection.estimated_fee, + required: spend_amount_for_checks + selection.estimated_fee, }); } let change_amount = - total_input.saturating_sub(total_output).saturating_sub(selection.estimated_fee); + total_input.saturating_sub(spend_amount_for_checks).saturating_sub(selection.estimated_fee);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around lines 294 - 319, The drain path in `TransactionBuilder` is still being checked against the caller’s placeholder output amount before `SelectionStrategy::All` overwrites it. Adjust the insufficient-funds validation in the builder logic so the `All` strategy skips validating against `total_output` and instead only ensures the balance covers the fee, then let the existing `tx_outputs`/single-output rewrite set the final amount.key-wallet-ffi/src/transaction.rs (1)
88-99: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftExpose the new source account and selection strategy through FFI.
This FFI entrypoint still hardcodes BIP44 +
BranchAndBound, so FFI callers cannot build BIP32/CoinJoin-funded transactions or use the new drain flow.Consider adding FFI-safe enums/parameters, or a dedicated drain entrypoint if this legacy API must keep its current ABI.
Also applies to: 140-144
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@key-wallet-ffi/src/transaction.rs` around lines 88 - 99, The FFI entrypoint wallet_build_and_sign_transaction still hardcodes the source account derivation and coin selection behavior, so callers cannot choose BIP32/CoinJoin funding or the new drain flow. Update the FFI surface around wallet_build_and_sign_transaction (and the related drain path referenced by the same comment) to accept FFI-safe parameters or enums for the source account and selection strategy, or add a separate drain-specific entrypoint if the legacy ABI must remain unchanged. Ensure the Rust-side transaction builder uses those incoming values instead of always forcing BIP44 and BranchAndBound.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/mod.rs`:
- Around line 177-179: The CoinJoin match arm in `managed_wallet_info::mod.rs`
currently panics via `unimplemented!`, but these public library methods should
not crash callers; update the `AccountTypePreference::CoinJoin` handling in both
affected match arms to return `None` just like the other “no address available”
cases. Use the same fix in the corresponding address lookup methods so
`CoinJoin` remains supported without panicking.
---
Outside diff comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 88-99: The FFI entrypoint wallet_build_and_sign_transaction still
hardcodes the source account derivation and coin selection behavior, so callers
cannot choose BIP32/CoinJoin funding or the new drain flow. Update the FFI
surface around wallet_build_and_sign_transaction (and the related drain path
referenced by the same comment) to accept FFI-safe parameters or enums for the
source account and selection strategy, or add a separate drain-specific
entrypoint if the legacy ABI must remain unchanged. Ensure the Rust-side
transaction builder uses those incoming values instead of always forcing BIP44
and BranchAndBound.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 83-86: `TransactionBuilder::set_funding` is deriving and consuming
a change address even for drain/sweep flows, which can advance
`ManagedCoreFundsAccount` state unnecessarily. Update the funding setup so
change-address derivation is skipped when the transaction will use
`SelectionStrategy::All` (or equivalent drain mode), and keep
`assemble_unsigned` as the place that clears/omits change handling for those
cases. Use the existing `set_funding` and `assemble_unsigned` flow, plus
`ManagedCoreFundsAccount::next_change_address`, to ensure no change address is
generated or marked for sweeps.
- Around line 294-319: The drain path in `TransactionBuilder` is still being
checked against the caller’s placeholder output amount before
`SelectionStrategy::All` overwrites it. Adjust the insufficient-funds validation
in the builder logic so the `All` strategy skips validating against
`total_output` and instead only ensures the balance covers the fee, then let the
existing `tx_outputs`/single-output rewrite set the final amount.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff46c80b-d76f-4aa0-955c-10843dc294ec
📒 Files selected for processing (8)
dash-spv/tests/dashd_sync/tests_transaction.rskey-wallet-ffi/src/transaction.rskey-wallet-manager/src/lib.rskey-wallet/src/wallet/managed_wallet_info/coin_selection.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
| AccountTypePreference::CoinJoin => { | ||
| unimplemented!("CoinJoin accounts are spend-only in our current use cases") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return None instead of panicking for CoinJoin address requests.
CoinJoin is a valid AccountTypePreference, so these public library methods can now crash callers instead of reporting “no address available.”
As per coding guidelines, **/*.rs: "Never panic in library code; only use panic! for unrecoverable errors in Rust."
Proposed fix
AccountTypePreference::CoinJoin => {
- unimplemented!("CoinJoin accounts are spend-only in our current use cases")
+ None
}Apply the same change in both match arms.
Also applies to: 223-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@key-wallet/src/wallet/managed_wallet_info/mod.rs` around lines 177 - 179, The
CoinJoin match arm in `managed_wallet_info::mod.rs` currently panics via
`unimplemented!`, but these public library methods should not crash callers;
update the `AccountTypePreference::CoinJoin` handling in both affected match
arms to return `None` just like the other “no address available” cases. Use the
same fix in the corresponding address lookup methods so `CoinJoin` remains
supported without panicking.
Source: Coding guidelines
There was a problem hiding this comment.
@xdustinface What would you do here?? Returning None doesn't explain that this functionality isn't supported and returning an error indicates an error in the library but it is not an error, so I wouldn't like to return one either
There was a problem hiding this comment.
@ZocoLini You’re right — None is a lossy fix here.
My concern is mainly the unimplemented!, not that None is the only acceptable replacement. If I were designing this API, I’d prefer:
Result<Option<Address>, AddressRequestError>with something explicit like:
enum AddressRequestError {
UnsupportedAccountType(AccountTypePreference),
}That way:
Ok(Some(addr))= got an addressOk(None)= no address currently availableErr(UnsupportedAccountType(AccountTypePreference::CoinJoin))= caller asked for an operation this account type does not support
So this would be an unsupported-operation error, not “the library broke”.
If you want to keep this PR scoped and avoid a signature change, then returning None is still safer than panicking, but I agree it’s not as expressive. In that case, I’d at least document that CoinJoin is spend-only and maybe follow up later with a typed/fallible API.
(•ᴗ•)
🧠 Learnings used
Learnt from: ZocoLini
Repo: dashpay/rust-dashcore PR: 782
File: key-wallet/src/wallet/mod.rs:129-131
Timestamp: 2026-05-22T17:37:15.824Z
Learning: In this repository, avoid adding unit tests for trivial methods such as simple getters, setters, or constructors (e.g., methods that only return a field, assign a value, or initialize state with no additional logic). During code review, do not flag or suggest test coverage solely for these trivial implementations; reserve unit tests for behavior with meaningful logic or side effects.
There was a problem hiding this comment.
Hm i think just returning some "not supported" error would be fine? Or just return a struct or tuple which contains some success flag or so? I guess it's kind of okay to panic but also kind of not. I dont have a strong opinion here to be honest but at the end the error feels just a less intense.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
github-actions
Bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Looks good. But i wonder, how will users with > 500 utxos be able to drain them? Maybe we should cap the All strategy at 500 and call it Consolidate or something instead?
Im fine with it either way i guess but its worth a thought. Apparently it's less likely to be relevant for CoinJoin at least.
| AccountTypePreference::CoinJoin => { | ||
| unimplemented!("CoinJoin accounts are spend-only in our current use cases") | ||
| } |
There was a problem hiding this comment.
Hm i think just returning some "not supported" error would be fine? Or just return a struct or tuple which contains some success flag or so? I guess it's kind of okay to panic but also kind of not. I dont have a strong opinion here to be honest but at the end the error feels just a less intense.
|
I like All, it better describes what the CoinSelector selects. About what to do with accounts with +500 UTXOs, we had a discussion about it in the standup, the options I like the most is:
Then, talking about the unimplemented!, I don't want to return an error honestly, what about debug_assert(false, "explanation") and the returning None so only debug builds panic?? |
Sure, go for it. |
3 participants
Generalize tx building by funding account + add All (drain) selection
Known limitation: there is currently no way to build a transaction with more than 500 inputs (MAX_STANDARD_TX_INPUTS) through the set_funding method because set_funding pulls all of the account's UTXOs at once. To sweep an account holding >500 UTXOs you must build the transaction manually, taking the account's UTXOs and feeding them in ≤500-input chunks. This is worth calling out because the builder can be used to sweep a CoinJoin account with All, where exceeding 500 inputs is quite likely
Summary by CodeRabbit
New Features
Bug Fixes
Tests