feat(sdk): add Kotlin SDK and KotlinExampleApp (Android port of SwiftExampleApp)#3999
feat(sdk): add Kotlin SDK and KotlinExampleApp (Android port of SwiftExampleApp)#3999bezibalazs wants to merge 181 commits into
Conversation
bezibalazs
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Important Review skippedToo many files! This PR contains 397 files, which is 247 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (397)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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 333986 +218
==========================================
+ Hits 291810 292028 +218
Misses 41958 41958
🚀 New features to boost your workflow:
|
|
⛔ Blockers found — Sonnet deferred (commit 6dbc72a) |
…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>
bezibalazs
force-pushed
the
feat/kotlin-sdk-and-example-app
branch
from
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 |
…#4118) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 5df8b2b, the latest delta fixes eight of the ten prior findings and introduces no new finding of its own. Two findings carry forward: one blocking document-capability calculation defect and one SDK teardown behavioral-test suggestion. Additional cumulative verification confirms two in-scope JNI blockers involving destructive nullable-string coercion and panic containment across nested C ABI calls; the reported ban-info JSON defect is refuted by the sole production producer path, and the focused JNI tests pass (2/2).
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 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/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/DocumentActionsScreen.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/DocumentActionsScreen.kt:136-138: [Carried forward — STILL VALID] Document capability gates still miscompute mutability and deletability
These predicates do not reproduce DPP's effective document-type flags. DPP falls back from an absent per-type `documentsMutable` or `canBeDeleted` field to the contract's `config.documentsMutableContractDefault` or `config.documentsCanBeDeletedContractDefault`, but this code instead defaults both to true. The delete expression is worse: `documentsCanBeDeleted` is not an allowed document-schema key, so for a canonical schema with `canBeDeleted: false` the missing alias evaluates as `null != false`, making the OR expression true. The Replace and Delete buttons therefore remain enabled for transitions consensus will reject as paid-invalid. Read the serialized contract's nested `config` object and use null-coalescing precedence on the canonical schema keys.
In `packages/rs-unified-sdk-jni/src/dashpay.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/dashpay.rs:720-753: Malformed non-null contact strings silently clear metadata
`opt_jstring_to_cstring` maps both a genuine Kotlin null and every conversion failure to `None`. A non-null Kotlin string containing U+0000 therefore fails `CString::new`, becomes a null pointer at lines 729-730, and reaches `platform_wallet_set_dashpay_contact_info_with_signer`, whose contract defines null alias/note as clearing that field. Invalid non-null input can silently delete existing contact metadata instead of failing the write. The profile bridge at lines 656-663 has the same conflation for three strings and uses `.ok()` for non-null `avatarBytes`, allowing a mutation to continue with requested fields omitted and potentially with a pending JNI exception. Return a fallible result for non-null conversions, throw `DashSDKException`, and abort before calling the C export.
In `packages/rs-unified-sdk-jni/src/support.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/support.rs:90-107: The outer JNI guard cannot catch panics inside nested C exports
`guard` catches only panics that can unwind back to the JNI closure. These guarded bodies call Rust functions declared `extern "C"`; for example, the DashPay profile JNI path calls `platform_wallet_create_or_update_dashpay_profile_with_signer`, which reaches `block_on_worker`, whose `expect("tokio worker panicked")` panics when its spawned future panics. A panic cannot unwind through the non-unwind C ABI, so Rust aborts at that boundary before this outer `catch_unwind` can observe it. Android's profiles deliberately retain `panic=unwind` and the PR promises conversion of panics to Java exceptions, but nested platform-wallet and rs-sdk C exports defeat that guarantee and can terminate the Android process. Catch inside each C export or let JNI call non-C internal implementations under this guard.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/Sdk.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/Sdk.kt:105-132: [Carried forward — STILL VALID] No behavioral test exercises Sdk.queryGate teardown wiring
The SDK teardown implementation is now fenced correctly, but no test constructs an `Sdk`, holds an SDK lease, and invokes either close path. `GateCoverageLintTest` only scans selected parameter names and checks for a gate-shaped source opener; it cannot prove that `closeSuspending()` waits for an active lease, that operations starting after close fail before reading the native handle, that cancellation cannot strand cleanup, or that `AutoCloseable.close()` follows the same fence. Add a behavioral regression test for the lifecycle semantics changed in this delta.
…clearing metadata opt_jstring_to_cstring mapped both a genuine Kotlin null and a conversion failure (an interior NUL that fails CString::new) to None. Several DashPay exports treat a null field as 'clear it', so a non-null string containing U+0000 became a null pointer and silently wiped an existing alias/note via platform_wallet_set_dashpay_contact_info_with_signer, or was silently dropped from the profile read-modify-write merge. The avatarBytes path likewise swallowed a read error with .ok() and continued the mutation. Make the helper fallible: a genuine null stays Ok(None) (the caller's 'field absent / clear' contract); a read/interior-NUL failure throws DashSDKException and returns Err(()) so each of the five call sites aborts before the C boundary. Replace the avatarBytes .ok() swallow with an explicit throw. Addresses a review finding on #3999. A pure-Rust unit test can't reach this (the helper needs a live JNIEnv); the regression belongs in a Kotlin instrumentation test (interior-NUL alias must throw, not clear), tracked as a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lities The Document Actions capability gate defaulted an absent documentsMutable/ canBeDeleted to true and OR-ed in a non-existent documentsCanBeDeleted schema key, so the Replace/Delete buttons stayed enabled for a document type with canBeDeleted: false — a transition Drive rejects as paid-invalid (fees + a burned nonce). DPP instead reads the per-type schema flag and falls back to the contract config's documentsMutableContractDefault / documentsCanBeDeletedContractDefault, then to true. Add a shared documentTypeCapabilities(schema, config) helper reproducing that resolution and use it in DocumentActionsScreen (the gate) and DocumentTypeDetailsScreen (which had the identical display bug). Unit test pins the canBeDeleted:false case, the config-default fallback, and that the phantom documentsCanBeDeleted key is ignored. Addresses a review finding on #3999. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nd-example-app Conflict: Cargo.lock (base bumped grovedb 5.0.1 + rust-dashcore for the masternode-list work) — took base's resolution and re-resolved for the branch-only workspace members. Merge adaptation: WalletRestoreEntryFFI gained provider_special_txs(+count) (#4112/#4116); the JNI load path initializes them null/0 — the documented "wallet has no provider special txs" value — since Android has no provider-special-tx persistence yet (masternode-list parity tracked as a follow-up). Also lands the Sdk teardown behavioral test the review asked for: SdkLifecycleTest (4 tests over a native-free Sdk.forLifecycleTest seam — handle 0 skips the Cleaner's native destroy) pins close-awaits-lease, post-close fail-fast, cancellation-cannot-strand-cleanup, and the AutoCloseable.close() fence. The nested-C-export panic-containment finding is intentionally NOT addressed here: shumkov confirmed the mechanism and scoped the fix into follow-up #4121 (cfg-gated panic-catch inert under the iOS panic=abort profiles + PlatformWalletError surfacing); an ABI-wide change from this PR would cut across that. Verified: cargo fmt, platform-wallet-ffi lib tests (155), JNI check (shielded), workspace clippy --all-features -D warnings, gradle :sdk+:app tests (SdkLifecycleTest 4/4). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All 10 compact-indexed prior findings are fixed at the current head, so no indexed findings carry forward. Two blocking FFI defects remain elsewhere in the cumulative PR, while the latest delta introduces two blocking UI logic defects and one lifecycle-test coverage gap. The four blockers require changes before approval.
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)
🔴 4 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/funding.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/funding.rs:766-768: JNI discards the identity ID for an unconfirmed shielded create
The C ABI writes `out_identity_id` on both success and `ErrorShieldedBroadcastUnconfirmed`, because an unconfirmed broadcast may already have created the identity and the host must retain the ID and hold its derivation slot. This JNI wrapper handles every non-success through `take_pwffi_error` and returns before copying `out_id`. Kotlin can consequently only throw a generic code-17 error, and `CreateIdentityScreen` supplies no `isUnconfirmed` classifier to `RegistrationCoordinator`, so the controller records a retryable `Failed` phase instead of the slot-holding `Unconfirmed` phase. A retry may reuse keys for an identity that is already live and route the spent value through the creation-failure penalty path. Return a result or typed exception that preserves both the unconfirmed status and the 32-byte identity ID.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.kt:94-188: Ungated DashPay persistence can use a freed JNI callback context
`ignoreContactSender`, `unignoreContactSender`, `syncProfiles`, and `syncContactRequests` run under plain `withContext` instead of the manager's `gate.op`. All four can call the shared `FFIPersister`: ignore/unignore store directly, profile sync persists changed profiles, and contact-request sync persists ingest and cursor changes. These methods run on caller-owned coroutine scopes, so manager-scope shutdown does not drain them; `closeInternal` only waits for gated calls before `nativeDestroy` frees `KotlinPersistenceCtx`. The independently retained `PlatformWallet` and `WalletPersister` can then finish a store and dereference the freed callback context in `with_bridge`, causing a reachable use-after-free during manager replacement. Run all four persistence-capable methods under `gate.op`.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/DocumentActionsScreen.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/DocumentActionsScreen.kt:573-576: Replace fallback makes optional properties impossible to remove
`mergeReplaceProperties` restores every existing key omitted by the form output. Because `buildPropertiesJson` omits blank text, numeric, array, and object inputs, clearing an optional property now silently preserves its old value; a seeded optional boolean likewise has no absent state. There is no separate remove control, so the user can submit and pay for a replacement that reports success without performing the requested deletion. Use tri-state field state or a full replacement JSON editor so preserve, replace, and remove are distinct operations.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/UpdateContractScreen.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/UpdateContractScreen.kt:111-115: Separator-only keywords enable a paid no-op update
`hasKeywords` checks whether the raw string is nonblank, while `keywordsAsJsonArray` removes empty comma-separated items and returns null when none remain. Entering `,` or `, ,` therefore enables submission but sends null keywords with the default empty overlays. The Rust update path still fetches the contract, increments its version, consumes the nonce, signs, and broadcasts, charging the user for a no-op. Derive the enablement predicate from the same normalized conversion used for submission.
In `packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/SdkLifecycleTest.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/SdkLifecycleTest.kt:75-102: Cancellation test cannot observe whether SDK cleanup ran
`Sdk.forLifecycleTest()` starts with handle 0, so the SDK already reports closed and `HandleCleanup.run()` has no observable effect. The test only verifies that `queryGate` rejects another operation, but `closeAndAwait()` permanently closes that gate even in the previously broken cancellable implementation. It would therefore still pass if cancellation skipped `cleanable.clean()`, which is the regression the test claims to cover. Add an injectable cleanup callback or another native-free seam and assert that cleanup executes exactly once after cancellation.
…rsisters, replace tri-state Review round at 7be281d: - Unconfirmed shielded identity create no longer loses the id: the JNI returns a tagged 33-byte payload ([0|1] || identity_id) instead of routing ErrorShieldedBroadcastUnconfirmed (17) through take_pwffi_error; the wrapper throws the new typed DashSdkError.PlatformWallet.ShieldedCreateUnconfirmed(identityId), and CreateIdentityScreen wires the coordinator's isUnconfirmed classifier so the controller records the slot-holding Unconfirmed phase instead of retryable Failed (a retry could reuse keys for a live identity). - ignoreContactSender / unignoreContactSender / syncProfiles / syncContactRequests run under gate.op: all four reach the shared FFIPersister from caller-owned scopes, so manager teardown must await them before nativeDestroy frees the KotlinPersistenceCtx (the handle-param lint cannot see persistence-context borrows). - Replace is now tri-state: seeded-then-cleared fields are explicit removals dropped from the merged payload; never-representable (composite) and never-populated fields still carry over; filled fields overwrite. Fixes "clearing an optional property silently preserves it". - UpdateContract's hasKeywords derives from keywordsAsJsonArray (the submit's own normalization), so ","/", ," can no longer enable a paid no-op broadcast. - SdkLifecycleTest's cancellation case now observes cleanup: an injectable onCleanup hook through HandleCleanup asserts cleanup ran exactly once after a cancelled closeSuspending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All ten prior findings are fixed at the current head. The latest delta introduces one blocking issue: the replacement form can remove required scalar properties and broadcast a paid-invalid transition; it also introduces a non-blocking JNI diagnostic-loss issue. JVM tests could not run because no Java runtime is installed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 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/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/DocumentActionsScreen.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/contracts/DocumentActionsScreen.kt:370-374: Replace tri-state permits removal of required fields
`required` is read directly from the document schema, but `clearedKeys` includes every seeded scalar changed to blank without excluding required properties. `buildPropertiesJson` omits the blank value and `mergeReplaceProperties` then removes the original key from the full replacement map. The Rust path only sanitizes values before `set_properties`; transition construction copies the resulting map without schema validation, while Drive validates required properties after broadcast and treats schema-invalid replacements as paid-invalid. Reject required keys before submitting so the UI cannot charge fees and consume a nonce for a guaranteed rejection.
In `packages/rs-unified-sdk-jni/src/funding.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/funding.rs:776-778: Unconfirmed shielded-create result drops the native diagnostic
The C ABI returns `ErrorShieldedBroadcastUnconfirmed` with both the derived identity ID and a detailed message containing the underlying confirmation failure. This branch frees that result without reading its message and returns only `[tag || identity_id]`; Kotlin consequently constructs `ShieldedCreateUnconfirmed` with a generic hardcoded message. The registration controller retains and displays the exception message, and the Swift wrapper preserves both fields, so Android unnecessarily loses the DAPI or result-proof failure detail needed to diagnose an ambiguous broadcast. Extend the tagged result to preserve both the identity ID and diagnostic.
…nfirmed-create diagnostic - The replace tri-state could remove REQUIRED properties: Drive validates them post-broadcast and treats their absence as a paid-invalid transition. The submit now rejects clearedKeys ∩ required with an explicit message before anything is built or signed. - The unconfirmed shielded-create payload now carries the native diagnostic: the JNI reads result.message before freeing and returns [tag || identity_id || message-utf8]; the wrapper decodes it into ShieldedCreateUnconfirmed instead of a hardcoded string, so the registration controller surfaces the underlying DAPI / result-proof failure (Swift parity — it preserves both fields). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nd-example-app Conflicts: Cargo.lock (theirs + re-resolve) and identity_top_up.rs (base #4093's rework — MIN_TOP_UP_DUFFS floor, out_new_balance sentinel, guard tests — supersedes the branch's older zero-guard). Breaking-FFI adaptations for the Android JNI/Kotlin side: - #4093/#4126: shielded identity-create/transfer/unshield/withdraw gained a mnemonic_resolver_handle param — threaded through the JNI exports, FundingNative externs, and PlatformWalletManager; the transfer/unshield/withdraw trio now runs under teardownGate.op (they borrow the manager's resolver, so the round-46 ungated rationale no longer holds). - #4127 typed provider keys: the pool-entry write trampoline forwards exactly public_key_len meaningful bytes (blob self-describing by length: 33 ECDSA / 48 BLS / 32 EdDSA); the load path rebuilds the 48-byte slot + key_type_tag from the stored blob length. No Kotlin bridge/descriptor change needed. - AccountSpecFFI lost derived_platform_node_keys(+count) (moved into the typed core-address rows) — the round-42 null-init is dropped. - #4126 Orchard viewing keys: the three new vtable slots are None — binds keep resolving the seed via the mnemonic resolver (the documented fallback); the seedless-bind persistence port is tracked as a follow-up. Verified: cargo fmt, JNI check (shielded), workspace clippy --all-features -D warnings, platform-wallet-ffi lib tests (170), gradle :sdk+:app tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current head resolves the author-claimed required-field-clear and native-diagnostic-loss defects. Two blocking issues remain: the wrong-seed cache uses a non-unique timestamp as its generation identifier, and invalid top-up indices are silently executed as zero. The variable-length JNI payload still lacks boundary coverage, and two API comments remain stale.
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— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
5 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift:186-190: Timestamp stamp can reuse a cached wrong-seed verification
`mnemonicKeychainStamp` treats creation and modification dates truncated to milliseconds as a unique item generation, but wall-clock dates provide no uniqueness guarantee. `storeMnemonic` also rewrites the item without rotating or clearing the persisted seed-binding marker. If a rewrite produces the same date pair, `PlatformWalletManager.swift:631-672` submits the stale marker and `seed_binding.rs:101-105` returns `MarkerMatched` without invoking the mnemonic resolver. A changed or mis-mapped mnemonic can therefore inherit an earlier wrong-seed verification. Store a random per-write generation token or explicitly invalidate the marker whenever the mnemonic is written.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift:677-679: Invalid top-up indices silently become zero
The account-index parser uses `UInt32(text) ?? 0`, and the resume path repeats this for `outPointVout` at lines 723-725. Consequently, a nonempty out-of-range value such as `4294967296` is executed as zero. The first path can broadcast an asset lock funded from account 0, while the resume path can consume output 0 even though its confirmation displays the invalid original value. Default only an empty optional field to zero and reject every nonempty value that cannot be represented as `UInt32` before either irreversible operation.
In `packages/rs-unified-sdk-jni/src/funding.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/funding.rs:776-800: Variable-length shielded-create payload lacks regression coverage
Rust manually encodes `[tag || identity_id || diagnostic_utf8]`, while Kotlin independently slices and decodes it in `PlatformWalletManager.kt:1035-1051`. No Rust, JVM, or Android test exercises this codec boundary. Add coverage for normal success, a nonempty diagnostic, the empty-message fallback, and multibyte UTF-8 so regressions cannot lose the identity ID or diagnostic on an ambiguous broadcast.
Extract the tagged-payload decode ([tag || identity_id || diagnostic]) into decodeShieldedCreatePayload so the Rust-encodes / Kotlin-decodes boundary is unit-testable without the native library, and add ShieldedCreatePayloadTest: success round-trip, unconfirmed with diagnostic, multibyte UTF-8 diagnostic, empty-diagnostic fallback, and short-payload rejection — a drift here would lose the identity id or diagnostic on an ambiguous broadcast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Round disposition for the preliminary review at [SUGGESTION] Variable-length shielded-create payload coverage — FIXED ( [BLOCKING] WalletStorage.swift seed-binding stamp and [BLOCKING] TransitionDetailView.swift 🤖 Addressed by Claude Code |
…nd-example-app Brings in DIP-13/15 DashPay invitations (#4041). Conflict resolution: - persistence.rs: kept the round_lock/RoundGuardState round serializer (a superset of v4.1-dev's store_round) + grafted #4041's persists_durably() invitation-durability attestation. Dropped v4.1-dev's duplicate store_round round test (RoundProbe collided with #4071's). - asset_lock/build.rs: kept both ensure_identity_topup_account and #4041's persist_asset_lock_account_pools; tightened broadcast_funded_asset_lock + invitation create_invitation to <ExtendedPubKeySigner> to match #4106's asset-lock signer tightening (extended the UnreachableSigner test double). - rs-unified-sdk-jni: left on_persist_invitations_fn None so Android's persists_durably() stays fail-closed (the invitation flow refuses to run without durable funding-index persistence). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact HEAD 7cabd78, four in-scope blocking defects remain in the PR-added Android persistence and secret-deletion paths: failed creation can leave restorable Room ghosts, wallet deletion can report success while retaining secrets, shared identity keys can be deleted out from under sibling-network wallets, and platform-address rows collide globally across those wallets. The shielded-payload coverage gap is fixed, but its stale fixed-length JNI comment remains as a nitpick. The latest target-branch merge introduces no new verified finding.
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— specialist (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— specialist (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:486-489: Delete Room rows when wallet creation rolls back
Rust persists the wallet metadata and account-registration changeset before the native creation call returns. If the subsequent mnemonic or label write fails, this catch unregisters only the native wallet and best-effort deletes the mnemonic. Native `removeWallet` does not invoke the Room cascade; this class performs that cascade separately through `persistenceHandler.deleteWalletData` during normal deletion. The persisted wallet and account rows therefore survive the failed creation, and `loadPersistedWallets` can restore the failed wallet as watch-only on the next launch. The rollback must explicitly remove the Room footprint as well.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:540-559: Do not swallow secret-store deletion failures
The private-key and mnemonic deletions discard every failure, so `removeWallet` can report success after a DataStore write error leaves encrypted secret material behind. The Room cascade then erases the public-key rows used to enumerate the affected aliases, preventing a normal retry from locating those private-key entries. Secret deletion must succeed before its ownership metadata is erased, or failures must remain durably discoverable and retryable.
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:146-147: Do not delete shared identity-key aliases with one wallet
Identity secrets are globally keyed only by public-key hex, without wallet ownership. Testnet, Devnet, and Regtest use the same DIP-9 testnet derivation path, while the shared Room database supports distinct network-scoped sibling wallets for the same seed. Siblings using the same identity and key slots can therefore reference the same alias. `PlatformWalletManager.removeWallet` unconditionally deletes that alias for one wallet, causing the remaining sibling wallets' identity signing to fail. Use wallet-scoped secret aliases or retain a shared alias while any other persisted wallet still references its public key.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PlatformAddressEntity.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PlatformAddressEntity.kt:21-39: Scope platform-address identity to its wallet
The Room schema makes `address` a global primary key and `addressHash` globally unique, even though Testnet, Devnet, and Regtest sibling wallets can derive the same DIP-17 key and share the same `tdash` encoding. The DAO already documents this collision and scopes balance lookup by `(walletId, addressHash)`, but address-pool persistence still looks up by address alone and overwrites the existing row's `walletId` and `accountId`. The first wallet then loses the row from wallet-scoped queries, and deleting either wallet can remove data needed by the other. Match the native SQLite schema's composite `(wallet_id, address)` identity and make lookup, upsert, signing, and uniqueness rules wallet-scoped, with a Room migration.
…tion, create-rollback Room cleanup - createWallet rollback now scrubs the Room footprint via persistenceHandler.deleteWalletData (the native removeWallet only unregisters — it fires no Room-cascading persistence callback) so an aborted create can't be resurrected as an orphan wallet. - removeWallet deletes identity private keys strictly BEFORE the Room cascade: any Keystore deletion failure aborts with the wallet fully intact (the public_keys rows a retry needs still exist), and the final mnemonic deletion propagates instead of silently reporting success. - Shared privkey.<pubkeyHex> aliases are retained while any identity outside the removed wallet still references the same publicKeyData (new PublicKeyDao.countReferencesOutsideIdentities). - platform_addresses reshaped to the composite (walletId, address) primary key with per-wallet addressHash uniqueness (Room v4 + create-copy-drop-rename migration + instrumented migration test); the pool-emit upsert is wallet-scoped and KeystoreSigner scans all hash candidates for a signable row instead of trusting a global one. - funding.rs: correct the stale tagged-payload comment (variable-length `[0|1] || identity_id || diagnostic_utf8`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest commit fixes all five previously published findings in their original form, including adding the missing Room rollback and correcting wallet-scoped address storage. Three blocking failure-path defects remain: failed cleanup can still leave restorable wallet rows, identity-key deletion is not atomic, and JNI publication failure cannot remove rows already persisted during native creation. These issues require changes before approval.
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)
🔴 3 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:491-494: Do not discard Room rollback failures
The rollback now invokes `deleteWalletData`, fixing the original omission, but `runCatching` discards any cleanup failure before rethrowing only the creation error. If the Room label write failed because of a database or storage fault, the transactional delete can fail for the same reason. Native state and the mnemonic are then removed while the wallet and account rows remain eligible for `loadPersistedWallets`, which restores the failed creation as a seedless watch-only wallet. Propagate a rollback error containing the wallet ID and preserve enough state to retry cleanup instead of silently abandoning the persisted rows.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:559-575: Delete identity-key aliases atomically
Each selected alias is removed in a separate DataStore transaction, while failures are reported only after the loop. An earlier deletion can therefore commit before a later reference query or DataStore edit fails. The method then throws before unregistering the native wallet or deleting its Room rows, leaving the wallet active but missing some signing keys despite claiming it was left intact. Resolve every alias to delete before mutating storage, then remove the complete set in one DataStore `edit` transaction so failure cannot partially erase a live wallet.
In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/wallet_manager.rs:282-314: Remove persisted rows when JNI cannot publish a created wallet
Native wallet creation synchronously commits wallet metadata, accounts, and address pools before returning, but publishing the handle and wallet ID to Kotlin remains fallible. On `set_long_array_region` or `byte_array_from_slice` failure, this rollback only unregisters in-memory Rust state and destroys the wallet handle; `platform_wallet_manager_remove_wallet` has no persistence-deletion callback. Kotlin throws while assigning `walletId`, before entering its cleanup `try`, so it cannot call `deleteWalletData`. The committed rows remain restorable as a seedless watch-only wallet. Use a rollback path that invokes host persistence cleanup, or change the JNI contract to caller-allocated output buffers so no fallible JNI allocation occurs after persistence succeeds.
…caller-allocated create out-buffers - createWallet rollback no longer discards a deleteWalletData failure: the mnemonic is retained (so the leftover rows stay a functional, removable wallet instead of a seedless watch-only orphan) and an IllegalStateException carrying the wallet id propagates with the creation error as cause and the rollback failures suppressed. - removeWallet's Keystore sweep is now atomic: all reads (key enumeration + shared-alias refcounts) run first, then the whole set is removed in ONE DataStore edit (new WalletStorage.deletePrivateKeys) — a single commit, so a failure deletes nothing and cannot leave a live wallet missing part of its signing keys. - createWalletFromMnemonic/WithBirthHeight JNI contract reworked to caller-allocated out-buffers (LongArray(1) + ByteArray(32), void return), validated BEFORE the native create commits persistence — the post-create region writes allocate nothing and are bounds-proven, so no fallible JNI work follows the persistence commit; the stale "remove_wallet runs the persistence cascade" comment is corrected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All three prior blockers are fixed in their original forms at the exact head, but the latest delta introduces two new blocking recovery/concurrency defects and the cumulative PR contains a blocking release-ref mismatch. The rollback branch can misclassify an unwritten mnemonic, wallet deletion is not fenced against identity-key persistence, and manual releases can publish code from a different ref than the requested tag. Rust formatting and diff whitespace checks passed; JVM verification could not run because no Java runtime is installed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
unknown model— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 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:496-515: Rollback assumes the mnemonic was successfully stored
`walletStorage.storeMnemonic` is inside the failing `try` and can throw during Keystore encryption or DataStore editing before a mnemonic entry exists. If the Room rollback also fails, this branch unconditionally claims the mnemonic was retained and closes the only managed handle. The new-wallet UI generates the phrase in a local variable and navigates to backup only after `createWallet` succeeds, so its catch path loses the only usable copy while the remaining Room rows can later load as a seedless watch-only wallet. Track whether mnemonic persistence completed and rely on retention only in that case; otherwise keep the phrase recoverable or ensure the persisted rows are removed before returning failure.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:585-601: Identity-key persistence can race the deletion snapshot
The single DataStore edit makes deletion internally atomic, but the preceding Room enumeration and external-reference checks are not serialized with key-producing operations. `onPersistIdentityKeyUpsert` can synchronously store a new `privkey.*` alias while `removeWallet` is taking this snapshot; the callback's native manager write lock does not protect these Kotlin-side reads, and `TeardownGate` tracks lifetime rather than mutual exclusion. An alias added after enumeration survives the DataStore deletion and then loses its discoverable public-key row in the Room cascade. Conversely, an external reference added after its count can lose the secret it requires. Serialize the complete removal sequence with every key-producing path and persistence callback before enumerating aliases.
In `.github/workflows/kotlin-sdk-release.yml`:
- [BLOCKING] .github/workflows/kotlin-sdk-release.yml:23-24: Manual releases do not check out the requested tag
The manual input names the tag to release, and the release step publishes the AAR under that input, but checkout has no matching `ref`. GitHub documents that checkout without `ref` uses the event ref, while `workflow_dispatch` receives a separately selected branch or tag ([actions/checkout](https://github.com/actions/checkout), [workflow_dispatch](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#workflow_dispatch)). Dispatching from the default branch with `tag=kotlin-sdk-X` therefore builds the default branch and attaches that artifact to `kotlin-sdk-X`. Checkout the requested tag whenever the input is present.
…nemonic persistence in rollback, checkout release tag - createWallet rollback tracks whether storeMnemonic actually completed: on a failed Room rollback it retries the store when the phrase never landed (the UI holds it only in a local) and reports the true disposition — "recoverable, retry removeWallet" vs "seedless, back up the phrase now" — instead of unconditionally claiming retention. - removeWallet is serialized against every key-producing path: the persistence handler gained a callbackExclusion mutex acquired by every guarded/guardedLoad callback at entry on the JNI thread (before any dispatcher hop, so a parked callback never holds the persistence thread), WalletStorage gained a privateKeyMutex taken by every privkey.* mutator, and removeWallet takes both — in the same order as the callback path — around snapshot + refcount + atomic alias delete + Room cascade. The native unregister moved BEFORE the exclusion section (a gated callback can hold the native manager write lock; gate-then- native would ABBA-deadlock) and deleteWalletData now also discards any open changeset round so buffered writes can't resurrect deleted rows. - kotlin-sdk-release.yml checks out the workflow_dispatch tag input, so a manual release builds the commit it publishes under. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes the release-ref mismatch, but both carried-forward wallet-lifecycle blockers remain at the exact assigned head. Fresh-wallet rollback can still discard the only accessible mnemonic, and wallet deletion still does not fence the full derive/store/register lifecycle, so a successful wipe can leave identity-key ciphertext behind.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
unknown model— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
…ive/store lifecycle - createWallet rollback no longer deletes the stored mnemonic: the UI holds the phrase only in a local the failure path discards, and on a re-import/orphan-recovery create the entry predates the call — the wallet-less entry is exactly what the orphan-mnemonic recovery flow surfaces (recover / keep / delete, the user decides). - The deriver-written privkey.* alias lifecycle is fenced end to end: the handler tracks aliases written during an OPEN changeset round (the row is buffered while the alias write is immediate — the gap); a rolled-back / abandoned / commit-failed round deletes its recorded aliases via the new PrivateKeyDeriver.deleteStored (their rows never commit, so the ciphertext would be stranded undiscoverably); the wallet-deletion sweep unions pendingAliasesFor into its enumeration before discarding the buffer; and a late onPersistIdentityKeyUpsert from an operation that survived unregistration skips derive+store when the wallet row is gone (checked under the same callback gate the wipe holds, so it's race-free). Regression tests cover the rolled-back-scrub and committed-keep paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current delta does not resolve either carried-forward wallet-lifecycle blocker. It also introduces one consolidated alias-cleanup blocker: the two latest-delta candidates are separate consequences of the same destructive, non-retryable cleanup invariant, leaving three canonical blockers.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
unknown model— general (failed),gpt-5.6-sol— general (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 `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:263-267: Rollback alias cleanup can delete valid keys or lose orphan tracking
pendingRoundAliases records every alias deriveAndStore touched, not whether the alias was absent before the round. The app already stores registration and add-key scalars before Rust persistence begins, and existing-key operations such as disable_keys re-emit breadcrumbs, so callbacks overwrite aliases that are already valid. If any callback or Room commit then fails, scrubPendingAliases deletes those pre-existing scalars even though Platform may already have accepted the registration or update. The same helper removes the pending set before deleteStored and swallows deletion failures; for a genuinely new alias whose buffered row never commits, a failed DataStore deletion therefore leaves ciphertext behind while discarding the only record removeWallet could later discover. The added tests use an infallible fake and no pre-existing alias, so they cover neither failure. Track only aliases created by the round with appropriate global reference checks, and retain and propagate pending cleanup state until atomic deletion succeeds.
…te until deletion succeeds - pendingRoundAliases records an alias only when it did NOT exist before the round's write (new PrivateKeyDeriver.hasStored, conservative existed-on-error): re-derives overwriting already-valid scalars (add-key flows that store before Rust persistence, disable_keys re-emitting breadcrumbs) are never rollback-deletion candidates. - Rollback scrub retains aliases a row committed by another round now references (same pubkey persisted concurrently — deleting would break its signing) and never drops cleanup state before deletion succeeds: failed atomic deletions move to an orphanedAliases map that the next round's begin retries and the wallet-deletion sweep unions via pendingAliasesFor; deleteStored's contract (atomic, throws) is documented on the interface. - createWallet's seedless corner (rollback failed AND the phrase can't be stored) now retries the Room cascade once before reporting, so seedless rows never survive when they can possibly be removed. - Tests cover the pre-existing-alias (no scrub) and failing-deletion (state retained, retry succeeds) paths the reviewer called out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd-example-app # Conflicts: # Cargo.lock
The 'snapshot a managed account's non-empty pools into AccountAddressPoolEntry rows' loop was hand-rolled in four places: the DashPay contacts helper, the DashPay payment-rotation path, the identity-top-up account deriver (ensure_identity_topup_account), and the asset-lock funding-index persistence (persist_asset_lock_account_pools) — the last one a copy the v4.1-dev merge brought in via #4041. Extract one crate::changeset::account_address_pool_entries(account_type, pools) helper. It takes the resolved &AddressPool iterator rather than a concrete account, so it works for both ManagedCoreFundsAccount (topup/DashPay sites) and ManagedAccountRef (the all_managed_accounts() bulk path) — the two diverged, which is why the earlier per-account helper couldn't be reused. wallet_lifecycle::register_wallet keeps its own inline snapshot: it must capture pools into owned entries BEFORE the wallet is moved into insert_wallet, an ordering constraint the shared helper's borrow can't meet. Behavior-preserving (same skip-empty, whole-pool, last-write-wins snapshot). cargo check + clippy clean; platform-wallet lib tests 479/479 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta protects pre-existing aliases and retries failed cleanup in memory, but all three prior wallet-lifecycle blockers remain at the exact requested head. Wallet creation can still lose a generated mnemonic, wallet deletion can miss app-prestored identity keys, and rollback cleanup loses its only alias association across process termination.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - 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 `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:513-530: Rollback still assumes the mnemonic can be recovered
If both mnemonic writes and both Room cleanup attempts fail, this branch returns an exception that tells the user to back up the phrase without preserving or returning the phrase itself. CreateWalletScreen.kt lines 197-209 keeps a generated mnemonic only in the try-block local and retains only the exception message in its catch block. The surviving Room rows are therefore seedless while the only plaintext recovery phrase becomes inaccessible; RecoverWalletsFlow cannot recover it because no mnemonic reached WalletStorage. Preserve the phrase in recoverable UI state or guarantee durable mnemonic storage or successful row removal before returning failure.
…e privkey owner index - createWallet's worst-case corner (rows undeletable AND phrase unstorable) now throws the typed WalletCreateRollbackException carrying the plaintext phrase as the last remaining copy; CreateWalletScreen surfaces it in a non-dismissable backup dialog (plain remember — never the saved-state Bundle) instead of losing it behind an error message. - Every privkey.* store can record its owning wallet in a durable privkeyowners.<walletIdHex> string-set index written in the same atomic DataStore edit: the persistence-callback deriver passes it, and both app prestore sites (CreateIdentityScreen registration key set, IdentityKeyAdditionFlow.prepareKeys — new walletId param) pass it. removeWallet's sweep unions the index into its enumeration, so app-prestored keys with no committed public_keys row and deriver writes orphaned by process death are found and deleted; batch deletions drop the hexes from every owner index in the same commit, and the wallet's index entry is removed after its sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest delta fixes the original mnemonic-disposition and rollback-alias cleanup defects, but the end-to-end identity-key lifecycle remains incompletely fenced. Three blocking issues remain: one carried-forward deletion race and two latest-delta defects involving emergency phrase retention and shared alias ownership.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 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:637-685: Identity-key persistence can still run after wallet deletion
The durable owner index fixes stores that finish before removeWallet takes its deletion snapshot, but it does not fence the complete preview/derive → store → register lifecycle. A key preview can finish, removeWallet can then unregister the wallet and complete this cascade while no owner entry exists, and the app coroutine can subsequently call WalletStorage.storePrivateKey. That store checks neither wallet existence nor a deletion tombstone, so it writes fresh ciphertext and a new owner-index entry after removal reported success. The next registration step fails against the closed or unregistered wallet, leaving the private key stranded. Quiesce the full key-producing lifecycle before deletion or reject stores for wallets whose deletion has started.
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt:666-680: Alias cleanup ignores ownership recorded only in another wallet's index
This sweep treats a globally keyed private-key alias as shared only when another committed public_keys row references it. Another wallet can already own the alias through its durable owner index while its registration or add-key row is still uncommitted. This is concrete for sibling Testnet, Devnet, and Regtest wallets created from one mnemonic: their wallet IDs differ, but DIP-9 uses the same non-mainnet derivation path and the managers share one WalletStorage. Deleting one wallet therefore removes the ciphertext and WalletStorage.deletePrivateKeysLocked removes the alias from every owner index, destroying the sibling's prestored scalar. The rollback path has the same gap because hasStored and deriveAndStore are separate operations, allowing another owner to store between them before a failed round classifies and deletes the alias as newly created. Treat other owner-index memberships as live references and determine created-versus-existing atomically under the shared private-key exclusion.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/CreateWalletScreen.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/CreateWalletScreen.kt:80-85: Configuration changes discard the only recovery phrase copy
WalletCreateRollbackException correctly carries the mnemonic when every durable store and rollback attempt fails, but this screen retains that sole copy only in plain remember state. Activity recreation caused by rotation, locale, theme, or another configuration change resets unrecoverablePhrase to null and removes the emergency dialog after the exception and coroutine-local mnemonic are gone. The surviving Room rows are then permanently seedless. Keep the phrase in an explicitly scrubbed in-memory ViewModel field that survives configuration changes without serializing plaintext into SavedStateHandle or the saved-state Bundle.
| persistenceHandler.withCallbackExclusion { | ||
| walletStorage.withPrivateKeyExclusion { | ||
| val ownedIdentityIds = persistenceHandler.identityIdsForWallet(walletId) | ||
| .map { it.toBase58String() } | ||
| val keysByPubkeyHex = LinkedHashMap<String, ByteArray>() | ||
| for (identityId in ownedIdentityIds) { | ||
| for (dbKey in database.publicKeyDao().getByIdentityId(identityId)) { | ||
| keysByPubkeyHex.putIfAbsent( | ||
| dbKey.publicKeyData.toHex(), | ||
| dbKey.publicKeyData, | ||
| ) | ||
| } | ||
| } | ||
| // Union the aliases the deriver wrote during a still-open | ||
| // changeset round: their rows are only buffered (invisible | ||
| // to the Room enumeration above) and the cascade below | ||
| // discards that buffer, so without this they would survive | ||
| // as stranded ciphertext. | ||
| for (pendingHex in persistenceHandler.pendingAliasesFor(walletId)) { | ||
| keysByPubkeyHex.putIfAbsent(pendingHex, pendingHex.hexToByteArray()) | ||
| } | ||
| // Union the DURABLE owner index: app-prestored keys whose | ||
| // registration hasn't broadcast (no public_keys row exists | ||
| // anywhere yet) and deriver writes orphaned by process | ||
| // death (the in-memory fence above doesn't survive it) are | ||
| // discoverable only here. | ||
| for (ownedHex in walletStorage.ownedPrivateKeyAliases(walletId)) { | ||
| keysByPubkeyHex.putIfAbsent(ownedHex, ownedHex.hexToByteArray()) | ||
| } | ||
| val aliasesToDelete = buildList { | ||
| for ((pubkeyHex, publicKeyData) in keysByPubkeyHex) { | ||
| val referencedElsewhere = database.publicKeyDao() | ||
| .countReferencesOutsideIdentities(publicKeyData, ownedIdentityIds) > 0 | ||
| if (!referencedElsewhere) add(pubkeyHex) | ||
| } | ||
| } | ||
| // ONE atomic DataStore commit; propagates on failure with | ||
| // nothing deleted and the cascade below not run. The same | ||
| // commit drops the deleted hexes from every owner index; | ||
| // this wallet's index entry is then removed outright (any | ||
| // alias it retained as shared stays discoverable through | ||
| // the surviving wallet's own index / Room rows). | ||
| deletePrivateKeys(aliasesToDelete) | ||
| deleteOwnerIndex(walletId) | ||
|
|
||
| // Room cascade (explicit, matching Swift — the native | ||
| // remove fires no wallet-level persistence callback). | ||
| persistenceHandler.deleteWalletDataLocked(walletId) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Identity-key persistence can still run after wallet deletion
The durable owner index fixes stores that finish before removeWallet takes its deletion snapshot, but it does not fence the complete preview/derive → store → register lifecycle. A key preview can finish, removeWallet can then unregister the wallet and complete this cascade while no owner entry exists, and the app coroutine can subsequently call WalletStorage.storePrivateKey. That store checks neither wallet existence nor a deletion tombstone, so it writes fresh ciphertext and a new owner-index entry after removal reported success. The next registration step fails against the closed or unregistered wallet, leaving the private key stranded. Quiesce the full key-producing lifecycle before deletion or reject stores for wallets whose deletion has started.
source: ['codex']
| // Last-resort phrase surface: when creation fails AND the SDK could not | ||
| // store the generated phrase durably, WalletCreateRollbackException | ||
| // carries the only remaining copy — it must be shown for manual backup | ||
| // before it is discarded. Plain remember (NOT rememberSaveable): the | ||
| // plaintext must never be written into the saved-state Bundle. | ||
| var unrecoverablePhrase by remember { mutableStateOf<String?>(null) } |
There was a problem hiding this comment.
🔴 Blocking: Configuration changes discard the only recovery phrase copy
WalletCreateRollbackException correctly carries the mnemonic when every durable store and rollback attempt fails, but this screen retains that sole copy only in plain remember state. Activity recreation caused by rotation, locale, theme, or another configuration change resets unrecoverablePhrase to null and removes the emergency dialog after the exception and coroutine-local mnemonic are gone. The surviving Room rows are then permanently seedless. Keep the phrase in an explicitly scrubbed in-memory ViewModel field that survives configuration changes without serializing plaintext into SavedStateHandle or the saved-state Bundle.
source: ['codex']
| val aliasesToDelete = buildList { | ||
| for ((pubkeyHex, publicKeyData) in keysByPubkeyHex) { | ||
| val referencedElsewhere = database.publicKeyDao() | ||
| .countReferencesOutsideIdentities(publicKeyData, ownedIdentityIds) > 0 | ||
| if (!referencedElsewhere) add(pubkeyHex) | ||
| } | ||
| } | ||
| // ONE atomic DataStore commit; propagates on failure with | ||
| // nothing deleted and the cascade below not run. The same | ||
| // commit drops the deleted hexes from every owner index; | ||
| // this wallet's index entry is then removed outright (any | ||
| // alias it retained as shared stays discoverable through | ||
| // the surviving wallet's own index / Room rows). | ||
| deletePrivateKeys(aliasesToDelete) | ||
| deleteOwnerIndex(walletId) |
There was a problem hiding this comment.
🔴 Blocking: Alias cleanup ignores ownership recorded only in another wallet's index
This sweep treats a globally keyed private-key alias as shared only when another committed public_keys row references it. Another wallet can already own the alias through its durable owner index while its registration or add-key row is still uncommitted. This is concrete for sibling Testnet, Devnet, and Regtest wallets created from one mnemonic: their wallet IDs differ, but DIP-9 uses the same non-mainnet derivation path and the managers share one WalletStorage. Deleting one wallet therefore removes the ciphertext and WalletStorage.deletePrivateKeysLocked removes the alias from every owner index, destroying the sibling's prestored scalar. The rollback path has the same gap because hasStored and deriveAndStore are separate operations, allowing another owner to store between them before a failed round classifies and deletes the alias as newly created. Treat other owner-index memberships as live references and determine created-versus-existing atomically under the shared private-key exclusion.
source: ['codex']
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