fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073)#4184
fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073)#4184bfoss765 wants to merge 12 commits into
Conversation
📝 WalkthroughWalkthroughShielded asset-lock funding now supports an optional BIP32 funding path that restricts UTXO selection to one account. Typed insufficient-funds errors are propagated through wallet, FFI, JNI, Kotlin, and Swift APIs, with Swift example-app input support. ChangesAsset-lock funding path
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Sonnet deferred (commit 155d34b) |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 47 minutes. |
|
The fix is correct and still reproduces on tip (the #4073 symptom is live in
Minor: the SwiftExampleApp funding picker still gates on the single BIP44 account, so the UI blocks the exact scenario this fixes (follow-up); the upstream-behavior pin tests (router/gap-limit) will trip on any rust-dashcore pin change — deliberate?; tracker refs ("finding 5b52d9844055") → PR description. |
|
Two additional architecture blockers after checking the existing comments:
The existing typed-error and Swift BIP44-only preflight comments are correct and are not duplicated here. |
…vacy-domain funding gate (dashpay#4184) Addresses two must-fix reviewer findings on PR dashpay#4184. 1. FFI error arm for the asset-lock shortfall (dashpay#4073 request 3). `AssetLockInsufficientFunds` never crossed the FFI: with no arm in `From<PlatformWalletError>` it flattened to `ErrorUnknown(99)`, hiding a typed shortfall behind the catch-all and forcing hosts to string-match the Display text. Add: - `ErrorAssetLockInsufficientFunds = 26` (sibling of ErrorCoreInsufficientFunds = 22; existing codes unchanged), mapped in the From impl (the structured available/required duffs still travel in the message), plus a Rust test that the error crosses as code 26 (not 99) with the message verbatim. - Kotlin `DashSdkError.PlatformWallet.AssetLockInsufficientFunds` (26 ->) and Swift `.errorAssetLockInsufficientFunds`; cbindgen emits the C constant. The Display text is UNCHANGED ("asset lock coin selection is short: ...") so dash-wallet's existing substring matcher keeps working while it migrates to the typed code. 2. Privacy-domain co-spend gate. Largest-first could union ordinary BIP44/BIP32, CoinJoin, and DashPay-receiving funds into one L1 tx (with BIP44 change), irreversibly linking those domains. Default funding now stays within a single privacy domain: - Domains: Transparent {BIP44,BIP32} > CoinJoin > DashPay-receiving. Transparent is the only default-eligible domain because it holds the primary account and is the sole source of change (key-wallet derives change only on Standard accounts) — so any non-transparent spend inherently crosses into it. - New `CrossDomainConsent` (Denied default / Allowed opt-in) threaded through the builder, orchestration, `shielded_fund_from_asset_lock`, the JNI bridge, and the `platform_wallet_manager_shielded_fund_from_asset_lock` FFI (`allow_cross_domain: bool`). Wrapper methods keep every existing caller on the safe default. - Cross-domain refusal returns the typed `AssetLockCrossDomainConsentRequired` (FFI code 27; Kotlin/Swift mapped) carrying transparent/union/required duffs. - Watch-only DashpayExternalAccount exclusion preserved via the domain classifier (returns None); identity-funding single-BIP44 carve-out preserved. Tests: single-domain success without consent; cross-domain refused without consent (typed error) and succeeds with consent; existing union/CoinJoin/DashPay tests moved to the consented path. cargo test -p platform-wallet --lib = 504 passed; --features shielded = 634 passed; -p platform-wallet-ffi --lib = 198 passed. clippy clean on all three crates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both must-fix items and both architecture blockers are addressed and pushed. On the reservation-ledger blocker: implemented upstream as you specified — a rust-dashcore key-wallet PR (opening shortly) adds |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 35 minutes. |
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Consolidated re-verification (two independent passes). The pushed substance checks out: typed codes 29/30 with by-ref mapping so the shielded entry point can't drift, Kotlin mappings + tests, the privacy-domain consent gate is thorough (transparent-vs-union precheck, fee-band reclassification, default-deny at every entry), and filing rust-dashcore#912 for the reservation ledger is the right split. The Rust asset-lock suite passes 8/8 locally. Remaining blockers:
Minor: the new |
blocker) PlatformWalletResultCode gained cases 29/30 (errorAssetLockInsufficientFunds, errorAssetLockCrossDomainConsentRequired) but PlatformWalletError had no matching cases, so init(result:) — which switches over result.code with no default — became non-exhaustive and the Swift package no longer compiled. Add the two matching cases (assetLockInsufficientFunds, assetLockCrossDomainConsentRequired) to PlatformWalletError, extend the errorDescription associated-value binding, and map both codes in init(result:). Semantics/messages mirror the Kotlin DashSdkError.PlatformWallet counterparts (codes 29/30). Verified: swiftc -typecheck of PlatformWalletResult.swift against a stub DashSDKFFI module built from the cbindgen-generated header now passes; removing the fix reproduces "switch must be exhaustive". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ShieldedFundFromAssetLockView built its funding picker from BIP44 accounts only and never set allowCrossDomain, so the all-funds/cross-domain flow could not be exercised or consented to on iOS. Add a minimal "Allow cross-domain funds" toggle (transparent-only by default) wired through to shieldedFundFromAssetLock(allowCrossDomain:), so the code-30 gate is reachable. A tracked TODO on crossDomainConsentSection records the intended fuller UX (submit false, catch errorAssetLockCrossDomainConsentRequired, show the transparent/union/ required breakdown, then retry with true) as a follow-up to file — the PR is held for rust-dashcore#912 before that lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dashpay#4184) - Remove the "finding <hash>" tracker references from build.rs comments (rationale text kept); these belong in the PR description, not the source. - Add an ABI/release-note line to the platform-wallet-ffi README recording the C-ABI break: platform_wallet_manager_shielded_fund_from_asset_lock gained a trailing `bool allow_cross_domain` parameter, plus result codes 29/30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`PlatformWalletResultCode` jumped from 25 straight to 98, so the three deferred build->broadcast/release codes this PR owns (26 StaleReservationToken, 27 ReservationTokenConsumed, 28 ReservationWalletMismatch) fell through to `.errorUnknown` on iOS, erasing their distinct retry semantics. Add the three raw codes to `PlatformWalletResultCode`, matching cases to `PlatformWalletError`, and map them in both `init(ffi:)` and `init(result:)`. The `init(result:)` switch (no default) stays exhaustive — the same non-exhaustive-switch class shumkov flagged on dashpay#4184. Messages pass the Rust `Display` string straight through, matching the Kotlin SDK's mapping verbatim. Verified with `swiftc -parse` (the DashSDKFFI xcframework — cbindgen header + cdylib — is built separately by build_ios.sh and is not present in this checkout, so a full `swift build` type-check isn't possible here). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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
🤖 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- Around line 275-302: Update the canSubmit logic in
ShieldedFundFromAssetLockView so enabling allowCrossDomain no longer requires
the selected BIP44 balance to cover the full lock amount. Permit submission when
cross-domain consent is enabled, while preserving the existing balance
validation when it is disabled and letting Rust perform the authoritative
union-funds check.
🪄 Autofix (Beta)
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
Run ID: 3eef4ea0-742b-45b1-883c-1ec0700a620b
📒 Files selected for processing (19)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/rs-platform-wallet-ffi/README.mdpackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/mod.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/seed_pool.rspackages/rs-unified-sdk-jni/src/funding.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift
|
Addressed the independent findings; the reservation race stays held for rust-dashcore#912.
|
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)
packages/rs-platform-wallet-ffi/src/shielded_send.rs (1)
1079-1085: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale error-reference comment.
AssetLockCrossDomainConsentRequiredis no longer present inPlatformWalletError/ the FFI code mapping, so this comment should only coverAssetLockInsufficientFundsand drop the cross-domain consent rationale.🤖 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/shielded_send.rs` around lines 1079 - 1085, The error-preservation comment in the result handling block should no longer reference AssetLockCrossDomainConsentRequired or cross-domain consent behavior. Update it to document only preservation of AssetLockInsufficientFunds as a dedicated FFI error code, while retaining the existing generic-error distinction and message-prefix rationale.
🤖 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/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs`:
- Line 153: Limit shielded asset-lock funding to the primary account until
reservation bookkeeping supports the selected funding_path, or carry that
account/path through every reservation and release operation in the asset-lock
build flow. Update the funding_path handling in the shielded builder and the
reservation logic in build.rs so rejected or concurrent secondary-account builds
cannot reserve, release, or reselect inputs under the primary
BIP44/account-index path.
---
Outside diff comments:
In `@packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- Around line 1079-1085: The error-preservation comment in the result handling
block should no longer reference AssetLockCrossDomainConsentRequired or
cross-domain consent behavior. Update it to document only preservation of
AssetLockInsufficientFunds as a dedicated FFI error code, while retaining the
existing generic-error distinction and message-prefix rationale.
🪄 Autofix (Beta)
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
Run ID: 850ec629-a6b8-4066-93fd-aafdb3f1dd72
📒 Files selected for processing (19)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/rs-platform-wallet-ffi/src/asset_lock/build.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-platform-wallet/src/wallet/asset_lock/orchestration.rspackages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rspackages/rs-platform-wallet/src/wallet/identity/network/invitation.rspackages/rs-platform-wallet/src/wallet/identity/network/registration.rspackages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rspackages/rs-platform-wallet/src/wallet/shielded/seed_pool.rspackages/rs-unified-sdk-jni/src/funding.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift
💤 Files with no reviewable changes (3)
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/rs-platform-wallet-ffi/src/error.rs
| dummy_outputs: usize, | ||
| settings: Option<PutSettings>, | ||
| cl_wait: Option<Duration>, | ||
| funding_path: Option<::key_wallet::bip32::DerivationPath>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope reservations to the selected funding account.
funding_path now allows shielded asset-lock builds to consume CoinJoin/DashPay UTXOs, but packages/rs-platform-wallet/src/wallet/asset_lock/build.rs still records and releases reservations through the primary BIP44/account-index path. A rejected or concurrent secondary-account build can therefore leave inputs reserved incorrectly—or selectable again—causing false shortfalls or duplicate input selection on retry. Carry the owning account/path through reservation bookkeeping and make release atomic, or gate explicit secondary-account funding until the upstream fix is adopted.
Based on the PR objectives, this reservation race is explicitly still unresolved and tracked for an upstream atomic-reservation fix.
Also applies to: 222-227
🤖 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/shielded/fund_from_asset_lock.rs` at
line 153, Limit shielded asset-lock funding to the primary account until
reservation bookkeeping supports the selected funding_path, or carry that
account/path through every reservation and release operation in the asset-lock
build flow. Update the funding_path handling in the shielded builder and the
reservation logic in build.rs so rejected or concurrent secondary-account builds
cannot reserve, release, or reselect inputs under the primary
BIP44/account-index path.
… on base's upstreamed router fix (dashpay#4074) Rebase of PR dashpay#4074 (fix/kotlin-sdk-assetlock-multi-account) onto feat/kotlin-sdk-and-example-app. The platform-wallet asset-lock changes are preserved; the PR's OWN rust-dashcore customization is dropped because the base branch now carries the same fix upstream. What this brings to the asset-lock code path (packages/rs-platform-wallet): - Multi-account funding builder `build_asset_lock_tx_from_all_funding_accounts` that unions spendable UTXOs across BIP44 + CoinJoin + DashPay funds accounts (dashpay#4073), with LargestFirst coin selection pinned for the many-small-denomination CoinJoin shape. - Exclusion of watch-only `DashpayExternalAccount` UTXOs from the union — those are a contact's coins the local mnemonic can't sign; selecting one yields an invalid input signature. The receiving (ours) DashPay account stays included. - `NoUtxosAvailable` mapped to the typed asset-lock insufficient-funds error. - Regression tests covering the router-fix persistence path (CoinJoin + DashpayReceivingFunds legs) and the watch-only exclusion, plus split-funded test fixtures (`split_funded_wallet_manager`, `..._dashpay`, `..._many_coinjoin`). Why the PR's rust-dashcore vendoring/[patch] is DROPPED: The PR originally shipped the asset-lock transaction-router fix by pinning rust-dashcore at 1860089e and redirecting it via a `[patch]` to a bfoss765 fork (rev e8c7335 = 1860089e + the router fix + a CoinJoin gap-limit 30->100 bump). The base branch's rust-dashcore rev 19690d31 now contains BOTH fixes upstream: * `TransactionRouter::get_relevant_account_types(AssetLock)` includes CoinJoin, DashpayReceivingFunds, and DashpayExternalAccount (via `fund_bearing_account_types()`); * `DEFAULT_COINJOIN_GAP_LIMIT = 100`. So the fork [patch], the pinned 1860089e rev, and the leftover `third_party/rust-dashcore` are all obsolete and removed. Cargo.toml/Cargo.lock are taken as-is from base (rust-dashcore resolves from dashpay @ 19690d31, no patch table). Because base's fix debits CoinJoin/DashPay asset-lock spends via the normal `check_core_transaction` scan, the PR's earlier broadcast-time `debit_router_omitted_asset_lock_spends` mitigation is gone — as it already was in the PR's final state (dashpay/dash-wallet#1507). History note: the PR's 10 original commits touched the same four files the base branch had independently rewritten (+928 lines), and included add-then-remove churn (the interim mitigation) plus vendor-then-git-patch churn that base's upstreamed fix makes moot. They are collapsed into this single commit to keep the rebased history coherent. Verified: `cargo check -p platform-wallet --all-targets`, `-p rs-unified-sdk-jni`, `-p platform-wallet-ffi` all green; `cargo test -p platform-wallet --lib` = 502 passed / 0 failed, including the router-fix and watch-only-exclusion regression tests, against base's 19690d31. Original commits folded in: 9be2e14, 5376235, e1593f4, da97eec (app-code only; vendoring dropped), ce482fd (app-code only; vendored gap-limit dropped), 380645a, b43caed (dropped: pure [patch] plumbing), a5ea9e5, 77561d2, 189e068. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…vacy-domain funding gate (dashpay#4184) Addresses two must-fix reviewer findings on PR dashpay#4184. 1. FFI error arm for the asset-lock shortfall (dashpay#4073 request 3). `AssetLockInsufficientFunds` never crossed the FFI: with no arm in `From<PlatformWalletError>` it flattened to `ErrorUnknown(99)`, hiding a typed shortfall behind the catch-all and forcing hosts to string-match the Display text. Add: - `ErrorAssetLockInsufficientFunds = 26` (sibling of ErrorCoreInsufficientFunds = 22; existing codes unchanged), mapped in the From impl (the structured available/required duffs still travel in the message), plus a Rust test that the error crosses as code 26 (not 99) with the message verbatim. - Kotlin `DashSdkError.PlatformWallet.AssetLockInsufficientFunds` (26 ->) and Swift `.errorAssetLockInsufficientFunds`; cbindgen emits the C constant. The Display text is UNCHANGED ("asset lock coin selection is short: ...") so dash-wallet's existing substring matcher keeps working while it migrates to the typed code. 2. Privacy-domain co-spend gate. Largest-first could union ordinary BIP44/BIP32, CoinJoin, and DashPay-receiving funds into one L1 tx (with BIP44 change), irreversibly linking those domains. Default funding now stays within a single privacy domain: - Domains: Transparent {BIP44,BIP32} > CoinJoin > DashPay-receiving. Transparent is the only default-eligible domain because it holds the primary account and is the sole source of change (key-wallet derives change only on Standard accounts) — so any non-transparent spend inherently crosses into it. - New `CrossDomainConsent` (Denied default / Allowed opt-in) threaded through the builder, orchestration, `shielded_fund_from_asset_lock`, the JNI bridge, and the `platform_wallet_manager_shielded_fund_from_asset_lock` FFI (`allow_cross_domain: bool`). Wrapper methods keep every existing caller on the safe default. - Cross-domain refusal returns the typed `AssetLockCrossDomainConsentRequired` (FFI code 27; Kotlin/Swift mapped) carrying transparent/union/required duffs. - Watch-only DashpayExternalAccount exclusion preserved via the domain classifier (returns None); identity-funding single-BIP44 carve-out preserved. Tests: single-domain success without consent; cross-domain refused without consent (typed error) and succeeds with consent; existing union/CoinJoin/DashPay tests moved to the consented path. cargo test -p platform-wallet --lib = 504 passed; --features shielded = 634 passed; -p platform-wallet-ffi --lib = 198 passed. clippy clean on all three crates. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-PR collision: the split-build-broadcast branch (dashpay#4185) already allocates 26-28 for the reservation-token errors on the same base, and both PRs would merge without textual conflict, silently misclassifying errors on whichever lands second. Codes 26-28 are now documented as reserved for dashpay#4185. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 2: the pre-selection consent gate compares fee-exclusive totals, so a transparent balance in [target, target+fee) passed the gate and then died in selection as plain insufficient-funds — the host never learned that consent would unlock the spend. The selection-failure mapping now converts the shortfall to AssetLockCrossDomainConsentRequired when consent is denied and the union covers what selection reported missing. Adds the fee-band test and the denied-both-short test the review found uncovered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
blocker) PlatformWalletResultCode gained cases 29/30 (errorAssetLockInsufficientFunds, errorAssetLockCrossDomainConsentRequired) but PlatformWalletError had no matching cases, so init(result:) — which switches over result.code with no default — became non-exhaustive and the Swift package no longer compiled. Add the two matching cases (assetLockInsufficientFunds, assetLockCrossDomainConsentRequired) to PlatformWalletError, extend the errorDescription associated-value binding, and map both codes in init(result:). Semantics/messages mirror the Kotlin DashSdkError.PlatformWallet counterparts (codes 29/30). Verified: swiftc -typecheck of PlatformWalletResult.swift against a stub DashSDKFFI module built from the cbindgen-generated header now passes; removing the fix reproduces "switch must be exhaustive". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ShieldedFundFromAssetLockView built its funding picker from BIP44 accounts only and never set allowCrossDomain, so the all-funds/cross-domain flow could not be exercised or consented to on iOS. Add a minimal "Allow cross-domain funds" toggle (transparent-only by default) wired through to shieldedFundFromAssetLock(allowCrossDomain:), so the code-30 gate is reachable. A tracked TODO on crossDomainConsentSection records the intended fuller UX (submit false, catch errorAssetLockCrossDomainConsentRequired, show the transparent/union/ required breakdown, then retry with true) as a follow-up to file — the PR is held for rust-dashcore#912 before that lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
dashpay#4184) - Remove the "finding <hash>" tracker references from build.rs comments (rationale text kept); these belong in the PR description, not the source. - Add an ABI/release-note line to the platform-wallet-ffi README recording the C-ABI break: platform_wallet_manager_shielded_fund_from_asset_lock gained a trailing `bool allow_cross_domain` parameter, plus result codes 29/30. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…optional derivation path (drop multi-account union + consent gate) Replace the multi-account union + privacy-domain consent gate with a single account-by-derivation-path selection. The caller either accepts the default (unmixed BIP44 account) or passes one explicit BIP32 path (e.g. the DIP-9 CoinJoin account) as a nullable String, threaded Kotlin -> JNI -> FFI -> Rust as Option<DerivationPath>. No union across accounts, no consent prompt. - rs-platform-wallet: drop build_asset_lock_tx_from_all_funding_accounts, CrossDomainConsent/PrivacyDomain, and AssetLockCrossDomainConsentRequired; add build_asset_lock_tx_from_selected_account (single account by path, change routed to the BIP44 account since non-Standard accounts cannot derive change); collapse the four *_with_consent wrapper pairs into a single funding_path arg. - rs-platform-wallet-ffi: swap allow_cross_domain bool for funding_path (ptr+len UTF-8 BIP32 string); drop error code 30. - rs-unified-sdk-jni / kotlin-sdk: allowCrossDomain: Boolean -> fundingPath: String?; drop DashSdkError.PlatformWallet.AssetLockCrossDomainConsentRequired. - Replace the union/consent tests with single-path tests (explicit CoinJoin path, default BIP44, no-union proof, typed shortfall, watch-only refusal). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g path (drop cross-domain consent + code 30) Mirror the Kotlin re-scope (2d86a29) on the second consumer of the same C ABI. The FFI dropped the allow_cross_domain bool for a nullable funding_path (ptr+len UTF-8 BIP32 string) and removed error code 30, so the old Swift signature no longer compiles. - PlatformWalletManagerShieldedFunding.swift: allowCrossDomain: Bool -> fundingPath: String? = nil; marshal the optional path as raw UTF-8 bytes via a new withOptionalFundingPath helper (nil/empty -> nil ptr + 0 len), mirroring withOptionalSurplusOutput, and pass funding_path_ptr/ funding_path_len to the C function. - PlatformWalletResult.swift: drop the errorAssetLockCrossDomainConsentRequired (code 30) FFI case and the assetLockCrossDomainConsentRequired PlatformWalletError case and their mappings. - SwiftExampleApp ShieldedFundFromAssetLockView: replace the cross-domain consent toggle with an optional funding-path text field; pass fundingPath (nil when blank) to the SDK call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…re-scope The dashpay#4184 re-scope dropped AssetLockCrossDomainConsentRequired (code 30) but left references behind: a DashSdkErrorTest mapping (broke :sdk:test compile), the rs-platform-wallet-ffi README ABI note, and a shielded_send.rs comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dfc670d to
bfb1f49
Compare
|
Consolidated follow-up on the re-scope (joint verdict from both review tracks). The single-selected-account redesign is the right resolution of the earlier privacy blocker — consent could never undo on-chain input clustering, and scoping selection to one account eliminates it. Accordingly: the multi-account reservation-ledger blocker is withdrawn as obsolete (rust-dashcore#912 remains good upstream work but is no longer this PR's dependency), and the Swift 29/30 compile fix is confirmed on the current head. Changes are still requested — three new findings on the re-scoped code:
Moderate:
Advisory: CoinJoin-funded locks route change to the BIP44 account (upstream limitation — non-Standard accounts can't derive change), so change-bearing spends still create one CoinJoin→BIP44 link. Prefer change-free selection when feasible (denomination/fee constraints permitting) and document the residual linkage. CI: still zero Rust/Swift jobs on this fork PR — local 507/507 doesn't exercise the reservation window. Same ask as before: a same-repo ref run before merge. |
…ntracts (dashpay#4184) Address maintainer/CodeRabbit review findings on the dashpay#4184 asset-lock funding re-scope. 1. Reserve the selected-account inputs (correctness / double-spend). `build_asset_lock_tx_from_selected_account` seeded inputs via `add_inputs` with NO ReservationSet, so a re-scoped shielded build reserved nothing — a concurrent normal Core send (the BIP44 `build_asset_lock_with_signer` path) or a CoinJoin mix could reselect the same UTXOs in the build->broadcast->reconcile window (the wallet write lock + `shield_guard` only serialize shielded builds). Point the key-wallet builder's `set_funding` at the SELECTED funds account instead of `add_inputs`, so the chosen inputs are reserved in that account's OWN reservation ledger (the same single-account machinery the BIP44 builder uses) and held across build->broadcast. Release on a rejected broadcast is now path-aware (`release_asset_lock_funding_reservation`): shielded funding releases from the account named by `funding_path` (possibly CoinJoin/DashPay); every other funding type from the BIP44 account. Adds two tests: a selected-account build reserves its CoinJoin input across broadcast, and a rejected selected-account broadcast releases it. 2. Restore `WalletOperation` for the generic shielded-funding failure arm (was regressed to `Unknown(99)`): `platform_wallet_error_code_or_wallet_operation` keeps typed codes but maps the catch-all to `ErrorWalletOperation`, at both the fund and resume-fund FFI entry points. 3. JNI: read `fundingPath` with a strict reader that throws on a genuine JNI read error instead of silently degrading a money-source param to the default BIP44 account; JVM null still means "absent". 4. Rewrite the stale multi-account-union/consent docs in `orchestration.rs` (FromWalletBalance) to the as-built single-path design. 5. Swift example: the funding-account picker now surfaces non-Standard funds accounts (CoinJoin/DashPay) with type labels, and `canSubmit` no longer gates on the selected Core account's balance when an explicit `fundingPath` is supplied — so a CoinJoin path is exercisable end-to-end. Validation: cargo build -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --features shielded; cargo test -p platform-wallet --features shielded (639 passed); cargo test -p platform-wallet-ffi --features shielded (215+26+6 passed); ./gradlew :sdk:assemble -x lint (BUILD SUCCESSFUL). Swift not built locally (no xcframework). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`PlatformWalletResultCode` jumped from 25 straight to 98, so the three deferred build->broadcast/release codes this PR owns (26 StaleReservationToken, 27 ReservationTokenConsumed, 28 ReservationWalletMismatch) fell through to `.errorUnknown` on iOS, erasing their distinct retry semantics. Add the three raw codes to `PlatformWalletResultCode`, matching cases to `PlatformWalletError`, and map them in both `init(ffi:)` and `init(result:)`. The `init(result:)` switch (no default) stays exhaustive — the same non-exhaustive-switch class shumkov flagged on dashpay#4184. Messages pass the Rust `Display` string straight through, matching the Kotlin SDK's mapping verbatim. Verified with `swiftc -parse` (the DashSDKFFI xcframework — cbindgen header + cdylib — is built separately by build_ios.sh and is not present in this checkout, so a full `swift build` type-check isn't possible here). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
v4.1-dev added ErrorTransactionBroadcastRejected = 26, colliding with this PR's three deferred-reservation siblings that also claimed 26/27/28. Keep v4.1-dev's 26 and shift this PR's codes up by one: 27 = ErrorStaleReservationToken (was 26) 28 = ErrorReservationTokenConsumed (was 27) 29 = ErrorReservationWalletMismatch (was 28) 29 is free on v4.1-dev (dashpay#4184's AssetLockInsufficientFunds is not yet merged there). The Rust FFI enum and Swift bindings were renumbered in the rebase conflict resolution; this finishes the propagation through the Kotlin runtime mapping and KDoc (DashSdkError.kt, WalletManagerNative.kt), the Kotlin error-code test, and the signed_payment FFI doc comments (also recast from fix-round narration to an as-built description). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This PR allocates FFI result code After #4185's rebase onto v4.1-dev, its deferred-reservation errors are renumbered to Whichever of #4184 / #4185 merges SECOND must shift its |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The single-account redesign correctly removes the earlier cross-account co-spending and reservation-ledger defects, and the dedicated insufficient-funds error now reaches Swift and Kotlin. Two blocking issues remain: reservations can be stranded when a signed transaction is abandoned before broadcast, and explicit BIP32 funding passes the BIP44 xpub into the selected account's address pool. The Swift example also has ambiguous account picker values, while two documentation issues remain.
Validated blockers were found in the Codex precheck. Sonnet 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)
1 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/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:427-473: Roll back reservations whenever a signed transaction is abandoned before broadcast
At the pinned key-wallet revision, `assemble_unsigned` reserves the selected inputs and `build_signed` releases them only when input signing itself fails. Once line 427 succeeds, the custom selected-account path can still return before exposing the transaction: the credit-account lookup and `peek_next_path` are written as fallible operations, the external `signer.public_key` callback can genuinely fail, and the second account lookup plus `mark_first_pool_index_used` are also propagated with `?`. The preceding funding-address preflight makes the account/pool failures unlikely under current invariants, but a transient resolver or Keychain failure at line 457 is directly reachable and leaves the transaction unbroadcast and untracked. The same reservation lifecycle also affects the pinned non-shielded `build_asset_lock_with_signer` call at lines 169-178, whose dependency implementation performs credit-key callbacks after `build_signed`, and the invitation durability branch at lines 1077-1090 can abandon an already-built transaction when pool persistence or `flush()` fails. Existing cleanup covers only input-signing failure inside key-wallet and definitive `BroadcastError::Rejected` at lines 1126-1150; accepted or `MaybeSent` transactions correctly retain reservations. Add rollback coverage around the complete post-signing, pre-broadcast phase, including the upstream builder path, so every error that definitively abandons the transaction immediately releases its inputs.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:423-424: Pass the selected Standard account's xpub to set_funding
`TransactionBuilder::set_funding` immediately calls `funds_acc.next_change_address(Some(&acc.account_xpub), true)` before the following `set_change_address` override. Here `funds_acc` is the account selected by `funding_path`, but `acc` is always the BIP44 change account. This is harmless for non-Standard accounts because their change derivation fails before mutation, and correct for the default BIP44 account, but it is wrong for an explicitly selected Standard BIP32 account. Once that BIP32 account has no pre-generated unused internal address, key-wallet derives `[1, index]` from the BIP44 xpub while recording the address under the BIP32 account's full path, inserts it into the BIP32 pool, and bumps the monitor revision. Overriding this transaction's change output does not undo that mutation; a later normal BIP32 send can use the poisoned pool entry as change and record a derivation path whose signer key does not match the address. Resolve the wallet `Account` corresponding to the selected Standard account and pass its xpub to `set_funding`, while retaining the separate BIP44 `set_change_address` override.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ShieldedFundFromAssetLockView.swift:255-260: Do not use accountIndex alone as the picker selection value
`CoreAccountOption.id` correctly distinguishes BIP44, BIP32, CoinJoin, and DashPay rows for `ForEach`, but every picker row is still tagged with only `accountIndex`. These account families commonly reuse indices, so BIP44 #0, BIP32 #0, and CoinJoin #0 all represent the same selection value; `selectedCoreAccountBalanceDuffs` likewise returns the first row with that index. SwiftUI therefore cannot preserve or display which row the user chose. If a non-Standard row has an index without a corresponding BIP44 account, submission passes that index as the mandatory BIP44 change sink and Rust rejects the operation. This does not silently change the input source—the separate `fundingPath` still controls that—but it makes the picker ambiguous and can select an invalid change account. Keep the picker restricted to BIP44 change accounts and display other balances separately, or bind selection to a composite account identity while maintaining a distinct BIP44 change-account selection.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:933-934: Document account_index as the default source and change sink
The opening public contract says unconditionally that `account_index` selects the BIP44 account whose UTXOs fund the lock. With an explicit `funding_path`, inputs instead come solely from the named account and `account_index` supplies the BIP44 change sink. The later funding-path paragraph documents this correctly, making the same API comment internally contradictory. The stale statement is also mirrored in `PlatformWalletManagerShieldedFunding.swift:110-111` and `PlatformWalletManager.kt:1263`; update all three surfaces so callers do not mistake the change-account index for the explicit input source.
| let (transaction, fee) = builder | ||
| .build_signed(signer, |addr| selected.address_derivation_path(&addr)) | ||
| .await | ||
| .map_err(|e| map_builder_error(e, target_duffs))?; | ||
| tracing::debug!( | ||
| selected_inputs = transaction.input.len(), | ||
| fee, | ||
| txid = %transaction.txid(), | ||
| "single-account asset-lock funding: transaction built + signed" | ||
| ); | ||
|
|
||
| // Derive the single credit-output key from the shielded-topup account, | ||
| // mirroring the pinned single-account builder's phase-1/2/3 sequence | ||
| // (peek without marking → signer round-trip → commit the index) so a | ||
| // signer failure never irreversibly consumes a pool index. | ||
| let (path, index) = { | ||
| let credit_account = info | ||
| .core_wallet | ||
| .accounts | ||
| .asset_lock_shielded_address_topup | ||
| .as_mut() | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::AssetLockTransaction( | ||
| "Asset lock shielded address top-up account not found".to_string(), | ||
| ) | ||
| })?; | ||
| credit_account | ||
| .peek_next_path() | ||
| .map_err(|e| PlatformWalletError::AssetLockTransaction(e.to_string()))? | ||
| }; | ||
| signer.public_key(&path).await.map_err(|e| { | ||
| PlatformWalletError::AssetLockTransaction(format!("signer public_key failed: {e}")) | ||
| })?; | ||
| { | ||
| let credit_account = info | ||
| .core_wallet | ||
| .accounts | ||
| .asset_lock_shielded_address_topup | ||
| .as_mut() | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::AssetLockTransaction( | ||
| "Asset lock shielded address top-up account not found".to_string(), | ||
| ) | ||
| })?; | ||
| credit_account | ||
| .mark_first_pool_index_used(index) | ||
| .map_err(|e| PlatformWalletError::AssetLockTransaction(e.to_string()))?; |
There was a problem hiding this comment.
🔴 Blocking: Roll back reservations whenever a signed transaction is abandoned before broadcast
At the pinned key-wallet revision, assemble_unsigned reserves the selected inputs and build_signed releases them only when input signing itself fails. Once line 427 succeeds, the custom selected-account path can still return before exposing the transaction: the credit-account lookup and peek_next_path are written as fallible operations, the external signer.public_key callback can genuinely fail, and the second account lookup plus mark_first_pool_index_used are also propagated with ?. The preceding funding-address preflight makes the account/pool failures unlikely under current invariants, but a transient resolver or Keychain failure at line 457 is directly reachable and leaves the transaction unbroadcast and untracked. The same reservation lifecycle also affects the pinned non-shielded build_asset_lock_with_signer call at lines 169-178, whose dependency implementation performs credit-key callbacks after build_signed, and the invitation durability branch at lines 1077-1090 can abandon an already-built transaction when pool persistence or flush() fails. Existing cleanup covers only input-signing failure inside key-wallet and definitive BroadcastError::Rejected at lines 1126-1150; accepted or MaybeSent transactions correctly retain reservations. Add rollback coverage around the complete post-signing, pre-broadcast phase, including the upstream builder path, so every error that definitively abandons the transaction immediately releases its inputs.
source: ['codex']
| .set_funding(selected, &bip44_acc) | ||
| .set_change_address(change_addr) |
There was a problem hiding this comment.
🔴 Blocking: Pass the selected Standard account's xpub to set_funding
TransactionBuilder::set_funding immediately calls funds_acc.next_change_address(Some(&acc.account_xpub), true) before the following set_change_address override. Here funds_acc is the account selected by funding_path, but acc is always the BIP44 change account. This is harmless for non-Standard accounts because their change derivation fails before mutation, and correct for the default BIP44 account, but it is wrong for an explicitly selected Standard BIP32 account. Once that BIP32 account has no pre-generated unused internal address, key-wallet derives [1, index] from the BIP44 xpub while recording the address under the BIP32 account's full path, inserts it into the BIP32 pool, and bumps the monitor revision. Overriding this transaction's change output does not undo that mutation; a later normal BIP32 send can use the poisoned pool entry as change and record a derivation path whose signer key does not match the address. Resolve the wallet Account corresponding to the selected Standard account and pass its xpub to set_funding, while retaining the separate BIP44 set_change_address override.
source: ['codex']
There was a problem hiding this comment.
Resolved in 155d34b — Pass the selected Standard account's xpub to set_funding no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| Picker("Core Account", selection: $fundingCoreAccountIndex) { | ||
| Text("Select…").tag(Optional<UInt32>.none) | ||
| ForEach(options, id: \.accountIndex) { opt in | ||
| Text("Account #\(opt.accountIndex) — \(formatDuffs(opt.balanceDuffs))") | ||
| ForEach(options) { opt in | ||
| Text("\(opt.typeLabel) #\(opt.accountIndex) — \(formatDuffs(opt.balanceDuffs))") | ||
| .tag(Optional(opt.accountIndex)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Do not use accountIndex alone as the picker selection value
CoreAccountOption.id correctly distinguishes BIP44, BIP32, CoinJoin, and DashPay rows for ForEach, but every picker row is still tagged with only accountIndex. These account families commonly reuse indices, so BIP44 #0, BIP32 #0, and CoinJoin #0 all represent the same selection value; selectedCoreAccountBalanceDuffs likewise returns the first row with that index. SwiftUI therefore cannot preserve or display which row the user chose. If a non-Standard row has an index without a corresponding BIP44 account, submission passes that index as the mandatory BIP44 change sink and Rust rejects the operation. This does not silently change the input source—the separate fundingPath still controls that—but it makes the picker ambiguous and can select an invalid change account. Keep the picker restricted to BIP44 change accounts and display other balances separately, or bind selection to a composite account identity while maintaining a distinct BIP44 change-account selection.
source: ['codex']
| /// Builds a testnet wallet manager whose balance is SPLIT across BIP44 account | ||
| /// 0 (`bip44_duffs`) and a DashPay funds account (`dashpay_duffs`) — the | ||
| /// DashPay analogue of [`split_funded_wallet_manager`]'s BIP44 + CoinJoin split. | ||
| /// `leg` selects which DashPay account type carries the mixed slice. | ||
| /// | ||
| /// This exercises the DashPay legs of the vendored asset-lock router fix that | ||
| /// the CoinJoin fixture does not reach: `get_relevant_account_types(AssetLock)` | ||
| /// covers `CoinJoin`, `DashpayReceivingFunds`, AND `DashpayExternalAccount`, so | ||
| /// an asset lock funded from a DashPay UTXO must have that input debited by the | ||
| /// `check_core_transaction` scan (dashpay/platform#4073, dashpay/dash-wallet#1507). | ||
| /// | ||
| /// `WalletAccountCreationOptions::Default` does not create DashPay accounts, so | ||
| /// this provisions one — identity ids are arbitrary-but-distinct test vectors — | ||
| /// on BOTH the signing `Wallet` and the `ManagedWalletInfo`, derives a fresh | ||
| /// receive address from its single pool (registering it so the checker | ||
| /// recognizes the funding), and funds it, mirroring how | ||
| /// [`split_funded_wallet_manager`] funds the CoinJoin account. | ||
| /// | ||
| /// Signability of the DashPay input matches production per arm (see the inline | ||
| /// note in the body): the `ReceivingFunds` account is derived from our own | ||
| /// seed and is signable end-to-end; the `ExternalAccount` account is watch-only | ||
| /// (its xpub is a contact's, from a foreign seed), so the local signer CANNOT | ||
| /// sign its UTXOs — the asset-lock builder must exclude them. | ||
| /// An account-level xpub whose private keys the wallet under test does NOT | ||
| /// hold — derived from a SEPARATE random wallet. Models a DashPay contact's | ||
| /// decrypted xpub, from which production builds the watch-only | ||
| /// `DashpayExternalAccount` (`is_watch_only: true`, | ||
| /// `wallet/identity/network/contacts.rs`). Any well-formed testnet account | ||
| /// xpub serves as a single-pool account key; using a FOREIGN one makes the | ||
| /// account unsignable by the local seed exactly as it is in production, so an | ||
| /// asset-lock builder that (wrongly) selected its UTXOs would sign them with | ||
| /// the local mnemonic's key and produce an invalid input signature. | ||
| fn foreign_contact_account_xpub() -> ExtendedPubKey { | ||
| let foreign = TestWalletContext::new_random(); | ||
| foreign | ||
| .wallet | ||
| .accounts | ||
| .standard_bip44_accounts | ||
| .get(&0) | ||
| .expect("foreign wallet has BIP44 account 0") | ||
| .account_xpub | ||
| } | ||
|
|
||
| pub(crate) async fn split_funded_wallet_manager_dashpay( |
There was a problem hiding this comment.
💬 Nitpick: Move the DashPay fixture documentation back to its target
The rustdoc beginning with “Builds a testnet wallet manager whose balance is SPLIT” describes split_funded_wallet_manager_dashpay, but the immediately following item is foreign_contact_account_xpub, so Rust attaches the entire contiguous block to that helper. The helper is consequently documented as constructing a funded wallet manager, while the actual fixture at line 425 has no rustdoc. Keep only the foreign-xpub paragraphs above foreign_contact_account_xpub and move the fixture-purpose paragraphs directly above split_funded_wallet_manager_dashpay.
source: ['codex']
There was a problem hiding this comment.
Resolved in 155d34b — Move the DashPay fixture documentation back to its target no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
review blocker 2) Reviewer (thepastaclaw) blocker 2 — build.rs:423-424: build_asset_lock_tx_from_selected_account passed the BIP44 change account (&bip44_acc) as set_funding's `acc`, but set_funding calls funds_acc.next_change_address(Some(&acc.account_xpub)) on the SELECTED funds account before the set_change_address override. For an explicitly selected Standard BIP32 account with no pre-generated unused internal address, that derived [1, index] from the wrong (BIP44) xpub and recorded it under the BIP32 account's own path, poisoning that pool so a later normal BIP32 send could use a change entry whose signer key does not match the address. Now resolve the wallet-level Account whose account-level derivation path equals funding_path and pass ITS xpub to set_funding, while keeping the separate BIP44 set_change_address override. Default BIP44 funding resolves to bip44_acc (unchanged); non-Standard (CoinJoin/DashPay) accounts fail change derivation regardless, so the xpub is immaterial and the bip44_acc fallback preserves prior behavior. Also (thepastaclaw nitpick, test_support.rs): move the DashPay fixture rustdoc so it attaches to split_funded_wallet_manager_dashpay rather than foreign_contact_account_xpub. Blocker 1 (build.rs:427-473, owner-guarded reservation rollback on the pre-broadcast abandonment path) is NOT addressed here: the pinned key-wallet rev 70d4bf8 exposes no owner-guarded release primitive (ReservationSet has only reserve/reserved/release keyed by outpoint; release_reservation is unconditional) and no reservation token, so the required release_if_owner(token) mechanism (rust-dashcore#916 / dashpay#4185) cannot be implemented against this pin without an upstream dependency change — the very atomic-reservation fix this PR is held for. Deferred pending that adoption rather than substituting an unconditional release on the money path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed the xpub fix in
Reservation rollback on pre-broadcast abandonment ( |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commit fixes the normal selected-Standard-account xpub lookup and the misplaced fixture documentation; the focused asset-lock suite passes all 25 tests. One blocking lifecycle issue remains: failures after signing but before returning the transaction strand the selected inputs even though the wallet-manager write guard makes immediate rollback safe. The Swift picker, public API documentation, and missing Standard BIP32 regression coverage remain actionable suggestions; the proposed missing-wallet-account fallback blocker is not reachable under supported construction and restoration invariants.
Validated blockers were found in the Codex precheck. Sonnet 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)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:933-934: Document account_index as the default source and change sink
These opening API lines say unconditionally that `account_index` selects the BIP44 account whose UTXOs fund the lock. When `funding_path` is supplied, inputs instead come exclusively from the named account and `account_index` remains the required BIP44 change sink, as the later paragraph at lines 958-965 correctly explains. The same stale description remains in `PlatformWalletManagerShieldedFunding.swift:110-111` and `PlatformWalletManager.kt:1263`; update all three public surfaces so callers do not confuse the change-account index with the explicit input source.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:411-419: Add a Standard BIP32 regression test for the selected xpub
This lookup fixes a subtle pool-corruption branch that occurs when an explicitly selected Standard BIP32 account needs a newly generated internal address. The focused suite passes 25 tests, but it covers default BIP44 and explicit CoinJoin or DashPay funding, not a selected Standard BIP32 account; reverting this lookup to the BIP44 xpub would leave all current tests green. Add a regression that funds a BIP32 account, exhausts its pre-generated unused internal entries, builds through that account's explicit path, and verifies that any new pool entry is derived from the selected BIP32 xpub rather than the BIP44 change-account xpub.
| let funding_wallet_acc = wallet | ||
| .all_accounts() | ||
| .into_iter() | ||
| .find(|a| { | ||
| a.derivation_path() | ||
| .map(|p| p == funding_path) | ||
| .unwrap_or(false) | ||
| }) | ||
| .unwrap_or(&bip44_acc); |
There was a problem hiding this comment.
🟡 Suggestion: Add a Standard BIP32 regression test for the selected xpub
This lookup fixes a subtle pool-corruption branch that occurs when an explicitly selected Standard BIP32 account needs a newly generated internal address. The focused suite passes 25 tests, but it covers default BIP44 and explicit CoinJoin or DashPay funding, not a selected Standard BIP32 account; reverting this lookup to the BIP44 xpub would leave all current tests green. Add a regression that funds a BIP32 account, exhausts its pre-generated unused internal entries, builds through that account's explicit path, and verifies that any new pool entry is derived from the selected BIP32 xpub rather than the BIP44 change-account xpub.
source: ['codex']
3 participants
Shields asset-lock funding from all funds accounts: a multi-account funding builder (
build_asset_lock_tx_from_all_funding_accounts) that unions spendable UTXOs across BIP44 + CoinJoin + DashPay funds accounts with LargestFirst coin selection, excludes watch-onlyDashpayExternalAccountUTXOs (a contact's coins the local mnemonic cannot sign), and mapsNoUtxosAvailableto the typed asset-lock insufficient-funds error, with regression tests for the router-fix persistence path and the watch-only exclusion.Re-opens #4074 which was auto-closed when the #3999 base branch was deleted; rebased onto
v4.1-dev. The PR's own rust-dashcore router customization stays dropped —v4.1-dev's rust-dashcore pin already carries that fix upstream (rust-dashcore#867), which the ported regression tests confirm against the pin. The platform-side changes inrs-platform-walletare NOT inv4.1-devand are all retained; the only conflict was a trivial import union intest_support.rs.Verified:
cargo test -p platform-wallet— 502 tests pass (38 asset-lock).🤖 Generated with Claude Code
Review-response summary (2026-07-21)
AssetLockInsufficientFundsnow crosses as dedicated code 29 (ErrorAssetLockInsufficientFunds), mapped in Kotlin and Swift, with an FFI-level test pinning the numeric code and verbatim message. The Display text is unchanged from what dash-wallet already matches, so no host breakage; hosts should migrate from substring-matching to the typed code at their convenience. Codes 26–28 are deliberately skipped — they're allocated by the reservation-token errors on the split-build-broadcast branch (feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4185); a comment in the enum documents the reservation so the two PRs can't collide.allow_cross_domain, default false everywhere, including the resume path); refusal is typed code 30 (ErrorAssetLockCrossDomainConsentRequired) carrying transparent/union/required amounts. The identity-funding carve-out is unchanged — consent is ignored there, pinned by test. Round-2 addition: a fee-band shortfall (transparent covers the amount but not amount+fee, union covers it) is reclassified to consent-required at the selection-failure site, so hosts prompt instead of dead-ending; both the fee-band and denied-both-short paths are now tested.cargo test -p platform-wallet --lib→ 506 passed;--features shielded --lib→ 636 passed;-p platform-wallet-ffi --lib→ 198 passed; clippy clean on all three crates.allowCrossDomain = trueafter explicit user opt-in; dash-wallet will need a consent touchpoint before adopting the next AAR.Summary by CodeRabbit
New Features
Bug Fixes
Provenance (tracker refs moved from code comments per review)
The
build.rsfunding-eligibility comments previously carried an internal review-tracking tokenfinding 5b52d9844055(4 sites). Removed from the comments (rationale text kept); it tracked the watch-onlyDashpayExternalAccountownership carve-out now expressed through the privacy-domain map.C-ABI note
platform_wallet_manager_shielded_fund_from_asset_lockgained a trailingallow_cross_domain: boolparameter — a C-ABI break — and result codes 29/30 (ErrorAssetLockInsufficientFunds/ErrorAssetLockCrossDomainConsentRequired) are added. Seepackages/rs-platform-wallet-ffi/README.md.