feat(sdk): add DashPay sync service, seedless unlock and writes to Kotlin SDK (migration K2)#4027
feat(sdk): add DashPay sync service, seedless unlock and writes to Kotlin SDK (migration K2)#4027shumkov wants to merge 2 commits into
Conversation
Second milestone of the DashPay Swift→Kotlin migration (docs/dashpay/KOTLIN_MIGRATION_SPEC.md §K2). Security pre-req — mnemonic out-buffer discipline (spec review must-fix): the resolver contract is now resolveMnemonicInto(byte[], byte[]): Int — Kotlin writes raw UTF-8 phrase bytes into a caller-sized buffer and zeroes every intermediate copy; the trampoline copies into Rust-owned memory and scrubs the Java buffer. No java.lang.String of the seed phrase exists anywhere on the resolver path (WalletStorage.retrieveMnemonicUtf8 + hasMnemonic existence check; KeystoreSigner capability check no longer decrypts). signWithMnemonicAndPath now scrubs its mnemonic CString on every path, including the NulError one. Bridges (13 exports in dashpay.rs/DashpayNative.kt): the 7 platform_wallet_manager_dashpay_sync_* fns, the seedless trio (verify_seed_binds_to_wallet, pending_contact_crypto_count, drain_pending_contact_crypto), profile/contactInfo writes, and dash_sdk_resolver_supports_key_type. Manager surface (PlatformWalletManager, following the codebase convention of manager-owned native loops — Swift keeps these in a manager extension too): - start/stop/isRunning/isSyncing/lastSync/setInterval/syncNow + dashPaySyncIsSyncing StateFlow via the 1 Hz change-gated poll (matching startSpvProgressPolling; poll-not-events is deliberate, it is exactly how iOS does it). - unlockWalletFromKeystore: verify → drain, auto-run per restored wallet inside loadPersistedWallets (load-bearing: the deferred contact-crypto queue is in-memory by design and self-heals only if every launch runs load → unlock → sweep). Seed-mismatch published from the verify result only (ErrorInvalidParameter scoped to that call); draining re-entrancy guard; per-wallet dashPayUnlockStatus StateFlow (draining / seedMismatch / pendingAccountBuilds). - Breadcrumb backfill deliberately NOT ported (no pre-breadcrumb Android installs exist — recorded in the spec). Dashpay surface: createOrUpdateProfile (avatar bytes hashed Rust-side) and setContactInfo returning ContactInfoPublishOutcome (the DIP-15 deferred-until-two-contacts state the UI must surface). Tests: instrumented DashPayUnlockAndSyncTest — the happy-path unlock is the end-to-end proof of the out-buffer resolver (verify derives the BIP44 xpub through the new vtable), the wrong-seed leg pins the seedMismatch contract with a foreign mnemonic fixture, and the sync-lifecycle leg pins start/stop/idempotent-start and manager-swap disposal. dashPaySyncNow and the write paths are network-bound and stay in the -Ptestnet=true tier. JVM suite 28/28 green; instrumented tests compile (CI emulator executes them). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three-lens review of f0ebdfc (mnemonic security, concurrency/ lifecycle, JNI bridge correctness) — two must-fixes and the actionable should-fixes folded: - Drain use-after-free (concurrency must-fix): the background drain now creates its OWN resolver + signer, owned by the coroutine and closed in its finally (the Swift Task.detached + withExtendedLifetime shape). Borrowing the manager's shared children risked dereferencing freed vtables when close()/manager-swap raced a slow blocking drain — resolver/signer handles are raw boxes, not storage-keyed like wallet handles. - dashpay.rs read_id32 threw through DashSDKException's non-existent (String) constructor → NoSuchMethodError instead of a DashSdkError (or a silent guard-default: a malformed contactId reported the benign DEFERRED outcome). Now throw_sdk_exception + null guard, matching the sibling readers. - signer.rs seed scrubbing completed: non-secret args decode first so a sibling failure can't orphan a decoded phrase, and reserve_exact(1) before CString::new prevents the NUL-append realloc from freeing the original plaintext allocation unscrubbed. - Drain re-entrancy guard is now a single atomic false→true transition on the status flow (the separate check-then-set let two concurrent unlocks stack drains). - loadPersistedWallets publishes each wallet into [wallets] BEFORE unlocking (Swift's ordering) — the 1 Hz poll's stale-key prune could otherwise silently drop a just-published seedMismatch; the status poll now also starts on load (banner state no longer depends on the sweep service running), and stopDashPaySync leaves it running. - RESOLVE_OTHER (-3) channel added across bridge/trampoline/resolver: a Keystore/decrypt failure is no longer reported as 'no seed stored' (the iOS .other discipline); zero-capacity out buffer rejected up front (removes a 1-byte OOB NUL edge, unreachable via the real caller). - Wording/marker fixes: 'wrong seed can never sign' scoped to network-valid signatures (enforcement is on-chain key ownership, not a signer gate); the String-shaped platform-address signing path is marked as the tracked residual exposure Kotlin-side; interior-NUL clear-vs-truncate divergence documented. Gates: cargo check/clippy/fmt clean; JVM 28/28; instrumented compile green (CI emulator executes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 complete (commit b8bcf7c) |
There was a problem hiding this comment.
Code Review
K2 layers DashPay sync, seedless unlock, and profile/contactInfo writes onto a carefully-designed mnemonic-out-buffer boundary. The core discipline (out-buffer contract, per-drain resolver/signer ownership, atomic drain slot) holds up. Real issues remain around Kotlin coroutine hygiene at the seedless-unlock boundary — a background drain path can leak the resolver's native handle if the signer constructor throws, CancellationException is silently swallowed in two spots, and one JNI marshalling path skips the mnemonic scrub on error. No blocking issues.
Source: reviewers: general=claude/opus, general=codex/gpt-5.5, security-auditor=claude/opus, security-auditor=codex/gpt-5.5, ffi-engineer=claude/opus, ffi-engineer=codex/gpt-5.5; verifier=claude/opus.
🟡 3 suggestion(s) | 💬 2 nitpick(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/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:1205-1224: Drain launch leaks resolver native handle if signer constructor throws
In the drain-launch block, `drainResolver = MnemonicResolverAndPersister(walletStorage)` immediately claims a `MnemonicResolverHandle` via `MnemonicNative.createResolver(this)`. If the very next line's `KeystoreSigner(...)` constructor throws (e.g. a JNI/global-ref failure surfaced from `SignerNative.createSigner(this)`), the coroutine unwinds before the `try` opens, so the `finally` closing `drainResolver` never runs and its native handle leaks for the process lifetime. The main `PlatformWalletManager` constructor already handles the analogous hazard at lines 352–364 with a nested try that closes the resolver on rethrow — the drain-launch construction sequence needs the same shape.
- [NITPICK] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:499-506: loadPersistedWallets swallows CancellationException from unlockWalletFromKeystore
`unlockWalletFromKeystore(managed)` is a suspend function that throws `kotlinx.coroutines.CancellationException` when the caller scope is cancelled. `CancellationException` extends `java.lang.Exception`, so `catch (_: Exception)` catches it and the loop continues restoring wallets, breaking structured concurrency — a cancellation from an outer scope (manager swap, teardown) cannot stop the per-wallet unlock loop. The comment correctly justifies swallowing wallet-scoped `DashSdkError`, but that reasoning does not apply to cancellation. Rethrow cancellation explicitly (`if (e is CancellationException) throw e`) or replace the catch with a `coroutineContext.ensureActive()` before the swallow. The same anti-pattern appears in the drain launch at line 1217.
- [NITPICK] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:1205-1224: Drain job is not awaited before nativeDestroy — persistence callbacks may be freed mid-drain
`close()` calls `scope.cancel()` and then `WalletManagerNative.nativeDestroy(bundle)`, but `scope.cancel()` does not wait for the launched drain to return from its blocking JNI call in `drainPendingContactCrypto`. The comment at lines 1191–1203 correctly identifies this hazard for the resolver/signer (fixed by per-drain fresh handles) but does not extend the same reasoning to wallet-level persistence callbacks that may reach through the manager bundle's persistence/event GlobalRefs. If a slow drain is mid-call when `nativeDestroy` frees those contexts, a persistence write from the drain could hit freed refs. If the wallet-side persister is fully storage-keyed and independent of the manager bundle this is a non-issue — worth confirming and, if not, tracking the launched drain job so `close()` can join/quiesce it before destroy.
In `packages/rs-unified-sdk-jni/src/mnemonic.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/mnemonic.rs:71-107: resolve_trampoline: `?` on get_byte_array_region skips the Java out-buffer scrub
Inside `with_local_frame`, `env.get_byte_array_region(&jout, 0, dst)?` (line 99) copies the phrase bytes out of the Java out-buffer. The trailing `env.set_byte_array_region(&jout, 0, &zeros)?` at line 106 that scrubs the Java buffer sits at the end of the closure — a failure inside `get_byte_array_region` unwinds through `?` and skips it. At that moment the Kotlin side has already written the plaintext BIP-39 phrase into `jout` (that is what is being copied out), so failure here leaves the mnemonic plaintext sitting in the JVM byte[] backing `jout` until GC. This is precisely the exposure the module-level out-buffer contract (lines 36–44) exists to eliminate. Scrub `jout` before propagating any copy error so the invariant holds on every exit path, including error.
In `packages/rs-unified-sdk-jni/src/dashpay.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/dashpay.rs:692-696: createOrUpdateProfile silently drops avatar bytes on JNI conversion failure
`env.convert_byte_array(&avatar_bytes).ok()` collapses any conversion failure into `None`, indistinguishable from the caller intentionally passing null. The `avatar_bytes.is_null()` predicate already distinguishes the two cases, so when the caller supplied a non-null byte[] but the JNI copy failed, the FFI is invoked with a null avatar pointer and length 0 — the profile state transition is published with no avatar, but the caller's `success` return value hides the loss. Given the write is on-chain, throwing `DashSDKException` (as `read_id32` does for its byte-array inputs) on a non-null conversion failure is safer than silently committing partial content.
| scope.launch(Dispatchers.IO) { | ||
| val drainResolver = MnemonicResolverAndPersister(walletStorage) | ||
| val drainSigner = | ||
| KeystoreSigner(walletStorage, network, biometricGate, database.platformAddressDao()) | ||
| try { | ||
| mapNativeErrors { | ||
| DashpayNative.drainPendingContactCrypto( | ||
| walletHandle, | ||
| drainSigner.nativeHandle, | ||
| drainResolver.nativeHandle, | ||
| ) | ||
| } | ||
| } catch (_: Exception) { | ||
| // Not fatal: the next signer-present DashPay action (or the | ||
| // next unlock) re-attempts; the queue rebuilds via the sweep. | ||
| } finally { | ||
| updateUnlockStatus(key) { it.copy(draining = false) } | ||
| runCatching { drainResolver.close() } | ||
| runCatching { drainSigner.close() } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Drain launch leaks resolver native handle if signer constructor throws
In the drain-launch block, drainResolver = MnemonicResolverAndPersister(walletStorage) immediately claims a MnemonicResolverHandle via MnemonicNative.createResolver(this). If the very next line's KeystoreSigner(...) constructor throws (e.g. a JNI/global-ref failure surfaced from SignerNative.createSigner(this)), the coroutine unwinds before the try opens, so the finally closing drainResolver never runs and its native handle leaks for the process lifetime. The main PlatformWalletManager constructor already handles the analogous hazard at lines 352–364 with a nested try that closes the resolver on rethrow — the drain-launch construction sequence needs the same shape.
source: ['claude']
| match env.with_local_frame(16, |env| -> Result<i32, jni::errors::Error> { | ||
| let wallet_id = std::slice::from_raw_parts(wallet_id_bytes, 32); | ||
| let jwallet_id = env.byte_array_from_slice(wallet_id)?; | ||
| let value = env | ||
| // Reserve one byte of the Rust capacity for the NUL terminator. | ||
| let usable = out_capacity - 1; | ||
| let jout = env.new_byte_array(usable as i32)?; | ||
| let written = env | ||
| .call_method( | ||
| ctx.bridge.as_obj(), | ||
| "resolveMnemonic", | ||
| "([B)Ljava/lang/String;", | ||
| &[(&jwallet_id).into()], | ||
| "resolveMnemonicInto", | ||
| "([B[B)I", | ||
| &[(&jwallet_id).into(), (&jout).into()], | ||
| )? | ||
| .l()?; | ||
| if value.is_null() { | ||
| return Ok(RESULT_NOT_FOUND); | ||
| } | ||
| .i()?; | ||
|
|
||
| let jstr = JString::from(value); | ||
| let java_str = env.get_string(&jstr)?; | ||
| let mut bytes = java_str.to_bytes().to_vec(); | ||
| drop(java_str); | ||
|
|
||
| let code = if bytes.len() + 1 > out_capacity { | ||
| RESULT_BUFFER_TOO_SMALL | ||
| } else { | ||
| std::ptr::copy_nonoverlapping( | ||
| bytes.as_ptr(), | ||
| out_mnemonic_utf8 as *mut u8, | ||
| bytes.len(), | ||
| ); | ||
| *(out_mnemonic_utf8.add(bytes.len())) = 0; | ||
| *out_len = bytes.len(); | ||
| RESULT_OK | ||
| let code = match written { | ||
| RESOLVE_NOT_FOUND => RESULT_NOT_FOUND, | ||
| RESOLVE_BUFFER_TOO_SMALL => RESULT_BUFFER_TOO_SMALL, | ||
| RESOLVE_OTHER => RESULT_OTHER, | ||
| len if len < 0 || len as usize > usable => RESULT_OTHER, | ||
| len => { | ||
| let len = len as usize; | ||
| // Copy the phrase bytes straight from the Java buffer | ||
| // into the Rust-owned out buffer, then scrub the Java | ||
| // buffer — the only JVM-side plaintext copy. | ||
| // `.cast()`: c_char is i8 on x86_64 but u8 on | ||
| // aarch64-linux-android; jbyte is always i8. | ||
| let dst = std::slice::from_raw_parts_mut(out_mnemonic_utf8.cast::<i8>(), len); | ||
| env.get_byte_array_region(&jout, 0, dst)?; | ||
| *(out_mnemonic_utf8.add(len)) = 0; | ||
| *out_len = len; | ||
| RESULT_OK | ||
| } | ||
| }; | ||
|
|
||
| // Zero the intermediate Rust copy of the phrase. (The JVM String | ||
| // itself is garbage-collected — same residual exposure the iOS | ||
| // Swift String has.) | ||
| bytes.iter_mut().for_each(|b| *b = 0); | ||
| let zeros = vec![0i8; usable]; | ||
| env.set_byte_array_region(&jout, 0, &zeros)?; | ||
| Ok(code) |
There was a problem hiding this comment.
🟡 Suggestion: resolve_trampoline: ? on get_byte_array_region skips the Java out-buffer scrub
Inside with_local_frame, env.get_byte_array_region(&jout, 0, dst)? (line 99) copies the phrase bytes out of the Java out-buffer. The trailing env.set_byte_array_region(&jout, 0, &zeros)? at line 106 that scrubs the Java buffer sits at the end of the closure — a failure inside get_byte_array_region unwinds through ? and skips it. At that moment the Kotlin side has already written the plaintext BIP-39 phrase into jout (that is what is being copied out), so failure here leaves the mnemonic plaintext sitting in the JVM byte[] backing jout until GC. This is precisely the exposure the module-level out-buffer contract (lines 36–44) exists to eliminate. Scrub jout before propagating any copy error so the invariant holds on every exit path, including error.
source: ['claude']
| let avatar: Option<Vec<u8>> = if avatar_bytes.is_null() { | ||
| None | ||
| } else { | ||
| env.convert_byte_array(&avatar_bytes).ok() | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: createOrUpdateProfile silently drops avatar bytes on JNI conversion failure
env.convert_byte_array(&avatar_bytes).ok() collapses any conversion failure into None, indistinguishable from the caller intentionally passing null. The avatar_bytes.is_null() predicate already distinguishes the two cases, so when the caller supplied a non-null byte[] but the JNI copy failed, the FFI is invoked with a null avatar pointer and length 0 — the profile state transition is published with no avatar, but the caller's success return value hides the loss. Given the write is on-chain, throwing DashSDKException (as read_id32 does for its byte-array inputs) on a non-null conversion failure is safer than silently committing partial content.
source: ['claude']
| try { | ||
| unlockWalletFromKeystore(managed) | ||
| } catch (_: Exception) { | ||
| // Outcome is published on [dashPayUnlockStatus] (seedMismatch | ||
| // for a binding rejection); one wallet's unlock failure can't | ||
| // fail the whole restore — the wallet simply stays | ||
| // external-signable. | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: loadPersistedWallets swallows CancellationException from unlockWalletFromKeystore
unlockWalletFromKeystore(managed) is a suspend function that throws kotlinx.coroutines.CancellationException when the caller scope is cancelled. CancellationException extends java.lang.Exception, so catch (_: Exception) catches it and the loop continues restoring wallets, breaking structured concurrency — a cancellation from an outer scope (manager swap, teardown) cannot stop the per-wallet unlock loop. The comment correctly justifies swallowing wallet-scoped DashSdkError, but that reasoning does not apply to cancellation. Rethrow cancellation explicitly (if (e is CancellationException) throw e) or replace the catch with a coroutineContext.ensureActive() before the swallow. The same anti-pattern appears in the drain launch at line 1217.
source: ['claude']
| scope.launch(Dispatchers.IO) { | ||
| val drainResolver = MnemonicResolverAndPersister(walletStorage) | ||
| val drainSigner = | ||
| KeystoreSigner(walletStorage, network, biometricGate, database.platformAddressDao()) | ||
| try { | ||
| mapNativeErrors { | ||
| DashpayNative.drainPendingContactCrypto( | ||
| walletHandle, | ||
| drainSigner.nativeHandle, | ||
| drainResolver.nativeHandle, | ||
| ) | ||
| } | ||
| } catch (_: Exception) { | ||
| // Not fatal: the next signer-present DashPay action (or the | ||
| // next unlock) re-attempts; the queue rebuilds via the sweep. | ||
| } finally { | ||
| updateUnlockStatus(key) { it.copy(draining = false) } | ||
| runCatching { drainResolver.close() } | ||
| runCatching { drainSigner.close() } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Drain job is not awaited before nativeDestroy — persistence callbacks may be freed mid-drain
close() calls scope.cancel() and then WalletManagerNative.nativeDestroy(bundle), but scope.cancel() does not wait for the launched drain to return from its blocking JNI call in drainPendingContactCrypto. The comment at lines 1191–1203 correctly identifies this hazard for the resolver/signer (fixed by per-drain fresh handles) but does not extend the same reasoning to wallet-level persistence callbacks that may reach through the manager bundle's persistence/event GlobalRefs. If a slow drain is mid-call when nativeDestroy frees those contexts, a persistence write from the drain could hit freed refs. If the wallet-side persister is fully storage-keyed and independent of the manager bundle this is a non-issue — worth confirming and, if not, tracking the launched drain job so close() can join/quiesce it before destroy.
source: ['codex']
|
Folded into the consolidated DashPay migration PR #4036 (rebased onto the current base + three follow-up hardening fixes). Closing in favor of that single PR. |
2 participants
Issue being fixed or feature implemented
Milestone K2 of the DashPay Swift→Kotlin migration (
docs/dashpay/KOTLIN_MIGRATION_SPEC.md§K2): the recurring DashPay sync service, the seedless unlock topology, and the profile/contactInfo writes. Stacked on #4025 (K1).What was done?
Security pre-req — mnemonic out-buffer discipline (spec-review must-fix): the resolver contract is now
resolveMnemonicInto(byte[], byte[]): Int— the phrase crosses as raw UTF-8 bytes written into a caller-sized buffer; every intermediate copy is zeroed and the trampoline scrubs the Java buffer after copying into Rust-owned memory. Nojava.lang.Stringof the seed exists on the resolver path (WalletStorage.retrieveMnemonicUtf8+ existence-onlyhasMnemonic; the capability check no longer decrypts). A distinctRESOLVE_OTHERchannel keeps Keystore/decrypt failures from masquerading as "no seed stored" (the iOS.otherdiscipline).signWithMnemonicAndPathnow scrubs its mnemonic copies on every path — non-secret args decode first, and the CString NUL-append can no longer realloc-orphan the plaintext buffer. The remaining String-shaped platform-address signing path is explicitly marked as the tracked follow-up.Bridges (13 exports): the 7
platform_wallet_manager_dashpay_sync_*fns, the seedless trio (verify_seed_binds_to_wallet,pending_contact_crypto_count,drain_pending_contact_crypto),create_or_update_dashpay_profile_with_signer,set_dashpay_contact_info_with_signer, anddash_sdk_resolver_supports_key_type.Manager surface (on
PlatformWalletManager, matching both the codebase's manager-owned-loop convention and Swift's manager extension):dashPaySyncNow+dashPaySyncIsSyncingStateFlow via the 1 Hz change-gated poll (poll-not-events is deliberate — it is exactly how iOS does it).unlockWalletFromKeystore(verify → drain) runs automatically per restored wallet insideloadPersistedWallets— load-bearing for DashPay recovery, since the deferred contact-crypto queue is in-memory by design and self-heals only via load → unlock → sweep. Seed-mismatch is published from the verify result only (RustSeedMismatch→ErrorInvalidParameter, catch scoped to that single call); the drain guard is an atomic false→true transition; each drain owns fresh resolver/signer handles bound to its coroutine (the SwiftwithExtendedLifetimeshape — no use-after-free when a manager swap races a slow drain). Per-walletdashPayUnlockStatusStateFlow (draining / seedMismatch / pendingAccountBuilds). The identity-key breadcrumb backfill step is deliberately NOT ported (no pre-breadcrumb Android installs exist).Dashpay.createOrUpdateProfile(avatar bytes hashed Rust-side) andDashpay.setContactInforeturningContactInfoPublishOutcome(the DIP-15 deferred-until-two-contacts state the UI must surface).Code-reviewed by three independent lenses (mnemonic security, concurrency/lifecycle, JNI bridge correctness); both must-fixes (drain use-after-free,
read_id32exception constructor) and the actionable should-fixes are folded.How Has This Been Tested?
DashPayUnlockAndSyncTest: the happy-path unlock is the end-to-end proof of the out-buffer resolver (verify_seed_binds_to_walletre-derives the BIP44 account-0 xpub throughresolveMnemonicInto— wrong bytes/length ⇒ mismatch ⇒ red); the wrong-seed leg pins the seedMismatch contract with a foreign-mnemonic fixture; the lifecycle leg pins start/stop/idempotent-start and manager-swap disposal. ⚠ First execution happens in this PR's CI run (no NDK/emulator locally).dashPaySyncNowand the write paths are live-network operations —-Ptestnet=truetier, exercised in the K3 end-to-end UAT.cargo check/clippy/fmtclean.Breaking Changes
None externally. Within the unreleased Kotlin SDK:
NativeMnemonicBridge.resolveMnemonic(byte[]): Stringis replaced byresolveMnemonicInto(byte[], byte[]): Int(the security fix), andstopDashPaySyncno longer stops the status poll (it dies with the manager).Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code