feat(key-wallet): fund transactions from several account types - #925
Merged
Conversation
github-actions
Bot
added
the
merge-conflict
Contributor
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTransaction building now supports multiple account preferences, pooled UTXO funding, per-account reservations, shared signing, and unsigned transaction construction. Reservation tokens use process-wide generation and explicit ownership. Call sites and tests use the updated APIs. ChangesMulti-account transaction construction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant TransactionBuilding
participant TransactionBuilder
participant ReservationSet
participant TransactionSigner
Wallet->>TransactionBuilding: request transaction with account sources
TransactionBuilding->>TransactionBuilder: add_funding for selected accounts
TransactionBuilder->>ReservationSet: reserve selected outpoints
TransactionBuilding->>TransactionSigner: sign inputs using derivation paths
TransactionSigner-->>Wallet: signed transaction
TransactionBuilding->>ReservationSet: release reservations if signing fails
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/transaction_building.rs`:
- Around line 137-181: Prevent duplicate funding accounts from adding the same
outpoints twice: in
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs lines 137-181,
reject or de-duplicate repeated entries in sources before the funding loop; in
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs lines 147-164,
enforce the one-call-per-funding-account rule by skipping accounts whose
outpoints are already present in self.funding. Use the existing funding and
add_funding logic without relying solely on documentation.
🪄 Autofix
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 Plus
Run ID: f9d6dbe2-206a-4460-9cca-1a5dd555e583
📒 Files selected for processing (6)
dash-spv/tests/dashd_sync/tests_transaction.rskey-wallet-ffi/src/transaction.rskey-wallet-manager/src/lib.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
ZocoLini
force-pushed
the
feat/multi-account-tx-funding
branch
from
August 6, 2026 07:16
0eca14a to
546ac4c
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
removed
the
merge-conflict
ZocoLini
marked this pull request as draft
August 6, 2026 07:17
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (1)
364-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the
reservation_tokenfield doc to matchsingle_token().The field doc at Line 114 states the token is
None"if the funding account carried no reservation set."single_token()now returnsNoneunder two different conditions: the build reserved nothing, or the build spans several funding accounts. An asset lock always uses one funding account, so only the first condition applies here. Correct the wording so callers do not infer the old meaning.📝 Proposed doc fix (applies to the unchanged field doc above)
/// Owner token for the reservation this build took on the funding inputs, - /// or `None` if the funding account carried no reservation set. + /// or `None` if the build reserved no inputs.🤖 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/asset_lock_builder.rs` around lines 364 - 366, Update the reservation_token field documentation near the managed wallet asset-lock builder to describe None as meaning the build reserved nothing, matching reservations.single_token(); do not document the multiple-funding-account case because asset locks always use one funding account.key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (2)
1353-1354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match the renamed API.
set_fundingno longer exists. The test nameset_funding_skips_reserved_utxosnow points at a removed method, which makes the behavior harder to find after the rename.♻️ Proposed rename
#[test] - fn set_funding_skips_reserved_utxos() { + fn add_funding_skips_reserved_utxos() {🤖 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 1353 - 1354, Rename the test function set_funding_skips_reserved_utxos to reference the current funding API name, while preserving its existing reserved-UTXO behavior and test implementation.
1456-1467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind the transaction without the underscore prefix.
The test binds
_txand then uses it at Line 1466 infunds.release_reservation_if_owner(&_tx, token). An underscore prefix signals an unused binding, so the name contradicts the use.♻️ Proposed rename
- let (_tx, _fee, reservations) = TransactionBuilder::new() + let (tx, _fee, reservations) = TransactionBuilder::new()- funds.release_reservation_if_owner(&_tx, token); + funds.release_reservation_if_owner(&tx, token);🤖 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 1456 - 1467, Rename the transaction binding in the test from _tx to tx, and update the funds.release_reservation_if_owner call to pass tx while leaving the reservation assertions unchanged.
🤖 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/asset_lock_builder.rs`:
- Around line 321-325: Update the reserved-builder documentation associated with
build_unsigned_reserved and related funding APIs: replace all stale set_funding
references with the current add_funding terminology, and document its third
return value as BuildReservations rather than Option<ReservationToken>. Keep the
implementation around build_signed_reserved unchanged.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 595-613: Update the opening documentation for
build_unsigned_reserved to describe its BuildReservations return value rather
than the obsolete ReservationToken/None behavior; state that empty reservations
represent no attached reservation set and that multiple funding-account entries
may be returned, while preserving the newer ownership and release guidance.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 364-366: Update the reservation_token field documentation near the
managed wallet asset-lock builder to describe None as meaning the build reserved
nothing, matching reservations.single_token(); do not document the
multiple-funding-account case because asset locks always use one funding
account.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 1353-1354: Rename the test function
set_funding_skips_reserved_utxos to reference the current funding API name,
while preserving its existing reserved-UTXO behavior and test implementation.
- Around line 1456-1467: Rename the transaction binding in the test from _tx to
tx, and update the funds.release_reservation_if_owner call to pass tx while
leaving the reservation assertions unchanged.
🪄 Autofix
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 Plus
Run ID: 94cf12e5-7245-4308-a7bd-8864be84bc00
📒 Files selected for processing (7)
dash-spv/tests/dashd_sync/tests_transaction.rskey-wallet-ffi/src/transaction.rskey-wallet-manager/src/lib.rskey-wallet/src/lib.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- key-wallet-manager/src/lib.rs
- key-wallet-ffi/src/transaction.rs
- dash-spv/tests/dashd_sync/tests_transaction.rs
- key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #925 +/- ##
==========================================
+ Coverage 74.75% 75.19% +0.44%
==========================================
Files 328 328
Lines 76700 77767 +1067
==========================================
+ Hits 57337 58477 +1140
+ Misses 19363 19290 -73
|
ZocoLini
force-pushed
the
feat/multi-account-tx-funding
branch
3 times, most recently
from
August 6, 2026 09:59
36b86c0 to
032893d
Compare
ZocoLini
force-pushed
the
feat/multi-account-tx-funding
branch
from
August 6, 2026 12:42
032893d to
97c80a5
Compare
ZocoLini
marked this pull request as ready for review
August 7, 2026 08:03
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
157-201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNothing de-duplicates funding accounts, so one account's UTXOs can enter the candidate input set twice.
add_fundingappends unreserved UTXOs toself.inputson every call, and reservation happens later inassemble_unsigned. A repeated funding account therefore appends the same outpoints again, and coin selection can select one outpoint twice, producing a transaction with duplicate inputs.
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs#L157-L201: de-duplicate or reject repeated entries inpreferencesbefore the funding loop, so a caller-supplied duplicate cannot reachadd_fundingtwice.key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs#L119-L136: enforce the documented "call it once per funding account" rule in code, for example by skipping an account whose outpoints already appear inself.funding.🤖 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_building.rs` around lines 157 - 201, Prevent duplicate funding accounts before they reach add_funding: in key-wallet/src/wallet/managed_wallet_info/transaction_building.rs:157-201, de-duplicate or reject repeated entries in preferences; in key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs:119-136, enforce the once-per-funding-account rule by skipping accounts whose outpoints already exist in self.funding.
🧹 Nitpick comments (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)
450-466: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider returning the per-account reservation handles.
The build stamps one token across several sets, but the caller receives only the token. To release an abandoned multi-account build, the caller must re-resolve every funding account itself. Returning the contributing
ReservationSethandles alongside the token would make cleanup self-contained. This is optional; the current API works if callers know their ownsourceslist.🤖 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 450 - 466, Consider updating the reservation setup around ReservationToken::next and the reservation_token return value to retain the contributing ReservationSet handles alongside the shared owner token. Return enough information for callers to release abandoned multi-account reservations without re-resolving the funding accounts, while preserving the existing behavior for empty funding and callers that already provide their own sources.
🤖 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/transaction_builder.rs`:
- Around line 504-505: Update the documentation near assemble_unsigned and
ManagedCoreFundsAccount::release_reservation_if_owner to instruct callers to
release the reservation token on every funding account, not only the first.
State that build_unsigned_reserved does not expose which accounts contributed,
so callers must track the contributing accounts independently.
---
Duplicate comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_building.rs`:
- Around line 157-201: Prevent duplicate funding accounts before they reach
add_funding: in
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs:157-201,
de-duplicate or reject repeated entries in preferences; in
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs:119-136,
enforce the once-per-funding-account rule by skipping accounts whose outpoints
already exist in self.funding.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 450-466: Consider updating the reservation setup around
ReservationToken::next and the reservation_token return value to retain the
contributing ReservationSet handles alongside the shared owner token. Return
enough information for callers to release abandoned multi-account reservations
without re-resolving the funding accounts, while preserving the existing
behavior for empty funding and callers that already provide their own sources.
🪄 Autofix
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 Plus
Run ID: a7ea0178-f23f-47e6-83df-e87e15b43026
📒 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/managed_account/reservation.rskey-wallet/src/tests/spent_outpoints_tests.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- dash-spv/tests/dashd_sync/tests_transaction.rs
- key-wallet-manager/src/lib.rs
- key-wallet-ffi/src/transaction.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
build_and_sign_transactiontakessources: &[AccountTypePreference]and pools those accounts' UTXOs. Empty list = every account type present at the index.set_funding→add_funding, additive: called once per funding account, first one gives the change address. Input derivation paths are collected across accounts for signing.reservecall: each account reserves only the inputs it contributed, in its own set, all stamped with the same token.reservenow takes the owner, and the counter behind it is process-wide (per-set counters would mint colliding tokens across accounts, reopening the feat(key-wallet): owner-tagged reservations to close the broadcast-release TOCTOU (platform#4185) #916 race).Summary by CodeRabbit
New Features
Bug Fixes