fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073)#4184
Open
bfoss765 wants to merge 10 commits into
Open
fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073)#4184bfoss765 wants to merge 10 commits into
bfoss765 wants to merge 10 commits into
Conversation
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
July 21, 2026 12:41
Contributor
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Collaborator
|
🕓 Ready for review — 5 ahead in queue (commit e6da504) |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 47 minutes. |
Collaborator
|
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. |
Collaborator
|
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. |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 21, 2026
…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>
Contributor
Author
|
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 |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 35 minutes. |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 21, 2026
…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>
Collaborator
|
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 |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 22, 2026
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>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 22, 2026
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>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 22, 2026
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>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 22, 2026
`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}
Contributor
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
Contributor
Author
|
Addressed the independent findings; the reservation race stays held for rust-dashcore#912.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
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>, |
Contributor
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>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 23, 2026
…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>
bfoss765
force-pushed
the
port/v4.1/assetlock-multi-account
branch
from
July 23, 2026 01:12
dfc670d to
bfb1f49
Compare
Collaborator
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.