feat(key-wallet): dashpay contact account funding - #929
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTransaction building now accepts multiple account preferences, including DashPay receiving-funds selectors. The wallet resolves matching accounts, pools UTXOs, preserves derivation paths, and supports unsigned transactions. Account lookup and address handling now cover the supported account types. ChangesMulti-account funding and account resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WalletInfoInterface
participant ManagedWalletInfo
participant ManagedAccountCollection
participant TransactionBuilder
WalletInfoInterface->>ManagedWalletInfo: provide account preference slice
ManagedWalletInfo->>ManagedAccountCollection: resolve funds accounts
ManagedWalletInfo->>TransactionBuilder: pool UTXOs from resolved accounts
TransactionBuilder->>ManagedAccountCollection: collect derivation paths and change data
TransactionBuilder-->>ManagedWalletInfo: return signed or unsigned transaction
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}
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 (1)
key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)
500-507: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCorrect the release guidance: one account no longer holds the whole reservation.
assemble_unsignednow reserves each account's own share of the selected inputs in that account'sReservationSet. The first funding account holds only the outpoints it contributed. A caller that follows this doc and releases on the first funding account alone leaves every other contributing account's inputs reserved until the 24-block TTL sweep reclaims them.State that the caller must call
release_reservation_if_owneron every account that funded the build, asbuild_signed_reservedalready does at Lines 571-574.📝 Proposed doc fix
- /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] on the first - /// funding account, whose set holds the whole build's reservation. See + /// [`ManagedCoreFundsAccount::release_reservation_if_owner`] on *every* + /// account passed to [`Self::add_funding`]: each one reserves only the + /// inputs it contributed, so releasing on one account leaves the rest + /// reserved until the TTL backstop reclaims them. See /// `ReservationSet::release_if_owner` for why owner-guarded release is /// required (`dashpay/platform#4185`).🤖 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 500 - 507, Update the documentation for the reservation token near assemble_unsigned to state that callers must invoke ManagedCoreFundsAccount::release_reservation_if_owner on every funding account that contributed inputs, not only the first account. Align the guidance with the existing build_signed_reserved release behavior.
🧹 Nitpick comments (3)
key-wallet/src/managed_account/managed_account_collection.rs (1)
621-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the catch-all arm with explicit non-funds variants.
_ => Nonealso absorbs any future funds-bearingAccountTypevariant. Such a variant would then silently return no funding account instead of failing to compile. List the keys-only andPlatformPaymentvariants explicitly so the compiler flags new variants here.insert_funds_bearing_accountalready handlesPlatformPaymentexplicitly, so the two methods would also stay aligned.Also applies to: 663-663
🤖 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/managed_account/managed_account_collection.rs` at line 621, Replace the catch-all `_ => None` arms in the affected account-type matching methods with explicit keys-only variants and `PlatformPayment`, matching the variants handled by `insert_funds_bearing_account`. Preserve the existing `None` behavior while making future funds-bearing variants require a compile-time match update.key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
130-147: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExpose the reservation token for unsigned builds.
build_unsigned_transactiondiscards theReservationTokenreturned bybuild_unsigned_reserved. A caller that abandons the unsigned transaction cannot then callrelease_if_owner, so the selected inputs stay reserved until the 24-block TTL sweep reclaims them. The doc comment states this, but the signed path already returns the token throughbuild_signed_reserved.Add a
build_unsigned_transaction_reservedvariant that returns the token, and keep this method as the convenience wrapper.🤖 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 130 - 147, Add a `build_unsigned_transaction_reserved` method alongside `build_unsigned_transaction` that performs the existing builder flow and returns `(Transaction, u64, ReservationToken)` from `build_unsigned_reserved`. Update `build_unsigned_transaction` to remain the convenience wrapper by delegating to the reserved variant and discarding only the token; preserve its current return type and behavior.key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs (1)
105-136: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeduplicate funding candidates and clarify change selection.
Exclude outpoints already present in
self.inputsbefore extending it. Coin selection does not deduplicate candidates, and assembly can emit duplicate transaction inputs. Repeatedsourcescan reach this path.Update the documentation to state that the first funding account that successfully generates a change address supplies change. DashPay accounts return
Errfromnext_change_address, so a later standard account can provide it.🤖 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 105 - 136, Update add_funding to filter candidates whose outpoints already exist in self.inputs before extending the input set or recording funding metadata. Revise the surrounding documentation so change comes from the first funding account that successfully generates an address, allowing later accounts to provide it when next_change_address returns Err.
🤖 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 236-289: The funding flow around fund must cover the DashPay-only
source case where change exceeds dust: add a regression test demonstrating the
current NoChangeAddress result, or document and enforce that a standard account
source is required for change. Use the existing account-preference and
transaction-building test setup, and ensure mixed sources with a standard
account retain valid change behavior.
---
Outside diff comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 500-507: Update the documentation for the reservation token near
assemble_unsigned to state that callers must invoke
ManagedCoreFundsAccount::release_reservation_if_owner on every funding account
that contributed inputs, not only the first account. Align the guidance with the
existing build_signed_reserved release behavior.
---
Nitpick comments:
In `@key-wallet/src/managed_account/managed_account_collection.rs`:
- Line 621: Replace the catch-all `_ => None` arms in the affected account-type
matching methods with explicit keys-only variants and `PlatformPayment`,
matching the variants handled by `insert_funds_bearing_account`. Preserve the
existing `None` behavior while making future funds-bearing variants require a
compile-time match update.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 105-136: Update add_funding to filter candidates whose outpoints
already exist in self.inputs before extending the input set or recording funding
metadata. Revise the surrounding documentation so change comes from the first
funding account that successfully generates an address, allowing later accounts
to provide it when next_change_address returns Err.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_building.rs`:
- Around line 130-147: Add a `build_unsigned_transaction_reserved` method
alongside `build_unsigned_transaction` that performs the existing builder flow
and returns `(Transaction, u64, ReservationToken)` from
`build_unsigned_reserved`. Update `build_unsigned_transaction` to remain the
convenience wrapper by delegating to the reserved variant and discarding only
the token; preserve its current return type and behavior.
🪄 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: 4322716a-03c5-4696-b5e5-35850ee90ba0
📒 Files selected for processing (11)
dash-spv/tests/dashd_sync/tests_transaction.rskey-wallet-ffi/src/transaction.rskey-wallet-manager/src/lib.rskey-wallet/src/managed_account/managed_account_collection.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/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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #929 +/- ##
==========================================
+ Coverage 75.23% 75.30% +0.07%
==========================================
Files 328 328
Lines 77767 77982 +215
==========================================
+ Hits 58507 58726 +219
+ Misses 19260 19256 -4
|
ddbaf6b to
38a9519
Compare
|
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. |
38a9519 to
b481600
Compare
b481600 to
f199177
Compare
f199177 to
3cf2653
Compare
3cf2653 to
229b671
Compare
Funds received from a DashPay contact (DIP-15) were unreachable from
`build_and_sign_transaction`: `AccountTypePreference` only named the
standard families, so a contact account could hold a balance the wallet
could never spend.
`AccountTypePreference::DashpayReceivingFunds` names one contact by its
identity pair. A contact may hold funds at several account indices, so the
source covers all of them and ignores `source_index` — unlike the standard
families, an index alone does not identify a contact account. Only the
receiving side is offered: a `DashpayExternalAccount` tracks what we sent
*to* the contact and its keys are theirs, so it can never fund a spend of
ours. Contact accounts also stay out of the implicit (empty) source list,
which cannot guess which contact to spend from.
The account lookup now goes through `AccountType`, which both sides of the
wallet already key on: `AccountCollection::account_of_type` on the wallet
side, and a new `ManagedAccountCollection::funds_account{,_mut}` on the
managed side, which addresses any funds-bearing account including the
DashPay ones (keyed by identity pair plus index).
Contact accounts keep a single address pool with no internal branch, so
they cannot supply change: a contact-only build fails with
`NoChangeAddress` unless a Standard account comes along to take it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ToH2xGXqVcxiwMkNYaWkh7
229b671 to
9f5b1d2
Compare
2 participants
Adds three
AccountTypePreferencesources for the coins DashPay contacts sent us —DashpayFriendshipReceivingFunds(one contact under one of our identities),DashpayIdentityReceivingFunds(every contact of one identity) andAllDashpayReceivingFunds— each spanning every account index those accounts sit at.Only the receiving side — a
DashpayExternalAccountholds the contact's own coins — andthey stay out of the default source list, which cannot guess whose funds to spend.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes