feat(key-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the asset-lock path - #935
Open
bfoss765 wants to merge 1 commit into
Open
feat(key-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the asset-lock path#935bfoss765 wants to merge 1 commit into
bfoss765 wants to merge 1 commit into
Conversation
… asset-lock path An asset lock could only ever be funded from ONE account. A wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them into BIP44 first and lock out of that — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege. The send path stopped needing that in dashpay/platform#4329; this is the same change for asset locks. Both builders now take a LIST of `AccountTypePreference` sources plus a `source_index` instead of a single `AssetLockFundingAccount`, and fold them through the same `transaction_building::fund` the send path uses: coin selection draws from the union, the first source supplies the change address, overlapping sources fund each account once (dashpay#931's dedup is what makes the repeated `add_funding` safe), and derivation paths are collected across every contributing account so the inputs can be signed. Reservation bookkeeping is the part that had to change shape. A pooled build reserves in EACH contributing account's own set under the one owner token, so the post-build failure paths — credit-key derivation on the soft-wallet builder, the peek/sign/commit loop on the signer builder, both running after the transaction is already signed — now release across every funded account instead of just the one. Releasing a single account's set would have stranded the rest of the inputs until the 24-block TTL sweep. `AssetLockResult` carries the contributing accounts so the caller's rejected-broadcast release can reach them all; it is the contributor list, not everything the sources offered, so a wallet's address book does not inflate the caller's bookkeeping. `fund`'s strictness rule now matches platform's: a SINGLE named source is strict (a caller asking for exactly one account's funds must not silently be given another's), while a pooled list skips the sources this wallet has nothing for — no BIP32 account, no contacts — and errors only when none of them funds anything. Without that, the default pooled set would fail on the very wallets it is meant to serve. CoinJoin funding is unchanged and stays excluded from pooling: it remains drain-only, and it must now be the sole source, because spending mixed outputs alongside transparent ones in one transaction links them and undoes the mixing. The `AssetLockFundingAccount::CoinJoin` + `drain: true` flow that dashpay/platform#4327 ships on converts to a single-element source list and behaves exactly as before. `AssetLockError::AccountNotFound(u32)` is removed — account resolution is now the builder's, and it reports `BuilderError::AccountNotFound` with the source that failed. `AssetLockFundingAccount` remains as the drain flows' single-account vocabulary, with a `From` conversion into the source list.
Contributor
📝 WalkthroughWalkthroughAsset-lock funding now pools BIP44, BIP32, and DashPay receiving accounts. Builders validate sources, track contributors, and release reservations across accounts after failures. The FFI API passes the shared source list to the builder. ChangesPooled asset-lock funding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ManagedWalletInfo
participant TransactionFunding
participant Reservations
Caller->>ManagedWalletInfo: Request asset-lock transaction
ManagedWalletInfo->>TransactionFunding: Fund from pooled account sources
TransactionFunding-->>ManagedWalletInfo: Return builder, paths, and accounts
ManagedWalletInfo->>Reservations: Reserve inputs across accounts
ManagedWalletInfo-->>Caller: Return asset-lock result and funding accounts
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet-ffi/src/transaction.rs (1)
862-869: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftExpose reservation cleanup for the FFI asset-lock path.
This function discards
result.reservation_tokenandresult.funding_accounts. When the host rejects the broadcast, it cannot release reservations for the contributing accounts. The inputs remain reserved until the 24-block TTL sweep. Expose the token and contributing accounts, or add an FFI release function.🤖 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 862 - 869, Update the FFI asset-lock flow around managed_wallet.build_asset_lock so it exposes result.reservation_token and result.funding_accounts to the host, either by returning them in the existing FFI result or by adding a dedicated FFI release function. Ensure hosts can release the contributing-account reservations when broadcast is rejected instead of waiting for TTL cleanup.
🧹 Nitpick comments (3)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (3)
89-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the dropped
account_indexin this conversion.
From<AssetLockFundingAccount>keeps only the family and dropsaccount_index. A caller that converts a drain source must also passAssetLockFundingAccount::account_index()assource_index, or the build silently targets account 0. Add a doc comment on the impl to state this requirement.📝 Proposed documentation
+/// Converts to the pooled-source vocabulary. The account index is NOT carried +/// over: pass [`AssetLockFundingAccount::account_index`] as the builder's +/// `source_index`. impl From<AssetLockFundingAccount> for AccountTypePreference {🤖 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 89 - 100, Add a doc comment to the From<AssetLockFundingAccount> implementation explaining that the conversion preserves only the account family and drops account_index; callers converting a drain source must pass AssetLockFundingAccount::account_index() as source_index to avoid defaulting to account 0.
302-310: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winInvert the lookup to scale with input count, not UTXO count.
The current filter scans every UTXO of every offered account. A pooled list can name many accounts, and an account can hold thousands of UTXOs, while
spentholds only the transaction inputs. Probe the account map with the spent outpoints instead.utxosis aBTreeMap, so each probe is logarithmic.♻️ Proposed refactor
offered .iter() .copied() .filter(|account_type| { accounts.funds_account(account_type).is_some_and(|account| { - account.utxos.keys().any(|outpoint| spent.contains(outpoint)) + spent.iter().any(|outpoint| account.utxos.contains_key(outpoint)) }) }) .collect()🤖 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 302 - 310, Update the filtering logic around the offered-account collection to iterate over the spent outpoints and probe each account’s utxos BTreeMap, rather than scanning all UTXOs for every account. Retain only offered account types whose account contains at least one spent outpoint, using logarithmic map lookups.
477-518: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared pooled-funding and reservation-capture prologue.
build_asset_lock(lines 361-405) andbuild_asset_lock_with_signer(lines 477-518) now carry the same builder construction, the same reservation capture across offered accounts, and the samerelease_reservationsclosure. The reservation-release logic is correctness-critical. If the two copies drift, one path can strand signed inputs until the TTL sweep.Extract a private helper that builds the
TransactionBuilder, callsself.fund, and returns the builder,paths,offered, and the clonedVec<ReservationSet>. Both builders then keep only their signer-specific tail.🤖 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 477 - 518, Extract the duplicated funding setup from build_asset_lock and build_asset_lock_with_signer into a private helper that constructs the TransactionBuilder, applies the drain selection strategy, calls self.fund, and captures cloned reservation handles for all offered accounts. Have both methods use the helper and retain only their signer-specific build_signed_reserved and release_reservations logic, preserving owner-guarded reservation release.
🤖 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-ffi/src/transaction.rs`:
- Around line 747-760: Preserve host control over asset-lock funding by adding
an explicit funding-source option to
wallet_build_and_sign_asset_lock_transaction or providing a separate entry point
that retains BIP44-only behavior, while keeping the existing behavior available
to callers. Update the crate changelog and the public header documentation to
clearly describe the new pooled funding behavior and its linkage implications.
---
Outside diff comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 862-869: Update the FFI asset-lock flow around
managed_wallet.build_asset_lock so it exposes result.reservation_token and
result.funding_accounts to the host, either by returning them in the existing
FFI result or by adding a dedicated FFI release function. Ensure hosts can
release the contributing-account reservations when broadcast is rejected instead
of waiting for TTL cleanup.
---
Nitpick comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 89-100: Add a doc comment to the From<AssetLockFundingAccount>
implementation explaining that the conversion preserves only the account family
and drops account_index; callers converting a drain source must pass
AssetLockFundingAccount::account_index() as source_index to avoid defaulting to
account 0.
- Around line 302-310: Update the filtering logic around the offered-account
collection to iterate over the spent outpoints and probe each account’s utxos
BTreeMap, rather than scanning all UTXOs for every account. Retain only offered
account types whose account contains at least one spent outpoint, using
logarithmic map lookups.
- Around line 477-518: Extract the duplicated funding setup from
build_asset_lock and build_asset_lock_with_signer into a private helper that
constructs the TransactionBuilder, applies the drain selection strategy, calls
self.fund, and captures cloned reservation handles for all offered accounts.
Have both methods use the helper and retain only their signer-specific
build_signed_reserved and release_reservations logic, preserving owner-guarded
reservation release.
🪄 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: 58c78653-b3e7-423f-9e49-ad03284cc372
📒 Files selected for processing (3)
key-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
Comment on lines
+747
to
+760
| /// The funding sources an asset lock pools, in order — the FIRST source | ||
| /// (BIP44) supplies the change address, so change from a pooled asset lock | ||
| /// always returns to the transparent primary account. | ||
| /// | ||
| /// CoinJoin is deliberately absent (spending mixed outputs alongside | ||
| /// transparent ones links them and undoes the mixing — the same reasoning as | ||
| /// `AccountTypePreference::DEFAULT`), and so are a contact's watch-only | ||
| /// `DashpayExternalAccount` coins, which `AllDashpayReceivingFunds` excludes by | ||
| /// construction (it selects only the receiving side the local seed can sign). | ||
| const ASSET_LOCK_FUNDING_SOURCES: [AccountTypePreference; 3] = [ | ||
| AccountTypePreference::BIP44, | ||
| AccountTypePreference::BIP32, | ||
| AccountTypePreference::AllDashpayReceivingFunds, | ||
| ]; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
The C ABI is unchanged, but the funding behavior changes silently for every existing host.
wallet_build_and_sign_asset_lock_transaction keeps its signature, so a host cannot opt out of the new pooled set. Two consequences follow:
- An asset lock can now spend DashPay contact-receiving coins. That links funds received from a contact to the identity funding transaction. A host that previously relied on BIP44-only funding gets this new linkage without any code change.
- A host that already swept funds into BIP44 before locking sees no break, but a host that intentionally kept BIP32 or contact funds separate does.
Consider adding a funding-source parameter, or a second entry point that keeps the BIP44-only behavior, so hosts can choose. At minimum, record the behavior change in the crate's changelog and in the header documentation that host developers read.
🤖 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 747 - 760, Preserve host
control over asset-lock funding by adding an explicit funding-source option to
wallet_build_and_sign_asset_lock_transaction or providing a separate entry point
that retains BIP44-only behavior, while keeping the existing behavior available
to callers. Update the crate changelog and the public header documentation to
clearly describe the new pooled funding behavior and its linkage implications.
5 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #935 +/- ##
==========================================
+ Coverage 75.18% 75.34% +0.15%
==========================================
Files 328 328
Lines 78194 78473 +279
==========================================
+ Hits 58792 59123 +331
+ Misses 19402 19350 -52
|
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
1 participant
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.
Issue being fixed or feature implemented
An asset lock could only ever be funded from ONE account (
AssetLockFundingAccount: a BIP44 index, or a CoinJoin index for the drain flow). A wallet holding its balance across the standard families and its DashPay contact-receiving accounts had to sweep them into BIP44 first and lock out of that — an extra on-chain hop, an extra fee, and a transparent address reused for the privilege.The send path stopped needing that sweep in dashpay/platform#4329. This is the same change for asset locks, so an invitation / identity registration / top-up funds from the union of the wallet's accounts in one transaction.
What was done?
Both
build_asset_lockandbuild_asset_lock_with_signernow take a list ofAccountTypePreferencesources plus asource_index, and fold them through the sametransaction_building::fundthe send path uses:add_fundingis what makes the repeated call safe;Reservation bookkeeping is the part that had to change shape, and it is where I would look hardest in review. A pooled build reserves in each contributing account's own
ReservationSetunder the one owner token. The post-build failure paths — credit-key derivation on the soft-wallet builder, the peek → sign → commit loop on the signer builder, both of which run after the transaction is already signed — therefore release across every funded account instead of just the one. Releasing a single account's set would have stranded the remaining inputs until the 24-block TTL sweep, with the caller holding no token to free them (it never received one on the error path). Release stays owner-guarded throughout (release_if_owner, platform#4185).AssetLockResultgainsfunding_accounts, so a caller's rejected-broadcast release can reach every account holding a share of the reservation. It is the contributor list — accounts that actually supplied an input — not everything the source list offered: selection routinely takes nothing from most offered accounts, and a list naming every DashPay contact would make the caller's release and bookkeeping scale with the address book while claiming contributions that never happened.fund's strictness rule now matches platform'sfinalize_transaction: a single named source is strict (a caller asking for exactly one account's funds must not silently be given another's), a pooled list skips the sources this wallet has nothing for — no BIP32 account, no contacts — and errors only when none of them funds anything. Without that, the default pooled set would fail on the very wallets it is meant to serve. This also fixes the send path, where a multi-source list previously required every named account to exist (platform reimplemented the fold to work around exactly that).CoinJoin is unchanged and stays out of the pool
CoinJoin funding remains drain-only, and it must now be the sole source: spending mixed outputs alongside transparent ones in one transaction links them and undoes the mixing — the same reasoning that keeps CoinJoin out of
AccountTypePreference::DEFAULT. TheAssetLockFundingAccount::CoinJoin+drain: trueflow that dashpay/platform#4327 ships on converts to a single-element source list and behaves exactly as before (test_drain_coinjoin_asset_lockis untouched and passing).Breaking changes
ManagedWalletInfo::build_asset_lock/build_asset_lock_with_signertakefunding_sources: &[AccountTypePreference], source_index: u32in place offunding_account: AssetLockFundingAccount.AssetLockFundingAccountremains as the drain flows' single-account vocabulary, withimpl From<AssetLockFundingAccount> for AccountTypePreferencefor the conversion.AssetLockResultgainsfunding_accounts: Vec<AccountType>.AssetLockError::AccountNotFound(u32)is removed: account resolution is the builder's now, and it reportsBuilderError::AccountNotFoundnaming the source that failed.sourceslist is lenient rather than strict (single-source and empty-list behavior unchanged).key-wallet-ffi'swallet_build_and_sign_asset_lock_transactionkeeps its exact C signature — no funding selector was ever exposed there, so flipping its internal default to the pooled set is the whole change.How Has This Been Tested?
cargo test -p key-wallet --all-features— 635 passed, 0 failed.cargo check --workspace --all-targetsclean;cargo clippy -p key-wallet -p key-wallet-ffi --all-targets --all-features -- -D warningsclean;cargo fmtapplied.New tests, weighted toward the reservation failure paths:
pooled_asset_lock_spans_the_standard_accounts— neither account covers the lock alone, so the build only succeeds by pooling; asserts every input is signed (proving paths were collected across accounts), change returns to BIP44, each account reserves its own contribution, andfunding_accountsnames both.credit_key_failure_releases_the_reservation_in_every_pooled_accountand itssigner_sibling — force a failure in the window where the transaction is built, signed and reserved but the caller holds no token, and assert both pooled accounts came out with nothing reserved.signing_failure_releases_the_reservation_in_every_pooled_account— same invariant one layer down, in the builder's own signer-failure release.pooled_sources_skip_what_the_wallet_does_not_have,pooled_sources_that_resolve_to_nothing_are_an_error— the leniency rule and its floor.coinjoin_cannot_be_pooled_with_transparent_sources(drain and exact-amount) — rejected before any wallet state is touched, with nothing reserved.a_pooled_list_skips_absent_sources_where_a_single_one_is_strict— pins the send path's strictness rule on both sides.Breaking Changes
See "Breaking changes" above.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes