feat(key-wallet): fund an asset lock from a caller-chosen list of accounts - #944
Conversation
…ounts 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 (#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. Which accounts to pool is the CALLER's decision, not this library's. The FFI entry point takes the source list as a parameter (`FFIAccountTypePreference`, a tag plus the two identity IDs the DashPay kinds read) rather than applying a default of its own: a client wanting today's behavior passes a single `BIP44` source, one wanting the whole spendable balance passes the wider list, and one funding out of a specific contact names that friendship. An empty list is rejected instead of falling through to `AccountTypePreference::DEFAULT`, since defaulting there would be this layer choosing a funding policy that only the client library knows. 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, a 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. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe asset-lock transaction API now accepts ordered funding-source preferences. Funding can pool UTXOs across available sources, route change to the first source, track contributors, and clean up reservations for soft-wallet and external-signer flows. ChangesAsset-lock pooled funding
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FFI
participant ManagedWalletInfo
participant TransactionFunding
participant AssetLockBuilder
FFI->>ManagedWalletInfo: Pass ordered funding sources
ManagedWalletInfo->>TransactionFunding: Resolve pooled funding
TransactionFunding-->>ManagedWalletInfo: Return builder, paths, and accounts
ManagedWalletInfo->>AssetLockBuilder: Build and sign asset-lock transaction
AssetLockBuilder-->>ManagedWalletInfo: Return contributors and reservation outcome
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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: 2
🧹 Nitpick comments (5)
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
281-286: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrictness is derived from list length, so a caller cannot request strict pooling.
strict = preferences.len() == 1couples the leniency policy to the number of sources. A caller that pools two sources and requires both to exist has no way to express that: an absent account is silently skipped. A caller that passes[BIP44, BIP44]also loses strictness for the one account it named.The current rule is documented at Lines 267-273 and matches the PR intent, so this is not a defect. If a strict pooled request becomes necessary later, consider a separate flag or a small enum instead of inferring policy from
len().🤖 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 281 - 286, Preserve the existing strictness behavior in the transaction-building logic: keep strict derived from preferences.len() == 1 and do not add a separate strictness flag or enum. No code change is required for this comment.key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (3)
474-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared funding-and-reservation prologue.
Lines 361-405 and Lines 477-518 are now near-identical: builder construction with
require_final_inputs, the drain strategy switch, thePooledFundingdestructure, theReservationSetcapture overoffered, and thereserved/release_reservationspair. The two copies differ only in comment wording.The blocks that follow do differ — one passes
wallettobuild_signed_reservedand the other passessigner, and the tails build differentAssetLockCreditKeysvariants. A helper that returns the funded builder, thepaths, theofferedaccounts, and the capturedVec<ReservationSet>would remove the duplication without touching either signing call.This matters for the reservation logic specifically: a future fix applied to one copy and not the other reintroduces the stranding bug this PR closes.
🤖 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 474 - 518, Extract the duplicated funding and reservation setup from the two asset-lock build paths into a shared helper. Have it construct the builder with require_final_inputs and the drain strategy, call fund, capture paths, offered accounts, and each account’s ReservationSet, and return those values for the existing signing flows; keep the wallet versus signer arguments and distinct AssetLockCreditKeys tail logic unchanged.
300-310: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueIterate the inputs, not each account's whole UTXO map.
The current filter scans every UTXO of every offered account. A transaction has few inputs, and an offered account can hold many UTXOs. Testing the inputs against each account's map inverts the loop and keeps the cost proportional to the input count.
ManagedCoreFundsAccount::utxosis aBTreeMap, socontains_keyis a log-time lookup on a small map. Thespentset is then unnecessary.♻️ Proposed refactor
fn contributing_accounts( accounts: &crate::account::ManagedAccountCollection, offered: &[AccountType], transaction: &Transaction, ) -> Vec<AccountType> { - let spent: HashSet<OutPoint> = - transaction.input.iter().map(|input| input.previous_output).collect(); offered .iter() .copied() .filter(|account_type| { accounts.funds_account(account_type).is_some_and(|account| { - account.utxos.keys().any(|outpoint| spent.contains(outpoint)) + transaction + .input + .iter() + .any(|input| account.utxos.contains_key(&input.previous_output)) }) }) .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 300 - 310, Update the account filtering logic around the spent-output collection to iterate over transaction.input and check each input’s previous_output directly against the offered account’s utxos using contains_key. Remove the spent HashSet and preserve the existing account-type filtering and collection behavior.
89-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNote that this conversion drops
account_index.Both arms discard
account_index.AccountTypePreferencecarries no index, so the builders take it as the separatesource_indexargument. A caller that follows the instruction at Line 59 ("Convert withAccountTypePreference::fromto hand one to a builder") and passes only the converted preference fundssource_index0, not the account it named.Add a short doc comment on this
Fromimpl stating that the caller must also passAssetLockFundingAccount::account_indexassource_index. That keeps the two halves of the conversion together.📝 Proposed doc addition
+/// The source family this account names. +/// +/// The index does not survive the conversion: [`AccountTypePreference`] names a +/// family only. Pass [`AssetLockFundingAccount::account_index`] as the +/// builder's `source_index` alongside the converted preference. impl From<AssetLockFundingAccount> for AccountTypePreference { fn from(account: AssetLockFundingAccount) -> Self {🤖 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 concise doc comment to the From<AssetLockFundingAccount> for AccountTypePreference implementation stating that conversion preserves only the account type and callers must separately pass AssetLockFundingAccount::account_index as the builder’s source_index.key-wallet-ffi/src/transaction.rs (1)
789-811: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument the valid
kinddiscriminants.Add a
# Safetyrequirement that everyfunding_sources[i].kindis one ofFFIAccountTypePreferenceKindvalues0..=5. If invalid values must returnFFIErrorCode::InvalidInput, validate an integer-valued FFI representation before creating a Rust reference and use a fallible conversion.🤖 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 789 - 811, Document and enforce valid kind discriminants in the FFI conversion surrounding From<FFIAccountTypePreference> for AccountTypePreference: require each funding_sources[i].kind to be within 0..=5, validate the raw integer before constructing any Rust reference, and use a fallible conversion that returns FFIErrorCode::InvalidInput for invalid values instead of relying on an infallible match.Source: Coding guidelines
🤖 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/FFI_API.md`:
- Around line 1308-1315: Add dedicated type-reference documentation in
FFI_API.md for FFIAccountTypePreference and FFIAccountTypePreferenceKind,
alongside the supporting function documentation or in a shared Type Reference
section. Document the struct’s C layout and field order, enum discriminant
values including BIP44 = 0, BIP32 = 1, and CoinJoin = 2 plus all remaining
variants, and specify which identity fields each kind uses.
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 813-836: Add a note to the asset-lock function’s # Parameters
documentation stating that this entry point is non-drain and rejects
FFIAccountTypePreferenceKind::CoinJoin, including when it is the sole funding
source; callers must use the drain-mode entry point for CoinJoin funding. Do not
change the implementation.
---
Nitpick comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 789-811: Document and enforce valid kind discriminants in the FFI
conversion surrounding From<FFIAccountTypePreference> for AccountTypePreference:
require each funding_sources[i].kind to be within 0..=5, validate the raw
integer before constructing any Rust reference, and use a fallible conversion
that returns FFIErrorCode::InvalidInput for invalid values instead of relying on
an infallible match.
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 474-518: Extract the duplicated funding and reservation setup from
the two asset-lock build paths into a shared helper. Have it construct the
builder with require_final_inputs and the drain strategy, call fund, capture
paths, offered accounts, and each account’s ReservationSet, and return those
values for the existing signing flows; keep the wallet versus signer arguments
and distinct AssetLockCreditKeys tail logic unchanged.
- Around line 300-310: Update the account filtering logic around the
spent-output collection to iterate over transaction.input and check each input’s
previous_output directly against the offered account’s utxos using contains_key.
Remove the spent HashSet and preserve the existing account-type filtering and
collection behavior.
- Around line 89-100: Add a concise doc comment to the
From<AssetLockFundingAccount> for AccountTypePreference implementation stating
that conversion preserves only the account type and callers must separately pass
AssetLockFundingAccount::account_index as the builder’s source_index.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_building.rs`:
- Around line 281-286: Preserve the existing strictness behavior in the
transaction-building logic: keep strict derived from preferences.len() == 1 and
do not add a separate strictness flag or enum. No code change is required for
this comment.
🪄 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: 28105d79-a318-436a-ae7b-9f1719add8ec
📒 Files selected for processing (4)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/transaction.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #944 +/- ##
==========================================
+ Coverage 75.17% 75.38% +0.20%
==========================================
Files 328 328
Lines 78194 78541 +347
==========================================
+ Hits 58783 59208 +425
+ Misses 19411 19333 -78
|
`validate_funding_sources` is a private free function, so linking to it from a public doc comment fails `rustdoc::private_intra_doc_links` under the Documentation job's `-D warnings`. The link was also useless to a reader of the public docs, who cannot follow it — state the CoinJoin rule inline instead. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…t lock
`wallet_build_and_sign_asset_lock_transaction` always builds with
`drain: false`, and `validate_funding_sources` rejects a CoinJoin source
outside drain mode — so a caller selecting that kind here always gets
`InvalidData`, even passing it alone. The kind's own doc described the
builder's rule ("sole source, drain only") without saying this entry point
never drains, which reads as though the sole-source form would work.
Raised by CodeRabbit on #944.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Both CodeRabbit points addressed or answered:
Type reference for |
…ologue `build_asset_lock` and `build_asset_lock_with_signer` had drifted into two near-identical copies of the same prologue: payload assembly, the drain strategy switch, `fund`, the per-account `ReservationSet` capture, and the release closure. They differed only in which signer reached `build_signed_reserved` and in comment wording. That duplication is a hazard on this specific logic. The reservation capture has to happen between funding and signing — a pooled build reserves in each contributing account's own set, and both builders reach their release paths after the transaction is already signed, with the caller holding no token. A fix applied to one copy and not the other silently reintroduces stranded inputs on the path that was missed. `build_signed_asset_lock` is now the single prologue, generic over `TransactionSigner` so the soft-wallet builder passes `wallet` and the signer builder passes `signer`. `BuildReservations` owns the sets, the reserved outpoints and the token, so releasing is one call that cannot reach only some of the funded accounts. Also from review: - `contributing_accounts` iterates the transaction's inputs rather than every UTXO of every offered account. A transaction has few inputs; an offered account can hold many UTXOs. - `From<AssetLockFundingAccount> for AccountTypePreference` documents that the index does not survive. The type's own doc said "convert to hand one to a builder", and a caller doing only that funds source_index 0 rather than the account it named. - The FFI entry point documents that `funding_sources[i].kind` must be a declared discriminant, since reading any other value as the enum is UB and so cannot be rejected at the boundary. Raised by CodeRabbit on #944. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…urces The new boundary had no test of its own: the entry point's marshalling body is where the funding policy now lives, and nothing exercised it. Three cases, driven against a created-but-unfunded wallet so an accepted call fails in coin selection rather than at the guards — which is what tells the two apart: - an empty list is rejected with `InvalidInput` instead of being forwarded, where it would mean `AccountTypePreference::DEFAULT` and reinstate a policy this layer must not choose; - a well-formed pooled list gets past the guards into the build; - `CoinJoin` is rejected, pinning the behavior documented in f0d2cbf — this entry point always builds non-drain, so mixed funds can never back it. Prompted by the patch-coverage report on #944. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Worked through the review nitpicks. Four applied in 5a19149, one declined. Applied
Declined
Also added |
…ty note The `# Safety` requirement added in 5a19149 changed the doc comment that `scripts/generate_ffi_docs.py` extracts, and the generated file was not refreshed alongside it — which the verify-ffi pre-commit hook catches. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/test_asset_lock_funding_sources.rs`:
- Around line 1-8: Rename the pull request title to use the required semantic
prefix: “feat: configurable pooled asset-lock funding”.
- Line 53: Update call_with_sources to accept an FFINetwork parameter and pass
it to wallet_manager_create instead of hardcoding Testnet. Extend every FFI
contract test using this helper to execute each case for both
FFINetwork::Mainnet and FFINetwork::Testnet.
🪄 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: e8107887-4713-4de6-b83d-31f50425c9d0
📒 Files selected for processing (4)
key-wallet-ffi/FFI_API.mdkey-wallet-ffi/src/transaction.rskey-wallet-ffi/tests/test_asset_lock_funding_sources.rskey-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- key-wallet-ffi/FFI_API.md
- key-wallet-ffi/src/transaction.rs
- key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs
`wallet_manager_get_wallet` returns an independently boxed clone that the caller owns — its own docs say to free it with `wallet_free_const` — and the test dropped it, leaking a whole `Wallet` per case. The Address Sanitizer job caught it: 42144 bytes in 24 allocations, three tests' worth. Also frees `tx_bytes` defensively. Every case here fails before a transaction is produced, so it is always null today, but a future success case would leak it the same way. Verified by reproducing the exact CI figure locally under `-Zsanitizer=address` with `detect_leaks=1`, then confirming it goes away. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The helper hardcoded Testnet, so every FFI contract case here skipped Mainnet — against the repo's standing rule to test both configurations. Account derivation is coin-type-scoped, so a guard exercised on one network only could hide a network-conditional path. `call_with_sources` now takes the network and each case loops over both, naming the network in its assertion messages so a one-sided failure says which. Raised by CodeRabbit on #944. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed |
2 participants
Supersedes #935 — same change, recreated on a
dashpay/rust-dashcorebranch rather than a personal fork, with the FFI's funding policy handed back to the caller (see "The FFI does not choose the accounts" below). Original commit and authorship preserved (@bfoss765).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, a 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).The FFI does not choose the accounts
This is what changed from #935, which had
key-wallet-ffikeep its C signature and quietly hardcode a[BIP44, BIP32, AllDashpayReceivingFunds]default internally. Which accounts a lock may spend from is a client library policy — it depends on what the client is funding, what it shows the user, and whether contact funds are in scope — and it is not something the FFI layer can know.So
wallet_build_and_sign_asset_lock_transactiontakes the source list as a parameter:FFIAccountTypePreferenceis a#[repr(C)]tag (FFIAccountTypePreferenceKind) plus the two 32-byte identity IDs the DashPay kinds read, so the fullAccountTypePreferencevocabulary crosses the boundary — including naming one specific contact's receiving account, which a hardcoded set could never express.BIP44source and gets exactly the pre-pooling result.funding_sources_count == 0is rejected withInvalidInputrather than falling through toAccountTypePreference::DEFAULT— defaulting there would be this layer picking the policy again, just less visibly.The first source supplies the change address, so the caller also decides where change lands.
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).wallet_build_and_sign_asset_lock_transactiongainsfunding_sources/funding_sources_countahead ofaccount_index. Callers must pass at least one source;[BIP44]reproduces the previous behavior exactly.How Has This Been Tested?
cargo test -p key-wallet -p key-wallet-ffi— 637 + 235 passed, 0 failed.cargo clippy -p key-wallet -p key-wallet-ffi --all-targetsclean;cargo fmt --checkclean.FFI_API.mdregenerated viascripts/generate_ffi_docs.py.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 the first source, 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.every_funding_source_kind_maps_to_its_own_preference,dashpay_sources_carry_the_identity_ids_they_name— the new FFI conversion. A transposed arm there would fund from an account the caller never named and the build would still succeed, so nothing downstream would catch it.Breaking Changes
See "Breaking changes" above.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation