feat(kotlin-sdk): CoinJoin-drain shielded funding binding + typed asset-lock shortfall - #4349
feat(kotlin-sdk): CoinJoin-drain shielded funding binding + typed asset-lock shortfall#4349bfoss765 wants to merge 1 commit into
Conversation
…et-lock shortfall The two Android-facing pieces that dashpay#4327 left out. dashpay#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 dashpay#4184, and every host mirror already documents the number, but the code was never allocated: dashpay#4184 and its successor dashpay#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>
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit a711c55) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The CoinJoin-drain JNI/Kotlin binding correctly validates signed indices and fixed-size inputs, but the new asset-lock shortfall discriminator is not propagated consistently across existing host APIs. Swift converts raw code 29 to an unknown error, while the regular exact-amount shielded-funding export rewrites the typed shortfall to generic wallet-operation code 6.
Source: Reviewer backend model: gpt-5.6-sol (Codex general, FFI engineer, and security auditor); final verifier backend model: gpt-5.6-sol (Codex verifier). Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
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— security-auditor (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)
🟡 2 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:88-97: Swift still treats native asset-lock shortfalls as unknown errors
This PR activates `PlatformWalletFFIResultCode::ErrorAssetLockInsufficientFunds` with raw value 29, and the existing Swift `shieldedFundFromCoinJoinDrain` wrapper can now receive that value. However, `PlatformWalletResultCode` only mentions 29 in this reservation comment: it has no raw-value case, `init(ffi:)` has no arm for the generated C constant, and `PlatformWalletError` has no corresponding public case. The initializer therefore falls through to `.errorUnknown`, so Swift callers cannot branch on the newly typed ABI discriminator. Add the result-code case, C-enum conversion arm, public error case, conversion branch, and a mirror test alongside the existing asset-lock result-code tests.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1024-1030: The regular shielded-funding export erases code 29
The changed asset-lock builder now returns `PlatformWalletError::AssetLockInsufficientFunds` when the selected BIP44 account cannot cover an exact-amount lock, but this FFI export rewrites every error to `ErrorWalletOperation` (6). JNI consequently throws code 1006, which Kotlin maps to `DashSdkError.PlatformWallet.WalletOperation`; the new code-1029 `AssetLockInsufficientFunds` type documented for `shieldedFundFromAssetLock` is unreachable. Preserve the typed conversion for this variant while retaining the existing contextual wrapper for other errors. The new CoinJoin-drain sibling already preserves its typed errors with `e.into()`.
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.