feat(sdk): add Kotlin SDK and KotlinExampleApp (Android port of SwiftExampleApp) - #3999
Conversation
|
Important Review skippedToo many files! This PR contains 494 files, which is 394 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (494)
You can disable this status message by setting the ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3999 +/- ##
==========================================
Coverage 87.42% 87.43%
==========================================
Files 2644 2646 +2
Lines 333768 333982 +214
==========================================
+ Hits 291810 292024 +214
Misses 41958 41958
🚀 New features to boost your workflow:
|
|
⛔ Blockers found — Sonnet deferred (commit 6efa83b) |
…f SwiftExampleApp) Adds the Android counterpart of packages/swift-sdk: - packages/rs-unified-sdk-jni: Rust JNI cdylib (110 exports) bridging rs-sdk-ffi, platform-wallet-ffi and key-wallet-ffi as rlib deps — no C glue; panics caught at every export; callbacks attach Tokio threads as JVM daemons (persistence vtable, async signer, mnemonic resolver, sync events). - packages/kotlin-sdk/sdk: Kotlin SDK mirroring SwiftDashSDK — 28 Room entities transcribed from the SwiftData models, Keystore-wrapped secret storage, network-locked PlatformWalletManager, sync services, per the persist/load/bridge doctrine (see kotlin-sdk/CLAUDE.md). - packages/kotlin-sdk/KotlinExampleApp: Compose app porting the SwiftExampleApp screens 1:1 (PARITY.md: 75 ported / 8 partial / 7 deferred of 90 views, each gap naming its missing FFI export). Cross-cutting changes: - rs-sdk-ffi + rs-sdk-trusted-context-provider: reqwest switched to rustls-tls-webpki-roots (OpenSSL is unavailable on Android; this also changes the TLS backend used by iOS builds). - rs-sdk-ffi: re-export dash_sdk_document_sum/average from document/mod.rs. - rs-platform-wallet-ffi: new platform_wallet_derive_identity_private_key_at_slot entry point (the CLAUDE.md "one allowed exception" primitive for Keystore persistence). - CI: kotlin-sdk-build.yml (PR build + emulator smoke) and kotlin-sdk-release.yml (tag-triggered AAR release). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t nightly - build_android.sh and gradlew were committed 644 (the source volume is exFAT, which drops POSIX modes) — Kotlin SDK CI failed with "Permission denied". Restored via update-index --chmod=+x. - cargo fmt on rs-platform-wallet-ffi's new identity_private_key_at_slot module + lib.rs (Rust workspace fmt gate) and rs-unified-sdk-jni. - Add kotlin-sdk-nightly.yml: scheduled testnet integration run (-Ptestnet=true lifts the TestnetGuard) on the API-35 emulator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
diskutil/hdiutil do not exist on Linux runners; under set -e the failed command substitution aborted the CI build with exit 127. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dash-proto) Matches the repo-standard protoc version installed by .github/actions/rust; tenderdash-proto's build script cannot parse the "libprotoc 3.21.12" version string apt ships. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a23d45d to
89a97c7
Compare
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Reviewed as codex 5.5 xtra high.
Findings:
-
[P0] Android native builds pass an invalid Cargo features argument —
packages/kotlin-sdk/build_android.sh:149-151The default path leaves
SHIELDED=1, so line 151 expands as a single argv item,--features shielded, not two argv items. Cargo rejects that shape before it builds anything (error: unexpected argument '--features shielded' found). The PR and release workflows both call./build_android.shwithout--no-shielded, so the native library build fails on the default CI/release path. I reproduced the shell argv expansion and confirmed Cargo rejects the single combined argument. Build the optional args as an array instead, for exampleFEATURE_ARGS=(); [[ -n "$FEATURES" ]] && FEATURE_ARGS+=(--features "$FEATURES"), then expand"${FEATURE_ARGS[@]}"before--no-default-features. -
[P1] JNI local references leak on daemon-attached callback threads —
packages/rs-unified-sdk-jni/src/persistence.rs:224-245with_bridge/with_bridge_loadattach Tokio/native worker threads withattach_current_thread_as_daemon()and then the callback bodies allocate JNI locals (byte[],String, object arrays, holder objects) withoutPushLocalFrame/PopLocalFrame,with_local_frame,AutoLocal, or explicitdelete_local_ref. Injni0.21 the daemon attach returns a plainJNIEnv, so these refs are not cleaned up by a detaching guard, and these callbacks do not have a Java native-call frame that returns after each callback. The leak is especially reachable in loops such as address balance persistence (persistence.rs:380-386) and identity upserts (persistence.rs:813-825), and the same pattern appears inevents.rs:69-85,signer.rs:77-103,mnemonic.rs:50-78, andfunding.rs:393-410. Large or repeated sync/sign/resolver/progress callbacks can overflow ART's local reference table and turn sync into JNI failures or process crashes. Please wrap daemon-thread callback invocations in local frames, and use per-entry frames orAutoLocal/delete_local_refinside large loops. -
[P2] Kotlin SDK PR workflow skips several native build inputs —
.github/workflows/kotlin-sdk-build.yml:7-12The new Android build depends on workspace-level Cargo files and transitive Rust crates, but the PR path filter only watches
packages/kotlin-sdk/**,packages/rs-unified-sdk-jni/**,packages/rs-sdk-ffi/**,packages/rs-platform-wallet-ffi/**, and the workflow file itself. This PR already changesCargo.toml,Cargo.lock, andpackages/rs-sdk-trusted-context-provider/Cargo.toml, all of which can affect the Android JNI build, but a future PR that changes only those inputs would bypass the Kotlin SDK build/test workflow. Please include at leastCargo.toml,Cargo.lock,packages/rs-sdk-trusted-context-provider/**, and any other Rust workspace inputs whose changes should revalidate the Android artifact.
Verification notes: cargo check -p rs-unified-sdk-jni --no-default-features passes on the latest head (89a97c7d54). I could not run the Gradle tasks locally because this machine has no Java runtime on PATH.
|
CI status note: Kotlin SDK build + tests is green (full pipeline incl. the API-35 emulator instrumented suite). Rust workspace tests / Tests (macOS) fails with Swift-coverage tooling errors on the self-hosted mac runner (llvm-cov "failed to load coverage … -arch specifier is invalid" against SwiftExampleApp DerivedData). The same job fails intermittently on |
…/withdraw, error namespacing, sync clear) Catches the Kotlin SDK/app up with the 14 commits merged to v4.1-dev since the branch point: - Bridge the platform-address wallet surface from #3923 (ADDR-02/04): transfer, withdraw-to-address, withdrawal preflight, min amounts — new TransferPlatformAddressScreen/WithdrawPlatformAddressScreen wired from WalletDetail's Platform Credits section. - Fix latent error-code collision: PlatformWalletFFIResultCode values were thrown on the same integer channel as rs-sdk-ffi's DashSDKErrorCode. All pwffi throws now use a shared support::take_pwffi_error with a +1000 offset; DashSdkError gains a PlatformWallet subtree incl. the new retryability-bearing codes (ShieldedNoRecordedAnchor retryable, TransactionBroadcastUnconfirmed must-not-retry) mirroring PlatformWalletResult.swift. - Port #3959: Platform Sync "Clear" now runs the native platform_address_sync_reset then zeroes address rows / deletes sync states in one transaction (fail-closed ordering). - Mirror the persistence-handler fix scoping address-balance lookups by (walletId, addressHash) — multi-wallet collision fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HashEngineering
left a comment
HashEngineering
left a comment
There was a problem hiding this comment.
I have only one observation.
Round-4 run failed because the runner's emulator booted without working DNS resolution (quorums.testnet.networks.dash.org NXDOMAIN); identical code passed the previous round. Pinning public resolvers removes the flake class. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
The PR ports Android v4.1-dev deltas (platform-address transfer/withdraw, sync clear, error namespacing) cleanly, and prior finding #7 (platform-wallet FFI result-code translation) is FIXED by the new PWFFI_CODE_OFFSET + Kotlin PlatformWallet sealed subtree. Seven prior findings are carried forward at current head (1 blocking, 5 suggestions, 1 nitpick): DataContractRef double-free, negative Core/token casts, private-key stack zeroization, macOS sort -V, iOS TLS validation, and PARITY.md staleness. New latest-delta findings: the platform-address preflight/min-amount composites can leak their transient handle on JVM long-array allocation failure, and negative platform account indexes are silently clamped to account 0. Codex's Success-message leak claim is a false positive — PlatformWalletFFIResult has a Drop impl that frees the CString when the by-value result goes out of scope.
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5, claude rust-quality opus, codex rust-quality gpt-5.5; verifier claude opus.
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 1 nitpick(s)
9 additional finding(s)
blocking: DataContractRef.close() is not atomic and can double-free the Rust handle
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt (line 122)
DataContractRef stores the native pointer in a plain private var handle: Long (line 123) and close() performs a non-atomic load-then-store: val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h). Two threads or coroutines calling close() concurrently can both read the same non-zero pointer before either writes 0, resulting in two dataContractDestroy(h) calls. On the Rust side, dash_sdk_data_contract_destroy reconstructs the allocation via Box::from_raw(handle as *mut DataContract), so a second call is a real double-free / use-after-free across the JNI boundary. Every other owning wrapper in this SDK (Sdk, ManagedPlatformWallet.HandleCleanup, PlatformWalletManager.bundleRef) already uses AtomicLong.getAndSet(0) precisely for this ownership handoff; DataContractRef should match.
class DataContractRef internal constructor(handle: Long) : AutoCloseable {
private val handleRef = java.util.concurrent.atomic.AtomicLong(handle)
internal val value: Long
get() = handleRef.get().also { check(it != 0L) { "DataContractRef has been closed" } }
override fun close() {
val h = handleRef.getAndSet(0)
if (h != 0L) QueriesNative.dataContractDestroy(h)
}
}
suggestion: walletPlatformAddressPreflightWithdrawal/MinAmounts leak the transient platform-address handle on JVM array-alloc failure
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1258)
Both newly added composites acquire a transient platform-address handle via platform_wallet_get_platform that must be released by platform_address_wallet_destroy. In walletPlatformAddressPreflightWithdrawal (lines 1266-1271) and walletPlatformAddressMinAmounts (lines 1330-1335), the success branch inlines two early returns — let Ok(arr) = env.new_long_array(...) else { return ptr::null_mut(); }; and if env.set_long_array_region(...).is_err() { return ptr::null_mut(); } — that return directly from the enclosing guard closure without hitting the destroy call. The failure is silent and strands a live handle inside the platform-wallet manager's Arc table. The sibling exports (walletPlatformAddressTransfer at 1073-1134, walletPlatformAddressWithdraw at 1155-1216) intentionally funnel every path through let out = if ... else { ... }; before the shared destroy, and these two new exports should adopt the same shape.
suggestion: Negative Core send amounts are silently cast to huge u64 values
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 466)
walletCoreSendToAddresses reads Kotlin Long duff amounts into amount_buf: Vec<i64> and then constructs amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect() (line 478) with no sign check before passing them to core_wallet_send_to_addresses. Core duff amounts have no legitimate negative representation, so a caller-side -1 sentinel or arithmetic underflow becomes 18_446_744_073_709_551_615 on the Rust side instead of failing cleanly at the JNI boundary. Reject non-positive values at the boundary so the failure surfaces as a DashSDKException.
if amount_buf.iter().any(|&v| v <= 0) {
throw_sdk_exception(env, 1, "amounts must be positive duff values");
return ptr::null_mut();
}
let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();
suggestion: Derived private-key scalar left un-zeroized on the JNI stack
packages/rs-unified-sdk-jni/src/identity.rs (line 240)
out_key.private_key_bytes is [u8; 32] (Copy). let scalar = out_key.private_key_bytes; at line 242 copies the ECDSA scalar into an independent stack local before building the JVM byte[]; the identical pattern recurs at line 326 in deriveIdentityPrivateKeyWithResolver. The subsequent _free calls zeroize only the original field inside out_key / out_row — they cannot reach the independent scalar local, so plaintext key material persists in the stack slot until unrelated frames overwrite it. This contradicts the module's own stated invariant that the only escaping copy is the JVM byte array, and breaks the deliberate key-scrubbing discipline elsewhere in this crate (Zeroizing buffers, non_secure_erase of xprivs).
// Copy the scalar out into a JVM byte[] before freeing the
// Rust-owned (soon-to-be-zeroized) buffer.
let mut scalar = out_key.private_key_bytes;
let jarr = env
.byte_array_from_slice(&scalar)
.map(|a| a.into_raw())
.unwrap_or(ptr::null_mut());
zeroize::Zeroize::zeroize(&mut scalar);
// Zeroize + free the Rust-owned buffer (scrubs the scalar and
// reclaims the path string).
unsafe {
platform_wallet_ffi::platform_wallet_derive_identity_private_key_at_slot_free(
&mut out_key as *mut IdentityPrivateKeyFFI,
)
};
jarr
suggestion: NDK autodetection uses GNU-only `sort -V` on a macOS-oriented script
packages/kotlin-sdk/build_android.sh (line 80)
The script is explicitly macOS-oriented (sparse-image handling is Darwin-gated; $HOME/Library/Android/sdk default at line 82), but line 84 still uses ls "$NDK_ROOT" | sort -V | tail -1. BSD sort on macOS does not support -V consistently — depending on the release, the flag is silently ignored or errors out, so the ordering is not the version-aware ordering the caller expects. A developer without ANDROID_NDK_HOME set will hit an unexpected NDK selection or autodetection failure on a normal macOS Android SDK install. Use a portable numeric sort keyed on the version components.
ANDROID_NDK_HOME="$NDK_ROOT/$(find "$NDK_ROOT" -mindepth 1 -maxdepth 1 -type d -exec basename {} \; | sort -t. -k1,1n -k2,2n -k3,3n | tail -1)"
suggestion: Negative token amounts and costs are reinterpreted as huge u64 values
packages/rs-unified-sdk-jni/src/tokens.rs (line 704)
Java_..._TokensNative_tokenPurchase receives amount: jlong and expected_total_cost: jlong and passes them directly into platform_wallet_token_purchase as amount as u64 and expected_total_cost as u64 (lines 710-711). The Kotlin surface exposes these as signed Long, so a caller-side -1 sentinel becomes 18_446_744_073_709_551_615 instead of erroring cleanly at the JNI edge; the same direct-cast pattern is used across the mint / burn / transfer / set-price entry points in this file. Reject negative values at the JNI edge so the failure surfaces as a DashSDKException, matching the guard already suggested on the Core send path.
suggestion: Negative platform account indexes are silently clamped to account 0
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1093)
walletPlatformAddressTransfer (line 1096), walletPlatformAddressWithdraw (line 1178), and walletPlatformAddressPreflightWithdrawal (line 1252) all take account_index: jint and pass account_index.max(0) as u32 to platform-wallet FFI without rejecting negatives. A caller-side -1 therefore silently operates on account 0, which can move credits from the wrong platform-address account or produce a preflight result for the wrong account. Same pattern as the negative-amount findings — reject at the boundary rather than clamp.
suggestion: reqwest TLS backend swap silently changes iOS trust behavior — needs explicit iOS validation
packages/rs-sdk-ffi/Cargo.toml (line 71)
The switch to default-features = false + rustls-tls-webpki-roots (line 77) is required for Android (no OpenSSL, no readable system store) but is unconditional, so it also flips the TLS backend for every iOS build of rs-sdk-ffi and rs-sdk-trusted-context-provider. TrustedHttpContextProvider is the SDK's stated root of trust for proof verification (quorum public keys), so any regression here would silently affect iOS. Consequences: (1) MDM/user-installed roots are no longer honored for iOS SDK traffic; (2) OS trust-store updates are ignored until the SDK is rebuilt against a newer webpki-roots; (3) the SDK now owns a webpki-roots version-bump cadence. The PR's test plan documents Android/host verification only. Either target-cfg-gate this (rustls-tls-native-roots on Apple targets) until Swift-side validation is done, or add an explicit iOS smoke test and note the behavior change in the CHANGELOG.
nitpick: PARITY.md is internally inconsistent: SendTransactionView still 'partial', ported totals stale
packages/kotlin-sdk/PARITY.md (line 122)
Two related staleness issues: (1) Line 122 lists SendTransactionView as 'partial — form + fee UI ported; broadcast deferred on core_wallet_send_to_addresses', but the FFI is bridged (WalletManagerNative.walletCoreSendToAddresses builds+signs+broadcasts, ManagedPlatformWallet.sendToAddresses exposes it, and SendTransactionScreen.kt calls it). (2) The latest delta added two new | ported | rows but did not touch the totals block on lines 132-133, which still says ported: 75 (of 90 Swift views) and partial: 8. Since the PR promotes PARITY.md as the source of truth for missing FFI exports, correct the SendTransactionView row and refresh the totals.
| SendTransactionView.swift | ui/wallet/SendTransactionScreen.kt · `SendTransaction` | ported |
🤖 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.
- [BLOCKING] In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt`:122-132: DataContractRef.close() is not atomic and can double-free the Rust handle
`DataContractRef` stores the native pointer in a plain `private var handle: Long` (line 123) and `close()` performs a non-atomic load-then-store: `val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h)`. Two threads or coroutines calling `close()` concurrently can both read the same non-zero pointer before either writes 0, resulting in two `dataContractDestroy(h)` calls. On the Rust side, `dash_sdk_data_contract_destroy` reconstructs the allocation via `Box::from_raw(handle as *mut DataContract)`, so a second call is a real double-free / use-after-free across the JNI boundary. Every other owning wrapper in this SDK (`Sdk`, `ManagedPlatformWallet.HandleCleanup`, `PlatformWalletManager.bundleRef`) already uses `AtomicLong.getAndSet(0)` precisely for this ownership handoff; `DataContractRef` should match.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1258-1284: walletPlatformAddressPreflightWithdrawal/MinAmounts leak the transient platform-address handle on JVM array-alloc failure
Both newly added composites acquire a transient platform-address handle via `platform_wallet_get_platform` that must be released by `platform_address_wallet_destroy`. In `walletPlatformAddressPreflightWithdrawal` (lines 1266-1271) and `walletPlatformAddressMinAmounts` (lines 1330-1335), the success branch inlines two early returns — `let Ok(arr) = env.new_long_array(...) else { return ptr::null_mut(); };` and `if env.set_long_array_region(...).is_err() { return ptr::null_mut(); }` — that return directly from the enclosing `guard` closure without hitting the destroy call. The failure is silent and strands a live handle inside the platform-wallet manager's `Arc` table. The sibling exports (`walletPlatformAddressTransfer` at 1073-1134, `walletPlatformAddressWithdraw` at 1155-1216) intentionally funnel every path through `let out = if ... else { ... };` before the shared destroy, and these two new exports should adopt the same shape.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:466-478: Negative Core send amounts are silently cast to huge u64 values
`walletCoreSendToAddresses` reads Kotlin `Long` duff amounts into `amount_buf: Vec<i64>` and then constructs `amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect()` (line 478) with no sign check before passing them to `core_wallet_send_to_addresses`. Core duff amounts have no legitimate negative representation, so a caller-side `-1` sentinel or arithmetic underflow becomes `18_446_744_073_709_551_615` on the Rust side instead of failing cleanly at the JNI boundary. Reject non-positive values at the boundary so the failure surfaces as a `DashSDKException`.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/identity.rs`:240-256: Derived private-key scalar left un-zeroized on the JNI stack
`out_key.private_key_bytes` is `[u8; 32]` (Copy). `let scalar = out_key.private_key_bytes;` at line 242 copies the ECDSA scalar into an independent stack local before building the JVM `byte[]`; the identical pattern recurs at line 326 in `deriveIdentityPrivateKeyWithResolver`. The subsequent `_free` calls zeroize only the *original* field inside `out_key` / `out_row` — they cannot reach the independent `scalar` local, so plaintext key material persists in the stack slot until unrelated frames overwrite it. This contradicts the module's own stated invariant that the only escaping copy is the JVM byte array, and breaks the deliberate key-scrubbing discipline elsewhere in this crate (`Zeroizing` buffers, `non_secure_erase` of xprivs).
- [SUGGESTION] In `packages/kotlin-sdk/build_android.sh`:80-87: NDK autodetection uses GNU-only `sort -V` on a macOS-oriented script
The script is explicitly macOS-oriented (sparse-image handling is Darwin-gated; `$HOME/Library/Android/sdk` default at line 82), but line 84 still uses `ls "$NDK_ROOT" | sort -V | tail -1`. BSD `sort` on macOS does not support `-V` consistently — depending on the release, the flag is silently ignored or errors out, so the ordering is not the version-aware ordering the caller expects. A developer without `ANDROID_NDK_HOME` set will hit an unexpected NDK selection or autodetection failure on a normal macOS Android SDK install. Use a portable numeric sort keyed on the version components.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/tokens.rs`:704-716: Negative token amounts and costs are reinterpreted as huge u64 values
`Java_..._TokensNative_tokenPurchase` receives `amount: jlong` and `expected_total_cost: jlong` and passes them directly into `platform_wallet_token_purchase` as `amount as u64` and `expected_total_cost as u64` (lines 710-711). The Kotlin surface exposes these as signed `Long`, so a caller-side `-1` sentinel becomes `18_446_744_073_709_551_615` instead of erroring cleanly at the JNI edge; the same direct-cast pattern is used across the mint / burn / transfer / set-price entry points in this file. Reject negative values at the JNI edge so the failure surfaces as a `DashSDKException`, matching the guard already suggested on the Core send path.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1093-1252: Negative platform account indexes are silently clamped to account 0
`walletPlatformAddressTransfer` (line 1096), `walletPlatformAddressWithdraw` (line 1178), and `walletPlatformAddressPreflightWithdrawal` (line 1252) all take `account_index: jint` and pass `account_index.max(0) as u32` to platform-wallet FFI without rejecting negatives. A caller-side `-1` therefore silently operates on account 0, which can move credits from the wrong platform-address account or produce a preflight result for the wrong account. Same pattern as the negative-amount findings — reject at the boundary rather than clamp.
- [SUGGESTION] In `packages/rs-sdk-ffi/Cargo.toml`:71-77: reqwest TLS backend swap silently changes iOS trust behavior — needs explicit iOS validation
The switch to `default-features = false` + `rustls-tls-webpki-roots` (line 77) is required for Android (no OpenSSL, no readable system store) but is unconditional, so it also flips the TLS backend for every iOS build of `rs-sdk-ffi` and `rs-sdk-trusted-context-provider`. `TrustedHttpContextProvider` is the SDK's stated root of trust for proof verification (quorum public keys), so any regression here would silently affect iOS. Consequences: (1) MDM/user-installed roots are no longer honored for iOS SDK traffic; (2) OS trust-store updates are ignored until the SDK is rebuilt against a newer `webpki-roots`; (3) the SDK now owns a `webpki-roots` version-bump cadence. The PR's test plan documents Android/host verification only. Either target-cfg-gate this (`rustls-tls-native-roots` on Apple targets) until Swift-side validation is done, or add an explicit iOS smoke test and note the behavior change in the CHANGELOG.
- [NITPICK] In `packages/kotlin-sdk/PARITY.md`:122-134: PARITY.md is internally inconsistent: SendTransactionView still 'partial', ported totals stale
Two related staleness issues: (1) Line 122 lists `SendTransactionView` as 'partial — form + fee UI ported; broadcast deferred on `core_wallet_send_to_addresses`', but the FFI is bridged (`WalletManagerNative.walletCoreSendToAddresses` builds+signs+broadcasts, `ManagedPlatformWallet.sendToAddresses` exposes it, and `SendTransactionScreen.kt` calls it). (2) The latest delta added two new `| ported |` rows but did not touch the totals block on lines 132-133, which still says `ported: 75 (of 90 Swift views)` and `partial: 8`. Since the PR promotes PARITY.md as the source of truth for missing FFI exports, correct the SendTransactionView row and refresh the totals.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push (4640fbd) is a CI-only DNS pin on Kotlin emulator jobs; no source touched between 1cc1334 and HEAD. No new latest-delta findings. All 9 prior findings verified STILL VALID against the current worktree — 1 blocking JNI ownership race (DataContractRef.close double-free, pattern also present in ContactRequestRef/EstablishedContactRef), 7 JNI boundary suggestions (handle leak, negative-signed-to-u64 casts in Core send/tokens, unzeroized private-key stack copy, GNU sort -V on macOS, negative account-index clamp, unconditional rustls TLS on iOS), and 1 stale-parity docs nit (SendTransactionScreen.kt actually calls the FFI now, so PARITY.md row should be 'ported').
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 1 nitpick(s)
Carried-forward prior findings
blocking: DataContractRef.close() is non-atomic and can double-free the Rust handle
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt (line 122-133)
DataContractRef stores the native pointer in a plain private var handle: Long and close() does a non-atomic load → store → destroy: val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h). Two concurrent closers (e.g. an explicit use {} finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires) can each read the same non-zero handle before either has stored 0, and both will invoke dataContractDestroy(h). The Rust side of that JNI symbol reconstructs the allocation with Box::from_raw, so the second call is a real double-free / use-after-free of the Rust DataContract. The SDK's own Sdk and ManagedPlatformWallet already use AtomicLong.getAndSet(0) for exactly this ownership handoff — apply the same pattern here. Note: ContactRequestRef and EstablishedContactRef in tokens/Dashpay.kt:226-241 have the identical shape and need the same fix.
class DataContractRef internal constructor(handle: Long) : AutoCloseable {
private val handleRef = java.util.concurrent.atomic.AtomicLong(handle)
internal val value: Long
get() = handleRef.get().also { check(it != 0L) { "DataContractRef has been closed" } }
override fun close() {
val h = handleRef.getAndSet(0)
if (h != 0L) QueriesNative.dataContractDestroy(h)
}
}
suggestion: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1258-1284)
After platform_wallet_get_platform succeeds, addr_handle MUST be paired with platform_address_wallet_destroy on every exit path. In walletPlatformAddressPreflightWithdrawal the branches at lines 1266-1268 (let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); }) and 1269-1270 (set_long_array_region.is_err() { return ptr::null_mut(); }) return directly from the guard closure, bypassing the destroy call at 1275-1281. walletPlatformAddressMinAmounts has the identical shape at 1330-1334. Result: a live transient platform-address handle is stranded in PlatformWalletManager's Arc registry every time the JVM cannot allocate the 2- or 3-long array — exactly the memory-pressure path where leaks compound. The neighboring transfer/withdraw exports funnel every path through a shared out binding before the unconditional destroy — mirror that pattern here.
suggestion: Negative Core send amounts silently bit-cast to huge u64 values
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 466-478)
walletCoreSendToAddresses reads Kotlin long[] duffs into Vec<i64> and then does amount_buf.iter().map(|&v| v as u64).collect(). A caller-side -1 (sentinel, off-by-one, arithmetic underflow) becomes u64::MAX ≈ 1.8e19 duffs and is forwarded to core_wallet_send_to_addresses. Even when Rust rejects it downstream, the failure mode is 'obscure fee/overflow error' rather than the intended boundary-level DashSDKException. Validate at the boundary: reject v <= 0 with a clear error before casting. Same treatment applies to core_fee_per_byte.
if amount_buf.iter().any(|&v| v <= 0) {
throw_sdk_exception(env, 1, "amounts must be positive duff values");
return ptr::null_mut();
}
let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();
suggestion: Derived private-key scalar left un-zeroized on the JNI stack
packages/rs-unified-sdk-jni/src/identity.rs (line 240-256)
let scalar = out_key.private_key_bytes; at line 242 (and the identical pattern at line 326 in deriveIdentityPrivateKeyWithResolver) copies the 32-byte scalar into a bare [u8; 32] stack local before handing it to byte_array_from_slice. The paired ..._at_slot_free scrubs the Rust-owned buffer, but the stack copy is never zeroized — it remains in the JNI stack frame until overwritten by later frames. Given the FFI author explicitly implemented a zeroize-on-free helper, keeping an untracked plaintext copy on the stack defeats the guarantee. Fix by either passing &out_key.private_key_bytes directly to byte_array_from_slice (no stack copy) or wrapping the local in zeroize::Zeroizing::new(...) so Drop scrubs it.
suggestion: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
packages/kotlin-sdk/build_android.sh (line 80-87)
Line 84 pipes ls "$NDK_ROOT" | sort -V | tail -1 to pick the newest NDK, but BSD sort on stock macOS does not implement -V and prints sort: invalid option -- V. This script is explicitly macOS-oriented (exFAT-on-APFS sparse image handling elsewhere in the file, macOS Android SDK default $HOME/Library/Android/sdk), so on a developer machine with unset ANDROID_NDK_HOME and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (e.g. 9.x sorts after 28.x lexically). Use a numeric-key sort (sort -t. -k1,1n -k2,2n -k3,3n) or fall back to ls -t | head -1.
suggestion: Negative token amounts and expected-total-costs reinterpreted as huge u64 values
packages/rs-unified-sdk-jni/src/tokens.rs (line 704-716)
tokenPurchase casts amount as u64 and expected_total_cost as u64 at lines 710-711 with no sign check. A Kotlin caller-side -1 sentinel or arithmetic underflow becomes u64::MAX — a valid u64 that platform-wallet then treats as a colossal purchase amount or expected cost. The same direct signed-to-unsigned bit-cast pattern is present across the tokens.rs surface (mint, burn, transfer, set-price). Validate at the JNI boundary and throw DashSDKException for negatives.
suggestion: Negative platform account indexes and fee rates are silently clamped to 0
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1093-1252)
walletPlatformAddressTransfer (1096), walletPlatformAddressWithdraw (1178), and walletPlatformAddressPreflightWithdrawal (1252) all convert the signed Kotlin int with account_index.max(0) as u32 (and core_fee_per_byte.max(0) as u32 at 1185/1253). A caller passing -1 — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can move funds from or preflight against the wrong account with no error crossing the boundary. This is arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw DashSDKException on negatives instead of clamping.
suggestion: reqwest TLS backend swap silently changes iOS trust behavior — needs explicit iOS validation or target gating
packages/rs-sdk-ffi/Cargo.toml (line 71-77)
The unconditional switch to default-features = false, features = ["json", "rustls-tls-webpki-roots"] fixes the Android build (no OpenSSL) but also changes iOS: previously iOS clients validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation state); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) trust store updates (revocations, root removals) no longer propagate until an SDK rebuild bumps webpki-roots. rs-sdk-trusted-context-provider uses the same crate on the SDK trust path for proof-related network calls, amplifying the blast radius. The Cargo.toml comment justifies the change for Android/mobile parity but the PR test plan shows no iOS validation. Either narrow with a #[cfg(target_os)]-driven feature split (native-tls on iOS, rustls on Android) or land an explicit iOS integration smoke test hitting a real HTTPS endpoint before merge.
nitpick: PARITY.md still marks SendTransactionView partial after the broadcast bridge landed
packages/kotlin-sdk/PARITY.md (line 122-134)
Row 122 still says SendTransactionView is partial with 'broadcast deferred on core_wallet_send_to_addresses', but the JNI surface now includes WalletManagerNative.walletCoreSendToAddresses, ManagedPlatformWallet.sendToAddresses wraps it, and KotlinExampleApp/.../SendTransactionScreen.kt calls that wrapper — so broadcast is wired end-to-end. The Totals block on lines 132-134 still counts SendTransactionView among the 8 partial views. Since this file is presented as the source of truth for missing FFI exports (grep for notBridged under ui/), the stale row and totals will mislead follow-up interop work. Flip the row to ported and bump totals to 76/7/7.
| SendTransactionView.swift | ui/wallet/SendTransactionScreen.kt · `SendTransaction` | ported |
New findings in latest delta
None. The latest delta from 1cc13348 to 4640fbd4 only updates the Kotlin SDK workflow emulator DNS settings.
🤖 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.
- [BLOCKING] In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt:122-133`: DataContractRef.close() is non-atomic and can double-free the Rust handle
`DataContractRef` stores the native pointer in a plain `private var handle: Long` and `close()` does a non-atomic load → store → destroy: `val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h)`. Two concurrent closers (e.g. an explicit `use {}` finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires) can each read the same non-zero handle before either has stored 0, and both will invoke `dataContractDestroy(h)`. The Rust side of that JNI symbol reconstructs the allocation with `Box::from_raw`, so the second call is a real double-free / use-after-free of the Rust `DataContract`. The SDK's own `Sdk` and `ManagedPlatformWallet` already use `AtomicLong.getAndSet(0)` for exactly this ownership handoff — apply the same pattern here. Note: `ContactRequestRef` and `EstablishedContactRef` in `tokens/Dashpay.kt:226-241` have the identical shape and need the same fix.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs:1258-1284`: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
After `platform_wallet_get_platform` succeeds, `addr_handle` MUST be paired with `platform_address_wallet_destroy` on every exit path. In `walletPlatformAddressPreflightWithdrawal` the branches at lines 1266-1268 (`let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); }`) and 1269-1270 (`set_long_array_region.is_err() { return ptr::null_mut(); }`) return directly from the guard closure, bypassing the destroy call at 1275-1281. `walletPlatformAddressMinAmounts` has the identical shape at 1330-1334. Result: a live transient platform-address handle is stranded in `PlatformWalletManager`'s Arc registry every time the JVM cannot allocate the 2- or 3-long array — exactly the memory-pressure path where leaks compound. The neighboring transfer/withdraw exports funnel every path through a shared `out` binding before the unconditional destroy — mirror that pattern here.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs:466-478`: Negative Core send amounts silently bit-cast to huge u64 values
`walletCoreSendToAddresses` reads Kotlin `long[]` duffs into `Vec<i64>` and then does `amount_buf.iter().map(|&v| v as u64).collect()`. A caller-side `-1` (sentinel, off-by-one, arithmetic underflow) becomes `u64::MAX ≈ 1.8e19` duffs and is forwarded to `core_wallet_send_to_addresses`. Even when Rust rejects it downstream, the failure mode is 'obscure fee/overflow error' rather than the intended boundary-level `DashSDKException`. Validate at the boundary: reject `v <= 0` with a clear error before casting. Same treatment applies to `core_fee_per_byte`.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/identity.rs:240-256`: Derived private-key scalar left un-zeroized on the JNI stack
`let scalar = out_key.private_key_bytes;` at line 242 (and the identical pattern at line 326 in `deriveIdentityPrivateKeyWithResolver`) copies the 32-byte scalar into a bare `[u8; 32]` stack local before handing it to `byte_array_from_slice`. The paired `..._at_slot_free` scrubs the Rust-owned buffer, but the stack copy is never zeroized — it remains in the JNI stack frame until overwritten by later frames. Given the FFI author explicitly implemented a zeroize-on-free helper, keeping an untracked plaintext copy on the stack defeats the guarantee. Fix by either passing `&out_key.private_key_bytes` directly to `byte_array_from_slice` (no stack copy) or wrapping the local in `zeroize::Zeroizing::new(...)` so Drop scrubs it.
- [SUGGESTION] In `packages/kotlin-sdk/build_android.sh:80-87`: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
Line 84 pipes `ls "$NDK_ROOT" | sort -V | tail -1` to pick the newest NDK, but BSD `sort` on stock macOS does not implement `-V` and prints `sort: invalid option -- V`. This script is explicitly macOS-oriented (exFAT-on-APFS sparse image handling elsewhere in the file, macOS Android SDK default `$HOME/Library/Android/sdk`), so on a developer machine with unset `ANDROID_NDK_HOME` and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (e.g. `9.x` sorts after `28.x` lexically). Use a numeric-key sort (`sort -t. -k1,1n -k2,2n -k3,3n`) or fall back to `ls -t | head -1`.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/tokens.rs:704-716`: Negative token amounts and expected-total-costs reinterpreted as huge u64 values
`tokenPurchase` casts `amount as u64` and `expected_total_cost as u64` at lines 710-711 with no sign check. A Kotlin caller-side `-1` sentinel or arithmetic underflow becomes `u64::MAX` — a valid u64 that platform-wallet then treats as a colossal purchase amount or expected cost. The same direct signed-to-unsigned bit-cast pattern is present across the tokens.rs surface (mint, burn, transfer, set-price). Validate at the JNI boundary and throw `DashSDKException` for negatives.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs:1093-1252`: Negative platform account indexes and fee rates are silently clamped to 0
`walletPlatformAddressTransfer` (1096), `walletPlatformAddressWithdraw` (1178), and `walletPlatformAddressPreflightWithdrawal` (1252) all convert the signed Kotlin int with `account_index.max(0) as u32` (and `core_fee_per_byte.max(0) as u32` at 1185/1253). A caller passing `-1` — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can move funds from or preflight against the wrong account with no error crossing the boundary. This is arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw `DashSDKException` on negatives instead of clamping.
- [SUGGESTION] In `packages/rs-sdk-ffi/Cargo.toml:71-77`: reqwest TLS backend swap silently changes iOS trust behavior — needs explicit iOS validation or target gating
The unconditional switch to `default-features = false, features = ["json", "rustls-tls-webpki-roots"]` fixes the Android build (no OpenSSL) but also changes iOS: previously iOS clients validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation state); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) trust store updates (revocations, root removals) no longer propagate until an SDK rebuild bumps webpki-roots. `rs-sdk-trusted-context-provider` uses the same crate on the SDK trust path for proof-related network calls, amplifying the blast radius. The Cargo.toml comment justifies the change for Android/mobile parity but the PR test plan shows no iOS validation. Either narrow with a `#[cfg(target_os)]`-driven feature split (native-tls on iOS, rustls on Android) or land an explicit iOS integration smoke test hitting a real HTTPS endpoint before merge.
- [NITPICK] In `packages/kotlin-sdk/PARITY.md:122-134`: PARITY.md still marks SendTransactionView partial after the broadcast bridge landed
Row 122 still says `SendTransactionView` is partial with 'broadcast deferred on `core_wallet_send_to_addresses`', but the JNI surface now includes `WalletManagerNative.walletCoreSendToAddresses`, `ManagedPlatformWallet.sendToAddresses` wraps it, and `KotlinExampleApp/.../SendTransactionScreen.kt` calls that wrapper — so broadcast is wired end-to-end. The Totals block on lines 132-134 still counts SendTransactionView among the 8 partial views. Since this file is presented as the source of truth for missing FFI exports (`grep for notBridged under ui/`), the stale row and totals will mislead follow-up interop work. Flip the row to `ported` and bump totals to 76/7/7.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the verified findings as a top-level review body.
Add KotlinExampleApp (code 1) to the QA contract lookup codes and create a full Android test plan mirroring the iOS SwiftExampleApp TEST_PLAN.md with Compose screen entry points. 126 test cases with identical IDs/tiers/categories enable per-app tracking on the QA dashboard. Seed with: node src/seed.mjs --app KotlinExampleApp \ --plan packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The new QA contract (9tshSfq5…) dropped Group as a category and renumbered System to code 10. Update codes.mjs to match on-chain state and remove the Group section from the Kotlin test plan. Also point contract-id.testnet.json at the new contract. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta 4640fbd..675402b is docs-only: adds KotlinExampleApp/TEST_PLAN.md and one qa-contract app-code row. All 9 prior findings from the review at 4640fbd re-verified STILL VALID at HEAD 675402b, including the blocking DataContractRef.close() double-free. One new latest-delta finding: the new Android TEST_PLAN marks several still-deferred JNI features as automatable.
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.
🔴 1 blocking | 🟡 8 suggestion(s) | 💬 1 nitpick(s)
10 additional finding(s)
blocking: DataContractRef.close() is non-atomic and can double-free the Rust handle
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt (line 122)
DataContractRef stores the native pointer in a plain private var handle: Long (line 123) and close() does a non-atomic load → store → destroy: val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h) (lines 128-132). Two concurrent closers — e.g. an explicit use {} finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires, or two coroutines racing on the ref — can each observe the same non-zero h before either has stored 0, and both will invoke dataContractDestroy(h). The Rust side of that JNI symbol reconstructs the allocation with Box::from_raw(handle as *mut DataContract), so the second call is a real double-free / use-after-free across the JNI boundary. The rest of this SDK (Sdk.handle, ManagedPlatformWallet.HandleCleanup, PlatformWalletManager.bundleRef) already uses AtomicLong.getAndSet(0) precisely for this ownership handoff — apply the same pattern here. The identical non-atomic shape is also present in ContactRequestRef and EstablishedContactRef in tokens/Dashpay.kt and needs the same fix (those handles are registry removals rather than direct Box::from_raw frees, but repeated close is still incorrect).
class DataContractRef internal constructor(handle: Long) : AutoCloseable {
private val handleRef = java.util.concurrent.atomic.AtomicLong(handle)
internal val value: Long
get() = handleRef.get().also { check(it != 0L) { "DataContractRef has been closed" } }
override fun close() {
val h = handleRef.getAndSet(0)
if (h != 0L) QueriesNative.dataContractDestroy(h)
}
}
suggestion: Negative Core send amounts silently bit-cast to huge u64 values at the JNI boundary
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 466)
walletCoreSendToAddresses reads Kotlin long[] duffs into Vec<i64> (line 466) and then does let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect(); (line 478) with no sign check before passing them to core_wallet_send_to_addresses. A caller-side -1 sentinel, off-by-one, or arithmetic underflow becomes u64::MAX ≈ 1.8e19 duffs. Even when Rust rejects it downstream the failure mode is a confusing fee/overflow error rather than the intended boundary-level DashSDKException. Validate at the boundary; the same treatment applies to core_fee_per_byte.
if amount_buf.iter().any(|&v| v <= 0) {
throw_sdk_exception(env, 1, "amounts must be positive duff values");
return ptr::null_mut();
}
let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();
suggestion: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1258)
After platform_wallet_get_platform succeeds, addr_handle must be paired with platform_address_wallet_destroy on every exit path. In walletPlatformAddressPreflightWithdrawal the branches at lines 1266-1268 (let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); }) and 1269-1271 (if env.set_long_array_region(&arr, 0, &triple).is_err() { return ptr::null_mut(); }) return directly from the guard closure, bypassing the destroy at lines 1275-1281. walletPlatformAddressMinAmounts has the identical shape at lines 1330-1335 before its destroy at 1340-1346. A live transient platform-address handle is stranded in PlatformWalletManager's Arc registry every time the JVM cannot allocate the 2- or 3-long array — exactly the memory-pressure path where leaks compound. Mirror the neighboring transfer/withdraw exports that funnel every path through a shared let out = if … else …; binding before the unconditional destroy.
suggestion: Negative token amounts and expected-total-costs reinterpreted as huge u64 values at the JNI boundary
packages/rs-unified-sdk-jni/src/tokens.rs (line 704)
Java_..._TokensNative_tokenPurchase casts amount as u64 and expected_total_cost as u64 at lines 710-711 with no sign check before handing them to platform_wallet_token_purchase. A Kotlin caller-side -1 sentinel or arithmetic underflow becomes u64::MAX — a valid u64 that the platform-wallet layer will interpret as a colossal purchase amount or expected cost. The same direct signed-to-unsigned bit-cast pattern is used across mint, burn, transfer, and set_price entry points in this file. Validate at the JNI edge and throw DashSDKException for negatives, matching the guard suggested on the Core send path.
suggestion: Derived private-key scalar left un-zeroized on the JNI stack
packages/rs-unified-sdk-jni/src/identity.rs (line 240)
let scalar = out_key.private_key_bytes; at line 242 (and the identical resolver-keyed sibling at line 326 in deriveIdentityPrivateKeyWithResolver) copies the 32-byte ECDSA scalar into a bare [u8; 32] stack local — [u8; 32] is Copy, so this is a truly independent copy — before byte_array_from_slice produces the JVM byte[]. The paired platform_wallet_derive_identity_private_key_at_slot_free / dash_sdk_derive_identity_key_at_slot_free scrubs the Rust-owned buffer but cannot reach this independent stack copy, so plaintext key material persists in the JNI stack frame until unrelated frames overwrite it. This directly contradicts the module's own zeroize discipline (Zeroizing buffers, volatile zeroize on free, non_secure_erase of xprivs). Either pass &out_key.private_key_bytes directly to byte_array_from_slice (no stack copy) or wrap the local in zeroize::Zeroizing::new(...) so Drop scrubs it before the guard returns.
suggestion: Negative platform account indexes and fee rates are silently clamped to 0
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1093)
walletPlatformAddressTransfer (line 1096), walletPlatformAddressWithdraw (line 1178), and walletPlatformAddressPreflightWithdrawal (lines 1252-1253) all convert the signed Kotlin int with account_index.max(0) as u32 (and core_fee_per_byte.max(0) as u32). A caller passing -1 — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can move credits from or preflight against the wrong account with no error crossing the boundary. Arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw DashSDKException on negatives instead of clamping.
suggestion: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
packages/kotlin-sdk/build_android.sh (line 80)
Line 84 pipes ls "$NDK_ROOT" | sort -V | tail -1 to pick the newest NDK, but BSD sort on stock macOS does not implement -V. The script is explicitly macOS-oriented (exFAT sparse-image handling elsewhere; macOS $HOME/Library/Android/sdk default). On a developer machine with unset ANDROID_NDK_HOME and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (e.g. 9.x sorts after 28.x lexically). Use a portable numeric-key sort over dotted version components.
ANDROID_NDK_HOME="$NDK_ROOT/$(find "$NDK_ROOT" -mindepth 1 -maxdepth 1 -type d -exec basename {} \; | sort -t. -k1,1n -k2,2n -k3,3n | tail -1)"
suggestion: reqwest TLS backend swap silently changes iOS trust behavior — needs iOS validation or target gating
packages/rs-sdk-ffi/Cargo.toml (line 71)
The unconditional switch to default-features = false, features = ["json", "rustls-tls-webpki-roots"] fixes the Android build (no OpenSSL, no readable system store) but also flips the TLS backend for every iOS build of rs-sdk-ffi and rs-sdk-trusted-context-provider. iOS clients previously validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) OS trust-store updates (revocations, root removals) no longer propagate until the SDK rebuilds against a newer webpki-roots. Since rs-sdk-trusted-context-provider sits on the SDK trust path for proof-related traffic, the blast radius is broad. The Cargo.toml comment acknowledges the mobile parity intent, but the PR test plan documents Android/host verification only. Either narrow with a cfg(target_os)-driven feature split (native-tls on Apple targets) or land an explicit iOS integration smoke test against a real HTTPS endpoint through both crates, and note the behavior change in the CHANGELOG.
suggestion: Android TEST_PLAN marks deferred JNI features as automatable
packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.md (line 121)
The test-plan legend says rows with ✅, 🧪, or ⚠️ are automatable now, but several rows mark still-deferred features as ✅: ID-06 top-up-from-addresses, ID-07 add public key, ID-08 create-from-addresses, ID-11 transfer-to-addresses, and ID-12 disable key. Verification confirms the mismatch: KeysListScreen.kt:34 explicitly names add/disable key as deferred pending the updateIdentity FFI; AddIdentityKeyScreen does not exist in the app tree (only the TEST_PLAN references it); AddressQueriesScreen.kt contains no TopUpIdentityFromAddresses / CreateIdentityFromAddresses / TransferIdentityToAddresses symbols; and dash_sdk_identity_top_up_from_addresses / _create_from_addresses / _transfer_credits_to_addresses exist in rs-sdk-ffi but have zero Kotlin/JNI callers. A QA agent following this file will try to run impossible tests and report false failures. Downgrade these rows to a deferred marker until the JNI symbols and screens land.
nitpick: PARITY.md still marks SendTransactionView partial after the broadcast bridge landed
packages/kotlin-sdk/PARITY.md (line 122)
Row 122 still says SendTransactionView is partial with 'broadcast deferred on core_wallet_send_to_addresses', but the JNI surface now includes WalletManagerNative.walletCoreSendToAddresses, ManagedPlatformWallet.sendToAddresses wraps it, and SendTransactionScreen.kt calls that wrapper — broadcast is wired end-to-end. The Totals block on lines 132-134 still counts SendTransactionView among the 8 partial views. Since this file is presented as the source of truth for missing FFI exports, the stale row and totals will mislead follow-up interop work.
| SendTransactionView.swift | ui/wallet/SendTransactionScreen.kt · `SendTransaction` | ported |
🤖 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.
- [BLOCKING] In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt`:122-133: DataContractRef.close() is non-atomic and can double-free the Rust handle
`DataContractRef` stores the native pointer in a plain `private var handle: Long` (line 123) and `close()` does a non-atomic load → store → destroy: `val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h)` (lines 128-132). Two concurrent closers — e.g. an explicit `use {}` finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires, or two coroutines racing on the ref — can each observe the same non-zero `h` before either has stored 0, and both will invoke `dataContractDestroy(h)`. The Rust side of that JNI symbol reconstructs the allocation with `Box::from_raw(handle as *mut DataContract)`, so the second call is a real double-free / use-after-free across the JNI boundary. The rest of this SDK (`Sdk.handle`, `ManagedPlatformWallet.HandleCleanup`, `PlatformWalletManager.bundleRef`) already uses `AtomicLong.getAndSet(0)` precisely for this ownership handoff — apply the same pattern here. The identical non-atomic shape is also present in `ContactRequestRef` and `EstablishedContactRef` in `tokens/Dashpay.kt` and needs the same fix (those handles are registry removals rather than direct `Box::from_raw` frees, but repeated close is still incorrect).
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:466-478: Negative Core send amounts silently bit-cast to huge u64 values at the JNI boundary
`walletCoreSendToAddresses` reads Kotlin `long[]` duffs into `Vec<i64>` (line 466) and then does `let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();` (line 478) with no sign check before passing them to `core_wallet_send_to_addresses`. A caller-side `-1` sentinel, off-by-one, or arithmetic underflow becomes `u64::MAX ≈ 1.8e19` duffs. Even when Rust rejects it downstream the failure mode is a confusing fee/overflow error rather than the intended boundary-level `DashSDKException`. Validate at the boundary; the same treatment applies to `core_fee_per_byte`.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1258-1346: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
After `platform_wallet_get_platform` succeeds, `addr_handle` must be paired with `platform_address_wallet_destroy` on every exit path. In `walletPlatformAddressPreflightWithdrawal` the branches at lines 1266-1268 (`let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); }`) and 1269-1271 (`if env.set_long_array_region(&arr, 0, &triple).is_err() { return ptr::null_mut(); }`) return directly from the guard closure, bypassing the destroy at lines 1275-1281. `walletPlatformAddressMinAmounts` has the identical shape at lines 1330-1335 before its destroy at 1340-1346. A live transient platform-address handle is stranded in `PlatformWalletManager`'s Arc registry every time the JVM cannot allocate the 2- or 3-long array — exactly the memory-pressure path where leaks compound. Mirror the neighboring transfer/withdraw exports that funnel every path through a shared `let out = if … else …;` binding before the unconditional destroy.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/tokens.rs`:704-716: Negative token amounts and expected-total-costs reinterpreted as huge u64 values at the JNI boundary
`Java_..._TokensNative_tokenPurchase` casts `amount as u64` and `expected_total_cost as u64` at lines 710-711 with no sign check before handing them to `platform_wallet_token_purchase`. A Kotlin caller-side `-1` sentinel or arithmetic underflow becomes `u64::MAX` — a valid u64 that the platform-wallet layer will interpret as a colossal purchase amount or expected cost. The same direct signed-to-unsigned bit-cast pattern is used across `mint`, `burn`, `transfer`, and `set_price` entry points in this file. Validate at the JNI edge and throw `DashSDKException` for negatives, matching the guard suggested on the Core send path.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/identity.rs`:240-256: Derived private-key scalar left un-zeroized on the JNI stack
`let scalar = out_key.private_key_bytes;` at line 242 (and the identical resolver-keyed sibling at line 326 in `deriveIdentityPrivateKeyWithResolver`) copies the 32-byte ECDSA scalar into a bare `[u8; 32]` stack local — `[u8; 32]` is `Copy`, so this is a truly independent copy — before `byte_array_from_slice` produces the JVM `byte[]`. The paired `platform_wallet_derive_identity_private_key_at_slot_free` / `dash_sdk_derive_identity_key_at_slot_free` scrubs the Rust-owned buffer but cannot reach this independent stack copy, so plaintext key material persists in the JNI stack frame until unrelated frames overwrite it. This directly contradicts the module's own zeroize discipline (`Zeroizing` buffers, volatile zeroize on free, `non_secure_erase` of xprivs). Either pass `&out_key.private_key_bytes` directly to `byte_array_from_slice` (no stack copy) or wrap the local in `zeroize::Zeroizing::new(...)` so `Drop` scrubs it before the guard returns.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1093-1253: Negative platform account indexes and fee rates are silently clamped to 0
`walletPlatformAddressTransfer` (line 1096), `walletPlatformAddressWithdraw` (line 1178), and `walletPlatformAddressPreflightWithdrawal` (lines 1252-1253) all convert the signed Kotlin int with `account_index.max(0) as u32` (and `core_fee_per_byte.max(0) as u32`). A caller passing `-1` — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can move credits from or preflight against the wrong account with no error crossing the boundary. Arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw `DashSDKException` on negatives instead of clamping.
- [SUGGESTION] In `packages/kotlin-sdk/build_android.sh`:80-87: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
Line 84 pipes `ls "$NDK_ROOT" | sort -V | tail -1` to pick the newest NDK, but BSD `sort` on stock macOS does not implement `-V`. The script is explicitly macOS-oriented (exFAT sparse-image handling elsewhere; macOS `$HOME/Library/Android/sdk` default). On a developer machine with unset `ANDROID_NDK_HOME` and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (e.g. `9.x` sorts after `28.x` lexically). Use a portable numeric-key sort over dotted version components.
- [SUGGESTION] In `packages/rs-sdk-ffi/Cargo.toml`:71-77: reqwest TLS backend swap silently changes iOS trust behavior — needs iOS validation or target gating
The unconditional switch to `default-features = false, features = ["json", "rustls-tls-webpki-roots"]` fixes the Android build (no OpenSSL, no readable system store) but also flips the TLS backend for every iOS build of `rs-sdk-ffi` and `rs-sdk-trusted-context-provider`. iOS clients previously validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) OS trust-store updates (revocations, root removals) no longer propagate until the SDK rebuilds against a newer `webpki-roots`. Since `rs-sdk-trusted-context-provider` sits on the SDK trust path for proof-related traffic, the blast radius is broad. The Cargo.toml comment acknowledges the mobile parity intent, but the PR test plan documents Android/host verification only. Either narrow with a `cfg(target_os)`-driven feature split (native-tls on Apple targets) or land an explicit iOS integration smoke test against a real HTTPS endpoint through both crates, and note the behavior change in the CHANGELOG.
- [SUGGESTION] In `packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.md`:121-134: Android TEST_PLAN marks deferred JNI features as automatable
The test-plan legend says rows with `✅`, `🧪`, or `⚠️` are automatable now, but several rows mark still-deferred features as `✅`: ID-06 top-up-from-addresses, ID-07 add public key, ID-08 create-from-addresses, ID-11 transfer-to-addresses, and ID-12 disable key. Verification confirms the mismatch: `KeysListScreen.kt:34` explicitly names add/disable key as deferred pending the `updateIdentity` FFI; `AddIdentityKeyScreen` does not exist in the app tree (only the TEST_PLAN references it); `AddressQueriesScreen.kt` contains no `TopUpIdentityFromAddresses` / `CreateIdentityFromAddresses` / `TransferIdentityToAddresses` symbols; and `dash_sdk_identity_top_up_from_addresses` / `_create_from_addresses` / `_transfer_credits_to_addresses` exist in `rs-sdk-ffi` but have zero Kotlin/JNI callers. A QA agent following this file will try to run impossible tests and report false failures. Downgrade these rows to a deferred marker until the JNI symbols and screens land.
- [NITPICK] In `packages/kotlin-sdk/PARITY.md`:122-134: PARITY.md still marks SendTransactionView partial after the broadcast bridge landed
Row 122 still says `SendTransactionView` is partial with 'broadcast deferred on `core_wallet_send_to_addresses`', but the JNI surface now includes `WalletManagerNative.walletCoreSendToAddresses`, `ManagedPlatformWallet.sendToAddresses` wraps it, and `SendTransactionScreen.kt` calls that wrapper — broadcast is wired end-to-end. The Totals block on lines 132-134 still counts SendTransactionView among the 8 partial views. Since this file is presented as the source of truth for missing FFI exports, the stale row and totals will mislead follow-up interop work.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta 675402b..579fb31 is docs + QA-contract config only (removed Group category, renumbered System 11→10, dropped Group TEST_PLAN section). No Rust/Kotlin source touched. All 10 prior findings against 675402b re-verified STILL VALID at HEAD, including the blocking non-atomic DataContractRef.close() double-free. One minor new nit in qa-contract/codes.mjs (renumber contradicts the file's own stability invariant), dropped for budget in favor of the higher-signal convergent findings.
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.
🔴 1 blocking | 🟡 8 suggestion(s) | 💬 1 nitpick(s)
10 additional finding(s)
blocking: DataContractRef.close() is non-atomic and can double-free the Rust handle across the JNI boundary
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt (line 122)
Verified at HEAD: DataContractRef stores the native pointer in a plain private var handle: Long (line 123) and close() performs a non-atomic read → store → destroy (val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h), lines 128–132). Two concurrent closers — e.g. a use {} finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires, or two coroutines racing on the ref — can each observe the same non-zero h before either stores 0, and both will call dataContractDestroy(h). The Rust destructor reconstructs the allocation via Box::from_raw, so the second destroy is a real double-free / use-after-free across the JNI boundary. Every other owning wrapper in this SDK (Sdk.handle, ManagedPlatformWallet.HandleCleanup, PlatformWalletManager.bundleRef) already uses AtomicLong.getAndSet(0) for exactly this ownership handoff — apply the same pattern here. The same non-atomic shape is present in ContactRequestRef / EstablishedContactRef in sdk/.../tokens/Dashpay.kt (registry-remove rather than direct Box::from_raw, but repeated close is still incorrect) and should be fixed together.
class DataContractRef internal constructor(handle: Long) : AutoCloseable {
private val handleRef = java.util.concurrent.atomic.AtomicLong(handle)
internal val value: Long
get() = handleRef.get().also { check(it != 0L) { "DataContractRef has been closed" } }
override fun close() {
val h = handleRef.getAndSet(0)
if (h != 0L) QueriesNative.dataContractDestroy(h)
}
}
suggestion: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1258)
Verified at HEAD. After platform_wallet_get_platform succeeds, addr_handle must be paired with platform_address_wallet_destroy on every exit path. In walletPlatformAddressPreflightWithdrawal, let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); }; (lines 1266–1268) and if env.set_long_array_region(&arr, 0, &triple).is_err() { return ptr::null_mut(); } (1269–1271) return directly from the guard closure, bypassing the destroy at 1275–1281. walletPlatformAddressMinAmounts has the identical shape at 1330–1335 before its destroy at 1340–1346. Each such failure strands a live transient platform-address handle in PlatformWalletManager's Arc registry — exactly the memory-pressure path where leaks compound. Funnel every branch through a single let out = if … else { … }; binding before the unconditional destroy, matching the neighboring transfer/withdraw exports.
suggestion: Negative Core send amounts silently bit-cast to huge u64 values at the JNI boundary
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 466)
Verified at HEAD. walletCoreSendToAddresses reads Kotlin long[] duffs into amount_buf: Vec<i64> (line 466) and then does let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect(); (line 478) with no sign check before forwarding to core_wallet_send_to_addresses. A caller-side -1 sentinel or arithmetic underflow becomes u64::MAX ≈ 1.8e19 duffs; even when Rust rejects it downstream, the failure mode is an obscure fee/overflow error rather than the intended boundary-level DashSDKException. The same guard should be applied to core_fee_per_byte.
if amount_buf.iter().any(|&v| v <= 0) {
throw_sdk_exception(env, 1, "amounts must be positive duff values");
return ptr::null_mut();
}
let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();
suggestion: Negative token amounts and expected-total-costs reinterpreted as huge u64 values at the JNI boundary
packages/rs-unified-sdk-jni/src/tokens.rs (line 704)
Verified at HEAD. Java_..._TokensNative_tokenPurchase casts amount as u64 and expected_total_cost as u64 at lines 710–711 with no sign check before handing them to platform_wallet_token_purchase. A Kotlin caller-side -1 sentinel or arithmetic underflow becomes u64::MAX — a valid u64 that the platform-wallet layer will interpret as a colossal purchase amount or expected cost. The same direct signed-to-unsigned bit-cast pattern is present across the mint / burn / transfer / set_price entry points in this file. Validate at the JNI edge and throw DashSDKException for negatives.
suggestion: Derived private-key scalar left un-zeroized on the JNI stack
packages/rs-unified-sdk-jni/src/identity.rs (line 240)
Verified at HEAD (line 242 and the identical resolver-keyed sibling around line 326). out_key.private_key_bytes is [u8; 32] (Copy), so let scalar = out_key.private_key_bytes; copies the 32-byte ECDSA scalar into an independent stack local before byte_array_from_slice builds the JVM byte[]. The paired platform_wallet_derive_identity_private_key_at_slot_free scrubs the Rust-owned buffer but cannot reach the independent stack copy, so plaintext key material persists in the JNI stack frame until unrelated frames overwrite it. This contradicts the module's own zeroize discipline (Zeroizing buffers, volatile zeroize on free, non_secure_erase of xprivs). Either pass &out_key.private_key_bytes directly to byte_array_from_slice (no stack copy) or wrap the local in zeroize::Zeroizing::new(...) so Drop scrubs it before the guard returns.
suggestion: Negative platform account indexes and fee rates are silently clamped to 0
packages/rs-unified-sdk-jni/src/wallet_manager.rs (line 1093)
Verified at HEAD (line 1252–1253 shows account_index.max(0) as u32, core_fee_per_byte.max(0) as u32). walletPlatformAddressTransfer, walletPlatformAddressWithdraw, and walletPlatformAddressPreflightWithdrawal all convert the signed Kotlin int with .max(0) as u32. A caller passing -1 — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can move credits from or preflight against the wrong account with no error crossing the boundary. Arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw DashSDKException on negatives instead of clamping.
suggestion: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
packages/kotlin-sdk/build_android.sh (line 80)
Verified at HEAD (line 84: ANDROID_NDK_HOME="$NDK_ROOT/$(ls "$NDK_ROOT" | sort -V | tail -1)"). BSD sort on stock macOS does not implement -V. The script is explicitly macOS-oriented (exFAT sparse-image handling elsewhere; macOS $HOME/Library/Android/sdk default at line 82). On a developer machine with unset ANDROID_NDK_HOME and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (9.x sorts after 28.x lexically). Use a portable numeric-key sort over dotted version components.
ANDROID_NDK_HOME="$NDK_ROOT/$(find "$NDK_ROOT" -mindepth 1 -maxdepth 1 -type d -exec basename {} \; | sort -t. -k1,1n -k2,2n -k3,3n | tail -1)"
suggestion: reqwest TLS backend swap silently changes iOS trust behavior — needs iOS validation or target gating
packages/rs-sdk-ffi/Cargo.toml (line 71)
Verified at HEAD (line 77 still default-features = false, features = ["json", "rustls-tls-webpki-roots"], no target-os gate). The unconditional switch fixes the Android build (no OpenSSL, no readable system store) but also flips the TLS backend for every iOS build of rs-sdk-ffi and rs-sdk-trusted-context-provider. iOS clients previously validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) OS trust-store updates (revocations, root removals) no longer propagate until the SDK rebuilds against a newer webpki-roots. Since rs-sdk-trusted-context-provider sits on the SDK trust path for proof-related traffic, the blast radius is broad. The Cargo.toml comment acknowledges the mobile parity intent, but the PR test plan documents Android/host verification only. Either narrow with a cfg(target_os)-driven feature split (rustls-tls-native-roots / native-tls on Apple targets) or land an explicit iOS integration smoke test against a real HTTPS endpoint through both crates, and note the behavior change in the CHANGELOG.
suggestion: Android TEST_PLAN marks deferred JNI features as automatable
packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.md (line 121)
The test-plan legend says rows with ✅, 🧪, or ⚠️ are automatable now, but ID-06 top-up-from-addresses, ID-07 add public key, ID-08 create-from-addresses, ID-11 transfer-to-addresses, and ID-12 disable key remain ✅ while their referenced screens/symbols do not exist: AddIdentityKeyScreen, TopUpIdentityFromAddresses, CreateIdentityFromAddresses, TransferIdentityToAddresses are referenced only from TEST_PLAN.md (and PARITY.md's deferred list). dash_sdk_identity_top_up_from_addresses / _create_from_addresses / _transfer_credits_to_addresses exist in rs-sdk-ffi but have zero JNI exports or Kotlin call sites in packages/rs-unified-sdk-jni / packages/kotlin-sdk. A QA agent following this file will try to run impossible tests and report false failures. Downgrade these rows to a deferred marker until the JNI symbols and screens land.
nitpick: PARITY.md still marks SendTransactionView partial after the broadcast bridge landed
packages/kotlin-sdk/PARITY.md (line 122)
Row 122 still says SendTransactionView is partial with 'broadcast deferred on core_wallet_send_to_addresses', but the JNI surface now includes WalletManagerNative.walletCoreSendToAddresses, ManagedPlatformWallet.sendToAddresses wraps it, and SendTransactionScreen.kt calls that wrapper — broadcast is wired end-to-end. The Totals block on lines 132–134 still counts SendTransactionView among the 8 partial views. Since this file is presented as the source of truth for missing FFI exports, the stale row and totals will mislead follow-up interop work. Flip the row to ported and bump totals accordingly.
| SendTransactionView.swift | ui/wallet/SendTransactionScreen.kt · `SendTransaction` | ported |
🤖 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.
- [BLOCKING] In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt`:122-133: DataContractRef.close() is non-atomic and can double-free the Rust handle across the JNI boundary
Verified at HEAD: `DataContractRef` stores the native pointer in a plain `private var handle: Long` (line 123) and `close()` performs a non-atomic read → store → destroy (`val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h)`, lines 128–132). Two concurrent closers — e.g. a `use {}` finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires, or two coroutines racing on the ref — can each observe the same non-zero `h` before either stores 0, and both will call `dataContractDestroy(h)`. The Rust destructor reconstructs the allocation via `Box::from_raw`, so the second destroy is a real double-free / use-after-free across the JNI boundary. Every other owning wrapper in this SDK (`Sdk.handle`, `ManagedPlatformWallet.HandleCleanup`, `PlatformWalletManager.bundleRef`) already uses `AtomicLong.getAndSet(0)` for exactly this ownership handoff — apply the same pattern here. The same non-atomic shape is present in `ContactRequestRef` / `EstablishedContactRef` in `sdk/.../tokens/Dashpay.kt` (registry-remove rather than direct `Box::from_raw`, but repeated close is still incorrect) and should be fixed together.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1258-1346: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
Verified at HEAD. After `platform_wallet_get_platform` succeeds, `addr_handle` must be paired with `platform_address_wallet_destroy` on every exit path. In `walletPlatformAddressPreflightWithdrawal`, `let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); };` (lines 1266–1268) and `if env.set_long_array_region(&arr, 0, &triple).is_err() { return ptr::null_mut(); }` (1269–1271) return directly from the guard closure, bypassing the destroy at 1275–1281. `walletPlatformAddressMinAmounts` has the identical shape at 1330–1335 before its destroy at 1340–1346. Each such failure strands a live transient platform-address handle in `PlatformWalletManager`'s Arc registry — exactly the memory-pressure path where leaks compound. Funnel every branch through a single `let out = if … else { … };` binding before the unconditional destroy, matching the neighboring transfer/withdraw exports.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:466-478: Negative Core send amounts silently bit-cast to huge u64 values at the JNI boundary
Verified at HEAD. `walletCoreSendToAddresses` reads Kotlin `long[]` duffs into `amount_buf: Vec<i64>` (line 466) and then does `let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();` (line 478) with no sign check before forwarding to `core_wallet_send_to_addresses`. A caller-side `-1` sentinel or arithmetic underflow becomes `u64::MAX ≈ 1.8e19` duffs; even when Rust rejects it downstream, the failure mode is an obscure fee/overflow error rather than the intended boundary-level `DashSDKException`. The same guard should be applied to `core_fee_per_byte`.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/tokens.rs`:704-716: Negative token amounts and expected-total-costs reinterpreted as huge u64 values at the JNI boundary
Verified at HEAD. `Java_..._TokensNative_tokenPurchase` casts `amount as u64` and `expected_total_cost as u64` at lines 710–711 with no sign check before handing them to `platform_wallet_token_purchase`. A Kotlin caller-side `-1` sentinel or arithmetic underflow becomes `u64::MAX` — a valid u64 that the platform-wallet layer will interpret as a colossal purchase amount or expected cost. The same direct signed-to-unsigned bit-cast pattern is present across the `mint` / `burn` / `transfer` / `set_price` entry points in this file. Validate at the JNI edge and throw `DashSDKException` for negatives.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/identity.rs`:240-256: Derived private-key scalar left un-zeroized on the JNI stack
Verified at HEAD (line 242 and the identical resolver-keyed sibling around line 326). `out_key.private_key_bytes` is `[u8; 32]` (Copy), so `let scalar = out_key.private_key_bytes;` copies the 32-byte ECDSA scalar into an independent stack local before `byte_array_from_slice` builds the JVM `byte[]`. The paired `platform_wallet_derive_identity_private_key_at_slot_free` scrubs the Rust-owned buffer but cannot reach the independent stack copy, so plaintext key material persists in the JNI stack frame until unrelated frames overwrite it. This contradicts the module's own zeroize discipline (`Zeroizing` buffers, volatile zeroize on free, `non_secure_erase` of xprivs). Either pass `&out_key.private_key_bytes` directly to `byte_array_from_slice` (no stack copy) or wrap the local in `zeroize::Zeroizing::new(...)` so `Drop` scrubs it before the guard returns.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1093-1253: Negative platform account indexes and fee rates are silently clamped to 0
Verified at HEAD (line 1252–1253 shows `account_index.max(0) as u32, core_fee_per_byte.max(0) as u32`). `walletPlatformAddressTransfer`, `walletPlatformAddressWithdraw`, and `walletPlatformAddressPreflightWithdrawal` all convert the signed Kotlin int with `.max(0) as u32`. A caller passing `-1` — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can move credits from or preflight against the wrong account with no error crossing the boundary. Arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw `DashSDKException` on negatives instead of clamping.
- [SUGGESTION] In `packages/kotlin-sdk/build_android.sh`:80-87: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
Verified at HEAD (line 84: `ANDROID_NDK_HOME="$NDK_ROOT/$(ls "$NDK_ROOT" | sort -V | tail -1)"`). BSD `sort` on stock macOS does not implement `-V`. The script is explicitly macOS-oriented (exFAT sparse-image handling elsewhere; macOS `$HOME/Library/Android/sdk` default at line 82). On a developer machine with unset `ANDROID_NDK_HOME` and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (`9.x` sorts after `28.x` lexically). Use a portable numeric-key sort over dotted version components.
- [SUGGESTION] In `packages/rs-sdk-ffi/Cargo.toml`:71-77: reqwest TLS backend swap silently changes iOS trust behavior — needs iOS validation or target gating
Verified at HEAD (line 77 still `default-features = false, features = ["json", "rustls-tls-webpki-roots"]`, no target-os gate). The unconditional switch fixes the Android build (no OpenSSL, no readable system store) but also flips the TLS backend for every iOS build of `rs-sdk-ffi` and `rs-sdk-trusted-context-provider`. iOS clients previously validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) OS trust-store updates (revocations, root removals) no longer propagate until the SDK rebuilds against a newer `webpki-roots`. Since `rs-sdk-trusted-context-provider` sits on the SDK trust path for proof-related traffic, the blast radius is broad. The Cargo.toml comment acknowledges the mobile parity intent, but the PR test plan documents Android/host verification only. Either narrow with a `cfg(target_os)`-driven feature split (`rustls-tls-native-roots` / native-tls on Apple targets) or land an explicit iOS integration smoke test against a real HTTPS endpoint through both crates, and note the behavior change in the CHANGELOG.
- [SUGGESTION] In `packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.md`:121-134: Android TEST_PLAN marks deferred JNI features as automatable
The test-plan legend says rows with `✅`, `🧪`, or `⚠️` are automatable now, but ID-06 top-up-from-addresses, ID-07 add public key, ID-08 create-from-addresses, ID-11 transfer-to-addresses, and ID-12 disable key remain `✅` while their referenced screens/symbols do not exist: `AddIdentityKeyScreen`, `TopUpIdentityFromAddresses`, `CreateIdentityFromAddresses`, `TransferIdentityToAddresses` are referenced only from TEST_PLAN.md (and PARITY.md's deferred list). `dash_sdk_identity_top_up_from_addresses` / `_create_from_addresses` / `_transfer_credits_to_addresses` exist in `rs-sdk-ffi` but have zero JNI exports or Kotlin call sites in `packages/rs-unified-sdk-jni` / `packages/kotlin-sdk`. A QA agent following this file will try to run impossible tests and report false failures. Downgrade these rows to a deferred marker until the JNI symbols and screens land.
- [NITPICK] In `packages/kotlin-sdk/PARITY.md`:122-134: PARITY.md still marks SendTransactionView partial after the broadcast bridge landed
Row 122 still says `SendTransactionView` is partial with 'broadcast deferred on `core_wallet_send_to_addresses`', but the JNI surface now includes `WalletManagerNative.walletCoreSendToAddresses`, `ManagedPlatformWallet.sendToAddresses` wraps it, and `SendTransactionScreen.kt` calls that wrapper — broadcast is wired end-to-end. The Totals block on lines 132–134 still counts SendTransactionView among the 8 partial views. Since this file is presented as the source of truth for missing FFI exports, the stale row and totals will mislead follow-up interop work. Flip the row to `ported` and bump totals accordingly.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the same verified findings as a top-level review body.
…eys, document pricing, voting, diagnostics) Takes PARITY.md from 75/8/7 to 88 ported / 2 partial / 0 deferred of 90 Swift views: - 33 new read-only query bridges (identity keys/nonces/balances, addresses, voting/contested resources, evonodes, epochs, protocol upgrade state, groups, system incl. GroveDB path elements, token supply/distributions/prices, contract-with-serialization) — the app's query registry now mirrors the full 45-entry iOS catalog with testnet fixtures. - Write paths: identity update (add/disable keys, incl. a keypair-returning slot derive), document purchase/set-price, contested vote casting, address ban info, wallet memory snapshots, withdrawal preflight reason. - New screens: AddIdentityKey, DocumentWithPrice, GroveDBPathElements; ContestDetail now shows live vote state with cast-vote; IdentityDetail gains contested-name rows; TransitionDetail executes 18 of 23 catalog entries; diagnostics screens fully wired (address queries, banned addresses, run-all summary, memory drill-downs). - rs-sdk-ffi: re-export identity nonce/contract-nonce/contract-keys and data-contract fetch-with-serialization from their module roots. Remaining (named in PARITY.md): 5 transition catalog entries lacking backing FFIs (dataContractUpdate, documentCreate/Replace/Delete/ Transfer) and the memory-explorer asset-lock drill-down. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
One blocking finding at HEAD 2c81ae7: DataContractRef.close() is non-atomic and can double-free the Rust handle across JNI. The prior 579fb31 findings were explicitly reconciled: all still-valid prior issues are carried forward here, with the PARITY.md row fixed and the TEST_PLAN row narrowed to the address-funded identity flows that remain unbridged. The latest delta also adds three new correctness/security issues: document_price_op silently clamps negative price/signingKeyId, the ECDSA_HASH160 add-key path submits the 33-byte compressed pubkey instead of HASH160, and castContestedResourceVote leaves the voting private key in an un-zeroized JNI stack local.
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.
🔴 1 blocking | 🟡 11 suggestion(s)
Verified Findings
blocking: DataContractRef.close() is non-atomic and can double-free the Rust handle
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt (lines 620-631)
Verified at HEAD. DataContractRef stores the native pointer in a plain private var handle: Long (line 621) and close() does a non-atomic read → store → destroy: val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h). Two concurrent closers — e.g. a use {} finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires, or two coroutines racing on the ref — can each observe the same non-zero h before either stores 0, and both will call QueriesNative.dataContractDestroy(h). The Rust destroy at rs-sdk-ffi/src/data_contract/mod.rs reconstructs the allocation via Box::from_raw, so the second destroy is a real double-free / use-after-free across JNI. Every other owning wrapper in this SDK (Sdk.handle, ManagedPlatformWallet.HandleCleanup, PlatformWalletManager.bundleRef) already uses AtomicLong.getAndSet(0) for exactly this ownership handoff — apply it here. The same non-atomic shape is in ContactRequestRef / EstablishedContactRef at sdk/.../tokens/Dashpay.kt:226-250; those free via registry remove rather than direct Box::from_raw, but a concurrent second close() is still a defect (JNI destroy on a possibly-recycled slot) and should be fixed together.
class DataContractRef internal constructor(handle: Long) : AutoCloseable {
private val handleRef = java.util.concurrent.atomic.AtomicLong(handle)
internal val value: Long
get() = handleRef.get().also { check(it != 0L) { "DataContractRef has been closed" } }
override fun close() {
val h = handleRef.getAndSet(0)
if (h != 0L) QueriesNative.dataContractDestroy(h)
}
}
suggestion: ECDSA_HASH160 add-key rows submit the 33-byte compressed pubkey instead of the required 20-byte HASH160
packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/IdentityKeyAdditionFlow.kt (lines 141-150)
Verified: AddIdentityKeyScreen.kt:164 allows selecting KeyType.ECDSA_HASH160, wires a real deriver (mgr.deriveIdentityKeyPair, line 332), and calls IdentityKeyAdditionFlow.prepareKeys. prepareKeys always stores and submits derived.publicKey as IdentityPubkey.pubkeyBytes regardless of spec.keyType. The Swift reference (swift-sdk/.../Views/IdentityKeyAddition.swift:152-163) explicitly computes HASH160 for ecdsaHash160 and passes the 20-byte hash as pubkeyBytes (see comment: 'For ECDSA_HASH160 the on-chain payload is the 20-byte HASH160 of the compressed pubkey, not the pubkey itself'). Additionally, the Swift signer trampoline stores the metadata under the 20-byte HASH160 hex for HASH160 keys and the 33-byte pubkey hex otherwise; the Kotlin flow stores under 33-byte hex unconditionally (line 132/136). Selecting ECDSA_HASH160 in the current build either produces an invalid identity update or, if it is accepted, persists the private key under a hex key the signer will not look up. Compute the HASH160 (RIPEMD160(SHA256(pubkey))) for ECDSA_HASH160 rows, submit that as pubkeyBytes, and key the Keystore entry the same way the signer will look it up.
suggestion: documentPurchase / documentSetPrice silently clamp negative price and signingKeyId to zero at the JNI boundary
packages/rs-unified-sdk-jni/src/transactions.rs (lines 425-452)
New in this delta. document_price_op (transactions.rs:397-475) forwards Kotlin's signed price: jlong and signing_key_id: jint with price.max(0) as u64 and signing_key_id.max(0) as u32 (lines 433-434 for platform_wallet_document_purchase, 446-447 for _set_price). Concrete consequences: (a) a Kotlin-side -1 sentinel or arithmetic underflow silently sets the trade price to 0 credits — the user posts a for-sale document that anyone can buy for free — with no error crossing the boundary; (b) a negative signingKeyId silently signs the transition under key id 0 (typically MASTER), which either rejects downstream with a confusing error or, worse, succeeds under a key the caller did not intend. Blast radius is higher than the sibling account-index clamp because a silently-zero price is directly asset-affecting. Reject negatives at the JNI edge with throw_sdk_exception before casting.
suggestion: Derived private-key scalars left un-zeroized on the JNI stack (three sibling exports, including new keypair variant)
packages/rs-unified-sdk-jni/src/identity.rs (lines 240-396)
Verified at HEAD. Three exports copy the 32-byte ECDSA scalar into an independent stack local before byte_array_from_slice builds the JVM byte[]: deriveIdentityPrivateKey (line 242), deriveIdentityPrivateKeyWithResolver (line 326), and the new deriveIdentityKeyPairWithResolver added in this delta (line 384). out_row.private_key_bytes is [u8; 32] (Copy), so let scalar = out_row.private_key_bytes; is a truly independent copy. The paired platform_wallet_derive_identity_private_key_at_slot_free / dash_sdk_derive_identity_key_at_slot_free zeroize the Rust-owned buffer but cannot reach the independent stack copy, so plaintext key material persists in the JNI stack frame until unrelated frames overwrite it. This contradicts the module's own zeroize discipline (Zeroizing buffers, volatile zeroize on free, non_secure_erase of xprivs). The Kotlin caller (IdentityKeyAdditionFlow.prepareKeys) scrubs the JVM byte[] after use (derived.privateKey.fill(0)), amplifying the asymmetry — only the Rust stack copy stays warm. Either pass &out_row.private_key_bytes directly to byte_array_from_slice (no stack copy) or wrap the local in zeroize::Zeroizing::new(...) so Drop scrubs it before the guard returns.
suggestion: castContestedResourceVote copies the 32-byte voting private key into an un-zeroized JNI stack local
packages/rs-unified-sdk-jni/src/transactions.rs (lines 602-662)
New in this delta. read_id32(env, &voting_private_key, "votingPrivateKey") at transactions.rs:605 produces voting_key: [u8; 32] — a bare Copy stack local that receives the masternode voting private key. It is passed to dash_sdk_contested_resource_cast_vote via voting_key.as_ptr() at line 654 and implicitly dropped when the closure returns at 662, with no zeroize step. Same class of leak as the identity-key derive path, but on a caller-owned Kotlin ByteArray — scrubbing on the Rust side is the only line of defense between the JVM copy and the FFI call. read_id32's intermediate bytes: Vec<u8> also drops without zeroize. Wrap voting_key in zeroize::Zeroizing::new(...) (or explicitly zeroize before return) and either add a zeroizing sibling of read_id32 on the key path or scrub the intermediate bytes there.
suggestion: Negative Core send amounts silently bit-cast to huge u64 values at the JNI boundary
packages/rs-unified-sdk-jni/src/wallet_manager.rs (lines 466-484)
Verified at HEAD. walletCoreSendToAddresses reads Kotlin long[] duffs into amount_buf: Vec<i64> (line 472) and then does let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect(); (line 484) with no sign check before forwarding to core_wallet_send_to_addresses. A caller-side -1 sentinel or arithmetic underflow becomes u64::MAX ≈ 1.8e19 duffs; even when Rust rejects it downstream, the failure mode is an obscure fee/overflow error rather than the intended boundary-level DashSDKException. Apply the same guard to core_fee_per_byte.
if amount_buf.iter().any(|&v| v <= 0) {
throw_sdk_exception(env, 1, "amounts must be positive duff values");
return ptr::null_mut();
}
let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();
suggestion: Negative token amounts and expected total costs reinterpreted as huge u64 values at the JNI boundary
packages/rs-unified-sdk-jni/src/tokens.rs (lines 704-716)
Verified at HEAD. Java_..._TokensNative_tokenPurchase casts amount as u64 and expected_total_cost as u64 at lines 710-711 with no sign check before handing them to platform_wallet_token_purchase. A Kotlin caller-side -1 sentinel or arithmetic underflow becomes u64::MAX — a valid u64 the platform-wallet layer interprets as a colossal purchase amount or expected cost. The same signed-to-unsigned bit-cast pattern is present across the mint / burn / transfer / set_price entry points in this file. Validate at the JNI edge and throw DashSDKException for negatives.
suggestion: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
packages/rs-unified-sdk-jni/src/wallet_manager.rs (lines 1266-1352)
Verified at HEAD. After platform_wallet_get_platform succeeds, addr_handle must be paired with platform_address_wallet_destroy on every exit path. In walletPlatformAddressPreflightWithdrawal, let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); }; (lines 1272-1274) and if env.set_long_array_region(&arr, 0, &triple).is_err() { return ptr::null_mut(); } (1275-1277) return directly from the guard closure, bypassing the destroy at 1281-1287. walletPlatformAddressMinAmounts has the identical shape at 1336-1341 before its destroy at 1346-1352. Each such failure strands a live transient platform-address handle in PlatformWalletManager's Arc registry — exactly the memory-pressure path where leaks compound. The new walletPlatformAddressPreflightWithdrawalReason in this delta already funnels every branch through a single out binding before the unconditional destroy — mirror that structure here.
suggestion: Negative platform account indexes and fee rates are silently clamped to account 0
packages/rs-unified-sdk-jni/src/wallet_manager.rs (lines 1255-1260)
Verified at HEAD (lines 1258-1259: account_index.max(0) as u32, core_fee_per_byte.max(0) as u32). walletPlatformAddressTransfer (906-908), the credit-transfer resume path (1007), walletPlatformAddressWithdraw (1102, 1184, 1191), walletPlatformAddressPreflightWithdrawal (1258-1259), and the new walletPlatformAddressPreflightWithdrawalReason (2146-2147) all use the same clamp. A caller passing -1 — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can preflight or move credits from the wrong account with no error crossing the boundary. Arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw DashSDKException on negatives instead of clamping.
suggestion: reqwest TLS backend swap silently changes iOS trust behavior — needs iOS validation or target gating
packages/rs-sdk-ffi/Cargo.toml (lines 71-77)
Verified at HEAD — still default-features = false, features = ["json", "rustls-tls-webpki-roots"], no target-os gate. The unconditional switch fixes the Android build (no OpenSSL, no readable system store) but also flips the TLS backend for every iOS build of rs-sdk-ffi and rs-sdk-trusted-context-provider. iOS clients previously validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) OS trust-store updates (revocations, root removals) no longer propagate until the SDK rebuilds against a newer webpki-roots. Since rs-sdk-trusted-context-provider sits on the SDK trust path for proof-related traffic, blast radius is broad. Either narrow with a cfg(target_os)-driven feature split (rustls-tls-native-roots / native-tls on Apple targets) or land an explicit iOS integration smoke test against a real HTTPS endpoint through both crates, and note the behavior change in the CHANGELOG.
suggestion: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
packages/kotlin-sdk/build_android.sh (lines 80-87)
Verified at HEAD (line 84: ANDROID_NDK_HOME="$NDK_ROOT/$(ls "$NDK_ROOT" | sort -V | tail -1)"). BSD sort on stock macOS does not implement -V. The script is explicitly macOS-oriented (exFAT sparse-image handling elsewhere; macOS $HOME/Library/Android/sdk default at line 82). On a developer machine with unset ANDROID_NDK_HOME and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (9.x sorts after 28.x lexically). Use a portable numeric-key sort over dotted version components.
ANDROID_NDK_HOME="$NDK_ROOT/$(find "$NDK_ROOT" -mindepth 1 -maxdepth 1 -type d -exec basename {} \; | sort -t. -k1,1n -k2,2n -k3,3n | tail -1)"
suggestion: Android TEST_PLAN still marks unbridged address-funded identity flows as automatable
packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.md (lines 128-133)
Partially re-validated from the 579fb31 finding. The latest head did add real paths for add-key/disable-key and top-up-from-addresses, but ID-08 and ID-11 still remain marked ✅ while their referenced actions are not bridged through the Android JNI/Kotlin SDK. dash_sdk_identity_create_from_addresses and dash_sdk_identity_transfer_credits_to_addresses exist only in rs-sdk-ffi; there are no matching exports/call sites in packages/rs-unified-sdk-jni or packages/kotlin-sdk/sdk. A QA agent following this file will still try to automate flows that Android cannot execute. Downgrade those rows to deferred until the JNI symbols and screens land.
🤖 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.
- [BLOCKING] In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt`:620-631: DataContractRef.close() is non-atomic and can double-free the Rust handle
Verified at HEAD. `DataContractRef` stores the native pointer in a plain `private var handle: Long` (line 621) and `close()` does a non-atomic read → store → destroy: `val h = handle; handle = 0; if (h != 0L) QueriesNative.dataContractDestroy(h)`. Two concurrent closers — e.g. a `use {}` finishing on Dispatchers.IO while a Cleaner/finalizer backstop fires, or two coroutines racing on the ref — can each observe the same non-zero `h` before either stores 0, and both will call `QueriesNative.dataContractDestroy(h)`. The Rust destroy at `rs-sdk-ffi/src/data_contract/mod.rs` reconstructs the allocation via `Box::from_raw`, so the second destroy is a real double-free / use-after-free across JNI. Every other owning wrapper in this SDK (`Sdk.handle`, `ManagedPlatformWallet.HandleCleanup`, `PlatformWalletManager.bundleRef`) already uses `AtomicLong.getAndSet(0)` for exactly this ownership handoff — apply it here. The same non-atomic shape is in `ContactRequestRef` / `EstablishedContactRef` at `sdk/.../tokens/Dashpay.kt:226-250`; those free via registry remove rather than direct `Box::from_raw`, but a concurrent second `close()` is still a defect (JNI destroy on a possibly-recycled slot) and should be fixed together.
- [SUGGESTION] In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/services/IdentityKeyAdditionFlow.kt`:141-150: ECDSA_HASH160 add-key rows submit the 33-byte compressed pubkey instead of the required 20-byte HASH160
Verified: `AddIdentityKeyScreen.kt:164` allows selecting `KeyType.ECDSA_HASH160`, wires a real deriver (`mgr.deriveIdentityKeyPair`, line 332), and calls `IdentityKeyAdditionFlow.prepareKeys`. `prepareKeys` always stores and submits `derived.publicKey` as `IdentityPubkey.pubkeyBytes` regardless of `spec.keyType`. The Swift reference (`swift-sdk/.../Views/IdentityKeyAddition.swift:152-163`) explicitly computes HASH160 for `ecdsaHash160` and passes the 20-byte hash as `pubkeyBytes` (see comment: 'For ECDSA_HASH160 the on-chain payload is the 20-byte HASH160 of the compressed pubkey, not the pubkey itself'). Additionally, the Swift signer trampoline stores the metadata under the 20-byte HASH160 hex for HASH160 keys and the 33-byte pubkey hex otherwise; the Kotlin flow stores under 33-byte hex unconditionally (line 132/136). Selecting ECDSA_HASH160 in the current build either produces an invalid identity update or, if it is accepted, persists the private key under a hex key the signer will not look up. Compute the HASH160 (RIPEMD160(SHA256(pubkey))) for ECDSA_HASH160 rows, submit that as `pubkeyBytes`, and key the Keystore entry the same way the signer will look it up.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/transactions.rs`:425-452: documentPurchase / documentSetPrice silently clamp negative price and signingKeyId to zero at the JNI boundary
New in this delta. `document_price_op` (transactions.rs:397-475) forwards Kotlin's signed `price: jlong` and `signing_key_id: jint` with `price.max(0) as u64` and `signing_key_id.max(0) as u32` (lines 433-434 for `platform_wallet_document_purchase`, 446-447 for `_set_price`). Concrete consequences: (a) a Kotlin-side `-1` sentinel or arithmetic underflow silently sets the trade price to 0 credits — the user posts a for-sale document that anyone can buy for free — with no error crossing the boundary; (b) a negative `signingKeyId` silently signs the transition under key id 0 (typically MASTER), which either rejects downstream with a confusing error or, worse, succeeds under a key the caller did not intend. Blast radius is higher than the sibling account-index clamp because a silently-zero price is directly asset-affecting. Reject negatives at the JNI edge with `throw_sdk_exception` before casting.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/identity.rs`:240-396: Derived private-key scalars left un-zeroized on the JNI stack (three sibling exports, including new keypair variant)
Verified at HEAD. Three exports copy the 32-byte ECDSA scalar into an independent stack local before `byte_array_from_slice` builds the JVM `byte[]`: `deriveIdentityPrivateKey` (line 242), `deriveIdentityPrivateKeyWithResolver` (line 326), and the new `deriveIdentityKeyPairWithResolver` added in this delta (line 384). `out_row.private_key_bytes` is `[u8; 32]` (`Copy`), so `let scalar = out_row.private_key_bytes;` is a truly independent copy. The paired `platform_wallet_derive_identity_private_key_at_slot_free` / `dash_sdk_derive_identity_key_at_slot_free` zeroize the Rust-owned buffer but cannot reach the independent stack copy, so plaintext key material persists in the JNI stack frame until unrelated frames overwrite it. This contradicts the module's own zeroize discipline (`Zeroizing` buffers, volatile zeroize on free, `non_secure_erase` of xprivs). The Kotlin caller (`IdentityKeyAdditionFlow.prepareKeys`) scrubs the JVM byte[] after use (`derived.privateKey.fill(0)`), amplifying the asymmetry — only the Rust stack copy stays warm. Either pass `&out_row.private_key_bytes` directly to `byte_array_from_slice` (no stack copy) or wrap the local in `zeroize::Zeroizing::new(...)` so `Drop` scrubs it before the guard returns.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/transactions.rs`:602-662: castContestedResourceVote copies the 32-byte voting private key into an un-zeroized JNI stack local
New in this delta. `read_id32(env, &voting_private_key, "votingPrivateKey")` at transactions.rs:605 produces `voting_key: [u8; 32]` — a bare `Copy` stack local that receives the masternode voting private key. It is passed to `dash_sdk_contested_resource_cast_vote` via `voting_key.as_ptr()` at line 654 and implicitly dropped when the closure returns at 662, with no zeroize step. Same class of leak as the identity-key derive path, but on a caller-owned Kotlin ByteArray — scrubbing on the Rust side is the only line of defense between the JVM copy and the FFI call. `read_id32`'s intermediate `bytes: Vec<u8>` also drops without zeroize. Wrap `voting_key` in `zeroize::Zeroizing::new(...)` (or explicitly zeroize before return) and either add a zeroizing sibling of `read_id32` on the key path or scrub the intermediate `bytes` there.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:466-484: Negative Core send amounts silently bit-cast to huge u64 values at the JNI boundary
Verified at HEAD. `walletCoreSendToAddresses` reads Kotlin `long[]` duffs into `amount_buf: Vec<i64>` (line 472) and then does `let amounts_u64: Vec<u64> = amount_buf.iter().map(|&v| v as u64).collect();` (line 484) with no sign check before forwarding to `core_wallet_send_to_addresses`. A caller-side `-1` sentinel or arithmetic underflow becomes `u64::MAX ≈ 1.8e19` duffs; even when Rust rejects it downstream, the failure mode is an obscure fee/overflow error rather than the intended boundary-level `DashSDKException`. Apply the same guard to `core_fee_per_byte`.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/tokens.rs`:704-716: Negative token amounts and expected total costs reinterpreted as huge u64 values at the JNI boundary
Verified at HEAD. `Java_..._TokensNative_tokenPurchase` casts `amount as u64` and `expected_total_cost as u64` at lines 710-711 with no sign check before handing them to `platform_wallet_token_purchase`. A Kotlin caller-side `-1` sentinel or arithmetic underflow becomes `u64::MAX` — a valid u64 the platform-wallet layer interprets as a colossal purchase amount or expected cost. The same signed-to-unsigned bit-cast pattern is present across the `mint` / `burn` / `transfer` / `set_price` entry points in this file. Validate at the JNI edge and throw `DashSDKException` for negatives.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1266-1352: PreflightWithdrawal / MinAmounts leak the transient platform-address handle on JVM array-alloc failure
Verified at HEAD. After `platform_wallet_get_platform` succeeds, `addr_handle` must be paired with `platform_address_wallet_destroy` on every exit path. In `walletPlatformAddressPreflightWithdrawal`, `let Ok(arr) = env.new_long_array(3) else { return ptr::null_mut(); };` (lines 1272-1274) and `if env.set_long_array_region(&arr, 0, &triple).is_err() { return ptr::null_mut(); }` (1275-1277) return directly from the guard closure, bypassing the destroy at 1281-1287. `walletPlatformAddressMinAmounts` has the identical shape at 1336-1341 before its destroy at 1346-1352. Each such failure strands a live transient platform-address handle in `PlatformWalletManager`'s Arc registry — exactly the memory-pressure path where leaks compound. The new `walletPlatformAddressPreflightWithdrawalReason` in this delta already funnels every branch through a single `out` binding before the unconditional destroy — mirror that structure here.
- [SUGGESTION] In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:1255-1260: Negative platform account indexes and fee rates are silently clamped to account 0
Verified at HEAD (lines 1258-1259: `account_index.max(0) as u32, core_fee_per_byte.max(0) as u32`). `walletPlatformAddressTransfer` (906-908), the credit-transfer resume path (1007), `walletPlatformAddressWithdraw` (1102, 1184, 1191), `walletPlatformAddressPreflightWithdrawal` (1258-1259), and the new `walletPlatformAddressPreflightWithdrawalReason` (2146-2147) all use the same clamp. A caller passing `-1` — a common Kotlin 'unset' sentinel — silently operates on account 0, the primary account. For a wallet with multiple platform accounts this can preflight or move credits from the wrong account with no error crossing the boundary. Arguably more dangerous than the amount clamps because the operation still 'succeeds' — just against the wrong target. Throw `DashSDKException` on negatives instead of clamping.
- [SUGGESTION] In `packages/rs-sdk-ffi/Cargo.toml`:71-77: reqwest TLS backend swap silently changes iOS trust behavior — needs iOS validation or target gating
Verified at HEAD — still `default-features = false, features = ["json", "rustls-tls-webpki-roots"]`, no target-os gate. The unconditional switch fixes the Android build (no OpenSSL, no readable system store) but also flips the TLS backend for every iOS build of `rs-sdk-ffi` and `rs-sdk-trusted-context-provider`. iOS clients previously validated HTTPS against the system trust store via Security.framework (respecting MDM-installed CAs, enterprise pinning, per-device revocation); now they trust only the bundled Mozilla webpki roots frozen at build time. Concrete risks: (a) iOS users on enterprise networks with custom root CAs will see TLS failures the previous build tolerated; (b) OS trust-store updates (revocations, root removals) no longer propagate until the SDK rebuilds against a newer `webpki-roots`. Since `rs-sdk-trusted-context-provider` sits on the SDK trust path for proof-related traffic, blast radius is broad. Either narrow with a `cfg(target_os)`-driven feature split (`rustls-tls-native-roots` / native-tls on Apple targets) or land an explicit iOS integration smoke test against a real HTTPS endpoint through both crates, and note the behavior change in the CHANGELOG.
- [SUGGESTION] In `packages/kotlin-sdk/build_android.sh`:80-87: NDK autodetection uses GNU-only `sort -V` on an otherwise macOS-oriented script
Verified at HEAD (line 84: `ANDROID_NDK_HOME="$NDK_ROOT/$(ls "$NDK_ROOT" | sort -V | tail -1)"`). BSD `sort` on stock macOS does not implement `-V`. The script is explicitly macOS-oriented (exFAT sparse-image handling elsewhere; macOS `$HOME/Library/Android/sdk` default at line 82). On a developer machine with unset `ANDROID_NDK_HOME` and multiple NDK versions installed, autodetection either errors out or picks a lexicographically-max version instead of the semver-max (`9.x` sorts after `28.x` lexically). Use a portable numeric-key sort over dotted version components.
- [SUGGESTION] In `packages/kotlin-sdk/KotlinExampleApp/TEST_PLAN.md`:128-133: Android TEST_PLAN still marks unbridged address-funded identity flows as automatable
Partially re-validated from the 579fb313 finding. The latest head did add real paths for add-key/disable-key and top-up-from-addresses, but ID-08 and ID-11 still remain marked `✅` while their referenced actions are not bridged through the Android JNI/Kotlin SDK. `dash_sdk_identity_create_from_addresses` and `dash_sdk_identity_transfer_credits_to_addresses` exist only in `rs-sdk-ffi`; there are no matching exports/call sites in `packages/rs-unified-sdk-jni` or `packages/kotlin-sdk/sdk`. A QA agent following this file will still try to automate flows that Android cannot execute. Downgrade those rows to deferred until the JNI symbols and screens land.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's 20,000-line diff limit, so I posted the verified findings as a top-level review body.
…handle refs, HASH160 add-key, TLS target gating Review fixes for PR #3999 (QuantumExplorer, thepastaclaw, HashEngineering): - rs-unified-sdk-jni: reject negative amounts/fees/account indexes/key ids at the JNI boundary instead of clamping or bit-casting (core send, asset lock funding, platform-address transfer/withdraw/preflight, token mint/burn/transfer/purchase/set-price, document purchase/set-price); zeroize private-key stack copies in the three slot-derive exports and the contested-vote path; funnel preflight/min-amounts exports through a single out binding so the transient platform-address handle is always destroyed. - kotlin-sdk: atomic getAndSet(0) close() on DataContractRef, ContactRequestRef, EstablishedContactRef (double-free race); ECDSA_HASH160 add-key rows now submit the 20-byte HASH160 payload and key the Keystore entry the way KeystoreSigner looks it up (pure-Kotlin RIPEMD-160 with published test vectors); TEST_PLAN ID-08/ID-11 downgraded to deferred; portable NDK version sort in build_android.sh. - rs-dapi-client/rs-sdk-trusted-context-provider: treat Android like iOS at every target_os gate (native-roots exclusion, DNS pre-check skip, platform user agent) — fixes the channel-create panic on Android. - rs-sdk-ffi/rs-sdk-trusted-context-provider: reqwest TLS backend is now target-gated — Android keeps rustls + webpki roots; all other targets (incl. iOS) restore the default native-tls system trust store. Already fixed at HEAD via #4002 (no change needed): build_android.sh features argv array, JNI local-frame coverage on daemon threads, CI path filters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review round addressed in 50d40ce: Fixed in this push — negative-value rejection at every flagged JNI boundary (core send, platform-address transfer/withdraw/preflight account+fee, token mint/burn/transfer/purchase/set-price, document price/signingKeyId); zeroized private-key stack copies (3 slot-derive exports + contested-vote key, via Already fixed at the reviewed-after HEAD via #4002 (no change needed): build_android.sh Verified: 🤖 Generated with Claude Code |
…fecycle hardening commit Three independent review agents (correctness, security, test-coverage) scrutinized the previous commit's implementation against the spec and the full compiled codebase. No blocking issues; folds in their nits: - Fix a stale KDoc reference to the removed PrivateKeyDeriver.hasStored. - Document storeIfAbsent's scrub-contract obligation on the caller (the derived scalar isn't zeroized by storeIfAbsent itself). - Pin the assumption behind CoreChildren's rollback catch not needing a persistenceHandler-close branch (it's built last; if its constructor ever grows a fallible step after allocating its owned Executor, this would need updating). - Add KeystoreManagerTest: isKeysBlobDecryptable is a pure structural check (no real Keystore call) and was untested at any tier despite being the exact boundary storeIfAbsent's re-derive path relies on. - Add storeIfAbsentDiscardsTheLosingDerivationWhenAnotherWriterWinsTheRace to WalletStorageOwnershipTest, covering the race-losing branch. Deliberately NOT added: a test for storeIfAbsent's "present but undecryptable legacy blob -> re-derive" branch end-to-end — WalletStorage's public API has no seam to plant a raw legacy-shaped blob without adding a test-only hook into fund/key-custody code, which needs a sync, not a default. Noted in a comment; the boundary it depends on (isKeysBlobDecryptable) is covered directly by the new KeystoreManagerTest. Verified: full CI-equivalent build green — `:sdk:assembleDebug :sdk:testDebugUnitTest :app:assembleDebug :app:testDebugUnitTest :sdk:compileDebugAndroidTestKotlin` — 306 JVM unit tests pass, 0 failures.
…rt.rs cargo fmt --check --all failed CI's macOS job: rustfmt wants the #[cfg(test)] Txid import ordered before the ungated Network/Transaction import, not after. Ran cargo fmt -p platform-wallet to apply its own canonical ordering rather than guess at it.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The recovery-phrase retention fix is effective, and the ordinary post-deletion key-store race is now fenced by wallet tombstones. Two blocking lifecycle gaps remain: failed recreation can clear the tombstone without restoring it, and changeset rollback can delete a shared alias claimed only through another wallet's durable owner index.
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 (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:632-648: Failed wallet re-import disables the deletion tombstone
createWallet clears the deterministic wallet ID's deletion tombstone before the fallible mnemonic and Room-label writes, but none of the rollback exits at lines 648-718 restores it. An app-side identity-key operation can preview or derive private-key material before the original wallet is deleted, remain suspended before WalletStorage.storePrivateKey, and then resume after a re-import of the same wallet ID fails and removes its replacement. Because the failed re-import left the tombstone cleared, that stale operation can persist ciphertext and recreate the durable owner index for a wallet that is again absent. Keep the tombstone armed until every fallible creation step succeeds, or restore it on each creation rollback path before the failed replacement is released.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:225-235: Rollback still deletes aliases claimed by another wallet
A failed changeset deletes each newly created alias whenever no committed public_keys row references it, but this check ignores durable owner-index claims. After wallet A creates an alias and before A rolls back, wallet B can call WalletStorage.storeIfAbsent, adopt the existing usable ciphertext, and record the same alias in B's owner index while B's public-key row is still uncommitted. scrubAliases then selects the alias for deletion, and IdentityKeyPrivateKeyDeriver.deleteStored invokes WalletStorage.deletePrivateKeys, which removes both the ciphertext and the alias from every wallet's owner index. Wallet B consequently loses a private key it has already adopted. Perform the committed-row and other-owner checks under WalletStorage's private-key exclusion, then remove only A's ownership and delete the ciphertext only when no other owner remains.
…Unit @test method CI's connectedDebugAndroidTest failed twice with: InvalidTestClassError: Method storeIfAbsentRejectsATombstonedWallet() should be void The method used expression-body syntax (`= runBlocking { ... }`) ending in assertThrows(...), which returns the caught exception rather than Unit — so the function's inferred return type was non-Unit. Kotlin's compiler allows this (no Unit requirement on functions generally), but JUnit4's runtime reflection-based validator (ParentRunner.validate()) rejects any @test method that isn't void, so this surfaced only on-device, not at compile time — exactly the "androidTest is compile-verified locally, not execution-verified" gap already flagged in the prior commit's message. Converted to a block body, which always returns Unit regardless of its last expression. Also caught a real mistake from the first attempt at this fix: the block-body rewrite dropped the tombstoneWallet(walletB) setup call entirely, which would have made the test pass for the wrong reason (storeIfAbsent would reject for lack of any private-key mutex contention... no — actually WITHOUT tombstoning, storeIfAbsent would have just succeeded and assertThrows would have failed loudly instead of silently — but it's still wrong test setup). Restored it. Verified: ./gradlew :sdk:compileDebugAndroidTestKotlin succeeds (JUnit's runtime validation obviously still needs the real CI emulator run to confirm — this class of bug is exactly why compile success alone isn't proof).
…close alias-rollback cross-wallet gap Two new blocking findings from automated review of the wallet-lifecycle hardening commit, both real gaps in that commit's own new code: - PlatformWalletManager.createWallet clears a wallet's deletion tombstone eagerly (needed so a delete-then-reimport of the same deterministic walletId isn't permanently rejected), but no rollback exit re-armed it if creation then failed. A stale in-flight identity-key store from a PRIOR delete of the same walletId, suspended before its own storePrivateKey call, could resume after the failed re-create and resurrect the walletId's owner-index entry with fresh ciphertext for a wallet that (once again) doesn't exist. Fixed: re-arm the tombstone as the first action in the catch block, best-effort, before any rollback path returns. - PlatformWalletPersistenceHandler.scrubAliases (the changeset-rollback alias-deletion path, distinct from removeWallet's) checked only committed public_keys rows before deleting a round-created alias, missing the same cross-wallet durable-owner-index gap the original :778 finding fixed in removeWallet. A sibling wallet that adopted the same shared alias via WalletStorage.storeIfAbsent (no committed row needed) could have its ciphertext deleted out from under it when the FIRST wallet's round rolled back. Fixed: PrivateKeyDeriver gains isOwnedByAnotherWallet (delegating to WalletStorage's existing isOwnedByAnotherWallet under its private-key exclusion, same lock order removeWallet already uses), and scrubAliases checks it alongside the committed-row check. Verified: full local build+test green (306 JVM unit tests, 0 failures); :sdk:compileDebugAndroidTestKotlin succeeds.
…y generation works The Kotlin SDK emulator job failed 4 of 6 new WalletStorageOwnershipTest cases with: InvalidAlgorithmParameterException: ... Secure lock screen must be enabled to create keys requiring user authentication WalletStorageOwnershipTest is the first androidTest in this module to exercise WalletStorage.storePrivateKey, which generates the identity-key KEYS_ALIAS RSA keypair via KeystoreManager.ensureKeysKeyPair — a pre-existing production requirement (setUserAuthenticationRequired(true) on the key spec), not something this PR introduced. Android Keystore refuses to create that key without a secure lock screen enrolled on the device; every existing androidTest happened to avoid identity-key storage, so CI's emulator never needed one until now. Fix: `adb shell locksettings set-pin 1234` in the emulator-runner script, after boot and before Gradle runs. This only enrolls a lock-screen credential — it doesn't gate anything the test suite does at runtime (private-key ENCRYPT, which storePrivateKey/storeIfAbsent use, is never auth-gated per KeystoreManager's own contract; only DECRYPT is, and nothing here decrypts).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head b797c90, both carried-forward blockers remain: wallet recreation still permits stale key persistence, and rollback ownership validation is not atomic with global alias deletion. The latest delta also introduces a retained-row recovery regression by tombstoning wallets that intentionally survive failed rollback; this is combined with the existing recreation finding because both require outcome-aware creation fencing. Separately, the cumulative JNI layer cannot fulfill its panic-to-Java conversion guarantee when nested non-unwind C exports panic.
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 (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:632-660: Wallet creation uses the tombstone in both unsafe states
createWallet clears the wallet-ID tombstone before the fallible mnemonic and Room-label writes. A key operation started against the previously deleted wallet can derive its private bytes before deletion and resume WalletStorage.storePrivateKey between that clear and the catch block's re-arm. Re-arming does not remove ciphertext or owner-index entries already committed during this interval, and the subsequent rollback only removes native and Room state. The opposite failure occurs when deleteWalletData fails: line 660 re-arms the tombstone even though lines 673-720 intentionally retain the persisted wallet rows, whose documented contract says they can be loaded as a functional wallet. loadPersistedWallets restores those rows without clearing the process-local tombstone, so every later identity-key write fails until process restart or another re-import. Keep the prior instance fenced until creation commits, and reconcile the fence with the rollback outcome: deleted rows must remain fenced with any exposed key writes swept, while a validated retained wallet must not remain tombstoned.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:231-241: Rollback ownership validation is not atomic with deletion
scrubAliases checks the durable sibling-owner index and then deletes the selected aliases through two separate WalletStorage critical sections. IdentityKeyPrivateKeyDeriver.isOwnedByAnotherWallet releases privateKeyMutex before deleteStored reacquires it. During that interval, an app-side storePrivateKey/storeIfAbsent call or a writer using another manager can add a sibling wallet's durable claim to the alias; callbackExclusion does not cover those writers. deletePrivateKeys then removes the ciphertext and the alias from every owner index, destroying the sibling wallet's adopted signing key. Perform the other-owner check and conditional deletion as one WalletStorage operation under a single private-key exclusion.
…as-rollback ownership check atomic with delete Second round of automated review findings against the wallet-lifecycle hardening work, both real: - createWallet cleared its tombstone eagerly at the TOP of the try block, before the fallible mnemonic-store/name-update steps ran, and the catch block re-armed it unconditionally. Two problems: (1) a stale in-flight identity-key store from a PRIOR instance of this deterministic walletId had a real window — between the clear and the re-arm — to slip a write through that the re-arm can't retroactively undo; (2) when Room rollback itself fails and the ORIGINAL create's rows survive intact as a fully valid, loadable wallet (loadPersistedWallets restores whatever Room says exists, with no tombstone awareness), the unconditional re-arm left it permanently unable to store identity keys. Fixed: clear the tombstone LAST, only once every fallible step has succeeded (no window at all for the stale-writer race), and explicitly un-arm it again in the one rollback branch where the wallet is retained as a valid entity. - scrubAliases' cross-wallet ownership check (added in the previous commit) ran as a separate WalletStorage call from the delete that followed, each independently acquiring and releasing the private-key lock — leaving a window for a sibling wallet's storeIfAbsent to adopt one of the candidate aliases in between and have it deleted anyway. removeWallet's equivalent check doesn't have this gap (its check and delete already share one continuous lock hold); scrubAliases's indirection through PrivateKeyDeriver did not. Fixed: WalletStorage gains deleteUnownedPrivateKeys, doing the recheck and the delete under one lock hold (same two-phase-outside-the-lock-then-recheck shape as storeIfAbsent). PrivateKeyDeriver.deleteStored + isOwnedByAnotherWallet collapse into one deleteUnownedStored method; scrubAliases simplifies back to its original single-filter shape since the ownership check now lives inside the atomic delete instead of a separate up-front pass. Verified: full local build+test green (306 JVM unit tests, 0 failures); :sdk:compileDebugAndroidTestKotlin succeeds.
…k screen The previous lock-screen fix (adb shell locksettings set-pin) fixed KEYS_ALIAS RSA generation but broke every existing test: enrolling a lock-screen credential puts the device in a "locked" security state until first unlock, and MASTER_ALIAS (the mnemonic AES key every existing test relies on via WalletStorage.storeMnemonic) is generated with setUnlockedDeviceRequired(true) — confirmed on CI, all 5 previously green WalletManagerRoundTripTest/DashPayUnlockAndSyncTest cases started throwing InvalidKeyException. Fix: adb shell wm dismiss-keyguard right after set-pin, so the device counts as unlocked for setUnlockedDeviceRequired(true) keys while the PIN stays enrolled for setUserAuthenticationRequired(true) keys. Not verified locally — no local emulator. This is the second attempt at this CI change; if it also regresses existing tests, fall back to @ignore on WalletStorageOwnershipTest and fully revert the lock-screen change rather than continuing to iterate blindly on shared CI infra.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes both indexed races: tombstone clearing now follows fallible creation setup, and rollback ownership checking and deletion are atomic. Two in-scope blockers remain: shared-alias rollback retains a stale owner claim, and RSA ciphertext shape is treated as proof that the current Android Keystore key can decrypt it. This is preliminary Codex-only coverage.
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 (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:258-262: Rollback leaves a phantom owner for shared aliases
Suppose wallet A creates an alias during a changeset and wallet B adopts it through storeIfAbsent before A rolls back. deleteUnownedPrivateKeys correctly retains the ciphertext because B is another owner, but because deletePrivateKeysLocked is skipped, A is never removed from privkeyowners.A. PlatformWalletPersistenceHandler then treats cleanup as complete and drops the pending record. If A was a failed wallet-creation round, there may be no wallet left through which to remove that claim; when B is later deleted, removeWallet sees phantom owner A, retains the ciphertext, and removes only B's index. The atomic edit must remove these candidates from excludingWalletId's owner set even when another owner requires retaining the ciphertext, and delete the ciphertext and remaining claims only when no other owner exists.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:95-96: Ciphertext shape is mistaken for current-key decryptability
This predicate proves only that a blob resembles RSA-2048 ciphertext; it does not prove that the current KEYS_ALIAS keypair encrypted it. The SDK DataStore has no backup exclusion in its library manifest, so a consuming app using Android's default backup policy can restore privkey blobs while Android Keystore keys remain device-bound and are not restored. The example app disables backup, but SDK consumers do not inherit that guarantee. After KEYS_ALIAS is recreated, addOwnerIfUsableLocked accepts the old 256-byte blob, records ownership, and skips deterministic re-derivation; the key-health check also reports it as usable, while signing fails OAEP decryption under the replacement key. Persist and compare a wrapping-key fingerprint or generation identifier with each blob so ciphertext from a lost or replaced key is treated as missing and re-derived.
…re lock-screen failure NOT for merge. Opens an interactive SSH session on the CI runner, inside the emulator-runner's script (the only place the booted emulator is reachable), to test lock-screen commands live against the real x86_64 CI emulator — local Apple Silicon hardware cannot run x86_64 Android system images at all, so this specific failure (MASTER_ALIAS InvalidKeyException surviving set-pin + dismiss-keyguard) can't be reproduced locally. Will be reverted once the actual fix is found.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 0b1be7f, all three reported blockers are confirmed. Both carried-forward key-custody defects remain unchanged, and the latest delta adds a foreground tmate session that prevents the Android instrumented tests from running unattended. This is preliminary Codex-only coverage; Sonnet has not been admitted.
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 (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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 `.github/workflows/kotlin-sdk-build.yml`:
- [BLOCKING] .github/workflows/kotlin-sdk-build.yml:168-169: Interactive tmate session prevents emulator tests from completing
`tmate -F` starts the interactive session in the foreground and does not return until the session is terminated, so the `connectedDebugAndroidTest` command on line 170 is never reached in unattended CI. This required job instead remains open until its 120-minute timeout. The commit and inline comment both identify this as temporary, not-for-merge debugging code; remove the tmate installation and session before merging.
…ore running tests Removed the temporary tmate debug step. Root cause found via an interactive tmate session on the actual CI runner: adb shell wm dismiss-keyguard returns before LockSettingsService/Keystore fully propagate the "unlocked" state to the keystore daemon. Every manual re-run of the full 23-test suite in that session passed, including the previously-failing MASTER_ALIAS-touching tests — but every manual run had multi-second gaps between commands (SSH round-trip latency) that this script's back-to-back execution doesn't have, consistent with a settling race rather than a logic error in the fix itself. Fix: a short sleep between dismiss-keyguard and the first Keystore operation (the test run itself).
…crets from Android backup deleteUnownedPrivateKeys retained aliases owned by another wallet but left the excluded wallet's own owner-index entry pointing at them, stranding a phantom owner that would block cleanup when the real owner later deletes the alias. Strip the excluded wallet from the retained aliases' index too. Also add ready-made backup-exclusion XML (legacy full-backup-content + modern data-extraction-rules) for consuming apps: Keystore keys are device-bound and never restored, so a backed-up private-key blob can pass KeystoreManager's structural decryptability check yet be permanently undecryptable, skipping the automatic re-derive path.
…LIAS keypair isKeysBlobDecryptable only proved a blob was RSA-2048-shaped, not that it was encrypted under the KEYS_ALIAS keypair currently in the Keystore. A blob from a replaced keypair (Keystore data loss + regeneration, or a restored DataStore backup landing on a device with its own key) is the same size and passes the shape check, so addOwnerIfUsableLocked accepted it and skipped re-derivation while signing would fail OAEP decryption under the new key. Persist KeystoreManager.keysAliasFingerprint() (SHA-256 of the current public key) alongside each RSA blob and require it to match on read; a missing or mismatched fingerprint is now treated the same as an absent key, so storeIfAbsent re-derives it. Covered by a new instrumented test that deletes the live KEYS_ALIAS entry to force a fingerprint mismatch and confirms the stale blob is rejected and re-derived (ran green on-device).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 6efa83b, all three prior findings are fixed: the foreground tmate session is gone, retained aliases no longer leave phantom owners, and fingerprints now reject ciphertext from a replaced KEYS_ALIAS keypair. Two cumulative in-scope blockers remain: Android registration cannot provision the bounded encryption keys required by the advertised DashPay flows, and permanent Keystore invalidation is not detected. A separate signer capability mismatch remains a suggestion; this is preliminary Codex-only coverage.
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 (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 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/identity.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/identity.rs:742-765: Registration JNI cannot create DashPay-capable identities
Kotlin's registration payload encodes only each key ID and public key. The JNI registration paths reconstruct purpose and security level through `role_for_registration_key_id` and always set `contract_bounds_kind` to zero; IDs 0–3 produce only authentication and transfer keys, while every higher ID becomes another unbounded authentication key. The Kotlin create-identity flow passes this fixed key set through Core, Platform-address, shielded, and recovery registration, whereas the Swift reference flow appends bounded ENCRYPTION and DECRYPTION keys for DashPay's `contactRequest` document type. Consequently, an identity created by the Android app reaches its advertised Add Contact flow without an enabled ECDSA_SECP256K1 encryption key, and `select_own_encryption_key` rejects the request. Extend the registration row format and JNI decoders to carry explicit key type, purpose, security level, and contract bounds, then provision the DashPay pair during creation.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.kt:109-111: Fingerprint does not detect a permanently invalidated Keystore key
The fingerprint detects replacement of KEYS_ALIAS, but it proves only that the stored ciphertext references the same public certificate. Android can permanently invalidate an authentication-bound private key after the secure credential is removed or reset while leaving that certificate and key handle present. The fingerprint therefore remains unchanged, `WalletStorage.isCurrentKeysBlob` and the key-health screen report the ciphertext as usable, and `storeIfAbsent` skips re-derivation. Signing later fails when the private operation raises `KeyPermanentlyInvalidatedException`; `KeystoreSigner` retries only `UserNotAuthenticatedException`. Probe private-key usability in a way that treats authentication-required as valid but detects permanent invalidation, rotate KEYS_ALIAS when invalidated, and route affected keys through the existing re-derivation or wallet-recovery path.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.kt:198: Signer capability still accepts ciphertext from a replaced keypair
The Rust `VTableSigner::can_sign_with` callback asks whether the exact private key is usable, but the identity-key branch checks only whether a `privkey.*` entry exists. After KEYS_ALIAS replacement, the new fingerprint check correctly makes `WalletStorage.isPrivateKeyDecryptable` return false, while this callback still returns true. Rust operations that consult signer capability can consequently select the stale key and fail later during OAEP decryption instead of rejecting it during key selection.
| // Build the FFI rows referencing the owned buffers. `read_only` | ||
| // false, no contract bounds (auth / transfer keys never carry | ||
| // bounds — only ENCRYPTION / DECRYPTION do). The per-key role | ||
| // (key_type / purpose / security_level) is the canonical, | ||
| // positional function of `key_id` (see | ||
| // `role_for_registration_key_id`), matching iOS exactly — so a | ||
| // freshly created identity gets keyId 0 MASTER/AUTH, keyId 1 | ||
| // CRITICAL/AUTH, keyId 2 HIGH/AUTH, keyId 3 TRANSFER/CRITICAL. | ||
| let ffi_rows: Vec<IdentityPubkeyFFI> = decoded | ||
| .iter() | ||
| .map(|(key_id, bytes)| { | ||
| let (key_type, purpose, security_level) = role_for_registration_key_id(*key_id); | ||
| IdentityPubkeyFFI { | ||
| key_id: *key_id, | ||
| key_type, | ||
| purpose, | ||
| security_level, | ||
| pubkey_bytes: bytes.as_ptr(), | ||
| pubkey_len: bytes.len(), | ||
| read_only: false, | ||
| contract_bounds_kind: 0, | ||
| contract_bounds_id: ptr::null(), | ||
| contract_bounds_document_type: ptr::null(), | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Registration JNI cannot create DashPay-capable identities
Kotlin's registration payload encodes only each key ID and public key. The JNI registration paths reconstruct purpose and security level through role_for_registration_key_id and always set contract_bounds_kind to zero; IDs 0–3 produce only authentication and transfer keys, while every higher ID becomes another unbounded authentication key. The Kotlin create-identity flow passes this fixed key set through Core, Platform-address, shielded, and recovery registration, whereas the Swift reference flow appends bounded ENCRYPTION and DECRYPTION keys for DashPay's contactRequest document type. Consequently, an identity created by the Android app reaches its advertised Add Contact flow without an enabled ECDSA_SECP256K1 encryption key, and select_own_encryption_key rejects the request. Extend the registration row format and JNI decoders to carry explicit key type, purpose, security level, and contract bounds, then provision the DashPay pair during creation.
source: ['codex']
There was a problem hiding this comment.
Being addressed in a separate follow-up PR (branch feat/kotlin-sdk-dashpay-registration-keys, stacked on this PR's head) rather than folded in here — it's a JNI wire-format change across 4-5 call sites plus the Kotlin-side provisioning wiring, large enough to warrant its own review/rollback unit rather than growing this PR further. Spec written, reviewed twice (feasibility/security pass, then an independent second review that caught several real gaps — a base-key-role rebuild I'd initially missed, a Swift-parity mismatch on the resume/funding path, a silent duplicate-key-ID issue), implemented, and code-reviewed with fixes folded in (a real secret-lifecycle leak on cancellation, and an unenforced six-key funding floor, both closed with red→green tests). Will link the PR here once it's pushed.
| fun keysAliasFingerprint(): String = | ||
| MessageDigest.getInstance("SHA-256").digest(keysPublicKey().encoded) | ||
| .joinToString("") { "%02x".format(it) } |
There was a problem hiding this comment.
🔴 Blocking: Fingerprint does not detect a permanently invalidated Keystore key
The fingerprint detects replacement of KEYS_ALIAS, but it proves only that the stored ciphertext references the same public certificate. Android can permanently invalidate an authentication-bound private key after the secure credential is removed or reset while leaving that certificate and key handle present. The fingerprint therefore remains unchanged, WalletStorage.isCurrentKeysBlob and the key-health screen report the ciphertext as usable, and storeIfAbsent skips re-derivation. Signing later fails when the private operation raises KeyPermanentlyInvalidatedException; KeystoreSigner retries only UserNotAuthenticatedException. Probe private-key usability in a way that treats authentication-required as valid but detects permanent invalidation, rotate KEYS_ALIAS when invalidated, and route affected keys through the existing re-derivation or wallet-recovery path.
source: ['codex']
There was a problem hiding this comment.
Being addressed in a separate follow-up PR (branch fix/kotlin-sdk-pr3999-followups) alongside the sibling signer-capability finding below and some doc reconciliation — kept separate from the DashPay registration-key work since they touch unrelated subsystems (Keystore/signer vs. JNI registration wire format). Fix catches KeyPermanentlyInvalidatedException (distinct from the replacement case this session's fingerprint check already handles), rotates KEYS_ALIAS by reusing the existing regenerate-on-next-use path, and closes a real encrypt/fingerprint race an independent review found (ciphertext could be encrypted under the old key but labeled with the new key's fingerprint) by making the fingerprint capture atomic with the encrypt call. Implemented and code-reviewed. Will link the PR here once it's pushed.
| } | ||
| } | ||
| } else { | ||
| runBlocking { storage.hasPrivateKey(storageKeyFor(pubkeyBytes)) } |
There was a problem hiding this comment.
🟡 Suggestion: Signer capability still accepts ciphertext from a replaced keypair
The Rust VTableSigner::can_sign_with callback asks whether the exact private key is usable, but the identity-key branch checks only whether a privkey.* entry exists. After KEYS_ALIAS replacement, the new fingerprint check correctly makes WalletStorage.isPrivateKeyDecryptable return false, while this callback still returns true. Rust operations that consult signer capability can consequently select the stale key and fail later during OAEP decryption instead of rejecting it during key selection.
| runBlocking { storage.hasPrivateKey(storageKeyFor(pubkeyBytes)) } | |
| runBlocking { storage.isPrivateKeyDecryptable(storageKeyFor(pubkeyBytes)) } |
source: ['codex']
There was a problem hiding this comment.
Being addressed in the same follow-up PR as the fingerprint-invalidation finding above (branch fix/kotlin-sdk-pr3999-followups) — applied your suggested fix as-is (storage.isPrivateKeyDecryptable(...) in place of the existence-only check). Implemented and code-reviewed. Will link the PR here once it's pushed.
6 participants
Issue being fixed or feature implemented
The Kotlin/Android SDK has been planned but never started (
docs/SDK_ARCHITECTURE.md,book/src/sdk-support.mdlist it as "coming"). This PR delivers it, together with KotlinExampleApp — a one-for-one Android port of SwiftExampleApp — so Android has the same reference integration iOS has.What was done?
Three new components (~50k insertions, mirroring the
packages/swift-sdklayering):packages/rs-unified-sdk-jni— Rust JNI cdylib (110 exports, arm64-v8a + x86_64, 16KB-aligned for Android 15). Calls theextern "C"entry points ofrs-sdk-ffi/platform-wallet-ffi/key-wallet-ffias rlib dependencies (no C glue, no cbindgen headers on Android). Panics are caught at every export; Rust→Kotlin callbacks (32-slot persistence vtable, async signer, mnemonic resolver, sync events) attach Tokio threads as JVM daemons and copy payloads before return.packages/kotlin-sdk/sdk— the Kotlin SDK (org.dashfoundation.dashsdk): 28 Room entities transcribed 1:1 from the SwiftData models, Keystore-wrapped secret storage (org.dashfoundation.wallet.*aliases), network-lockedPlatformWalletManager/WalletManagerStore, sync services, per the persist/load/bridge doctrine (kotlin-sdk/CLAUDE.md, ported fromswift-sdk/CLAUDE.md).packages/kotlin-sdk/KotlinExampleApp— single-activity Compose app: 5 tabs, wallet create/seed-backup/send/receive with QR, identity registration coordinators, DPNS, contracts/documents/storage explorer, the TokenActionScaffold with 12 token actions, transitions catalog, asset-lock + shielded funding, DashPay, diagnostics.packages/kotlin-sdk/PARITY.mdtracks all 90 Swift views: 75 ported / 8 partial / 7 deferred — every partial/deferred row names the exact missing FFI export.Cross-cutting changes reviewers should look at:
rs-sdk-ffi+rs-sdk-trusted-context-provider:reqwestswitched from default features (native-tls) torustls-tls-webpki-roots— OpenSSL doesn't exist on Android. This also changes the TLS backend used by iOS builds (previously Security.framework via native-tls); trust roots now come from the bundled Mozilla set, matching whatdapi-grpcalready uses (tls-webpki-roots).rs-platform-wallet-ffi: newplatform_wallet_derive_identity_private_key_at_slot(+_free) entry point returning ready-to-persist identity key bytes (zeroizing). Note: the identity-key persist callback fires while platform-wallet holds the wallet-manager write lock, so the callback path uses the lock-free resolver-keyed derive instead — documented in-code.rs-sdk-ffi:dash_sdk_document_sum/dash_sdk_document_averagere-exported fromdocument/mod.rs(previously unreachable as Rust items).kotlin-sdk-build.yml(PR build + API-35 emulator smoke) andkotlin-sdk-release.yml(tag-triggered AAR release, both ABIs, release profile).How Has This Been Tested?
./gradlew :sdk:testDebugUnitTest :app:testDebugUnitTest— ~100 JVM/Robolectric tests: Room round-trips/FK cascades, persistence-handler changeset bracketing, coordinator state machines (registration, asset-lock, shielded), sync-state reduction, base58/bech32m codecs, group-action rules../gradlew :sdk:connectedDebugAndroidTest :app:connectedDebugAndroidTeston an API-35 arm64 emulator — 10 instrumented tests green, includingWalletManagerRoundTripTest(create wallet from mnemonic → persistence vtable → Room → reload) andAppSmokeTest(real bootstrap + tab navigation).cargo check -p rs-unified-sdk-jni(host +aarch64-linux-android) zero warnings;cargo test -p platform-wallet-ffi identity_private_key_at_slotgreen.build_android.sh --verify: dev + release profiles, both ABIs, JNI symbol counts and 16KB LOAD alignment checked (release .so: 57MB vs 176MB dev).@TestnetTest,-Ptestnet=truegate) compile and are wired for a nightly.Breaking Changes
None for public APIs. Behavioral note: iOS/mobile HTTPS in
rs-sdk-ffi/rs-sdk-trusted-context-providernow uses rustls + webpki roots instead of the platform TLS stack (see above) — no API change, but worth a look from the iOS side.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code