feat(kotlin-sdk): expose core_wallet_set_gap_limit to Kotlin (migrated-wallet address-window heal) - #4377
feat(kotlin-sdk): expose core_wallet_set_gap_limit to Kotlin (migrated-wallet address-window heal)#4377bfoss765 wants to merge 2 commits into
Conversation
The Rust seam existed end-to-end (AddressPool::set_gap_limit -> ManagedCoreFundsAccount -> CoreWallet::set_gap_limit -> the core_wallet_set_gap_limit C export, in-tree since #3970) but stopped at the C boundary: WalletManagerNative had no trampoline, so no Kotlin host could widen an address window. Adds the JNI export (account-type mapping via the existing core_account_type; from-height guard mirrors the sibling exports), the external fun, and a ManagedCoreWallet.setGapLimit wrapper under mapNativeErrors. Motivation: a migrated wallet whose OTHER same-seed client (dashj) kept deriving past the SDK's watched window goes silently blind to the change output and every descendant — the wallet reports synced with the wrong balance. Widening the gap limit (Rust caps at 1000) and re-scanning recovers the history; the Android app's one-shot migration heal is the first consumer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds an account-specific ChangesWallet gap-limit API
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedCoreWallet
participant WalletManagerNative
participant JNIWalletManager
participant PlatformWalletFFI
ManagedCoreWallet->>WalletManagerNative: set gap limit for account
WalletManagerNative->>JNIWalletManager: call coreWalletSetGapLimit
JNIWalletManager->>PlatformWalletFFI: validate and forward request
PlatformWalletFFI-->>JNIWalletManager: return result or error
JNIWalletManager-->>ManagedCoreWallet: map native error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit 69cacb6) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- Around line 645-648: Restrict the account type handling around
core_account_type to accept only BIP44, BIP32, and CoinJoin for this per-account
bridge operation. Explicitly reject AllSpendable (account_type 3) before
forwarding the value, while preserving the existing out-of-range exception
behavior.
🪄 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: 76b15fb1-440b-4305-a885-ecb512cb0c64
📒 Files selected for processing (3)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/WalletManagerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/rs-unified-sdk-jni/src/wallet_manager.rs
A gap limit belongs to one account's address pools; the aggregate (3) has none, so refuse it at the boundary with a clear message instead of forwarding it to a per-account FFI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4377 +/- ##
============================================
- Coverage 87.80% 87.63% -0.18%
============================================
Files 2641 2670 +29
Lines 336510 339447 +2937
============================================
+ Hits 295468 297465 +1997
- Misses 41042 41982 +940
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The Kotlin declaration, public wrapper, and JNI trampoline correctly validate signed inputs, reject the AllSpendable aggregate, map concrete account types, and forward native errors. The remaining in-scope issue is the absence of automated Android binding coverage for the new cross-language symbol and validation branches.
Source: codex-general reviewer backend gpt-5.6-sol; codex-ffi-engineer reviewer backend gpt-5.6-sol; final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and 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)
🟡 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-unified-sdk-jni/src/wallet_manager.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:648-675: Add an instrumented binding test for the new JNI export
No repository test invokes `coreWalletSetGapLimit`, so the Kotlin external declaration, generated JNI symbol, parameter descriptor, and the new validation branches can regress without detection. `cargo check -p rs-unified-sdk-jni` verifies only the Rust side and cannot detect a Kotlin/JNI naming or signature mismatch that would produce `UnsatisfiedLinkError` on Android. The existing Android instrumented suite already uses invalid handles to pin JNI bindings without requiring a funded wallet; add equivalent coverage that loads the native library, verifies that account type `3`, a negative account index, and a non-positive gap limit are rejected as invalid parameters, then calls a concrete account type with handle `0` and verifies that execution reaches the underlying FFI's invalid-handle path.
| let account_type = match core_account_type(account_type) { | ||
| Some(platform_wallet_ffi::CoreAccountTypeFFI::AllSpendable) | None => { | ||
| throw_sdk_exception( | ||
| env, | ||
| 1, | ||
| "accountType must be a concrete account (0=BIP44, 1=BIP32, 2=CoinJoin)", | ||
| ); | ||
| return; | ||
| } | ||
| Some(concrete) => concrete, | ||
| }; | ||
| if account_index < 0 { | ||
| throw_sdk_exception(env, 1, "accountIndex must be non-negative"); | ||
| return; | ||
| } | ||
| if gap_limit <= 0 { | ||
| throw_sdk_exception(env, 1, "gapLimit must be positive"); | ||
| return; | ||
| } | ||
| let result = unsafe { | ||
| platform_wallet_ffi::core_wallet_set_gap_limit( | ||
| wallet_handle as Handle, | ||
| account_type, | ||
| account_index as u32, | ||
| gap_limit as u32, | ||
| ) | ||
| }; | ||
| let _ = take_pwffi_error(env, result); |
There was a problem hiding this comment.
🟡 Suggestion: Add an instrumented binding test for the new JNI export
No repository test invokes coreWalletSetGapLimit, so the Kotlin external declaration, generated JNI symbol, parameter descriptor, and the new validation branches can regress without detection. cargo check -p rs-unified-sdk-jni verifies only the Rust side and cannot detect a Kotlin/JNI naming or signature mismatch that would produce UnsatisfiedLinkError on Android. The existing Android instrumented suite already uses invalid handles to pin JNI bindings without requiring a funded wallet; add equivalent coverage that loads the native library, verifies that account type 3, a negative account index, and a non-positive gap limit are rejected as invalid parameters, then calls a concrete account type with handle 0 and verifies that execution reaches the underlying FFI's invalid-handle path.
source: ['codex']
2 participants
Issue being fixed or feature implemented
A wallet migrated from dashj can be silently blind to its own funds: if a same-seed client (the shipped dashj app, still run in parallel by migrated users) spends past the SDK's derived address window, the change output — and every descendant transaction — never enters the SDK's filter-scan query. The wallet reports synced with the wrong balance and missing history.
Field case (mainnet, long-lived ~150-contact wallet): a 70 DASH payment's 37.17 change went to an address one step past the watched window. The SDK saw the 70 leave and never saw the change return — confirmed balance collapsed from 70.6 to 0.6 at the spend height, and everything downstream of that change stayed invisible.
The Rust seam to fix this existed end-to-end (
AddressPool::set_gap_limit→ManagedCoreFundsAccount→CoreWallet::set_gap_limit→ thecore_wallet_set_gap_limitC export, in-tree since #3970) but stopped at the C boundary — no JNI trampoline, so no Kotlin host could reach it.What was done?
~40 lines of plumbing, no engine changes:
Java_…_WalletManagerNative_coreWalletSetGapLimit(account-type mapping via the existingcore_account_type, same guard shape as the sibling exports).external fundeclaration and aManagedCoreWallet.setGapLimit(accountType, accountIndex, gapLimit)wrapper undermapNativeErrors. Rust clamps toMAX_GAP_LIMIT = 1000;mark_usedkeeps rolling the window forward during the subsequent re-scan, so frontiers deeper than one window still recover.The intended host usage is: widen the standard families once (BIP44/BIP32/CoinJoin), rewind the SPV filter watermark (
rescanSpvFilters, already exposed), and let the scan re-match history against the widened script set. The Android wallet implements this as a one-shot per-version migration heal.How Has This Been Tested?
cargo check -p rs-unified-sdk-jniclean on this branch.🤖 Generated with Claude Code
Summary by CodeRabbit