Skip to content

feat(platform-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the send path - #4329

Merged
QuantumExplorer merged 5 commits into
v4.2-devfrom
feat/send-multi-account-funding
Aug 7, 2026
Merged

feat(platform-wallet): pool BIP44 + BIP32 + DashPay receiving funds on the send path#4329
QuantumExplorer merged 5 commits into
v4.2-devfrom
feat/send-multi-account-funding

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Aug 7, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

When sending funds, the wallet previously spent from exactly one account family the caller named — so coins sitting in DashPay contact-receiving accounts (or the "other" standard family) were unspendable without account gymnastics. rust-dashcore#925 made the transaction builder multi-account (additive add_funding, one process-wide reservation token spanning every contributing account) and rust-dashcore#929 added the DashPay receiving-side sources. This PR adopts both: a plain send pools BIP44 + BIP32 + every DashPay contact-receiving account.

What was done?

  • Pin bump dca5b05b944e53a5 — exactly three upstream commits: feat(dashmate): replace js-drive-abci with rs-drive-abci  #926 (watch-only coins excluded from balances), fix(dashmate): deprecation warning on start #925 (multi-account funding), doc: prerequisites for wasm build #929 (DashPay sources). Lock regenerated minimally.
  • SEND_FUNDING_SOURCES = [BIP44, BIP32, AllDashpayReceivingFunds] — funding order is meaningful: the first source supplies the change address, so change from a pooled send always returns to the transparent primary. CoinJoin is deliberately excluded (spending mixed coins alongside transparent ones undoes the mixing — same reasoning as upstream's DEFAULT), and a contact's watch-only DashpayExternalAccount coins are excluded upstream by the receiving-side selector.
  • finalize_transaction takes a source list. A single-element list keeps the old strict one-account contract (asking for a missing account errors); a pooled list skips sources that resolve to nothing (a wallet with no BIP32 account or no contacts still sends) and errors only if nothing funds. Source resolution mirrors key-wallet's own fund() semantics, including the overlap dedup that prevents double-offering one account's UTXOs.
  • Multi-account reservation lifecycle. SignedCoreTransaction and the deferred-payment registry now record every contributing AccountType; abandon / rejected-broadcast / stale-token reconciliation run the one build token against each contributing account's ReservationSet (upstream reserves per account, all stamped with the same token — the fix(dashmate): deprecation warning on start #925 design).
  • FFI/host surface: CoreAccountTypeFFI::AllSpendable (single-account APIs like gap limits reject it with a typed parameter error), JNI value 3, Kotlin ALL_SPENDABLE + Swift .allSpendable on the builder enums, and the send entry points (sendToAddresses, buildSignedPayment, finalizeAtomic) default to pooled. Explicit single-family selection still works everywhere.

How Has This Been Tested?

  • New: pooled_send_spans_families_and_abandon_releases_all — a payment above either family's balance funds from both, both contributors are recorded, the empty DashPay selector contributes nothing rather than erroring, and abandon releases every account (an identical rebuild succeeds). single_source_missing_account_still_errors pins the strict contract.
  • Suites: platform-wallet 576 + platform-wallet-ffi 236 lib tests green; clippy -D warnings clean across platform-wallet/ffi/jni; cargo fmt clean; full workspace cargo check --all-targets clean; Kotlin :sdk:compileDebugKotlin + :sdk:testDebugUnitTest BUILD SUCCESSFUL (exit codes checked directly).

Coordination notes

Breaking Changes

None consensus-side. Host-visible behavior change (deliberate): the default send now pools sources; hosts wanting the old behavior pass an explicit single-family selector.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added an “All Spendable” funding option for wallet transactions.
    • Payments can now combine funds from multiple eligible account types, including DashPay receiving funds.
    • Kotlin and Swift SDKs support pooled funding and updated default payment behavior.
  • Bug Fixes
    • Improved insufficient-funds reporting for pooled transactions.
    • Enhanced reservation cleanup when payments use multiple funding sources.
    • Invalid pooled account selections are now rejected for single-account operations.

…n the send path

Adopt rust-dashcore #925 (multi-account funding) and #929 (DashPay
contact-account sources): a plain send now draws from every spendable
transparent source — SEND_FUNDING_SOURCES = [BIP44, BIP32,
AllDashpayReceivingFunds] — so coins a contact paid us are spendable
without picking an account. Change returns to BIP44 (the first pooled
source); CoinJoin stays out (separate privacy domain), as do a
contact's watch-only external coins (excluded upstream by the
receiving-side selector).

