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 5 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>
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 (2)
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe change adds an account-specific ChangesWallet gap-limit API
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds a localized Kotlin/JNI bridge for widening migrated-wallet address windows, with the described recovery and regression tests covering the intended behavior. No actionable merge-blocking risk remains beyond normal checks and review. 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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
✅ Final review complete — no blockers (commit ceb42b6) |
![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.63% 85.33% -2.30%
============================================
Files 2670 2712 +42
Lines 339447 355932 +16485
============================================
+ Hits 297465 303750 +6285
- Misses 41982 52182 +10200
🚀 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']
There was a problem hiding this comment.
Added in 25000de — CoreWalletSetGapLimitBindingTest (androidTest, mirrors the existing CoreTxBuilderOpReturnBindingTest no-wallet pattern): five cases pin the validation branches (AllSpendable and unknown account types, negative accountIndex, gapLimit 0/-1) to raw code 1 with branch-naming message assertions that double as int-parameter-order pins, and each concrete account type with a dead handle must surface an FFI-translated code >= PLATFORM_WALLET_CODE_OFFSET — proving execution crossed the JNI symbol into core_wallet_set_gap_limit rather than dying in the trampoline. Compiles green (:sdk:compileDebugAndroidTestKotlin); rides the next instrumented device run since the only prebuilt local .so predates the export.
There was a problem hiding this comment.
Resolved in this update — Add an instrumented binding test for the new JNI export 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.
There was a problem hiding this comment.
Added in the commit above — setGapLimitSymbolBindsAndRejectsInvalidArguments in FfiSmokeTest.
It follows the existing syncFaultDetectedSymbolBindsAndRejectsInvalidHandle pattern you pointed at, so it needs no funded wallet: every case is rejected before the handle is dereferenced, which makes an invalid handle a legitimate argument rather than a workaround.
Covers all three validation branches — AllSpendable (3), a negative account index, and a zero/negative gap limit — plus otherwise-valid arguments against a null handle. That last case is the control: it proves the earlier rejections came from the validation branches and not merely from the bad handle.
On your point about UnsatisfiedLinkError: that is exactly why this is worth having as an instrumented test. A binding or descriptor mismatch raises UnsatisfiedLinkError, which is not a DashSDKException, so assertThrows(DashSDKException::class.java) fails loudly rather than passing on the wrong exception.
There was a problem hiding this comment.
Done — added in 6637b684 as a dedicated instrumented suite, CoreWalletSetGapLimitBindingTest, rather than folding it into the smoke test. It loads the native library and calls through the real external fun (so a name/descriptor mismatch surfaces as UnsatisfiedLinkError, which is not a DashSDKException and fails the assertion loudly rather than passing on the wrong exception), and needs no funded wallet because every case is rejected before the handle is dereferenced:
allSpendableAggregateIsRejectedBeforeTheFfi— account type3unknownAccountTypeIsRejectedBeforeTheFfinegativeAccountIndexIsRejectedBeforeTheFfinonPositiveGapLimitIsRejectedBeforeTheFfi— zero and negativeconcreteAccountTypesReachTheFfiInvalidHandlePath— all three concrete mappings with handle0, proving the earlier rejections came from validation rather than the handle
It also asserts on branch-specific messages, so parameter ordering is pinned too.
…imit No test invoked the new JNI export, so the Kotlin external declaration, generated symbol, parameter descriptor, and the trampoline's validation branches could regress silently (an UnsatisfiedLinkError only in production). Mirror the CoreTxBuilderOpReturnBindingTest no-wallet discipline: pin the AllSpendable (3) and unknown account-type rejections, the negative accountIndex rejection, and the non-positive gapLimit rejections to the JNI-side DashSDKException (raw code 1, branch- naming messages — which also pins the int parameter order), then prove every concrete account type (0/1/2) passes validation and crosses into core_wallet_set_gap_limit by asserting handle 0 surfaces the FFI's translated error in the platform-wallet code range instead. Validated with :sdk:compileDebugAndroidTestKotlin (BUILD SUCCESSFUL); an on-device run needs a libdash_sdk_jni.so built from this branch — the machine's only prebuilt binary predates the export. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current head correctly exposes the per-account gap-limit operation through Kotlin and JNI, rejects invalid signed inputs and the AllSpendable aggregate, and forwards concrete account requests through the existing platform-wallet FFI error path. The newly added Android instrumented test invokes the native export and covers symbol resolution, argument ordering, all JNI validation branches, all three concrete account types, and native error translation; no in-scope issues remain.
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)
|
Status for a resolution pass — head `25000dea5`. The single thread (instrumented binding test for coreWalletSetGapLimit) is addressed in `25000dea58`. MERGEABLE, workspace tests passing. Ready to resolve — thanks. |
#4377 review finding 3c43490f1527: no repository test invoked the new export, so the Kotlin external declaration, the generated JNI symbol, its parameter descriptor and the validation branches could all regress silently. `cargo check -p rs-unified-sdk-jni` proves only that the Rust compiles; it cannot catch a name or descriptor mismatch between the Kotlin `external fun` and the generated symbol, which surfaces as an UnsatisfiedLinkError on device rather than at build time. Loading the library and calling through the real declaration is the only thing that pins that contract, hence instrumented rather than unit. Follows the existing invalid-handle pattern (syncFaultDetectedSymbolBindsAndRejects InvalidHandle), so it needs no funded wallet: every case is rejected before the handle is dereferenced. Covers all three validation branches — AllSpendable (3), a negative account index, and a zero/negative gap limit — plus otherwise-valid arguments against a null handle, which proves the earlier rejections came from validation rather than from the handle. A binding failure raises UnsatisfiedLinkError, which is NOT a DashSDKException, so the assertions fail loudly instead of passing on the wrong exception.
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 wrapper and JNI trampoline correctly expose the per-account gap-limit operation, reject invalid inputs and the AllSpendable aggregate, and preserve native error translation. The only remaining in-scope suggestion is to remove the redundant FfiSmokeTest coverage because the dedicated binding suite runs in the same unfiltered Android task and covers the contract more precisely.
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/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/FfiSmokeTest.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/FfiSmokeTest.kt:105-145: Remove the duplicate gap-limit instrumented test
This method duplicates a subset of `CoreWalletSetGapLimitBindingTest`, and both classes run under the same unfiltered `:sdk:connectedDebugAndroidTest` task in CI. The dedicated suite already invokes the JNI symbol and covers AllSpendable, unknown account types, negative indexes, non-positive limits, all three concrete account mappings, parameter ordering through branch-specific messages, raw JNI validation codes, and translated FFI errors. Keeping this second copy adds redundant device execution and another test that must be updated whenever the binding contract changes without providing distinct coverage; retain the dedicated binding test and remove this method and its KDoc.
| /** | ||
| * `coreWalletSetGapLimit` binds, and rejects every invalid argument. | ||
| * | ||
| * `cargo check -p rs-unified-sdk-jni` proves only the Rust side compiles; | ||
| * it cannot catch a name or descriptor mismatch between the Kotlin | ||
| * `external fun` and the generated JNI symbol, which surfaces as an | ||
| * `UnsatisfiedLinkError` on device rather than at build time. Loading the | ||
| * library and calling through the real declaration is the only thing that | ||
| * pins that contract — hence an instrumented test rather than a unit one. | ||
| * | ||
| * Needs no funded wallet: every case here is rejected before the handle is | ||
| * dereferenced, so an invalid handle is a legitimate argument. A binding | ||
| * failure surfaces as `UnsatisfiedLinkError`, which is NOT a | ||
| * `DashSDKException`, so the assertion below fails loudly rather than | ||
| * passing on the wrong exception. | ||
| */ | ||
| @Test | ||
| fun setGapLimitSymbolBindsAndRejectsInvalidArguments() { | ||
| NativeLoader.ensureLoaded() | ||
|
|
||
| // AllSpendable (3) is not a single account — a gap limit is per-account. | ||
| assertThrows(DashSDKException::class.java) { | ||
| WalletManagerNative.coreWalletSetGapLimit(0L, 3, 0, 20) | ||
| } | ||
| // Negative account index. | ||
| assertThrows(DashSDKException::class.java) { | ||
| WalletManagerNative.coreWalletSetGapLimit(0L, 0, -1, 20) | ||
| } | ||
| // Non-positive gap limit: zero and negative. | ||
| assertThrows(DashSDKException::class.java) { | ||
| WalletManagerNative.coreWalletSetGapLimit(0L, 0, 0, 0) | ||
| } | ||
| assertThrows(DashSDKException::class.java) { | ||
| WalletManagerNative.coreWalletSetGapLimit(0L, 0, 0, -5) | ||
| } | ||
| // Otherwise-valid arguments against a null handle still fail — proving | ||
| // the earlier rejections came from validation and not from the handle. | ||
| assertThrows(DashSDKException::class.java) { | ||
| WalletManagerNative.coreWalletSetGapLimit(0L, 0, 0, 20) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Remove the duplicate gap-limit instrumented test
This method duplicates a subset of CoreWalletSetGapLimitBindingTest, and both classes run under the same unfiltered :sdk:connectedDebugAndroidTest task in CI. The dedicated suite already invokes the JNI symbol and covers AllSpendable, unknown account types, negative indexes, non-positive limits, all three concrete account mappings, parameter ordering through branch-specific messages, raw JNI validation codes, and translated FFI errors. Keeping this second copy adds redundant device execution and another test that must be updated whenever the binding contract changes without providing distinct coverage; retain the dedicated binding test and remove this method and its KDoc.
source: ['codex']
There was a problem hiding this comment.
Agreed and removed in ceb42b68 — you're right that it was a strict subset of CoreWalletSetGapLimitBindingTest and bought only redundant device time plus a second place to update when the binding contract moves. The dedicated suite is now the only coverage of this symbol.
There was a problem hiding this comment.
Resolved in ceb42b6 — Remove the duplicate gap-limit instrumented test 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.
`CoreWalletSetGapLimitBindingTest` already pins the JNI binding on device and covers a strict superset of this method: AllSpendable, unknown account types, negative indexes, non-positive limits, all three concrete account mappings, parameter ordering via branch-specific messages, the raw JNI validation codes, and the translated FFI errors. Both classes run under the same unfiltered `:sdk:connectedDebugAndroidTest`, so the copy only bought redundant device time and a second place to update whenever the binding contract moves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head ceb42b6, the Kotlin declaration, managed wrapper, JNI validation and forwarding, and dedicated Android binding coverage are consistent; no in-scope defects remain. The prior duplicate-test finding is fixed because commit ceb42b6 removed the redundant FfiSmokeTest method and KDoc while retaining the broader CoreWalletSetGapLimitBindingTest suite as the sole coverage for this symbol. 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)
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