feat(kotlin-sdk): coinjoin-drain shielded funding binding + typed asset-lock shortfall - #4361
Open
bfoss765 wants to merge 2 commits into
Open
feat(kotlin-sdk): coinjoin-drain shielded funding binding + typed asset-lock shortfall#4361bfoss765 wants to merge 2 commits into
bfoss765 wants to merge 2 commits into
Conversation
…et-lock shortfall The two Android-facing pieces that #4327 left out. #4327 added the CoinJoin-drain asset-lock funding FFI export (`platform_wallet_manager_shielded_fund_from_asset_lock_coinjoin_drain`) and its Swift wrapper, but no Kotlin/JNI, so Android could not call CoinJoin-funded shielding at all. This adds the JNI export and the Kotlin surface, following the Swift wrapper's contract: no amount (the lock value is the builder's Sigma inputs - L1 fee) and no surplus output (the single-recipient remainder flow pins the consensus surplus to zero). It also allocates the typed asset-lock shortfall at its long-reserved code 29. The FFI error registry has held 29 for `ErrorAssetLockInsufficientFunds` since #4184, and every host mirror already documents the number, but the code was never allocated: #4184 and its successor #4316 were both closed unmerged, leaving the producing `PlatformWalletError` variant absent too. Without it an asset-lock coin-selection shortfall flattens to `ErrorUnknown` (99) and hosts must substring-match the Display text. The empty-candidate-set case now stays on the same structured path as a partial shortfall instead of falling through to the generic string form. Draining an empty CoinJoin account is exactly a coin-selection shortfall, so the two halves meet at the same call: the Android mixed-funds migration needs the binding to run and the typed code to explain a failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
August 10, 2026 20:02
Contributor
📝 WalkthroughWalkthroughThe PR adds typed asset-lock insufficient-funds errors with native code 29. It also adds CoinJoin account drain funding across the Rust platform wallet, JNI bridge, and Kotlin SDK. ChangesAsset-lock CoinJoin funding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KotlinCaller
participant PlatformWalletManager
participant FundingNative
participant JNI
participant RustPlatformWallet
KotlinCaller->>PlatformWalletManager: call shieldedFundFromCoinJoinDrain
PlatformWalletManager->>FundingNative: pass wallet, recipient, and account index
FundingNative->>JNI: invoke native drain method
JNI->>RustPlatformWallet: drain CoinJoin account into asset lock
RustPlatformWallet-->>JNI: return status or typed error code
JNI-->>FundingNative: forward result
FundingNative-->>PlatformWalletManager: complete or raise mapped error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
|
✅ Final review complete — no blockers (commit bf9a134) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4361 +/- ##
=========================================
Coverage 87.63% 87.63%
=========================================
Files 2670 2670
Lines 339449 339449
=========================================
Hits 297467 297467
Misses 41982 41982
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The new JNI/Kotlin drain binding follows the sibling safety patterns, but the typed asset-lock shortfall remains incomplete across three paths: empty drains carry a 0/0 amount pair, exact BIP44 funding erases code 29, and Swift decodes code 29 as unknown. These are client-facing error-contract defects rather than consensus blockers, so the review contains three suggestions.
Source: reviewer backend model gpt-5.6-sol (Codex general, FFI engineer, and Rust quality lanes); final verifier backend model gpt-5.6-sol (Codex). Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 3 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:193: Empty CoinJoin drains report a zero required amount
`DrainAll` is converted to the builder's zero-valued output placeholder at lines 126-132, and that placeholder is passed here as the fallback `required` amount. When an empty CoinJoin account produces `SelectionError::NoUtxosAvailable`, the new mapper therefore returns `AssetLockInsufficientFunds { available: 0, required: 0 }`, even though the operation cannot proceed with zero funds. The shielded flow has already computed a positive `minimum_lock_duffs` from the Type 18 pool fee and threads it through `AssetLockBuildAmount::DrainAll`; use that floor for amount-less selection errors so the advertised available/required pair describes the actual shortfall.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1024-1030: Exact-amount shielded funding erases the typed shortfall
The PR promotes asset-lock builder shortfalls to `PlatformWalletError::AssetLockInsufficientFunds` and maps that variant to FFI code 29, but this existing exact-amount endpoint replaces every wallet error with `ErrorWalletOperation` (6). Consequently, Kotlin's newly documented `shieldedFundFromAssetLock` producer can never raise `DashSdkError.PlatformWallet.AssetLockInsufficientFunds`; only the CoinJoin-drain sibling preserves the typed conversion. Route the new shortfall variant through `From<PlatformWalletError>` while retaining the endpoint's existing catch-all behavior and contextual message for unrelated failures.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:89-97: Swift does not decode the newly allocated FFI error code
Rust now emits `ErrorAssetLockInsufficientFunds` at code 29, including from the existing Swift `shieldedFundFromCoinJoinDrain` wrapper. Swift only mentions the reservation in this comment: `PlatformWalletResultCode` has no code-29 case, `init(ffi:)` has no corresponding C-enum arm and falls through to `.errorUnknown`, and `PlatformWalletError` has no typed shortfall case. Add code 29 to the Swift result enum, C-enum conversion, typed error construction and description handling, plus a regression mapping test analogous to the existing code mappings.
…r, in typed shortfalls
An empty CoinJoin drain surfaced AssetLockInsufficientFunds
{ available: 0, required: 0 } because the drain's zero-valued
credit-output placeholder was passed as the requested amount. The
shielded flow already threads the positive minimum through
AssetLockBuildAmount::DrainAll; use that floor for amount-less
selection errors so the advertised pair describes the actual gap.
Co-Authored-By: Claude Fable 5 <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/rs-platform-wallet/src/error.rs`:
- Around line 272-278: Update the drain-shortfall documentation in
packages/rs-platform-wallet/src/error.rs lines 272-278 to state that required
equals minimum_lock_duffs when a drain floor is configured, and equals zero only
when no floor exists. Also update the mapper contract in
packages/rs-platform-wallet/src/wallet/asset_lock/build.rs lines 994-999 to
match the minimum_lock_duffs.unwrap_or(0) value passed by the asset-lock build
flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34dd7946-0802-4d9a-b188-4162636f6c89
📒 Files selected for processing (8)
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/src/error.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/asset_lock/build.rspackages/rs-unified-sdk-jni/src/funding.rs
Comment on lines
+272
to
+278
| /// On a *drain* build (whole-account funding, e.g. the CoinJoin → shielded | ||
| /// migration) the requested target is the zero credit-output placeholder | ||
| /// that key-wallet rewrites to `Σ inputs − fee`, so an empty account | ||
| /// surfaces here as `available: 0, required: 0` — the "this account has | ||
| /// nothing to drain" signal. The real floor for a drain is the Type 18 pool | ||
| /// fee, enforced downstream against the built payload once the lock value | ||
| /// is known. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the drain-shortfall documentation.
An empty drain now reports the positive minimum_lock_duffs as required when the caller provides a drain floor. The current text still promises required: 0.
packages/rs-platform-wallet/src/error.rs#L272-L278: describerequired: minimum_lock_duffsfor a drain with a configured floor. Describe zero only when no floor exists.packages/rs-platform-wallet/src/wallet/asset_lock/build.rs#L994-L999: update the mapper contract to match theminimum_lock_duffs.unwrap_or(0)value passed at Lines 200-205.
📍 Affects 2 files
packages/rs-platform-wallet/src/error.rs#L272-L278(this comment)packages/rs-platform-wallet/src/wallet/asset_lock/build.rs#L994-L999
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/error.rs` around lines 272 - 278, Update the
drain-shortfall documentation in packages/rs-platform-wallet/src/error.rs lines
272-278 to state that required equals minimum_lock_duffs when a drain floor is
configured, and equals zero only when no floor exists. Also update the mapper
contract in packages/rs-platform-wallet/src/wallet/asset_lock/build.rs lines
994-999 to match the minimum_lock_duffs.unwrap_or(0) value passed by the
asset-lock build flow.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The Kotlin/JNI CoinJoin-drain binding follows the existing ownership and boundary-validation patterns, and the latest commit fixes the previously reported empty-drain amount pair. Three in-scope suggestions remain: exact-amount funding still erases code 29, Swift does not decode code 29, and the public drain-shortfall documentation still describes the removed 0/0 behavior.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (FFI engineer); final verifier gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 3 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1024-1029: Exact-amount shielded funding erases the typed shortfall
This endpoint converts every `PlatformWalletError` into `ErrorWalletOperation` (6), bypassing the new blanket conversion that maps `AssetLockInsufficientFunds` to code 29. The Kotlin `shieldedFundFromAssetLock` path invokes this exact endpoint and only constructs `DashSdkError.PlatformWallet.AssetLockInsufficientFunds` when native code 29 reaches it, so an exact-amount coin-selection shortfall still arrives as the generic `WalletOperation` error despite the newly documented typed contract. Preserve the endpoint's contextual catch-all for unrelated failures, but pass the new shortfall variant through `From<PlatformWalletError>`.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:88-97: Swift does not decode the newly allocated FFI error code
Rust now emits `ErrorAssetLockInsufficientFunds` with raw value 29, including from the CoinJoin-drain endpoint called by Swift before `result.check()`. Swift only mentions code 29 in the reservation comment: `PlatformWalletResultCode` has no code-29 case, `init(ffi:)` has no corresponding C-enum arm and therefore falls through to `.errorUnknown`, and `PlatformWalletError` has no typed asset-lock shortfall case. Add the result-code case, C-enum conversion, typed error construction and description handling, plus a mapping regression test that pins raw code 29.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:272-278: Drain shortfall documentation still promises the removed 0/0 result
The latest commit passes `minimum_lock_duffs.unwrap_or(0)` to `map_builder_error`, and the shielded flow replaces the caller's value with a positive Type 18 pool-fee floor before building. An empty shielded CoinJoin drain therefore reports `available: 0` and that positive floor as `required`, not 0/0 as these public variant docs claim. The mapper contract in `wallet/asset_lock/build.rs` lines 994-999 repeats the obsolete behavior; update both comments to say that `required` is the configured drain floor, or zero only when no floor is provided.
Comment on lines
+272
to
+278
| /// On a *drain* build (whole-account funding, e.g. the CoinJoin → shielded | ||
| /// migration) the requested target is the zero credit-output placeholder | ||
| /// that key-wallet rewrites to `Σ inputs − fee`, so an empty account | ||
| /// surfaces here as `available: 0, required: 0` — the "this account has | ||
| /// nothing to drain" signal. The real floor for a drain is the Type 18 pool | ||
| /// fee, enforced downstream against the built payload once the lock value | ||
| /// is known. |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Drain shortfall documentation still promises the removed 0/0 result
The latest commit passes minimum_lock_duffs.unwrap_or(0) to map_builder_error, and the shielded flow replaces the caller's value with a positive Type 18 pool-fee floor before building. An empty shielded CoinJoin drain therefore reports available: 0 and that positive floor as required, not 0/0 as these public variant docs claim. The mapper contract in wallet/asset_lock/build.rs lines 994-999 repeats the obsolete behavior; update both comments to say that required is the configured drain floor, or zero only when no floor is provided.
source: ['codex', 'coderabbit']
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
2 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.
Supersedes #4349 — same change, recreated on a
dashpay/platformbranch per repo policy (no more personal-fork PRs). Commits and authorship unchanged; full review history on #4349.The two Android-facing pieces #4327 left out. Both are small and independent of each other; they meet at one user action.
1. Kotlin/JNI binding for CoinJoin-drain shielded funding
#4327 landed the FFI export
platform_wallet_manager_shielded_fund_from_asset_lock_coinjoin_drainand a Swift wrapper, but no Kotlin/JNI — so Android currently cannot call CoinJoin-funded shielding at all.This adds the JNI export in
rs-unified-sdk-jniand the Kotlin surface (FundingNative.shieldedFundFromCoinJoinDrain,PlatformWalletManager.shieldedFundFromCoinJoinDrain), shaped like the existingshieldedFundFromAssetLockbinding and following the Swift wrapper's contract:Σ inputs − L1 fee, computed Rust-side, so the mixed coins never hop through a transparent BIP44 address;The negative-index guard matches the sibling binding's boundary check (a negative
jintwould otherwise bit-cast to a hugeu32).The mixed-funds migration in the Android wallet depends on this binding — it is the CoinJoin → Shielded path.
2. Typed asset-lock shortfall at its reserved code 29
The FFI error registry has reserved 29 for
ErrorAssetLockInsufficientFundssince #4184, and the Swift and Kotlin mirrors already document the number — but the code was never allocated, because #4184 and its successor #4316 were both closed unmerged. That also left the producingPlatformWalletError::AssetLockInsufficientFundsvariant absent, so there was nothing to map from.This salvages the minimum needed to make the reserved code real (#4073):
AssetLockInsufficientFunds { available, required }variant;InsufficientFundsshapes keep their own exact amounts;NoUtxosAvailable— the most extreme shortfall — previously fell through to the generic string form while partial shortfalls stayed typed, and now maps toavailable: 0against the requested target. Every other builder error keeps its existing generic string;Fromarm, and tests pinning both the mapping and the number.Without the
Fromarm a shortfall flattens toErrorUnknown(99), forcing hosts to substring-match the Display text. The amounts still ride the message —PlatformWalletFFIResultis ABI-frozen to code + message — but hosts can now branch on the code.Two existing asset-lock tests asserted the old generic error for a "fails at input selection" rebuild; they now assert the typed shortfall with
available: 0, which is a stronger statement of the same intent.Why these two are in one PR
Draining an empty CoinJoin account is a coin-selection shortfall. The binding is how Android runs the migration; code 29 is how it explains the most likely failure without parsing English.
Testing
cargo test -p platform-wallet --lib— 611 passedcargo test -p platform-wallet-ffi— 263 + 26 + 6 passedcargo clippy -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --all-targets -- -D warnings— cleancargo fmt --check— clean./gradlew :sdk:assembleDebug :sdk:testDebugUnitTest— BUILD SUCCESSFULThe Halo 2 proving path itself is unchanged and untested here; this PR only adds the call surface to it.
Summary by CodeRabbit