fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073) - #4184
Open
bfoss765 wants to merge 16 commits into
Open
fix: shield asset-lock funding from all funds accounts incl. CoinJoin (#4073)#4184bfoss765 wants to merge 16 commits into
bfoss765 wants to merge 16 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: 55 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 (22)
📝 WalkthroughWalkthroughShielded asset-lock funding now supports an optional BIP32 funding path that restricts UTXO selection to one account. Typed insufficient-funds errors propagate through wallet, FFI, JNI, Kotlin, and Swift APIs. The Swift example app accepts the path for Core funding. ChangesAsset-lock funding path
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
Collaborator
|
✅ Final review complete — no blockers (commit 59d3dbf) |
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
… 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>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 25, 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>
…adcast path build_signed reserves the selected inputs in the funding account's ReservationSet and normally holds them from build through broadcast. But several post-signing, pre-broadcast error paths could return without rolling that reservation back, stranding the inputs until the reservation-TTL backstop (~24 blocks) and causing false shortfalls / withheld coins in the meantime (dashpay#4184 review, thepastaclaw): - build_asset_lock_tx_from_selected_account (shielded selected-account path): the credit-account lookups, peek_next_path, the external signer.public_key round-trip, and mark_first_pool_index_used are all fallible after build_signed. Derive the credit key in an inner future and, on any error, release the reservation on the selected funds account (matched by funding_path, mirroring release_asset_lock_funding_reservation) before propagating. - build_asset_lock_transaction (non-shielded / upstream builder path): the two defensive credit-key arms after build_asset_lock_with_signer abandon a signed+reserved tx; release on the BIP44 account at account_index. - broadcast_funded_asset_lock invitation durability branch: when the funding-index persist/flush fails we abort before broadcast; release the reservation via release_asset_lock_funding_reservation before bailing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 26, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commit correctly releases reservations on the selected-account post-signing failures and invitation durability aborts, and all 25 focused asset-lock tests pass. However, the delegated non-shielded builder can still fail after reserving its BIP44 inputs, so the reservation-lifecycle blocker remains. The Swift picker ambiguity, contradictory public parameter documentation, and missing Standard BIP32 regression coverage also remain actionable.
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 | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
3 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 state 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, while `account_index` remains the required BIP44 change sink; lines 958-965 document that behavior correctly. The same contradictory summaries remain in `PlatformWalletManagerShieldedFunding.swift:110-111` and `PlatformWalletManager.kt:1263`. Align all three public surfaces so callers do not confuse the change-account index with an explicit input source.
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 30, 2026
… funding build_signed_payment funded from the union of all signable funds accounts (BIP44 + CoinJoin + …) with BIP44 change — the privacy-domain-crossing design blocked on dashpay#4184 (shumkov, 2026-07-21) and replaced there by single-account selection. This code predated that re-scope. - add funding_path: Option<DerivationPath>; None = unmixed BIP44 account 0, Some(path) = strictly the one funds account whose account path matches. No union, no cross-account accumulation; shortfall returns PaymentInsufficientFunds for that account only. Change routes to BIP44 (explicit change addr when a non-Standard account funds). - new wallet::funding_privacy guardrail: crate-wide static test fails the build if any wallet-wide funds-account iteration lacks a PRIVACY-DOMAIN-OK marker. - replace the union-asserting test with default-never-crosses-domains and explicit-path-selects-strictly tests. - review-fix hardening: bounded FFI allocation + checked cursor math, output-total overflow guard, fee-rate bound, typed PaymentInsufficientFunds (code 22). - thread funding_path through FFI/JNI/Kotlin (null = unmixed BIP44). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 31, 2026
…ts for the send hardening Addresses the review findings on dashpay#4247. Dust outputs (BLOCKING). `TransactionBuilder::add_output` applies no relay policy, so a one-duff recipient produced fully signed bytes for a transaction every standard node rejects as nonstandard — from a primitive documented as building a *standard* payment for later broadcast. Each recipient amount is now checked against its OWN destination script's `dust_value()` (546 duffs for P2PKH), before the wallet lock, before any input is reserved and before the signer is called. Fee/size overflow (BLOCKING). The `MAX_FEE_PER_KB = MAX_MONEY / 100` bound assumed the transaction stayed under the 100 kB standard limit, which the method never enforced; key-wallet's `calculate_fee` then multiplies `sat_per_kb * size_bytes` unchecked and overflows at ~878 kB — reachable both by a ~25.8k-recipient list and by a funding account with a few thousand small denominations. Two-sided fix: * the recipient count is bounded at build time against `MAX_STANDARD_TX_SIZE` (derived from `dashcore::policy::MAX_STANDARD_TX_WEIGHT / 4`), with checked arithmetic mirroring key-wallet's own base-size formula; * `MAX_FEE_PER_KB` is re-derived as `u64::MAX / u32::MAX`, which makes the product unrepresentable-free for ANY size a `u32` can express and therefore does not depend on the input count. ~43 DASH/kB is still three orders of magnitude above any legitimate rate; * the signed transaction is re-measured and refused if it exceeds the standard limit. Typed build errors. `PlatformWalletError::TransactionBuild` had no FFI arm, so every `funding_path` failure — "no spendable funds account matches" and "names a watch-only account", the two failure modes the single-account design rests on — reached Kotlin as `Generic(99)` and could only be told apart by string-matching. Adds `ErrorTransactionBuild = 32` (27-31 are claimed by sibling v4.1 stack PRs, so this needs no renumbering whichever order they land) plus the Kotlin `PlatformWallet.TransactionBuild` type. Tests for the six hardening fixes, which shipped with no coverage. `core_wallet/send.rs` had no test module at all; it now covers the `count` bound, `try_reserve_exact`, checked cursor math at every field boundary, UTF-8, and wrong-network address rejection. Also covers MAX_MONEY aggregation, the fee-rate bound, `parse_optional_derivation_path`, dust rejection (including that a refused request reserves nothing), and the new size bound. Also: corrects the stale `PaymentInsufficientFunds` doc, which still described the pre-dashpay#4184 union semantics; corrects an inverted fee-direction comment; adds the missing non-empty assertion to a funding-privacy guardrail test; and runs `cargo fmt` over the five files that failed `--check`. platform-wallet 510 passed, platform-wallet-ffi 222 passed, 0 failed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 31, 2026
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Jul 31, 2026
…s the funding path `build_signed_payment` / `finalize_signed_payment_from_funding_path` resolve the funding account twice: once in `wallet.all_accounts()` for the xpub `set_funding` derives change from, and once in the managed-account collection for the UTXOs and the reservation ledger. The first lookup fell back to the BIP44 account when it found no match, while the second can still resolve a CoinJoin or DashPay receival account — so a disagreement between them silently handed `set_funding` another account's xpub and recorded a change entry derived from it into the funding account's address pool. That is the same silent-fallback shape dashpay#4184 removed from the selector itself. Refusing is the only safe answer: the two lookups disagreeing is a wallet-state bug, not something to paper over with BIP44. Verified not to narrow any real path before changing it: `all_accounts()` does enumerate CoinJoin and DashPay receiving-funds accounts, so the whole send suite — including the receival and explicit-CoinJoin tests — passes with the fallback removed. It was dead code on every exercised path. Raised by shumkov (dashpay#4247) and CodeRabbit (dashpay#4256). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 1, 2026
…ode (33) `map_send_builder_error` folded `BuilderError::SigningFailed` into `TransactionBuild` → native code 32, whose documented contract is "the request itself is at fault; a verbatim retry fails identically". That is false for the production `MnemonicResolverCoreSigner`: a locked or missing Keychain mnemonic surfaces as `SigningFailed`, and key-wallet `release_if_owner`-releases the build's owner-stamped input reservation before returning, so the identical recipients/amount/fee/funding path succeed once the signer is usable. Hosts were being told to make the user edit a payment that was never wrong. Adds `PlatformWalletError::TransactionSigning` → `ErrorTransactionSigning = 33` → `DashSdkError.PlatformWallet.TransactionSigning` (isRetryable = true, matching the ShieldedNoRecordedAnchor convention for "nothing committed, reservations released, retry once the precondition is met"). Code choice — 33, not the reviewer's suggested 31. 27-32 are all claimed across the sibling v4.1 stack (27/28 dashpay#4185, 29 dashpay#4184, 30 reserved for dashpay#4184's cross-domain-consent, 31 dashpay#4183, 32 dashpay#4247), so 33 is the first slot free on every branch. 31 IS reserved, but for a different contract: dashpay#4183's `ErrorSigningKeyUnavailable` is a state-transition failure asserting the signer holds no usable private key for a requested public key, restored from the typed `DashSDKSignerErrorCode::SigningKeyUnavailable`. This is a Core L1 input signing failure with no such provenance — `BuilderError::SigningFailed` also covers an unresolved input derivation path, a sighash computation failure and a malformed signature encoding — so reusing 31 would assert "the key is unavailable" for failures that are nothing of the kind. Kept separate so neither contract is weakened; the rationale is recorded on the variant for maintainers reconciling the range. Tests: an end-to-end locked-signer build asserting TransactionSigning (not TransactionBuild), a retry-after-recovery test proving the inputs really were released, FFI code/mapping tests, and the Kotlin decode + retry-contract test. platform-wallet 539 passed, platform-wallet-ffi 225 passed, kotlin-sdk 190 passed; fmt clean, no new clippy warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Aug 1, 2026
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 1, 2026
…30 (dashpay#4185 review) Code 29 collided with `ErrorAssetLockInsufficientFunds` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and this PR moves to 30. Verified 30 was genuinely free by reading `rs-platform-wallet-ffi/src/error.rs` at the head of all 62 open PRs: no PR defines a code 30. The `ErrorAssetLockCrossDomainConsentRequired` that in-tree comments name as 30's holder does not exist anywhere after dashpay#4184's re-scope. The discriminant is public ABI, so every mirror moves together: - Rust enum + its three rustdoc cross-references (error.rs) - two doc references in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value + doc - Kotlin fromPlatformWalletNative branch, class KDoc, code-98 comment, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also disarms the NativeCleaner backstop in SignedCoreTransactionTest by closing the SignedCoreTransaction, so the armed native release cannot fire from the cleaner thread in a pure-JVM test. Note: dashpay#4256 is stacked downstream and still carries the pre-renumber 29; it must adopt 30 on rebase.
…on window Addresses the outstanding review findings on dashpay#4184 at a9e418a. Reservation lifecycle, delegated (non-shielded) builder — blocker. `build_asset_lock_with_signer` reserves the transaction's inputs inside `build_signed` and rolls that back only when *input* signing fails. Its credit-output loop runs afterwards, and a failure there returns `Err` with a fully-signed transaction abandoned and its inputs still reserved. That reservation cannot be rolled back from platform-wallet at this pin: the error carries no transaction to hand `release_reservation`, and the account's `ReservationSet` is `pub(crate)` to key-wallet. The abandon path is therefore removed rather than rolled back. Of the loop's fallible steps only the external `signer.public_key` round-trip is genuinely reachable — resolving the credit account, `peek_next_path` and `mark_first_pool_index_used` cannot fail once `peek_next_funding_address` has resolved and peeked that same account earlier in the call under the same held wallet write lock. So the signer round-trip now happens up front, before anything is reserved, and the builder receives a `PrefetchedCreditKeySigner` that answers the repeat request from cache. `peek_next_funding_address` returns the peeked path alongside the address to feed it. Standard BIP32 regression coverage. Adds the test the reviewer asked for around the `set_funding` xpub lookup: it exhausts a selected BIP32 account's pre-generated unused internal entries so `next_change_address` must derive a fresh one, builds through that account's explicit path, and asserts every internal-pool entry is still signable at its own recorded path. Reverting the lookup to the BIP44 xpub fails it. SwiftExampleApp funding picker. Picker rows were all tagged with a bare `accountIndex`, which account families reuse — BIP44 #0, BIP32 #0 and CoinJoin #0 collided, so SwiftUI could not tell which row was chosen and a non-BIP44 row without a BIP44 counterpart fed an invalid change account to Rust. The picker is now restricted to BIP44 accounts (the mandatory change sink, listed even at zero balance so the dashpay#4073 case still works) and the other funds accounts are shown read-only. Public API documentation. `account_index` / `fundingAccountIndex` is documented as the change sink that doubles as the default input source, rather than unconditionally as the input source, across the Rust FFI, Swift and Kotlin surfaces — it contradicted the `funding_path` docs immediately below it. Tests: platform-wallet 511/511 (2 new), platform-wallet-ffi 205/205 + 26 + 6; cargo fmt applied; clippy clean on both crates (only pre-existing warnings in recovery.rs / withdrawal.rs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet-ffi/src/shielded_send.rs (1)
131-164: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd unit tests for the two new funding-path parsing gates. Both
parse_optional_derivation_pathandread_cstring_opt_strictare new validation boundaries for the same money-source parameter (funding_path/fundingPath), and neither has a dedicated test.
packages/rs-platform-wallet-ffi/src/shielded_send.rs#L131-L164: add tests forparse_optional_derivation_pathcovering null pointer, zero length, a valid path (e.g."m/44'/5'/0'"), invalid UTF-8, and a malformed path string.packages/rs-unified-sdk-jni/src/funding.rs#L261-L300: add tests forread_cstring_opt_strictcovering null string, empty string, a valid path string, and a string with an interior NUL, and confirm it throws (rather than silently returningNone) on a genuine JNI read failure.🤖 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 131 - 164, Add dedicated unit tests for parse_optional_derivation_path in packages/rs-platform-wallet-ffi/src/shielded_send.rs (lines 131-164), covering null pointers, zero length, a valid BIP32 path, invalid UTF-8, and malformed paths. Add dedicated tests for read_cstring_opt_strict in packages/rs-unified-sdk-jni/src/funding.rs (lines 261-300), covering null and empty strings, a valid path, interior NUL rejection, and propagation of genuine JNI read failures instead of returning None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- Around line 131-164: Add dedicated unit tests for
parse_optional_derivation_path in
packages/rs-platform-wallet-ffi/src/shielded_send.rs (lines 131-164), covering
null pointers, zero length, a valid BIP32 path, invalid UTF-8, and malformed
paths. Add dedicated tests for read_cstring_opt_strict in
packages/rs-unified-sdk-jni/src/funding.rs (lines 261-300), covering null and
empty strings, a valid path, interior NUL rejection, and propagation of genuine
JNI read failures instead of returning None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aef95731-f83a-43ab-82e2-c555c87398e1
📒 Files selected for processing (21)
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/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/test_support.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.swift
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/rs-platform-wallet-ffi/src/asset_lock/build.rs
- packages/rs-platform-wallet/src/error.rs
- packages/rs-platform-wallet/src/wallet/identity/network/registration.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
- packages/rs-platform-wallet/src/wallet/identity/network/invitation.rs
- packages/rs-platform-wallet/src/wallet/platform_addresses/fund_from_asset_lock.rs
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.kt
- packages/rs-platform-wallet/src/wallet/shielded/seed_pool.rs
- packages/rs-platform-wallet/src/wallet/asset_lock/sync/recovery.rs
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedFunding.swift
- packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs
- packages/rs-platform-wallet/src/test_support.rs
- packages/rs-platform-wallet/src/wallet/asset_lock/build.rs
thepastaclaw
suggested changes
Aug 1, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes all four indexed prior findings: delegated credit-key callbacks now occur before reservation, the Standard BIP32 xpub regression is exercised, the Swift picker keeps only BIP44 change sinks selectable, and Rust/Swift/Kotlin documentation agrees. The focused asset-lock suite passes 27 tests and the platform-wallet-ffi suite passes 205 tests, but the PR is not mergeable because cargo fmt --check --all fails in the JNI bridge and the exact wallet CI clippy command fails on five PR-added test helpers compiled outside cfg(test). Two additional test-coverage suggestions remain for the prefetched signer's cache-hit invariant and the funding-path parser.
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)
🤖 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-unified-sdk-jni/src/funding.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/funding.rs:415-420: Run rustfmt on the JNI funding-path bridge
The repository-enforced `cargo fmt --check --all` command fails on this block. This makes the current head fail the wallet and workspace formatting jobs even though the code compiles. Apply rustfmt's replacement before merging.
In `packages/rs-platform-wallet/src/test_support.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/test_support.rs:290-585: Gate the new asset-lock fixtures to test builds
The exact wallet CI command, `cargo clippy --package platform-wallet --package platform-wallet-storage --package platform-wallet-ffi --package rs-unified-sdk-ffi --package rs-unified-sdk-jni --all-features --locked -- --no-deps -D warnings`, fails with `dead_code` errors for `split_funded_wallet_manager`, `DashpayLeg`, `foreign_contact_account_xpub`, `split_funded_wallet_manager_dashpay`, and `split_funded_wallet_manager_many_coinjoin`. These `pub(crate)` helpers are referenced only by the `#[cfg(test)]` asset-lock module, while `test_support` itself is compiled into the normal library target. Gate these items and their helper-only imports with `#[cfg(test)]` so the production target remains warning-free while unit tests still compile them.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:2896-2935: Exercise the prefetched signer's post-reservation cache hit
`CreditKeyFailingSigner` fails its first and every subsequent `public_key` request, so this test exits during the new up-front prefetch before the delegated builder reserves or signs anything. It proves that a prefetch failure is reservation-free, but it does not protect the load-bearing behavior in `PrefetchedCreditKeySigner`: the delegated builder must request exactly the prefetched path and receive it from cache without invoking the underlying signer again. Add a separate signer whose first `public_key` call succeeds and every later call fails, then assert that the delegated build succeeds and the underlying call count is exactly one. That regression would fail if the prefetch and delegated paths diverged or the wrapper accidentally delegated the cached request.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:140-163: Pin the funding-path parser's fail-closed behavior
`parse_optional_derivation_path` is a new unsafe C-boundary parser for the account that supplies money, but the existing FFI test module never calls it directly. Its absence/error distinction is important: a null pointer or zero length deliberately selects the default BIP44 source, while invalid UTF-8 or malformed path syntax must return `ErrorInvalidParameter` instead of degrading to that default and potentially selecting different coins. Add focused tests for null, zero length, a valid account-level path, invalid UTF-8, and malformed syntax.
Comment on lines
+415
to
+420
| let (funding_path_ptr, funding_path_len) = funding_path | ||
| .as_ref() | ||
| .map_or((ptr::null(), 0usize), |c| { | ||
| let b = c.as_bytes(); | ||
| (b.as_ptr(), b.len()) | ||
| }); |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Run rustfmt on the JNI funding-path bridge
The repository-enforced cargo fmt --check --all command fails on this block. This makes the current head fail the wallet and workspace formatting jobs even though the code compiles. Apply rustfmt's replacement before merging.
Suggested change
| let (funding_path_ptr, funding_path_len) = funding_path | |
| .as_ref() | |
| .map_or((ptr::null(), 0usize), |c| { | |
| let b = c.as_bytes(); | |
| (b.as_ptr(), b.len()) | |
| }); | |
| let (funding_path_ptr, funding_path_len) = | |
| funding_path.as_ref().map_or((ptr::null(), 0usize), |c| { | |
| let b = c.as_bytes(); | |
| (b.as_ptr(), b.len()) | |
| }); |
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 346f2cb — Run rustfmt on the JNI funding-path bridge 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.
Comment on lines
+290
to
+585
| pub(crate) async fn split_funded_wallet_manager( | ||
| bip44_duffs: u64, | ||
| coinjoin_duffs: u64, | ||
| ) -> ( | ||
| Arc<RwLock<WalletManager<PlatformWalletInfo>>>, | ||
| WalletId, | ||
| WalletSigner, | ||
| ) { | ||
| use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; | ||
|
|
||
| let mut ctx = TestWalletContext::new_random(); | ||
|
|
||
| // Fund BIP44 account 0 (the primary) at its pre-derived receive address. | ||
| let bip44_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[bip44_duffs]); | ||
| let bip44_result = ctx | ||
| .check_transaction( | ||
| &bip44_tx, | ||
| TransactionContext::InChainLockedBlock(BlockInfo::new( | ||
| 1, | ||
| BlockHash::all_zeros(), | ||
| 1_700_000_000, | ||
| )), | ||
| ) | ||
| .await; | ||
| assert!( | ||
| bip44_result.is_relevant && bip44_result.is_new_transaction, | ||
| "BIP44 funding tx should be recognized" | ||
| ); | ||
|
|
||
| // Derive a fresh CoinJoin receive address (registering it in the CoinJoin | ||
| // pool so the checker recognizes the funding), then fund CoinJoin account 0. | ||
| let coinjoin_xpub = ctx | ||
| .wallet | ||
| .get_coinjoin_account(0) | ||
| .expect("default wallet has CoinJoin account 0") | ||
| .account_xpub; | ||
| // CoinJoin is a single-pool (non-standard) account, so it derives via | ||
| // `next_address` rather than `next_receive_address`. | ||
| let coinjoin_address = ctx | ||
| .managed_wallet | ||
| .first_coinjoin_managed_account_mut() | ||
| .expect("default wallet has a managed CoinJoin account 0") | ||
| .next_address(Some(&coinjoin_xpub), true) | ||
| .expect("CoinJoin receive address"); | ||
| let coinjoin_tx = Transaction::dummy(&coinjoin_address, 0..1, &[coinjoin_duffs]); | ||
| let coinjoin_result = ctx | ||
| .check_transaction( | ||
| &coinjoin_tx, | ||
| TransactionContext::InChainLockedBlock(BlockInfo::new( | ||
| 2, | ||
| BlockHash::all_zeros(), | ||
| 1_700_000_100, | ||
| )), | ||
| ) | ||
| .await; | ||
| assert!( | ||
| coinjoin_result.is_relevant && coinjoin_result.is_new_transaction, | ||
| "CoinJoin funding tx should be recognized" | ||
| ); | ||
|
|
||
| let signer = WalletSigner { | ||
| wallet: ctx.wallet.clone(), | ||
| }; | ||
|
|
||
| let balance = Arc::new(WalletBalance::new()); | ||
| let info = PlatformWalletInfo { | ||
| core_wallet: ctx.managed_wallet, | ||
| balance, | ||
| identity_manager: IdentityManager::new(), | ||
| tracked_asset_locks: BTreeMap::new(), | ||
| }; | ||
|
|
||
| let mut wm = WalletManager::<PlatformWalletInfo>::new(Network::Testnet); | ||
| let wallet_id = wm.insert_wallet(ctx.wallet, info).expect("insert wallet"); | ||
|
|
||
| (Arc::new(RwLock::new(wm)), wallet_id, signer) | ||
| } | ||
|
|
||
| /// Which DashPay funds account arm a [`split_funded_wallet_manager_dashpay`] | ||
| /// fixture provisions. The two arms differ only in which collection they land | ||
| /// in and the DIP-15 derivation order (user/friend vs friend/user), but both | ||
| /// are fund-bearing and both are covered by the vendored asset-lock router fix | ||
| /// (`get_relevant_account_types(AssetLock)` lists `DashpayReceivingFunds` AND | ||
| /// `DashpayExternalAccount` alongside `CoinJoin`). | ||
| #[derive(Clone, Copy, Debug)] | ||
| pub(crate) enum DashpayLeg { | ||
| /// Incoming DashPay funds account (`user_id/friend_id`). | ||
| ReceivingFunds, | ||
| /// DashPay external (watch-only-style) account (`friend_id/user_id`). | ||
| ExternalAccount, | ||
| } | ||
|
|
||
| /// 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 | ||
| } | ||
|
|
||
| /// 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. | ||
| pub(crate) async fn split_funded_wallet_manager_dashpay( | ||
| bip44_duffs: u64, | ||
| dashpay_duffs: u64, | ||
| leg: DashpayLeg, | ||
| ) -> ( | ||
| Arc<RwLock<WalletManager<PlatformWalletInfo>>>, | ||
| WalletId, | ||
| WalletSigner, | ||
| ) { | ||
| use key_wallet::account::account_collection::DashpayAccountKey; | ||
| use key_wallet::managed_account::managed_account_trait::ManagedAccountTrait; | ||
| use key_wallet::wallet::managed_wallet_info::managed_account_operations::ManagedAccountOperations; | ||
| use key_wallet::AccountType; | ||
|
|
||
| let mut ctx = TestWalletContext::new_random(); | ||
|
|
||
| // Fund BIP44 account 0 (the primary) at its pre-derived receive address. | ||
| let bip44_tx = Transaction::dummy(&ctx.receive_address, 0..1, &[bip44_duffs]); | ||
| let bip44_result = ctx | ||
| .check_transaction( | ||
| &bip44_tx, | ||
| TransactionContext::InChainLockedBlock(BlockInfo::new( | ||
| 1, | ||
| BlockHash::all_zeros(), | ||
| 1_700_000_000, | ||
| )), | ||
| ) | ||
| .await; | ||
| assert!( | ||
| bip44_result.is_relevant && bip44_result.is_new_transaction, | ||
| "BIP44 funding tx should be recognized" | ||
| ); | ||
|
|
||
| // Provision a DashPay funds account of the requested arm on the wallet and | ||
| // mirror it into the managed side. The identity ids are arbitrary distinct | ||
| // test vectors; distinct ids keep the receiving (user/friend) and external | ||
| // (friend/user) derivations on different keys/paths. | ||
| let user_identity_id = [0x11u8; 32]; | ||
| let friend_identity_id = [0x22u8; 32]; | ||
| let account_type = match leg { | ||
| DashpayLeg::ReceivingFunds => AccountType::DashpayReceivingFunds { | ||
| index: 0, | ||
| user_identity_id, | ||
| friend_identity_id, | ||
| }, | ||
| DashpayLeg::ExternalAccount => AccountType::DashpayExternalAccount { | ||
| index: 0, | ||
| user_identity_id, | ||
| friend_identity_id, | ||
| }, | ||
| }; | ||
| // Provision the DashPay funds account of the requested arm, matching how | ||
| // production derives each so the local signer's capability is faithful: | ||
| // | ||
| // * `ReceivingFunds` is OURS. Production derives it from our own | ||
| // friendship xpub (`register_contact_account`, `is_watch_only: false`), | ||
| // so the local seed CAN sign it. `add_account(_, None)` models that by | ||
| // deriving the account from this wallet's own root xpriv. | ||
| // | ||
| // * `ExternalAccount` is the CONTACT's, WATCH-ONLY. Production builds it | ||
| // from the contact's decrypted xpub (`register_dashpay_external_account`, | ||
| // `is_watch_only: true`), whose private keys live under a DIFFERENT seed | ||
| // the wallet does not hold. Model that faithfully: derive the account | ||
| // xpub from a SEPARATE random wallet and insert it via | ||
| // `add_account(_, Some(xpub))`, which stores the account | ||
| // `is_watch_only: true`. The old shortcut — `add_account(_, None)` for | ||
| // BOTH arms — derived the external account from our OWN seed, making it | ||
| // locally signable and MASKING the union-funding bug (an asset lock | ||
| // would silently spend the contact's coins with a wrong-key, invalid | ||
| // signature). This arm now proves the builder excludes it. | ||
| match leg { | ||
| DashpayLeg::ReceivingFunds => { | ||
| ctx.wallet | ||
| .add_account(account_type, None) | ||
| .expect("add DashPay receiving account to wallet"); | ||
| } | ||
| DashpayLeg::ExternalAccount => { | ||
| let foreign_xpub = foreign_contact_account_xpub(); | ||
| ctx.wallet | ||
| .add_account(account_type, Some(foreign_xpub)) | ||
| .expect("add watch-only DashPay external account to wallet"); | ||
| } | ||
| } | ||
| ctx.managed_wallet | ||
| .add_managed_account(&ctx.wallet, account_type) | ||
| .expect("mirror DashPay account into managed wallet"); | ||
|
|
||
| // Derive a fresh DashPay receive address from the single-pool managed | ||
| // account, then fund it. DashPay accounts are single-pool (like CoinJoin), | ||
| // so they derive via `next_address` rather than `next_receive_address`. | ||
| let key = DashpayAccountKey { | ||
| index: 0, | ||
| user_identity_id, | ||
| friend_identity_id, | ||
| }; | ||
| let dashpay_xpub = match leg { | ||
| DashpayLeg::ReceivingFunds => ctx.wallet.accounts.dashpay_receival_accounts.get(&key), | ||
| DashpayLeg::ExternalAccount => ctx.wallet.accounts.dashpay_external_accounts.get(&key), | ||
| } | ||
| .expect("DashPay account present in wallet") | ||
| .account_xpub; | ||
| let dashpay_address = { | ||
| let managed = match leg { | ||
| DashpayLeg::ReceivingFunds => ctx | ||
| .managed_wallet | ||
| .accounts | ||
| .dashpay_receival_accounts | ||
| .get_mut(&key), | ||
| DashpayLeg::ExternalAccount => ctx | ||
| .managed_wallet | ||
| .accounts | ||
| .dashpay_external_accounts | ||
| .get_mut(&key), | ||
| } | ||
| .expect("managed DashPay account present"); | ||
| managed | ||
| .next_address(Some(&dashpay_xpub), true) | ||
| .expect("DashPay receive address") | ||
| }; | ||
| let dashpay_tx = Transaction::dummy(&dashpay_address, 0..1, &[dashpay_duffs]); | ||
| let dashpay_result = ctx | ||
| .check_transaction( | ||
| &dashpay_tx, | ||
| TransactionContext::InChainLockedBlock(BlockInfo::new( | ||
| 2, | ||
| BlockHash::all_zeros(), | ||
| 1_700_000_100, | ||
| )), | ||
| ) | ||
| .await; | ||
| assert!( | ||
| dashpay_result.is_relevant && dashpay_result.is_new_transaction, | ||
| "DashPay funding tx should be recognized" | ||
| ); | ||
|
|
||
| let signer = WalletSigner { | ||
| wallet: ctx.wallet.clone(), | ||
| }; | ||
|
|
||
| let balance = Arc::new(WalletBalance::new()); | ||
| let info = PlatformWalletInfo { | ||
| core_wallet: ctx.managed_wallet, | ||
| balance, | ||
| identity_manager: IdentityManager::new(), | ||
| tracked_asset_locks: BTreeMap::new(), | ||
| }; | ||
|
|
||
| let mut wm = WalletManager::<PlatformWalletInfo>::new(Network::Testnet); | ||
| let wallet_id = wm.insert_wallet(ctx.wallet, info).expect("insert wallet"); | ||
|
|
||
| (Arc::new(RwLock::new(wm)), wallet_id, signer) | ||
| } | ||
|
|
||
| /// Like [`split_funded_wallet_manager`] but seeds CoinJoin account 0 with | ||
| /// `coinjoin_values.len()` separate spendable UTXOs, each at its own derived | ||
| /// CoinJoin address (so the cross-account signer resolver can find a | ||
| /// derivation path for every one). Models the many-small-denomination shape a | ||
| /// real DIP-9 CoinJoin account carries (0.001 / 0.01 / 0.1 DASH mixing | ||
| /// outputs) — the shape that made the asset-lock coin selector blow up | ||
| /// on-device when it defaulted to the exponential BranchAndBound subset-sum. | ||
| pub(crate) async fn split_funded_wallet_manager_many_coinjoin( |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Gate the new asset-lock fixtures to test builds
The exact wallet CI command, cargo clippy --package platform-wallet --package platform-wallet-storage --package platform-wallet-ffi --package rs-unified-sdk-ffi --package rs-unified-sdk-jni --all-features --locked -- --no-deps -D warnings, fails with dead_code errors for split_funded_wallet_manager, DashpayLeg, foreign_contact_account_xpub, split_funded_wallet_manager_dashpay, and split_funded_wallet_manager_many_coinjoin. These pub(crate) helpers are referenced only by the #[cfg(test)] asset-lock module, while test_support itself is compiled into the normal library target. Gate these items and their helper-only imports with #[cfg(test)] so the production target remains warning-free while unit tests still compile them.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 346f2cb — Gate the new asset-lock fixtures to test builds 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.
Comment on lines
+2896
to
+2935
| #[tokio::test] | ||
| async fn delegated_builder_credit_key_failure_does_not_strand_bip44_inputs() { | ||
| let (manager, signer, _persistence) = | ||
| funded_asset_lock_manager(Arc::new(AlwaysOkBroadcaster)).await; | ||
|
|
||
| // The whole 0.1 DASH balance rides on one UTXO, so a leaked reservation | ||
| // is immediately visible as a shortfall on the next build. | ||
| let failing = CreditKeyFailingSigner { | ||
| inner: signer.clone(), | ||
| }; | ||
| let abandoned = manager | ||
| .build_asset_lock_transaction( | ||
| 5_000_000, | ||
| 0, | ||
| AssetLockFundingType::IdentityRegistration, | ||
| 0, | ||
| &failing, | ||
| None, | ||
| ) | ||
| .await; | ||
| assert!( | ||
| abandoned.is_err(), | ||
| "a failing credit-output signer must abandon the build, got {abandoned:?}" | ||
| ); | ||
|
|
||
| let rebuild = manager | ||
| .build_asset_lock_transaction( | ||
| 5_000_000, | ||
| 0, | ||
| AssetLockFundingType::IdentityRegistration, | ||
| 0, | ||
| &signer, | ||
| None, | ||
| ) | ||
| .await; | ||
| assert!( | ||
| rebuild.is_ok(), | ||
| "the abandoned delegated build must leave its BIP44 input reselectable, \ | ||
| got {rebuild:?}" | ||
| ); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Exercise the prefetched signer's post-reservation cache hit
CreditKeyFailingSigner fails its first and every subsequent public_key request, so this test exits during the new up-front prefetch before the delegated builder reserves or signs anything. It proves that a prefetch failure is reservation-free, but it does not protect the load-bearing behavior in PrefetchedCreditKeySigner: the delegated builder must request exactly the prefetched path and receive it from cache without invoking the underlying signer again. Add a separate signer whose first public_key call succeeds and every later call fails, then assert that the delegated build succeeds and the underlying call count is exactly one. That regression would fail if the prefetch and delegated paths diverged or the wrapper accidentally delegated the cached request.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 346f2cb — Exercise the prefetched signer's post-reservation cache hit 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.
Comment on lines
+140
to
+163
| unsafe fn parse_optional_derivation_path( | ||
| ptr: *const u8, | ||
| len: usize, | ||
| ) -> Result<Option<key_wallet::bip32::DerivationPath>, PlatformWalletFFIResult> { | ||
| use std::str::FromStr; | ||
| if ptr.is_null() || len == 0 { | ||
| return Ok(None); | ||
| } | ||
| let bytes = std::slice::from_raw_parts(ptr, len); | ||
| let text = std::str::from_utf8(bytes).map_err(|e| { | ||
| PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| format!("funding_path is not valid UTF-8: {e}"), | ||
| ) | ||
| })?; | ||
| key_wallet::bip32::DerivationPath::from_str(text) | ||
| .map(Some) | ||
| .map_err(|e| { | ||
| PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidParameter, | ||
| format!("invalid funding_path derivation path {text:?}: {e}"), | ||
| ) | ||
| }) | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Pin the funding-path parser's fail-closed behavior
parse_optional_derivation_path is a new unsafe C-boundary parser for the account that supplies money, but the existing FFI test module never calls it directly. Its absence/error distinction is important: a null pointer or zero length deliberately selects the default BIP44 source, while invalid UTF-8 or malformed path syntax must return ErrorInvalidParameter instead of degrading to that default and potentially selecting different coins. Add focused tests for null, zero length, a valid account-level path, invalid UTF-8, and malformed syntax.
source: ['codex']
Collaborator
There was a problem hiding this comment.
Resolved in 346f2cb — Pin the funding-path parser's fail-closed behavior 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.
Two mechanical CI-hard failures on bd19a3e: 1. `cargo fmt --check --all` failed on the JNI funding-path bridge (rs-unified-sdk-jni/src/funding.rs). Applied rustfmt's replacement. 2. The wallet clippy job (`--all-features -- --no-deps -D warnings`) failed with `dead_code` on five PR-added asset-lock fixtures in platform-wallet's `test_support`. The module is compiled into the normal lib target under `--all-features` (it is gated `cfg(any(test, feature = "test-utils"))`), so `pub(crate)` helpers consumed only by `#[cfg(test)]` unit tests have no consumer there. Gated `split_funded_wallet_manager`, `DashpayLeg`, `foreign_contact_account_xpub`, `split_funded_wallet_manager_dashpay` and `split_funded_wallet_manager_many_coinjoin` on `cfg(test)`, plus the imports that only they use (`OutPoint`/`TxOut`/`Txid`, `Utxo`). `cfg(test)` — not `cfg(any(test, feature = "test-utils"))` — matches the convention this file already documents on `RejectFirstBroadcaster` and applies to all four mock broadcasters. It cannot break the cross-crate test build: the only external consumer (rs-platform-wallet-ffi's broadcast tests) uses `funded_spv_core_wallet` and `WalletSigner`, both `pub` and both left untouched, while the five gated items are `pub(crate)` and were never reachable from outside. Also addresses the two review suggestions, both additive: - Added `delegated_builder_serves_credit_key_from_prefetch_cache`. The existing failure test's signer rejects every `public_key` call, so it only proves a prefetch failure is reservation-free. The new recording signer answers exactly one request and poisons the rest, pinning that the builder's credit-key request is served from `PrefetchedCreditKeySigner`'s cache and that the prefetched path is exactly the credit path the builder returns. - Added four `parse_optional_derivation_path` tests covering null, zero length with a non-null pointer, a valid account-level path, invalid UTF-8 and malformed syntax — pinning that the funding-path parser fails closed with ErrorInvalidParameter instead of silently falling back to the default BIP44 source. No error codes touched; ErrorAssetLockInsufficientFunds stays 29. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Both blockers fixed in 1. 2. Wallet clippy I went with To confirm this couldn't break the cross-crate test build I checked the actual consumers: the only external one is One addition to the finding: gating the items surfaced a second clippy failure — Both suggestions implemented. Agreed on the prefetch gap — Verification:
No error codes touched — |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 2, 2026
…30 (dashpay#4256) dashpay#4256 is stacked on dashpay#4185 and still carried the pre-renumber `29`, which now collides with `ErrorAssetLockInsufficientFunds = 29` on dashpay#4184. Per the resolution of record in dashpay#4261's ERROR_CODE_REGISTRY.md, dashpay#4184 keeps 29 and the `ErrorReservationWalletMismatch` family moves to 30; dashpay#4185 already made that move in `d854debb`. This brings dashpay#4256 in line. CI could not have caught this: dashpay#4256 and dashpay#4184 are both MERGEABLE with green checks, because two branches assigning the same discriminant produce no textual conflict. It surfaces only as an E0081 after a textual merge, or silently as a wrong error code on the host. The discriminant is public ABI, so every mirror moves together: - Rust enum + its rustdoc cross-reference (error.rs) - doc reference in core_wallet/signed_payment.rs - JNI rustdoc (rs-unified-sdk-jni/src/wallet_manager.rs) - Swift PlatformWalletResultCode raw value - Kotlin fromPlatformWalletNative branch, class KDoc, WalletManagerNative KDoc, and the DashSdkErrorTest offset assertion Both Swift switches are symbolic (cbindgen `PLATFORM_WALLET_FFI_RESULT_CODE_*` constants), so only the enum raw value carried the number. Also corrects this PR's own numbering rationale on `ErrorTransactionSigning` (33), which claimed 30 was "reserved for dashpay#4184's ErrorAssetLockCrossDomainConsentRequired". That code does not exist on any branch — dashpay#4184 dropped it in a re-scope — and 30 is now ErrorReservationWalletMismatch. dashpay#4256's codes are unchanged otherwise: it keeps 32 (ErrorTransactionBuild, shared with dashpay#4247) and 33. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 805 passed / 0 failed; :sdk:test BUILD SUCCESSFUL with DashSdkErrorTest 9/9. Swift is unverified — it cannot be compiled here.
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 2, 2026
…ollisions The 29 collision is resolved and the renumber has now landed on dashpay#4185's branch: dashpay#4184 keeps 29 (ErrorAssetLockInsufficientFunds), dashpay#4185 takes 30 (ErrorReservationWalletMismatch). Table rows updated to match the code. Fixes the "30 is both free and assigned" inconsistency: the next-free line claimed 27-33 were claimed while the table showed 30 unallocated. 30 is now genuinely allocated to dashpay#4185, so the two agree. Adds allocations the survey had omitted, verified 2026-08-01 by reading error.rs at the head of all 62 open PRs: - dashpay#3968 numbers 26/27/28 (Persister* + a pre-merge TransactionBroadcastRejected) -> contradicts merged ABI at 26 and collides with dashpay#4185 at 27 and 28 - dashpay#3954 numbers ErrorShutdownIncomplete = 27 -> collides with dashpay#4185 at 27 - dashpay#4259 carries ErrorSigningKeyUnavailable = 31, inherited from dashpay#4183 rather than a new allocation The same sweep confirms no open PR anywhere defines a code 30.
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 2, 2026
…cord dashpay#4196 scope Clears the two review blockers on dashpay#4261 and re-syncs the registry with what the code on each branch actually does, re-read at every head rather than trusted from this file. Blocker (a) — dashpay#3968 / dashpay#3954 / dashpay#4259 were described in prose but had no rows, which is exactly what rule 2 forbids. They now have them: - A "Non-conforming allocations" table for dashpay#3968 (26/27/28) and dashpay#3954 (27). These are deliberately kept out of the proposed table: each row is a claim to be withdrawn and reissued, not an allocation of record. - An inherited-code table for the 31 that dashpay#4204 and dashpay#4259 carry but did not allocate (dashpay#4183 owns it), so it is not double-counted. - dashpay#4196 is recorded as claiming no integer at all: it routes a new token-less `StaleReservation` variant through the existing `ErrorStaleReservationToken`. The dashpay#3968 half is the serious one and is called out as such. Its 28 is not a new claim — it *moves the already-shipped* `ErrorTransactionBroadcastRejected` off 26 to make room for its own persister code. Rule 3 forbids that: a host compiled against merged ABI returns 26 for a broadcast rejection, and after dashpay#3968 the same condition returns 28 while 26 means a transient persister failure. Neither branch's diff shows the contradiction. Blocker (b) — 30 marked both free and assigned was already resolved by the preceding commit; verified consistent here (30 is allocated to dashpay#4185 throughout, frontier is 34, and the one remaining "genuinely free" is past tense explaining why dashpay#4185 could take it). Also corrected, all verified against the branches: - Survey provenance had dashpay#4185 at `0b0d5c76d6` labelled "(post-renumber)". Wrong twice: that commit is the *parent* of the renumber `d854debb`, and the head has since moved to `6c37e8679e`. dashpay#4184, dashpay#4247 and dashpay#4256 SHAs refreshed too. - dashpay#4256 has now taken 30 (`9481e5783b`) and dropped its stale "30 is reserved for the consent code" rationale; the equivalent comments on dashpay#4183 and dashpay#4204 are flagged as still present. - dashpay#4184 has a comment-only drift: it reserves "Codes 27-28" but names three codes. Correct when the trio was 27/28/29; it is now 27/28/30. Its discriminant is right and is the resolution of record — only the prose is stale, and dashpay#4184 is left untouched. - The dashpay#4196 section now records why the restack has not happened: its three own commits conflict in 3 files / 10 hunks against dashpay#4185's head, and the registry redesign underneath it (mandatory `registered_height`, new `WalletRemoved` variant, owner-stamped funding token) makes it author work rather than conflict resolution. Its trio numbers come from the dashpay#4185 copy it carries, so the restack fixes 28 -> 30 for free; the number dashpay#4196 itself must chase is 27, not 30. Verified: cargo fmt --all -- --check clean; cargo test -p platform-wallet-ffi -p platform-wallet = 738 passed / 0 failed. Docs-only change.
Contributor
Author
|
@thepastaclaw the review appears to have stalled — the sticky has read "Stage: Sonnet review + final verification" at For context, |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 3, 2026
…ence (dashpay#4185 review) The doc comment on ErrorReservationWalletMismatch pointed at packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md, which does not exist in the tree (nor in dashpay#4184, which owns code 29). The inline note that 29 is reserved by ErrorAssetLockInsufficientFunds (dashpay#4184) already records the split, so remove the stale link rather than minting a new registry file. Flagged by CodeRabbit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rest of the wallet-crate docs use the American spelling ~2:1; align the new release-notes bullet with it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Pushed a new head 59d3dbf — a doc-only commit aligning the new FFI README release-notes bullet with the crate's dominant "behavior" spelling. No code changes vs @coderabbitai full review |
Contributor
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 55 minutes. |
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 3, 2026
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with this PR's `ErrorStaleReservationToken = 27`. Renumber the deferred build/broadcast trio to the contiguous block 34-36, which sits above every code currently claimed by a merged commit or an open PR: 27 ErrorShutdownIncomplete MERGED, dashpay#4268 29 ErrorAssetLockInsufficientFunds dashpay#4184 31 ErrorSigningKeyUnavailable dashpay#4183, dashpay#4259 32 ErrorTransactionBuild dashpay#4247, dashpay#4256 33 ErrorTransactionSigning dashpay#4256 28 and 30 are vacated and return to the free pool. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (which has no compile-time cross-ABI check, so it was verified by grep). Also addresses three review suggestions: * `PlatformWalletInfo::generation` is now `pub(crate)`. It was publicly assignable through `state_mut()` / `state_mut_blocking()`, so downstream safe code could swap the `Arc` while `PlatformWallet` and `CoreWallet` kept the original — splitting the generation identity `Arc::ptr_eq` compares, which would make `is_current_generation()` reject a live wallet, turn generation-bound reservation cleanup into a no-op, and let teardown exclude through a different lifecycle gate than the payments it must fence. All construction and mutation sites are already inside the crate. * `buildSignedPayment` now runs under `opWithCleanupOnCancellation`. Native finalization mints the token before the blocking JNI call returns, so `withContext`'s prompt-cancellation handoff could discard the completed `SignedCoreTransaction` and leave the reservation to the GC Cleaner or the TTL. The discarded result is now closed deterministically. * Native code 26 (`ErrorTransactionBroadcastRejected`) no longer falls through to `PlatformWallet.Generic`. It maps to a dedicated `TransactionBroadcastRejected` subtype so callers can tell a definitively rejected, consumed-and-released payment (rebuild it) from an unrelated generic wallet failure, with its non-retry-in-place semantics pinned in `DashSdkErrorTest`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfoss765
added a commit
to bfoss765/platform
that referenced
this pull request
Aug 3, 2026
…pay#4268 claimed dashpay#4268 merged `ErrorShutdownIncomplete = 27` into the v4.2-dev FFI ABI, colliding with the `ErrorStaleReservationToken = 27` this branch carries alongside dashpay#4185. Renumber the deferred build/broadcast trio to the contiguous block 34-36, matching dashpay#4185: ErrorStaleReservationToken 27 -> 34 ErrorReservationTokenConsumed 28 -> 35 ErrorReservationWalletMismatch 30 -> 36 34-36 sits above every code claimed by a merged commit or an open PR (27 dashpay#4268 merged, 29 dashpay#4184, 31 dashpay#4183/dashpay#4259, 32 dashpay#4247/dashpay#4256, 33 dashpay#4256), so it ends the renumbering churn. 28 and 30 are vacated and return to the free pool. This branch's own `ErrorTransactionBuild` (32) and `ErrorTransactionSigning` (33) are unaffected; their numbering-rationale rustdoc is updated to name dashpay#4268 as the owner of 27 and to record where the trio went. Applied across the Rust enum, the FFI/JNI rustdoc, the Kotlin mapping + KDoc + tests, and the Swift mirror (no compile-time cross-ABI check — verified by grep). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
reviewed
Aug 3, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Verified at exact head 59d3dbf: commit 346f2cb fixes both prior CI blockers and both prior coverage suggestions, while 59d3dbf is documentation-only. Workspace formatting, the exact wallet clippy CI command, all 28 shielded asset-lock builder tests, the prefetched-signer regression, and all four funding-path parser regressions pass. One in-scope suggestion remains: the selected-account builder's post-reservation credit-key failure rollback is reachable and important to this PR's reservation lifecycle, but no test exercises it.
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 (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s)
🤖 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`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:601-689: Exercise selected-account rollback after credit-key failure
`build_asset_lock_tx_from_selected_account` calls `build_signed` at lines 590-593, which reserves the selected account's inputs, and then performs the external `signer.public_key(&path)` request at lines 621-657. A failure there reaches the path-matched rollback loop at lines 659-689. The existing selected-account tests cover reservation retention and rejected-broadcast release, while `CreditKeyFailingSigner` is used only with `IdentityRegistration`, which follows the non-shielded prefetched builder and fails before reservation. Add a shielded selected-account regression using that signer, then verify a second build can immediately reselect the account's sole UTXO. This directly protects the account-by-`funding_path` rollback introduced by this PR from silently stranding CoinJoin, DashPay-receiving, or explicit BIP32 inputs.
Comment on lines
+601
to
+689
| // 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. | ||
| // | ||
| // `build_signed` above already RESERVED this transaction's inputs in the | ||
| // selected account's `ReservationSet`, and the reservation is normally | ||
| // held all the way to broadcast. But every step below can still fail | ||
| // (missing shielded-topup account, `peek_next_path`, the signer | ||
| // round-trip, `mark_first_pool_index_used`), and each such failure | ||
| // abandons a signed transaction that never reaches the wire. Without a | ||
| // rollback those reserved inputs would stay stranded until the | ||
| // reservation-TTL backstop (~24 blocks), silently withholding the coins | ||
| // from any other send in the meantime (dashpay/platform#4184 review, | ||
| // thepastaclaw). So derive the key inside an inner future whose `Result` | ||
| // we inspect, and on ANY error release the reservation before | ||
| // propagating. The lookup mirrors `release_asset_lock_funding_reservation` | ||
| // (match the selected funds account by `funding_path`); it must run on a | ||
| // fresh shared borrow of `info` because the `&mut selected` used for | ||
| // `set_funding` cannot be revived across the credit-account borrows above. | ||
| let derive_credit_key = async { | ||
| 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()))?; | ||
| } | ||
| Ok::<DerivationPath, PlatformWalletError>(path) | ||
| } | ||
| .await; | ||
|
|
||
| let path = match derive_credit_key { | ||
| Ok(path) => path, | ||
| Err(e) => { | ||
| // Abandon path: roll back the reservation `build_signed` placed | ||
| // on the selected funds account so its inputs are immediately | ||
| // reselectable, rather than stranded until the reservation-TTL | ||
| // backstop. | ||
| let mut released = false; | ||
| for acc in info.core_wallet.accounts.all_funding_accounts() { | ||
| if acc | ||
| .managed_account_type() | ||
| .to_account_type() | ||
| .derivation_path(network) | ||
| .map(|p| p == funding_path) | ||
| .unwrap_or(false) | ||
| { | ||
| acc.release_reservation(&transaction); | ||
| released = true; | ||
| break; | ||
| } | ||
| } | ||
| if !released { | ||
| tracing::warn!( | ||
| %funding_path, | ||
| "abandoned signed asset-lock tx but could not release its reservation: \ | ||
| no funds account matches the funding path (inputs will free on TTL)" | ||
| ); | ||
| } | ||
| return Err(e); | ||
| } | ||
| }; |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Exercise selected-account rollback after credit-key failure
build_asset_lock_tx_from_selected_account calls build_signed at lines 590-593, which reserves the selected account's inputs, and then performs the external signer.public_key(&path) request at lines 621-657. A failure there reaches the path-matched rollback loop at lines 659-689. The existing selected-account tests cover reservation retention and rejected-broadcast release, while CreditKeyFailingSigner is used only with IdentityRegistration, which follows the non-shielded prefetched builder and fails before reservation. Add a shielded selected-account regression using that signer, then verify a second build can immediately reselect the account's sole UTXO. This directly protects the account-by-funding_path rollback introduced by this PR from silently stranding CoinJoin, DashPay-receiving, or explicit BIP32 inputs.
source: ['claude']
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.