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 130 commits into
Conversation
bezibalazs
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Important Review skippedToo many files! This PR contains 376 files, which is 226 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 (376)
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.18% 87.21% +0.03%
============================================
Files 2643 2642 -1
Lines 328408 328287 -121
============================================
Hits 286330 286330
+ Misses 42078 41957 -121
🚀 New features to boost your workflow:
|
|
✅ Review complete (commit 118e14b) |
…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 |
|
Heads-up on the red Swift SDK build check — it's a self-hosted-runner keychain flake, not a code regression from this PR. The only two failures are:
🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: codex general gpt-5.5 ok; codex ffi-engineer gpt-5.5 ok; claude general/ffi opus failed due usage limit; verifier: codex gpt-5.5 ok.
The latest delta adds the DashPay contact-detail payment history path. I found one new latest-delta suggestion there, and the prior contact-crypto FFI persistence suggestion is still valid at the current head.
2 suggestion(s), 0 blocking.
Prior Findings Reconciliation
- prior-1: STILL VALID.
pending_contact_crypto_added/pending_contact_crypto_clearedstill have no FFI persistence vtable slots; Kotlin and Swift host comments still document the queue as non-durable.
New Findings In Latest Delta
- [SUGGESTION]
packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ContactDetailScreen.kt:181: Run the payment refresh after the manager and wallet are available.
Carried-Forward Prior Findings
- [SUGGESTION]
packages/rs-platform-wallet-ffi/src/persistence.rs:629: Persist deferred contact-crypto queue changes across FFI hosts.
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/dashpay/ContactDetailScreen.kt`:
- [SUGGESTION] lines 181-184: Run the payment refresh after the manager and wallet are available
The initial payment refresh is keyed only on `Unit`, while `refreshPayments()` returns before doing durable work until both `manager` and `walletId` are non-null. This screen collects the Room identity with `initialValue = null`, so a normal contact-detail open can run both current effects before Room emits the identity/wallet association or before the active manager finishes loading. Because neither effect is keyed on `manager` or `walletId`, the new Room-backed payment history is not retried until the user taps refresh or a later sync-state transition happens, even though `refreshDashPayPayments` is documented as the required durability path.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] lines 629-634: Persist deferred contact-crypto queue changes across FFI hosts
Carried-forward prior-1. `PlatformWalletChangeSet::pending_contact_crypto_added` and `pending_contact_crypto_cleared` are emitted by wallet changesets and persisted by the SQLite storage backend, but `PersistenceCallbacks` still has no FFI vtable slots for those deltas. The Rust, Kotlin, and Swift host comments all document that this queue is not durable on FFI hosts, so queued RegisterReceiving, RegisterExternal, ContactInfoDecrypt, and AutoAccept work can disappear on process death until a later sweep rediscovers and re-enqueues it. That leaves restart-immediate contact-crypto drains unreliable across the Android and Swift persistence boundary.
…lance refresh KeysListScreen queried publicKeyDao().observeByIdentityId(hex) only, but identity creation (ID-08) persists public keys keyed by the Base58 identity id, so the list rendered "No keys recorded" for created identities and the Disable-Key flow (ID-12) was unreachable. Mirror IdentityDetailScreen / KeyDetailScreen's Base58-first, hex-fallback dual lookup. IdentityDetailScreen had no way to refresh an identity's on-chain balance — a credit transfer credits the recipient (ID-14 A->B), but nothing on-device observed that, so a received-into identity showed a stale balance. Port the iOS toolbar Refresh button: sdk.identities.fetch + IdentityDao.updateBalance (targeted UPDATE preserves isLocal/alias/keys). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t are available The initial payment-history refresh in ContactDetailScreen was keyed on Unit, but refreshPayments() no-ops until both `manager` and `walletId` are non-null. The screen collects the Room identity with initialValue = null, so on a normal open the effect ran before Room emitted the identity/wallet association (and before the active manager finished loading), skipping the durable refreshDashPayPayments load — with no retry until a manual refresh or a later sync-state transition. Re-key the effect on the manager instance and wallet-id availability so it fires the refresh once they arrive (guarded so it doesn't run early; keyed on `walletId != null` rather than the ByteArray to avoid a redundant refresh on every identity-row re-emit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
[SUGGESTION] [SUGGESTION] contact-crypto queue persistence — unchanged: agreed-deferred (no FFI store-side vtable slots upstream), tracked as a follow-up. 🤖 Addressed by Claude Code |
…rmatted) The macOS "Rust workspace tests" job failed at the "Check formatting" step (cargo fmt --check) on signer.rs:398 — a long unsafe from_raw_parts_mut line the DashPay-migration merge left unwrapped. Pure rustfmt reflow; no behavior change. Unblocks the workspace fmt gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Asset-lock rows displayed "0.00000000 DASH" because the credit output is structurally self-owned, so the wallet's net diff reads as a zero-value send. Swap in the linked asset_locks.amountDuffs (the actual L1 burn), summed across a funding tx's DIP-0027 credit outputs, and thread it through the WalletTransactionDetail nav arg (Long.MIN_VALUE sentinel since nav args can't carry nullable Long). The detail header now labels Asset Lock / Asset Unlock types and keeps the Network Fee row for asset locks (netAmount is ~0 but the wallet did pay the fee). Port of TransactionListView / TransactionDetailView's assetLockAmountDuffs override. 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.
Code Review
The latest delta adds the identity balance refresh, and that new path introduces an in-scope crash risk because SDK or database failures escape the UI coroutine. The two unresolved findings from the previous reviewed SHA are still valid and carried forward: the ContactDetail payment refresh can still miss the first durable refresh, and the FFI persistence boundary still does not persist deferred contact-crypto queue deltas.
Source: reviewers claude general opus (failed: extra usage exhausted), codex general gpt-5.5, claude ffi-engineer opus (failed: extra usage exhausted), codex ffi-engineer gpt-5.5; verifier codex gpt-5.5 (recovered after opus quota failure).
Prior Findings Reconciliation
prior-contact-detail-payment-refresh: STILL VALID - Current ContactDetailScreen.kt still collects identity with initialValue = null, derives walletId from it, returns early from refreshPayments() when manager or walletId is unavailable, and keys the effects only on Unit and isSyncing.prior-ffi-contact-crypto-persistence: STILL VALID - Current persistence.rs lines 629-634 still state that pending_contact_crypto_added and pending_contact_crypto_cleared have no FFI vtable slots; Kotlin NativePersistenceBridge.kt and Swift PlatformWalletPersistenceHandler.swift document the same non-durable host limitation.
Carried-Forward Prior Findings
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ContactDetailScreen.kt:181: Run the payment refresh after the manager and wallet are available
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629: Persist deferred contact-crypto queue changes across FFI hosts
New Findings In Latest Delta
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/IdentityDetailScreen.kt:157: Catch refresh failures before they escape the UI coroutine
🔴 1 blocking | 🟡 2 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/identity/IdentityDetailScreen.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/IdentityDetailScreen.kt:157-170: Catch refresh failures before they escape the UI coroutine
The new toolbar refresh launches from `rememberCoroutineScope()` but only uses `finally` to reset `isRefreshing`. If `activeSdk.identities.fetch(...)` fails during a normal DAPI/network outage, or if the Room update throws, the exception is uncaught in the main UI coroutine. Nearby SDK-backed UI actions catch these failures and keep the screen usable; this refresh path should do the same instead of letting a failed manual refresh crash or tear down the UI.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Persist deferred contact-crypto queue changes across FFI hosts
`PlatformWalletChangeSet` still emits `pending_contact_crypto_added` and `pending_contact_crypto_cleared` deltas, and the Rust changeset docs say they are intended queue deltas. The FFI `PersistenceCallbacks` surface still has no callback slots for those fields, and the Rust, Kotlin, and Swift host comments all document that the queue is not durable on FFI hosts. Queued RegisterReceiving, RegisterExternal, ContactInfoDecrypt, and AutoAccept work can therefore disappear on Android or iOS process death until a later sweep rediscovers and re-enqueues it, making restart-immediate contact-crypto drains unreliable across the FFI persistence boundary.
In `packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ContactDetailScreen.kt`:
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/dashpay/ContactDetailScreen.kt:181-184: Run the payment refresh after the manager and wallet are available
`refreshPayments()` returns before doing durable work unless both `manager` and `walletId` are non-null, but the initial effect is keyed only on `Unit` and the sync-state effect is keyed only on `isSyncing`. This screen collects the Room identity with `initialValue = null`, so the effects can run before Room emits the identity/wallet association or before the manager is available. If `walletId` or `manager` arrives later without a sync-state transition, the Room-backed payment history is not refreshed until the user manually taps refresh.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub's diff limits, so I posted the verified findings as a top-level exact-SHA review body.
…crash the screen The IdentityDetail toolbar refresh launched from rememberCoroutineScope with only a finally to reset isRefreshing. A failing identities.fetch() (DAPI / network outage) or Room updateBalance() threw uncaught in the main UI coroutine, which can crash / tear down the screen. Added a catch that surfaces the error via the shared ErrorAlertDialog (matching the other SDK-backed identity actions like RegisterName/CreateIdentity/LoadIdentity), so a failed manual refresh keeps the screen usable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…platform into feat/kotlin-sdk-and-example-app
|
[BLOCKING] [SUGGESTION] [SUGGESTION] contact-crypto queue persistence — unchanged: agreed-deferred (no FFI store-side vtable slots upstream), tracked as a follow-up. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
Code Review
Prior reconciliation: prior-1 is STILL_VALID and remains in scope because this PR added the IdentityDetail refresh path; prior-2 is STILL_VALID (the current head documents that FFI hosts do not persist the deferred contact-crypto queue, but the callbacks are still not wired); prior-3 is FIXED by re-keying ContactDetailScreen's initial refresh on manager and wallet availability. Carried-forward prior findings: prior-1 and prior-2. New latest-delta findings: none.
Source: reviewers codex/gpt-5.5 general, codex/gpt-5.5 ffi-engineer; verifier codex/gpt-5.5. Failed lanes: claude/opus general, claude/opus ffi-engineer.
🔴 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/identity/IdentityDetailScreen.kt`:
- [BLOCKING] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/identity/IdentityDetailScreen.kt:157-170: Catch identity refresh failures in the UI coroutine
The PR-added toolbar refresh launches from rememberCoroutineScope(), calls activeSdk.identities.fetch(identityIdHex), and only uses finally to reset isRefreshing. That fetch crosses PlatformQueries.mapNativeErrors and can throw DashSdkError for normal native/DAPI failures; the Room update can also throw. With no catch, a manual refresh failure escapes the main UI coroutine instead of preserving the cached balance, unlike nearby SDK-backed actions that handle errors locally.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Persist deferred contact-crypto queue changes across FFI hosts
PlatformWalletChangeSet still emits pending_contact_crypto_added and pending_contact_crypto_cleared deltas, and the FFI PersistenceCallbacks surface still has no callback slots for those fields. Current HEAD only documents that the deferred contact-crypto queue is not durable on FFI hosts, so queued RegisterReceiving, RegisterExternal, ContactInfoDecrypt, and AutoAccept work can still disappear on Android or iOS process death until a later sweep rediscovers and re-enqueues it. This prior finding is intentionally deferred/documented but still valid and must be carried forward.
|
This blocking finding is already fixed at the current head b53ca63 (the reviewed SHA was one commit behind — it credits the ContactDetail re-key from
🤖 Addressed by Claude Code |
The Core send FFI sendToAddresses already accepts a list of recipients, but the UI only ever built a single-element list — there was no way to add extra outputs. Port the iOS 'Add recipient' flow (SendViewModel .additionalCoreRecipients + coreRecipientPlan): an Additional Recipients section (CORE_TO_CORE only) with per-row address/amount fields and a remove control, a validated coreRecipients batch (each row must parse as a Core address on this network AND scale to > 0 duffs; the whole batch is atomic so one bad row blocks the send), a multi-output Total in the summary, and the Send call now marshals the full [primary + extras] list. One L1 tx, N outputs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Latest delta: b53ca635..d34552b8 adds the CORE-10 multi-recipient Core send UI path in SendTransactionScreen.kt. I found no new latest-delta defects there.
Cumulative review: both prior findings from b53ca635 remain valid at the current head. The group-infos query is still blocking because the Kotlin/JNI/FFI path cannot pass the contract id required by the Rust implementation, and the deferred contact-crypto queue is still not durable across FFI hosts.
Source: reviewers claude general opus (failed), codex general gpt-5.5, claude ffi-engineer opus (failed), codex ffi-engineer gpt-5.5; verifier codex gpt-5.5.
Prior Findings Reconciliation
b53-prior-1: STILL VALID -PlatformQueries.ktstill exposesGroups.infos(startAtPosition, limit)withoutcontractIdat lines 397-399;QueryRegistry.ktstill advertises it as all groups in a contract at lines 688-701; JNI still forwards no contract id atqueries.rslines 1109-1124;rs-sdk-ffistill returnsOk(None)becauseGroup::fetch_manyneeds acontract_idatinfos.rslines 94-97.b53-prior-2: STILL VALID -persistence.rslines 629-634 still document no vtable slots forpending_contact_crypto_added/pending_contact_crypto_cleared;PlatformWalletChangeSetstill emits those fields atchangeset.rslines 1185-1199, and Kotlin/Swift host bridge comments still document the queue as non-durable.
Carried-Forward Prior Findings
- [BLOCKING] Thread contract id through group infos queries
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/queries/PlatformQueries.kt:397-399
The Kotlin SDK exposes sdk.groups.infos() as "All groups in a contract", and the example query catalog presents it as runnable API surface, but the method accepts only pagination and calls QueriesNative.groupGetInfos without a contract id. The JNI bridge forwards the same incomplete argument list to dash_sdk_group_get_infos, whose Rust implementation explicitly returns Ok(None) because Group::fetch_many needs a contract_id. Valid callers therefore get no data even when the selected contract has groups, with no way to request the target contract. Add contractId to the Kotlin API, QueryRegistry input, JNI signature, and FFI function, or remove/defer this query until the native implementation can fetch by contract.
- [SUGGESTION] Persist deferred contact-crypto queue changes across FFI hosts
packages/rs-platform-wallet-ffi/src/persistence.rs:629-634
PlatformWalletChangeSet still emits pending_contact_crypto_added and pending_contact_crypto_cleared deltas, but the FFI PersistenceCallbacks vtable has no callback slots for those fields. The Rust, Kotlin, and Swift bridge comments all document that the deferred contact-crypto queue is not durable on FFI hosts. Queued RegisterReceiving, RegisterExternal, ContactInfoDecrypt, and AutoAccept work can disappear across Android or iOS process death until a later sweep rediscovers and re-enqueues it, so restart-immediate contact-crypto drains are unreliable across this persistence boundary.
New Findings In Latest Delta
None.
🔴 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.
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 exact-SHA review body.
…ct_id) sdk.groups.infos() / QueriesNative.groupGetInfos bridged the shared rs-sdk-ffi dash_sdk_group_get_infos, but that FFI is a maintainer stub — it hardcodes Ok(None) with a "// TODO: needs a contract_id parameter" (the real Group::fetch_many impl is commented out until contract_id is threaded), so the query can never return data. The example query catalog presented it as runnable, so users ran it and got nothing. Removed the dead query from the Android surface until the native implementation can fetch by contract: dropped the getGroupInfos catalog entry (QueryRegistry), the Groups.infos() Kotlin API (PlatformQueries), the groupGetInfos extern (QueriesNative), and the JNI export + its now -unused rs-sdk-ffi import (queries.rs). The shared dash_sdk_group_get_infos FFI is left untouched (iOS's getGroupInfos calls it too and is equally dead) — the real fix is threading contract_id through that core FFI + Rust impl, a maintainer-owned change that fixes all hosts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…platform into feat/kotlin-sdk-and-example-app
|
[BLOCKING] group infos query — DEFERRED/REMOVED (13ee4bc). Took the reviewer's "remove/defer" option. The root cause is in the shared core FFI, not the Android bridge: Since re-implementing that shared FFI + [SUGGESTION] contact-crypto queue persistence — unchanged: agreed-deferred, tracked as a follow-up. 🤖 Addressed by Claude Code |
Bridge the shield-from-Platform-balance transition, which was unimplemented on Android (the Rust FFI platform_wallet_manager_shielded_shield and the iOS platformToShielded flow both existed; Kotlin had neither the JNI, the SDK method, nor the UI). Spends credits from the wallet's Platform-Payment addresses into its own bound Orchard pool, signed by the Keystore address signer (like ID-11) and proven by Halo 2 (~30s). - JNI: FundingNative.shieldedShield -> platform_wallet_manager_shielded_shield (takes the SignerHandle / VTableSigner, not a MnemonicResolverHandle). - SDK: PlatformWalletManager.shieldedShield(walletId, amount, shieldedAccount, paymentAccount), signing with the manager's own signerHandle. - UI: SendTransactionScreen gains a PLATFORM_TO_SHIELDED flow — a shielded recipient now offers a Platform fund source (→ shield); self-shield guard (recipient must be this wallet's own default Orchard address, since Rust always shields to self); shares the TransferOrShield fee kind and the credits/prover treatment of the other shielded flows. 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.
Code Review
At SHA 13ee4bc, the latest delta resolves the carried-forward group-infos blocker by deleting the incomplete Kotlin SDK, JNI, and example-app query surface. I found no new latest-delta findings. One carried-forward prior suggestion remains valid: the FFI persistence surface still cannot persist deferred contact-crypto queue add/clear deltas for Android/iOS hosts.
Source: reviewers = claude general opus (failed: no review JSON found); codex general gpt-5.5; claude ffi-engineer opus (failed: no review JSON found); codex ffi-engineer gpt-5.5; verifier = codex gpt-5.5.
Prior reconciliation:
- prior-1: FIXED by removing
getGroupInfosfrom the Kotlin API, QueryRegistry, JNI declaration, and native JNI bridge. - prior-2: STILL VALID at the FFI persistence boundary.
Carried-forward prior findings:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629: Persist deferred contact-crypto queue changes across FFI hosts — The Rust wallet changeset still carries
pending_contact_crypto_addedandpending_contact_crypto_cleareddeltas, butPersistenceCallbackshas no callback slots for those fields. The Rust, Kotlin, and Swift bridge comments all document that this queue is not durable on FFI hosts, so queued RegisterReceiving, RegisterExternal, ContactInfoDecrypt, and AutoAccept work can be lost across Android/iOS process death until a later sweep re-discovers and re-enqueues it. Add typed FFI persistence callbacks for these add/clear deltas before relying on restart-immediate contact-crypto drains.
New findings in latest delta:
- None
…os removal Removing dash_sdk_group_get_infos from the rs_sdk_ffi use-list in 13ee4bc left the multi-line import needing a rustfmt reflow, which the workspace "Check formatting" gate caught. Pure rustfmt reflow of the import list; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…platform into feat/kotlin-sdk-and-example-app
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude general/ffi-engineer/rust-quality opus failed: extra usage exhausted; codex general/ffi-engineer/rust-quality gpt-5.5 ok; verifier codex gpt-5.5 ok.
The latest push only reformats imports in packages/rs-unified-sdk-jni/src/queries.rs, so it does not introduce a new issue in the incremental delta. Both mandatory prior findings are still present at current HEAD and remain in scope for the cumulative PR because the PR adds the Kotlin send flow and FFI persistence bridge that expose these behaviors.
Prior Findings Reconciliation
- prior-1: STILL VALID - Platform shielding is still offered without checking
platformBalance. - prior-2: STILL VALID - deferred contact-crypto queue deltas still have no FFI persistence callbacks.
Carried-Forward Prior Findings
- [SUGGESTION] Only offer Platform shielding when Platform credits are available.
- [SUGGESTION] Persist deferred contact-crypto queue changes across FFI hosts.
New Findings In Latest Delta
None verified. The latest delta only reformats imports in packages/rs-unified-sdk-jni/src/queries.rs.
🔴 0 blocking | 🟡 2 suggestion(s) | 💬 0 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.
- [SUGGESTION] packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/SendTransactionScreen.kt:223-238: Only offer Platform shielding when Platform credits are available
`availableSources` still adds `FundSource.PLATFORM` for an Orchard recipient whenever shielded support is compiled in, without checking `platformBalance`. With zero Platform credits, the screen can auto-select or allow the Platform-to-shielded flow, show `0` as spendable, and enable submit because `canSend` only checks that the requested amount is positive. The mirrored Swift send model only exposes `.platform` when `platformBalance > 0`, so the Kotlin source list should use the same balance gate.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Persist deferred contact-crypto queue changes across FFI hosts
`PlatformWalletChangeSet` includes `pending_contact_crypto_added` and `pending_contact_crypto_cleared`, but the FFI `PersistenceCallbacks` table still has no callback slots for those deltas. The Rust, Kotlin, and Swift bridge comments all document that this queue is not durable on FFI hosts, so queued RegisterReceiving, RegisterExternal, ContactInfoDecrypt, and AutoAccept work can disappear across Android or iOS process death until a later recurring sweep rediscovers it. That makes restart-immediate contact-crypto drains unreliable for the FFI-backed hosts added by this PR.
Inline dry-run could not load the GitHub PR diff because this PR exceeds GitHub diff limits, so I posted the verified findings as a top-level exact-SHA review body.
|
Heads-up: the red Swift SDK build is a real compile error this time (not the earlier keychain flake), but it's an iOS-build/dependency-resolution mismatch on the shared side — not from this Android PR, and I can't fix it from here because the pinned dependency says the Swift is already correct. Details:
🤖 Addressed by Claude Code |
…exist availableSources added FundSource.PLATFORM for an Orchard recipient on any shielded build without checking platformBalance. With zero Platform credits that let the screen auto-select the Platform->shielded flow, show 0 spendable, and still enable submit (canSend only checks amount > 0). Gate the source on platformBalance > 0 (and add it to the remember key so the list recomputes as the balance changes), matching Swift SendViewModel .availableSources which exposes .platform only when platformBalance > 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
[SUGGESTION] Only offer Platform shielding when Platform credits are available — FIXED (18b6524). [SUGGESTION] contact-crypto queue persistence — unchanged: agreed-deferred (no FFI store-side vtable slots upstream), tracked as a follow-up. 🤖 Addressed by Claude Code |
|
Checked current head The Platform shielding suggestion is fixed: Also rechecked the latest Swift SDK job ( |
Bridge the Type-20 shielded identity-create, unimplemented on Android (CreateIdentityScreen deferred the shielded-pool funding source; no JNI). Spends a fixed-denomination note (0.1/0.3/0.5/1.0 DASH) from the wallet's bound Orchard pool to fund a new identity, Keystore-signed + Halo 2 proven. - JNI: FundingNative.shieldedIdentityCreateFromPool -> platform_wallet_manager_shielded_identity_create_from_pool. Reuses ID-08's identity-pubkey packing (decode_pubkeys_blob / role_for_registration_key_id made pub(crate)); takes the denomination, a required 21-byte fallback failure address, and the Keystore SignerHandle; returns the identity id. - SDK: PlatformWalletManager.shieldedIdentityCreateFromPool. - UI: CreateIdentityScreen gains the ShieldedBalance funding kind + a denomination picker (reusing amountText for the picked value); the fallback address is built from the wallet's first Platform-payment address. 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.
Code Review
Source: reviewers = claude/opus general (failed), codex/gpt-5.5 general (ok), claude/opus ffi-engineer (failed), codex/gpt-5.5 ffi-engineer (ok); verifier = codex/gpt-5.5.
Prior Reconciliation
prior-1FIXED: Only offer Platform shielding when Platform credits are available — Current head gates FundSource.PLATFORM on platformBalance > 0 for Orchard recipients.prior-2STILL VALID: Persist deferred contact-crypto queue changes across FFI hosts — No callbacks for pending_contact_crypto_added/cleared exist in PersistenceCallbacks at current head; host bridge comments still document the queue as non-durable.
Carried-Forward Prior Findings
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Persist deferred contact-crypto queue changes across FFI hosts
Carried-forward prior finding: current head still documents thatpending_contact_crypto_added/pending_contact_crypto_clearedhave noPersistenceCallbacksvtable slots, and Kotlin/Swift bridge comments still state this queue is not durable across FFI hosts. Any pending contact-crypto queue mutations can therefore be lost across host restart until a later sweep re-enqueues them, so the FFI persistence surface still needs durable callback slots before callers rely on restart-immediate drains.
New Findings In Latest Delta
- None
Prompt for review comments
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Persist deferred contact-crypto queue changes across FFI hosts
Carried-forward prior finding: current head still documents that `pending_contact_crypto_added` / `pending_contact_crypto_cleared` have no `PersistenceCallbacks` vtable slots, and Kotlin/Swift bridge comments still state this queue is not durable across FFI hosts. Any pending contact-crypto queue mutations can therefore be lost across host restart until a later sweep re-enqueues them, so the FFI persistence surface still needs durable callback slots before callers rely on restart-immediate drains.
5 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