Pin bump dca5b05b -> 944e53a5 (exactly #926 + #925 + #929).
finalize_transaction takes a source list; a single-element list keeps
the old strict one-account contract, a pooled list skips missing
sources and errors only when nothing funds. SignedCoreTransaction and
the deferred-payment registry record EVERY contributing AccountType,
and release/abandon/rejected-broadcast reconcile the one build token
against each account's ReservationSet. FFI gains the AllSpendable
selector (single-account APIs reject it with a typed parameter error);
JNI maps int 3; Kotlin and Swift builder enums gain the value and the
send entry points (sendToAddresses, buildSignedPayment, finalizeAtomic)
default to it.

Tests: pooled finalize spans both standard families when neither covers
the payment alone, tolerates a wallet with no DashPay accounts, records
both contributors, and abandon releases every account (identical
rebuild succeeds); single-source strictness pinned.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v4.2.0 milestone Aug 7, 2026
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@QuantumExplorer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 9104c31c-b454-4e75-a739-0eaaf504b01d

📥 Commits

Reviewing files that changed from the base of the PR and between f15fe29 and 0e1763c.

📒 Files selected for processing (1)
  • packages/rs-platform-wallet-ffi/src/error.rs
📝 Walkthrough

Walkthrough

The change adds ALL_SPENDABLE funding across Rust, FFI, Kotlin, and Swift APIs. Core transaction finalization pools UTXOs from standard and DashPay receiving accounts, tracks all contributors, and releases reservations across them.

Changes

Pooled funding support

Layer / File(s) Summary
Funding selector contracts
Cargo.toml, packages/kotlin-sdk/..., packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs, packages/rs-unified-sdk-jni/..., packages/swift-sdk/...
Adds ALL_SPENDABLE selectors and maps them to ordered BIP44, BIP32, and DashPay receiving sources.
Pooled transaction finalization
packages/rs-platform-wallet/src/error.rs, packages/rs-platform-wallet/src/wallet/core/transaction.rs
Pools available UTXOs, records contributing accounts, validates selected inputs, and reports pooled insufficient funds.
Reservation and payment integration
packages/rs-platform-wallet/src/wallet/core/broadcast.rs, packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs, packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
Passes all contributing accounts through finalization, broadcast, deferred payments, and reservation cleanup.
Validation and tests
packages/rs-platform-wallet/src/test_support.rs, packages/rs-platform-wallet/src/wallet/core/wallet.rs, packages/rs-platform-wallet-ffi/src/core_wallet/*
Rejects pooled selectors for single-account operations and adds standard-account and DashPay contact-account coverage.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SDK
  participant FFI
  participant CoreWallet
  participant WalletManager
  participant PaymentRegistry
  SDK->>FFI: select ALL_SPENDABLE
  FFI->>CoreWallet: finalize with funding sources
  CoreWallet->>WalletManager: resolve and reserve pooled UTXOs
  WalletManager-->>CoreWallet: selected inputs and signing paths
  CoreWallet->>PaymentRegistry: register all funding accounts
  PaymentRegistry->>CoreWallet: release reservations during cleanup
Loading

Possibly related issues

  • dashpay/platform#4255 — Adds pooled ALL_SPENDABLE funding that includes DashPay contact-received funds.

Possibly related PRs

Suggested reviewers: lklimek, llbartekll, shumkov

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: pooled BIP44, BIP32, and DashPay receiving funds for platform-wallet sends.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/send-multi-account-funding

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

⛔ Blockers found — Opus deferred (commit 0e1763c)
Canonical validated blockers: 2

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.61%. Comparing base (963f0d2) to head (0e1763c).
⚠️ Report is 1 commits behind head on v4.2-dev.

Additional details and impacted files
@@             Coverage Diff              @@
##           v4.2-dev    #4329      +/-   ##
============================================
- Coverage     87.78%   87.61%   -0.18%     
============================================
  Files          2677     2704      +27     
  Lines        342371   345211    +2840     
============================================
+ Hits         300551   302445    +1894     
- Misses        41820    42766     +946     
Components Coverage Δ
dpp 88.83% <ø> (ø)
drive 86.25% <ø> (ø)
drive-abci 89.66% <ø> (-0.01%) ⬇️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.88% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 48.02% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…act funds

The pooled-send test covered the two standard families but only asserted that
an EMPTY DashPay selector contributes nothing, so every contact-account lookup
in the pooled path could have resolved None and the feature would have silently
degraded to a BIP44+BIP32 send with no test failing.

Add a fixture that builds a real DashpayReceivingFunds account the way
DashPayView::register_contact_account does (DIP-15 xpub, Account in the key
collection, funds-bearing managed account in the managed collection, minus the
persistence round the fixture cannot reach) and funds it with a chain-locked
UTXO. The new test spends more than BIP44 alone holds, then asserts the contact
account is recorded as a contributor, that every pooled input is signed
(exercising the DIP-15 Normal256 derivation path through the signer), and that
abandon releases the contact account's reservation too.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Codex only

Two blocking regressions remain: finalizing a builder with explicitly added wallet inputs can now create duplicate prevouts, and Swift's deferred-payment convenience API still defaults to BIP44 instead of pooled funding. Additional issues affect contributor bookkeeping, pooled insufficient-funds diagnostics, and JNI selector documentation.
Source: codex-general=gpt-5.6-sol; codex-rust-quality=gpt-5.6-sol; codex-ffi-engineer=gpt-5.6-sol; final verifier=gpt-5.6-sol. Orchestration-only and not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — rust-quality (completed), gpt-5.6-sol — ffi-engineer (completed)
  • Verifier: gpt-5.6-sol — verifier
  • Sonnet: not run (deferred by blocker gate)

🔴 2 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)

2 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet/src/wallet/core/transaction.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/transaction.rs:338: Deduplicate explicitly added inputs before additive funding
  The public `core_wallet_tx_builder_add_inputs_from_outpoints`/Swift `addInputs` path seeds the builder through `TransactionBuilder::add_inputs`. The new pinned `add_funding` implementation then appends every unreserved UTXO from each funding account without excluding outpoints already present in the builder. The pinned coin selector performs no outpoint deduplication; in particular, `SelectionStrategy::All` clones every candidate, so an explicitly added wallet UTXO is guaranteed to appear twice in the resulting transaction, while ordinary strategies can also double-count it toward the target. Core rejects transactions containing duplicate prevouts. The previous `set_funding` behavior replaced the builder's inputs, so this invalid-transaction behavior is introduced by the additive funding change. Exclude pre-seeded outpoints from funding candidates, preferably in key-wallet's `add_funding`, before coin selection.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/transaction.rs:313-339: Separate offered accounts from selected contributors
  `funding_accounts` is simultaneously used for source deduplication and exposed as the accounts that contributed transaction inputs, but an account is pushed immediately after its UTXOs are offered, before coin selection. A transaction selected entirely from BIP44 therefore reports BIP32 and every existing DashPay receiving account as contributors even when none of their UTXOs were selected. This violates `SignedCoreTransaction::funding_accounts`' documented contract and makes registry and release work scale with every contact. The repeated `funding_accounts.contains(&at)` also makes default pooled funding O(n²) in the number of DashPay accounts. Use a `HashSet<AccountType>` for membership while retaining an ordered list of offered accounts for build-time cleanup, then derive the stored contributor list by mapping the selected prevouts back to their owning accounts.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/transaction.rs:356-358: Represent pooled insufficient funds without naming one account
  For a pooled build, key-wallet's `available` and `required` values describe the union of all offered sources, but `map_builder_error` receives the first preference and stores it as the failing `account_type`. The default path consequently reports aggregate BIP44+BIP32+DashPay values as "insufficient funds on BIP44 account 0" and can name BIP44 even when that account was absent and skipped. This is misleading in the rendered error and factually incorrect for Rust callers inspecting the typed field. Preserve the existing account-specific error for single-source builds and introduce an explicit pooled funding context or pooled insufficient-funds variant for multi-source builds.

In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:4227: Default Swift deferred payments to pooled funding
  The PR explicitly makes plain sends, including `buildSignedPayment`, default to BIP44+BIP32+DashPay receiving funds. This Swift convenience method still defaults to `.bip44` and forwards that selector to `finalizeSignedPayment`, so callers omitting `accountType` remain restricted to BIP44 and can receive an insufficient-funds error even when the pooled wallet balance is sufficient. Kotlin already defaults the corresponding API to `ALL_SPENDABLE`, and Swift callers that require the old strict behavior can still pass `.bip44` explicitly.

Comment thread packages/rs-platform-wallet/src/wallet/core/transaction.rs
Comment thread packages/rs-platform-wallet/src/wallet/core/transaction.rs Outdated
Comment thread packages/rs-platform-wallet/src/wallet/core/transaction.rs Outdated
…and guard duplicate prevouts

Review follow-ups on the pooled send path.

Contributors vs offered accounts. `funding_accounts` was pushed when an
account's UTXOs were OFFERED to selection, so a transaction funded entirely
from BIP44 still reported BIP32 and every DashPay contact as a contributor —
violating the field's documented contract and scaling release and registry
bookkeeping with the address book. Membership now rides a HashSet (the linear
`contains` made pooled funding quadratic in contact count), the ordered offered
list drives build-time cleanup only, and the list stored on the transaction is
derived by mapping each selected prevout back to the account that owns it.

Pooled shortfalls. A pooled build's `available`/`required` describe the union of
every offered source, so attributing them to the first preference reported
aggregate figures as "insufficient funds on BIP44 account 0" — and could name
BIP44 even when no such account existed and it was skipped. Single-source
builds keep the account-specific error; pooled builds get
`CorePooledInsufficientFunds`, carrying the source list instead of one account.
Both map to the same FFI code, so hosts classify a shortfall exactly as before.

Duplicate prevouts. Additive funding can offer an outpoint the builder was
already seeded with through `add_inputs` (the `add_inputs_from_outpoints` FFI
draws from the wallet's own account), and coin selection does not deduplicate,
so the transaction could spend one prevout twice — invalid, and Core rejects
it. Fixed upstream in dashpay/rust-dashcore#931; asserted here as well rather
than handing a signer and then the network a transaction that cannot confirm.

Swift `buildSignedPayment` defaulted to `.bip44` while its Kotlin counterpart
defaults to pooled, so Swift callers omitting `accountType` could hit an
insufficient-funds error with a sufficient pooled balance.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/core/transaction.rs (1)

287-486: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Update the rust-dashcore workspace pin to include pull request #931.

The current pin predates the add_funding de-duplication fix. It can add an input already supplied through add_inputs, causing pooled funding to reject a valid transaction because of duplicate prevouts.

🤖 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 `@packages/rs-platform-wallet/src/wallet/core/transaction.rs` around lines 287
- 486, Update the rust-dashcore workspace dependency pin to a revision that
includes pull request `#931`’s add_funding de-duplication fix. Keep the pooled
funding logic in the transaction-building flow unchanged, including its existing
duplicate-prevout validation.
🧹 Nitpick comments (2)
packages/rs-platform-wallet-ffi/src/error.rs (1)

459-466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extend the mapping test to the pooled variant.

atomic_core_insufficient_funds_maps_to_dedicated_code covers only CoreInsufficientFunds. Add an assertion for CorePooledInsufficientFunds, so a future re-order of the match arms cannot silently drop the pooled variant to ErrorUnknown.

💚 Proposed test addition
#[test]
fn pooled_core_insufficient_funds_maps_to_the_same_code() {
    let result: PlatformWalletFFIResult = PlatformWalletError::CorePooledInsufficientFunds {
        sources: vec![
            AccountTypePreference::BIP44,
            AccountTypePreference::BIP32,
            AccountTypePreference::AllDashpayReceivingFunds,
        ],
        available: Some(0),
        required: Some(1_000_000),
    }
    .into();
    assert_eq!(
        result.code,
        PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds
    );
}
🤖 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 `@packages/rs-platform-wallet-ffi/src/error.rs` around lines 459 - 466, Extend
the mapping tests around atomic_core_insufficient_funds_maps_to_dedicated_code
by adding coverage for PlatformWalletError::CorePooledInsufficientFunds with
representative fields. Assert that its converted PlatformWalletFFIResult uses
PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds.
packages/rs-platform-wallet/src/test_support.rs (1)

316-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the repeated fixture tail.

Four fixtures now repeat the same closing block: build WalletSigner, create the generation, assemble PlatformWalletInfo, create the WalletManager, and insert the wallet. Extract a small helper that consumes the TestWalletContext and returns (manager, wallet_id, generation, signer). This keeps future changes to PlatformWalletInfo to one place.

Also applies to: 417-437

🤖 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 `@packages/rs-platform-wallet/src/test_support.rs` around lines 316 - 330,
Extract the repeated fixture setup from the affected test helpers into a shared
helper that consumes TestWalletContext and returns the wallet manager, wallet
ID, generation, and signer tuple. Move the WalletSigner, WalletGeneration,
PlatformWalletInfo, WalletManager, and insert_wallet logic into this helper,
then update all four fixtures to call it while preserving their existing return
values.
🤖 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
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt`:
- Around line 40-46: Reformat the affected Kotlin and Swift blocks to use the
repository’s two-space indentation rule without changing behavior:
CoreTransactionBuilder.kt lines 40-46, ManagedPlatformWallet.kt lines 108-120,
154-165, and 354-378; CoreTransactionBuilder.swift lines 147-159 and 321-322;
and ManagedPlatformWallet.swift lines 4222-4231. Align the enum documentation,
selector declarations and mappings, FFI mapping, defaults, and parameters
consistently with surrounding code.

In `@packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- Around line 72-94: Move the funding-order and multi-source API documentation
from `single_preference` to `funding_sources`, and keep the
single-account-family and pooled-selector behavior documentation attached to
`single_preference`. Ensure each method’s doc comment describes only its own
contract.

In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 132-141: Update the documentation above the broadcast method
containing self.broadcaster.broadcast to describe the concrete contributing
accounts slice (accounts: &[AccountType]) instead of AccountTypePreference and
account index, and document that pooled reservations are released for those
accounts when the broadcast is rejected.

In `@packages/rs-platform-wallet/src/wallet/core/wallet.rs`:
- Around line 153-159: Update the error construction in the
account_type.account_type lookup within the surrounding wallet method to use the
input-parameter error variant for set-naming selectors instead of
WalletNotFound. Preserve WalletNotFound for the two genuine wallet lookup misses
below, and keep the existing error message and selector validation behavior.

---

Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/core/transaction.rs`:
- Around line 287-486: Update the rust-dashcore workspace dependency pin to a
revision that includes pull request `#931`’s add_funding de-duplication fix. Keep
the pooled funding logic in the transaction-building flow unchanged, including
its existing duplicate-prevout validation.

---

Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 459-466: Extend the mapping tests around
atomic_core_insufficient_funds_maps_to_dedicated_code by adding coverage for
PlatformWalletError::CorePooledInsufficientFunds with representative fields.
Assert that its converted PlatformWalletFFIResult uses
PlatformWalletFFIResultCode::ErrorCoreInsufficientFunds.

In `@packages/rs-platform-wallet/src/test_support.rs`:
- Around line 316-330: Extract the repeated fixture setup from the affected test
helpers into a shared helper that consumes TestWalletContext and returns the
wallet manager, wallet ID, generation, and signer tuple. Move the WalletSigner,
WalletGeneration, PlatformWalletInfo, WalletManager, and insert_wallet logic
into this helper, then update all four fixtures to call it while preserving
their existing return values.
🪄 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: 02b790dc-5174-4823-8722-b9f5f32a0957

📥 Commits

Reviewing files that changed from the base of the PR and between 96af88d and 329147a.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (21)
  • Cargo.toml
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/CoreTransactionBuilder.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.kt
  • packages/rs-platform-wallet-ffi/src/core_wallet/addresses.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
  • packages/rs-platform-wallet-ffi/src/error.rs
  • packages/rs-platform-wallet-ffi/src/manager.rs
  • packages/rs-platform-wallet-ffi/src/wallet.rs
  • packages/rs-platform-wallet/src/error.rs
  • packages/rs-platform-wallet/src/lib.rs
  • packages/rs-platform-wallet/src/test_support.rs
  • packages/rs-platform-wallet/src/wallet/core/broadcast.rs
  • packages/rs-platform-wallet/src/wallet/core/mod.rs
  • packages/rs-platform-wallet/src/wallet/core/transaction.rs
  • packages/rs-platform-wallet/src/wallet/core/wallet.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
  • packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
  • packages/rs-unified-sdk-jni/src/wallet_manager.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift

Comment thread packages/rs-platform-wallet/src/wallet/core/broadcast.rs
Comment thread packages/rs-platform-wallet/src/wallet/core/wallet.rs
…or, correct two stale docs

A DashPay set selector handed to the per-account gap-limit API is a
caller-argument error, but it returned WalletNotFound — rendering as "Wallet
not found: ... names a set of accounts" and telling a host the wallet is
missing. The FFI boundary already rejects the same class with its
invalid-parameter code, so add a typed InvalidParameter variant mapped to that
code and use it here. (AddressOperation, suggested in review, is the repo's
pinned example of a variant that falls through to ErrorUnknown, which would
report an internal failure for a bad argument.)

Docs: the funding-sources paragraph was attached to single_preference, leaving
funding_sources undocumented; and broadcast_payment_releasing_reservation still
described an AccountTypePreference plus index after it moved to a slice of
contributing AccountTypes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/error.rs (1)

459-465: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add direct conversion tests for both FFI error mappings.

Test CorePooledInsufficientFundsErrorCoreInsufficientFunds and InvalidParameterErrorInvalidParameter. Neither mapping has direct test coverage.

🤖 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 `@packages/rs-platform-wallet-ffi/src/error.rs` around lines 459 - 465, Add
direct conversion tests for the mapping logic containing CoreInsufficientFunds
and CorePooledInsufficientFunds, asserting CorePooledInsufficientFunds converts
to ErrorCoreInsufficientFunds, and add a separate assertion that
InvalidParameter converts to ErrorInvalidParameter. Reuse the existing
error-conversion test structure and helpers.
🤖 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.

Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/error.rs`:
- Around line 459-465: Add direct conversion tests for the mapping logic
containing CoreInsufficientFunds and CorePooledInsufficientFunds, asserting
CorePooledInsufficientFunds converts to ErrorCoreInsufficientFunds, and add a
separate assertion that InvalidParameter converts to ErrorInvalidParameter.
Reuse the existing error-conversion test structure and helpers.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4555aa72-fde8-4d0b-a472-164cbfcd4283

📥 Commits

Reviewing files that changed from the base of the PR and between 329147a and f15fe29.

📒 Files selected for processing (5)
  • packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
  • packages/rs-platform-wallet-ffi/src/error.rs
  • packages/rs-platform-wallet/src/error.rs
  • packages/rs-platform-wallet/src/wallet/core/broadcast.rs
  • packages/rs-platform-wallet/src/wallet/core/wallet.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/rs-platform-wallet/src/wallet/core/wallet.rs
  • packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs
  • packages/rs-platform-wallet/src/wallet/core/broadcast.rs

Neither new variant had direct coverage. The pooled one matters most: a pooled
shortfall deliberately shares the single-account insufficient-funds code so
hosts classify it identically, and splitting that later would silently
reclassify the most common failure on the default send path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@QuantumExplorer

Copy link
Copy Markdown
Member Author

Added both conversion tests in 0e1763c (pooled_insufficient_funds_shares_the_single_account_code, invalid_parameter_maps_to_the_parameter_code), reusing the existing conversion-test structure. The pooled one is the load-bearing assertion: sharing the single-account code is deliberate so hosts classify a shortfall identically, and splitting it later would silently reclassify the most common failure on the default send path.

🤖 Addressed by Claude Code

@QuantumExplorer
QuantumExplorer merged commit 14e2419 into v4.2-dev Aug 7, 2026
18 checks passed
@QuantumExplorer
QuantumExplorer deleted the feat/send-multi-account-funding branch August 7, 2026 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants