feat(kotlin-sdk): expose core_wallet_next_receive_address / next_change_address (Swift parity) - #4260
Conversation
…ge_address Kotlin parity for the existing address-derivation FFI: iOS binds core_wallet_next_receive_address directly (SwiftDashSDKReceiveAddressReader -> coreWallet().nextReceiveAddress), but no rs-unified-sdk-jni/Kotlin plumbing existed - Android callers had to read the Room core_addresses mirror instead. Adds the two JNI wrappers (same guard/take_pwffi_error/ core_wallet_free_address pattern as the neighboring core-wallet calls) and ManagedCoreWallet.nextReceiveAddress/nextChangeAddress(accountIndex). Engine-authoritative (in-memory used-set); same semantics and cold-start caveat as the Swift binding, documented on the Kotlin surface: no issued-marker, so repeated calls return the same address until it is seen on-chain. cargo check -p rs-unified-sdk-jni clean; :sdk:compileReleaseKotlin clean. 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 Kotlin SDK adds methods for the next unused receive and change addresses. The JNI layer validates inputs, calls platform-wallet FFI functions, converts native strings, maps errors, and releases native buffers. ChangesWallet next-address retrieval
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ManagedCoreWallet
participant WalletManagerNative
participant JNIWalletManager
participant PlatformWalletFFI
ManagedCoreWallet->>WalletManagerNative: request next receive or change address
WalletManagerNative->>JNIWalletManager: invoke native binding
JNIWalletManager->>PlatformWalletFFI: call address retrieval function
PlatformWalletFFI-->>JNIWalletManager: return native address or error
JNIWalletManager-->>WalletManagerNative: return Java string or exception
🚥 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 |
|
ℹ️ Review skipped (commit 10db4ad) |
![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
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt (1)
58-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCite the iOS source file in the KDoc.
Lines 60-62 name Swift symbols but do not cite the corresponding Swift source file. Add the repository-relative Swift file reference for the receive behavior and the change behavior.
As per coding guidelines, “When porting behavior from iOS, cite the corresponding Swift source file in KDoc.”
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt` around lines 58 - 84, Update the KDoc for nextReceiveAddress and its corresponding change-address accessor to cite the repository-relative Swift source file that defines the iOS receive and change behaviors. Keep the existing Swift symbol references and behavioral documentation, adding only the source-file references required by the portability guideline.Source: Coding guidelines
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- Around line 73-88: Add automated coverage for
ManagedCoreWallet.nextReceiveAddress and nextChangeAddress using the packaged
native library; verify both return the current address on repeated calls before
the used-set changes, and assert that negative accountIndex values are rejected.
---
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- Around line 58-84: Update the KDoc for nextReceiveAddress and its
corresponding change-address accessor to cite the repository-relative Swift
source file that defines the iOS receive and change behaviors. Keep the existing
Swift symbol references and behavioral documentation, adding only the
source-file references required by the portability guideline.
🪄 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 Plus
Run ID: 95f84cd8-e4d3-4e68-9165-9b6199a7bb3b
📒 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
| fun nextReceiveAddress(accountIndex: Int = 0): String { | ||
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | ||
| return WalletManagerNative.coreWalletNextReceiveAddress(handle, accountIndex) | ||
| } | ||
|
|
||
| /** | ||
| * The engine's next unused BIP-44 INTERNAL (change) address for | ||
| * [accountIndex], base58-encoded — the change-side twin of | ||
| * [nextReceiveAddress]; same used-set semantics and cold-start | ||
| * caveat. Builds pick change themselves (`setFunding`); this | ||
| * accessor exists for callers that must NAME a change address | ||
| * up front (e.g. `CoreTransactionBuilder.setChangeAddress`). | ||
| */ | ||
| fun nextChangeAddress(accountIndex: Int = 0): String { | ||
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | ||
| return WalletManagerNative.coreWalletNextChangeAddress(handle, accountIndex) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add automated coverage for both address APIs.
No test change accompanies these public JNI-backed methods, and the PR objective states that on-device testing is pending. Add coverage that calls both methods through the packaged native library, rejects negative account indices, and verifies repeated calls return the current address before the used-set changes.
As per coding guidelines, “Keep pull requests focused, link related issues, include tests, and complete the pull request template.”
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`
around lines 73 - 88, Add automated coverage for
ManagedCoreWallet.nextReceiveAddress and nextChangeAddress using the packaged
native library; verify both return the current address on repeated calls before
the used-set changes, and assert that negative accountIndex values are rejected.
Source: Coding guidelines
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The JNI bridge's handle/index validation and C-string copy/free path are sound. The public Kotlin methods nevertheless bypass the SDK's required native-error mapping, allowing an internal exception type to escape; the class documentation is also stale, and the new JNI paths lack automated device coverage. Request changes for the public error-contract violation, with documentation and tests as non-blocking follow-ups.
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— security-auditor (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 nitpick(s)
1 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt:73-88: Map native address errors into the public SDK hierarchy
Both new public methods call an `external fun` directly. The JNI functions use `take_pwffi_error` and `throw_sdk_exception`, so failures such as a nonexistent account or invalid native handle are raised as the internal `DashSDKException`. `DashSdkError.kt` explicitly requires every public SDK entry point that calls native code to use `mapNativeErrors`; without it, callers expecting `DashSdkError.NotFound` or `DashSdkError.PlatformWallet` receive an implementation-level exception instead. Wrap both native calls at this public boundary.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt:73-88: Add device coverage for both address APIs
No automated test exercises either new Kotlin-to-JNI path. This repository already builds the packaged native library and runs `connectedDebugAndroidTest`, while `WalletManagerRoundTripTest` creates a default wallet offline, so coverage can verify the actual exported symbols without network access. Add an instrumented test that calls both methods, confirms repeated calls return the same unused address, and confirms negative account indices are rejected; this protects the JNI signatures, handle plumbing, and documented current-address semantics.
| fun nextReceiveAddress(accountIndex: Int = 0): String { | ||
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | ||
| return WalletManagerNative.coreWalletNextReceiveAddress(handle, accountIndex) | ||
| } | ||
|
|
||
| /** | ||
| * The engine's next unused BIP-44 INTERNAL (change) address for | ||
| * [accountIndex], base58-encoded — the change-side twin of | ||
| * [nextReceiveAddress]; same used-set semantics and cold-start | ||
| * caveat. Builds pick change themselves (`setFunding`); this | ||
| * accessor exists for callers that must NAME a change address | ||
| * up front (e.g. `CoreTransactionBuilder.setChangeAddress`). | ||
| */ | ||
| fun nextChangeAddress(accountIndex: Int = 0): String { | ||
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | ||
| return WalletManagerNative.coreWalletNextChangeAddress(handle, accountIndex) |
There was a problem hiding this comment.
🔴 Blocking: Map native address errors into the public SDK hierarchy
Both new public methods call an external fun directly. The JNI functions use take_pwffi_error and throw_sdk_exception, so failures such as a nonexistent account or invalid native handle are raised as the internal DashSDKException. DashSdkError.kt explicitly requires every public SDK entry point that calls native code to use mapNativeErrors; without it, callers expecting DashSdkError.NotFound or DashSdkError.PlatformWallet receive an implementation-level exception instead. Wrap both native calls at this public boundary.
| fun nextReceiveAddress(accountIndex: Int = 0): String { | |
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | |
| return WalletManagerNative.coreWalletNextReceiveAddress(handle, accountIndex) | |
| } | |
| /** | |
| * The engine's next unused BIP-44 INTERNAL (change) address for | |
| * [accountIndex], base58-encoded — the change-side twin of | |
| * [nextReceiveAddress]; same used-set semantics and cold-start | |
| * caveat. Builds pick change themselves (`setFunding`); this | |
| * accessor exists for callers that must NAME a change address | |
| * up front (e.g. `CoreTransactionBuilder.setChangeAddress`). | |
| */ | |
| fun nextChangeAddress(accountIndex: Int = 0): String { | |
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | |
| return WalletManagerNative.coreWalletNextChangeAddress(handle, accountIndex) | |
| fun nextReceiveAddress(accountIndex: Int = 0): String { | |
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | |
| return org.dashfoundation.dashsdk.errors.mapNativeErrors { | |
| WalletManagerNative.coreWalletNextReceiveAddress(handle, accountIndex) | |
| } | |
| } | |
| /** | |
| * The engine's next unused BIP-44 INTERNAL (change) address for | |
| * [accountIndex], base58-encoded — the change-side twin of | |
| * [nextReceiveAddress]; same used-set semantics and cold-start | |
| * caveat. Builds pick change themselves (`setFunding`); this | |
| * accessor exists for callers that must NAME a change address | |
| * up front (e.g. `CoreTransactionBuilder.setChangeAddress`). | |
| */ | |
| fun nextChangeAddress(accountIndex: Int = 0): String { | |
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | |
| return org.dashfoundation.dashsdk.errors.mapNativeErrors { | |
| WalletManagerNative.coreWalletNextChangeAddress(handle, accountIndex) | |
| } | |
| } |
source: ['codex']
There was a problem hiding this comment.
Fixed in 70a4192 — both accessors now wrap their native call in mapNativeErrors, and the stale class KDoc is refreshed. 10db4ad adds host-JVM coverage: negative-index and closed-handle guards, the DashSDKException(7) → DashSdkError.NotFound mapping, and a source-scanning lint test (GateCoverageLintTest pattern) that structurally enforces the mapNativeErrors wrapper on both accessors so this cannot regress. Instrumented device coverage remains the documented follow-up. Ready for revalidation.
🤖 Generated with Claude Code
| fun nextReceiveAddress(accountIndex: Int = 0): String { | ||
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | ||
| return WalletManagerNative.coreWalletNextReceiveAddress(handle, accountIndex) | ||
| } | ||
|
|
||
| /** | ||
| * The engine's next unused BIP-44 INTERNAL (change) address for | ||
| * [accountIndex], base58-encoded — the change-side twin of | ||
| * [nextReceiveAddress]; same used-set semantics and cold-start | ||
| * caveat. Builds pick change themselves (`setFunding`); this | ||
| * accessor exists for callers that must NAME a change address | ||
| * up front (e.g. `CoreTransactionBuilder.setChangeAddress`). | ||
| */ | ||
| fun nextChangeAddress(accountIndex: Int = 0): String { | ||
| require(accountIndex >= 0) { "accountIndex must be non-negative, got $accountIndex" } | ||
| return WalletManagerNative.coreWalletNextChangeAddress(handle, accountIndex) |
There was a problem hiding this comment.
🟡 Suggestion: Add device coverage for both address APIs
No automated test exercises either new Kotlin-to-JNI path. This repository already builds the packaged native library and runs connectedDebugAndroidTest, while WalletManagerRoundTripTest creates a default wallet offline, so coverage can verify the actual exported symbols without network access. Add an instrumented test that calls both methods, confirms repeated calls return the same unused address, and confirms negative account indices are rejected; this protects the JNI signatures, handle plumbing, and documented current-address semantics.
source: ['coderabbit']
…ndary Review blocker (thepastaclaw): nextReceiveAddress/nextChangeAddress called the external fun directly, letting the JNI layer's internal DashSDKException escape a public SDK entry point. Wrap both calls in mapNativeErrors per the DashSdkError.kt contract, and refresh the stale ManagedCoreWallet class KDoc (the class now carries address accessors, not just broadcast entry points). :sdk:compileReleaseKotlin clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ndary Four host-JVM tests for nextReceiveAddress/nextChangeAddress covering everything on the Kotlin side of the JNI boundary (the native lib cannot load on the host): negative accountIndex rejected before any native call, the closed-handle guard fires first, DashSDKException(7) maps to DashSdkError.NotFound through mapNativeErrors, and - in the GateCoverageLintTest tradition - a source-scanning guard that both public accessors wrap their WalletManagerNative call in mapNativeErrors, making the review blocker structurally non-regressable. Real JNI linkage, derivation-vector, and used-set semantics remain the instrumented follow-up (connectedDebugAndroidTest). 4/4 pass via :sdk:testDebugUnitTest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
What
Kotlin parity for the existing address-derivation FFI.
rs-platform-wallet-ffihas shippedcore_wallet_next_receive_address/core_wallet_next_change_addressfor some time, and iOS binds the C ABI directly (SwiftDashSDKReceiveAddressReader→coreWallet().nextReceiveAddress(accountIndex:)— the DashWallet-iOS Receive screen's source). No rs-unified-sdk-jni/Kotlin plumbing existed, so Android callers (including the SDK's own KotlinExampleAppReceiveAddressSheet) have been reading the Roomcore_addressesmirror instead of asking the engine.rs-unified-sdk-jni/src/wallet_manager.rs:coreWalletNextReceiveAddress/coreWalletNextChangeAddress— sameguard/take_pwffi_error/core_wallet_free_addresspattern as the neighboring core-wallet JNI calls.WalletManagerNative.kt: the twoexternal fundeclarations.ManagedCoreWallet.kt: publicnextReceiveAddress(accountIndex: Int = 0)/nextChangeAddress(accountIndex: Int = 0), named to mirror the Swift surface.Semantics (documented on the Kotlin surface)
Engine-authoritative over the in-memory used-set — "unused" means never seen on-chain. There is no issued-marker, so repeated calls return the same address until it receives funds (current-address semantics, matching iOS exactly, including the documented cold-start caveat: index 0 until SPV replay populates the used-set). A per-invoice
fresh_addresswith an engine-side issued-marker is a separate, future ask — deliberately out of scope here.Why now
dash-wallet's BIP70 cutover work (dashpay/dash-wallet#1531) sources
Payment.refund_tofrom the Room pool as a stopgap; once this lands in a published AAR, that Room read swaps forcoreWallet().nextReceiveAddress()(one function body, no callers change), and the same call becomes the Receive screen's post-dashj source in Phase 2 — identical to how iOS already works.Verified
cargo check -p rs-unified-sdk-jniclean;:sdkKotlin compile clean. Purely additive — no existing surface touched. Not yet exercised on-device (needs an AAR rebuild viabuild_android.sh); the FFI itself is already proven in production by iOS.🤖 Generated with Claude Code
Summary by CodeRabbit