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 82 commits into
Conversation
bezibalazs
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Important Review skippedToo many files! This PR contains 342 files, which is 192 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 (342)
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 58.79% 55.39% -3.41%
============================================
Files 15 16 +1
Lines 1966 2087 +121
============================================
Hits 1156 1156
- Misses 810 931 +121
🚀 New features to boost your workflow:
|
|
✅ Review complete (commit 1fd86fb) |
…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 |
…rd, balance-service rebind - DashSdkError: native code 16 (ErrorShieldedBroadcastFailed — a definitive non-execution with reservations released) now maps to a dedicated retryable type instead of falling through to non-retryable Generic; the shielded outflow APIs can all return it on relay/CheckTx rejection. - Bech32m.decode rejects mixed-case input before lowercasing (BIP-350; the app hands Rust the decoded 43 bytes, so Rust never saw the malformed casing). - AppContainer: the wallet-present rebind branch re-calls platformBalanceSyncService.configure(manager) — a prior no-wallet pass reset() nulls its manager, and without reattachment the Sync tab stays "Not synced yet" with Sync Now a no-op until app restart (found by on-device SH-06 verification). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
On-device shielded outflow verification (Testnet, API-35 emulator, fresh faucet-funded fixture) + this round's suggestions: Verification results
Fixes pushed in 45491ce (found by the verification + this review round):
The two carried-forward blocking wallet-core findings (IdentityTopUp account persistence, DashPay pool used-flag) are actively being fixed with tests — that commit lands separately. The changeset.rs 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Code Review
Cumulative review at 45491ce. The latest 3-file, ~20-line delta cleanly resolves two prior suggestion findings — the Kotlin ShieldedBroadcastFailed mapping (code 16 now maps to a dedicated isRetryable=true variant matching Rust ErrorShieldedBroadcastFailed) and the Bech32m.decode BIP-350 mixed-case guard — plus adds a benign UI-only platformBalanceSyncService.configure(manager) rebind in AppContainer.rebindWalletScopedServices with no FFI impact. No new latest-delta defects. Three prior FFI persistence-boundary findings remain unaddressed in the code and are carried forward: two blocking restart-safety issues in rs-platform-wallet (lazily-created IdentityTopUp account never crosses the FFI persister; DashPay send_payment flips the external-account address-pool used flag without emitting an AccountAddressPoolEntry snapshot), and one suggestion (pending-contact-crypto queue has no FFI persister vtable slots — partially mitigated by host-side deviation docs on Kotlin/Swift but the Rust changeset field still describes it as persisted).
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.
Prior Findings Reconciliation
prior-1: STILL VALID - packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:312-385 unchanged. Verified at HEAD:ensure_identity_topup_accountonly mutates in-memory state viawallet.add_account(...)(333/356) andinfo.add_managed_account(...)(375); noAccountRegistrationEntry/AccountAddressPoolEntryis emitted through the FFI persister. Doc comment at 283-311 claims 'no persistence' via deterministic re-derivation, but that argument covers only xpub rederivation, not consumed-index/used-flag state for the address pool of the newly-derived account.prior-2: STILL VALID - packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:631-727 unchanged.send_paymentstill callsexternal_account.next_address(Some(&contact_xpub), true)at 631-633 to mark the address used in-memory; the only durable write after broadcast isrecord_dashpay_payment(...)at 721, which emits aPaymentEntrynot anAccountAddressPoolEntry.prior-3: FIXED - packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt:121-131 introducesclass ShieldedBroadcastFailed(...) : PlatformWallet(...) { override val isRetryable get() = true }, and line 204 maps native code 16 →PlatformWallet.ShieldedBroadcastFailed. Retry semantics now match the RustErrorShieldedBroadcastFailedcontract (definitive pre-chain rejection, note reservations released).prior-4: STILL VALID -Grep pending_contact_crypto_added|pending_contact_crypto_cleared|on_persist_pending_contact_cryptounder packages/rs-platform-wallet-ffi/src returns zero matches at HEAD.PersistenceCallbacksstill exposes no vtable slots for the pending contact-crypto queue andFFIPersister::storehas no fan-out. Kotlin/Swift host docs disclose the deviation, butPlatformWalletChangeSet.pending_contact_crypto_addedinchangeset.rsstill describes the queue as persisted. Kept as suggestion since a later sync sweep self-heals.prior-5: FIXED - packages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/util/Bech32m.kt:25-28 addsif (input != input.lowercase() && input != input.uppercase()) return nullbefore the lowercase pass. Mixed-case input now rejected per BIP-350, matching the SwiftBech32mhelper.
Carried-Forward Prior Findings
prior-1— blocking: lazily-createdIdentityTopUpaccount is not persisted.prior-2— blocking: DashPaysend_paymentdoes not persist the external-account used-flag flip.prior-4— suggestion: deferred contact-crypto queue still has no FFI persister vtable slots.
New Findings In Latest Delta
None. The latest delta fixes prior-3 (ErrorShieldedBroadcastFailed retry mapping) and prior-5 (mixed-case Bech32m rejection), and adds no new verified defects.
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/asset_lock/build.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/asset_lock/build.rs:312-385: Lazily-created IdentityTopUp account is not persisted — crash between broadcast and resume strands the top-up asset lock (carried forward from prior-1)
Verified STILL VALID at HEAD 45491cec — unchanged by this delta.
`ensure_identity_topup_account` (build.rs:312-385) mutates only in-memory state: `wallet.add_account(...)` at 333/356 and `info.add_managed_account(...)` at 375. No `AccountRegistrationEntry` or `AccountAddressPoolEntry` is emitted through the FFI persister for the newly derived per-index top-up account. The added doc comment at 283-311 defends 'no persistence needed' on the grounds of deterministic re-derivation, but that argument only covers cold-start xpub rederivation for full-signable wallets — it does not address (a) external-signable production wallets, where the hardened xpub is derived through the signer/Keychain round-trip and is not present on restart until re-derived, and (b) address-pool `used` state (consumed derivation indices) inside the newly-created account, which is not snapshotted anywhere.
`create_funded_asset_lock_proof` calls `track_asset_lock` + `queue_asset_lock_changeset` before broadcasting, so the `AssetLockEntry { funding_type = IdentityTopUp { registration_index } }` IS persisted across the FFI, but the account snapshot is not. `wallet/apply.rs` deliberately does not replay `account_registrations`/`account_address_pools` on load; those are rehydrated from stored snapshots. `resume_asset_lock` then looks up `wi.accounts.identity_topup.get(&lock.identity_index)` and fails with 'Funding account IdentityTopUp not found for re-derivation'.
Consequence: after an app crash between `Built`/`Broadcast` and durable restart on external-signable wallets, iOS/Android rehydrate the persisted asset-lock row but cannot reconstruct the hardened top-up account. The already-broadcast (possibly IS/CL-locked) top-up asset lock cannot be consumed on Platform — user credits stranded. This PR ships the Android surface that makes the failure observable.
Fix options: (a) atomically emit an `AccountRegistrationEntry { account_type, account_xpub }` plus the initial `AccountAddressPoolEntry` snapshot for the new account in the same `PlatformWalletChangeSet` round as the `Built` asset-lock row (mirroring `manager/wallet_lifecycle.rs`); or (b) have `resume_asset_lock` re-run `ensure_identity_topup_account` from the persisted `lock.funding_type + lock.identity_index` before touching the derivation path.
In `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:631-727: DashPay send_payment does not persist the external-account used-flag flip (carried forward from prior-2)
Verified STILL VALID at HEAD 45491cec — unchanged by this delta.
`send_payment` selects the contact payment address via `external_account.next_address(Some(&contact_xpub), true)` at payments.rs:631-633, where the `true` marks the selected `AddressInfo` used on the in-memory `DashpayExternalAccount` pool. The documented persistence contract on `AccountAddressPoolEntry` fires snapshots on register, pool extension, and used-flag flip; the initial pool snapshot fires once at `register_external_contact_account` in `contacts.rs`, but there is no follow-up here.
The only durable write in `send_payment` after broadcast is `managed.record_dashpay_payment(...)` at payments.rs:721, which stores a `PaymentEntry` — not an `AccountAddressPoolEntry`. So no row ever flows through `PersistenceCallbacks.on_persist_account_address_pools_fn` (or the Kotlin `NativePersistenceBridge.onPersistAccountAddressPoolEntry` handler shipped by this PR) for the used-flag mutation.
Consequence: after a mobile cold restart, iOS/Android rehydrate the last stored pool snapshot with the address still marked unused. The next `send_payment` to the same contact re-selects the same first-unused address, breaking DashPay per-payment address rotation and leaking payment linkage on-chain — on both the iOS host and the Android FFI host this PR ships.
Fix: after `next_address(..., true)` succeeds, snapshot the mutated `DashpayExternalAccount` pool into an `AccountAddressPoolEntry` and persist it — either appended to the same round as the `Sent` `PaymentEntry` that `record_dashpay_payment` already flushes, or as a dedicated round immediately after the flip. Mirror the pattern in `contacts.rs`.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:70-315: Deferred contact-crypto queue still has no FFI persister vtable slots (carried forward from prior-4, partial mitigation stands)
Verified STILL VALID at HEAD 45491cec — unchanged by this delta.
`PlatformWalletChangeSet` defines `pending_contact_crypto_added: Vec<PendingContactCrypto>` and `pending_contact_crypto_cleared: Vec<PendingContactCryptoKey>` (changeset.rs:1189, 1193), and mid-layer callers merge/apply them (changeset.rs:1298-1301). However `PersistenceCallbacks` in `rs-platform-wallet-ffi/src/persistence.rs` has no `on_persist_pending_contact_crypto_added` / `_cleared` slots, `build_vtable` has no corresponding JNI callbacks, and `FFIPersister::store` never fans these fields out. Verified: `Grep pending_contact_crypto_added|pending_contact_crypto_cleared|on_persist_pending_contact_crypto` under packages/rs-platform-wallet-ffi/src returns zero matches. Only the consumer-side drain (`platform_wallet_drain_pending_contact_crypto`) is exported.
Partial mitigation stands from the prior delta: `NativePersistenceBridge.kt` and `PlatformWalletPersistenceHandler.swift` carry a 'Known coverage deviation' note, but `PlatformWalletChangeSet.pending_contact_crypto_added` in `changeset.rs` still describes the queue as 'persisted', so future FFI/SDK contributors reading only the Rust field will still misread it as durable.
Consequence: on iOS/Android FFI hosts, `pending_contact_crypto_added/_cleared` entries never reach durable host storage. Kill-between-enqueue-and-drain loses the queued work until the recurring sync sweep re-discovers it — self-healing, hence suggestion rather than blocking.
Fix: either (better) add `on_persist_pending_contact_crypto_added` / `_cleared` slots to `PersistenceCallbacks` and fan them out from `FFIPersister::store` with matching Room + `NativePersistenceBridge` and Swift handlers, or (doc-only) add the missing deviation note on `PlatformWalletChangeSet.pending_contact_crypto_added` in `changeset.rs` to complete the alternative fix.
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.
…s rotation across restart Two cross-restart durability gaps in the wallet-core DashPay/top-up paths (PR #3999 review, prior-1/prior-2), plus the prior-4 doc caveat: - IdentityTopUp funding account (asset_lock/build.rs): the lazily-derived per-index account was in-memory only, so a crash between broadcast and resume left resume_asset_lock unable to re-derive the credit-output path ("Funding account IdentityTopUp not found") and stranded the already-broadcast top-up on external-signable wallets. Now emits an AccountRegistrationEntry + initial AccountAddressPoolEntry in the same changeset round as the Built row (mirroring wallet_lifecycle), with in-memory rollback on store failure. Restart-recovery regression test added (recovery.rs, recording persister that replays like the FFI load path). Re-derivation at resume is not viable — the hardened xpub needs the external signer, absent at resume. - DashPay send_payment (identity/network/payments.rs): the per-payment address rotation was never durable. Two bugs: (1) next_address(.., true) does NOT flip the used flag (add_to_state only gates insertion of newly-generated addresses) — added an explicit mark_address_used on the resident external account; (2) no AccountAddressPoolEntry snapshot was emitted for the flip — now persisted via the shared contacts::dashpay_account_pool_entries helper BEFORE funding/broadcast, so a relaunch can't hand the same address to the next payment (DIP-15 linkage leak). Regression test added. - changeset.rs: the pending_contact_crypto_added field now documents the FFI-host durability deviation (prior-4 doc-only mitigation; the note also lives on the FFI persister and both host handlers). platform-wallet: 516 passed / 0 failed (incl. the 3 asset_lock::build broadcast/reservation tests that were red on the merged head); clippy -D warnings + fmt clean across platform-wallet, platform-wallet-ffi, and the rs-unified-sdk-jni consumer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both blocking findings + the prior-4 doc gap are fixed in e7a9f25: prior-1 (IdentityTopUp account not persisted) — FIXED, option (a). prior-2 (send_payment used-flag flip not persisted) — FIXED, and it was deeper than the snapshot. Adding the snapshot alone wasn't enough: prior-4 (pending contact-crypto queue) — doc mitigation completed. The Verified: 🤖 Addressed by Claude Code |
…egister crash
The Kotlin app re-activates its per-network wallet manager on every
`appState.sdk` emission (network switch, devnet reconfigure, or a plain
StateFlow re-emission during navigation), which re-runs
`loadPersistedWallets` -> `load_from_persistor` against a manager that
already holds the persisted wallet. The second insert hit
`WalletManager::insert_wallet`'s `WalletExists`, wrapped as
`WalletCreation("Failed to register persisted wallet in WalletManager:
Wallet already exists: <id>")`, and — with no manager-reset path across
the FFI boundary — surfaced as an uncaught `DashSDKException` on the main
thread (`FATAL EXCEPTION: main`), intermittently killing the app.
Make `load_from_persistor` idempotent: recompute the canonical wallet id
up front, and under a single `wallet_manager` write lock skip any wallet
already registered (no TOCTOU). Skipped wallets are deliberately not
recorded for rollback, so a call only unwinds its own inserts, and an
already-hydrated wallet's live in-memory balance/state is preserved
rather than clobbered by a stale snapshot. The id-mismatch check moves
ahead of the insert as a cleaner side effect. Shared Rust, so iOS gets
the same fix; matches `loadPersistedWallets`' documented idempotency.
Adds a regression test loading the same persisted wallet three times and
asserting each call is `Ok` with exactly one wallet registered. The
duplicate-*create* path still correctly rejects with `WalletAlreadyExists`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(SH-06 credit) Root cause of the on-device SH-06 gap (a confirmed unshield credit never reflected app-side): build_wallet_restore_entry hardcoded platform_address_balances to null/0, so persisted platform-address balances were never rehydrated into the Rust provider on load. After a restart the BLAST loop trusts its watermark but has an empty per-account balance map; the tree-scan re-pins each address at checkpoint_height and the #4019 ADDR-09 gate (op_height <= as_of_height) then discards the credit delta — the balance stays 0 forever. iOS is unaffected (its Swift loadCachedBalances feeds (balance, as_of_height) back), which is why the fix belongs in the Android load path only, not core. - PlatformWalletPersistenceHandler.buildWalletRestoreData now reads the wallet's platform_addresses Room rows into a new platformAddressBalances restore list (asOfHeight = lastSeenHeight). - NativePersistenceBridge gains PlatformAddressBalanceRestoreData +ing the WalletRestoreData field; JNI build_wallet_restore_entry marshals it into WalletRestoreEntryFFI.platform_address_balances via the staging-vecs discipline (AddressBalanceEntryFFI is Copy POD; sealed + freed like ignored_senders). - as_of_height round-trips verbatim, so the height-pin still prevents the #4019 double-count; a genuine 0 ("unknown provenance") self-heals to the first pinned absolute on the next sync. Round-trip unit test added. Not a distinct pre-restart bug: the persisted-balance map is the backstop that keeps a tracked address in the sync pending-set once it falls out of the live gap window; restoring it fixes both the post-restart and the out-of-window paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…platform into feat/kotlin-sdk-and-example-app
|
SH-06 (shielded unshield → platform-address credit) is now RESOLVED and verified on-device. Fixed in 80e81e5: the Android JNI load path ( On-device re-verification (same fixture wallet whose 0.05 DASH credit was on-chain at height 380987 but read 0 in-app): cold restart → Platform balance moved 0 → 0.05 DASH, Active Addresses 0 → 1. Confirmed. So all four shielded rows are now green on Android: SH-05 ✅, SH-06 ✅, SH-08 (wired) ✅, SH-13 ✅. Note on the macOS 🤖 Generated with Claude Code |
macOS
|
There was a problem hiding this comment.
Code Review
Cumulative review at e7a9f256 for dashpay/platform#3999.
Source: reviewers claude/opus general, codex/gpt-5.5 general, claude/opus ffi-engineer, codex/gpt-5.5 ffi-engineer; verifier claude/opus.
Prior Findings Reconciliation:
prior-1FIXED: IdentityTopUp account creation now persists anAccountRegistrationEntryand initial address-pool snapshots before returning, with rollback on store failure and recovery coverage.prior-2FIXED: DashPaysend_paymentnow marks the external payment address used and persists the updated external-account pool before broadcast, with regression coverage.prior-4STILL VALID / documented defer: contact-crypto queue entries still have no FFI persister vtable slots, but the latest delta documents the process-lifetime limitation and recurring-sweep recovery. Carried forward as a suggestion.
Carried-Forward Prior Findings:
prior-4suggestion: contact-crypto queue still has no FFI persister vtable slots.
New Findings In Latest Delta:
NativeLoader.ensureLoaded()flipsloadedbeforeSystem.loadLibrary/nativeInitsucceeds, so a failed first load poisons future retries.
No in-scope blockers remain at this head.
🟡 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/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativeLoader.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativeLoader.kt:24-29: NativeLoader flips `loaded` to true before load/init succeeds — a failed init permanently poisons the process
`ensureLoaded()` uses `loaded.compareAndSet(false, true)` and only then calls `System.loadLibrary(LIBRARY_NAME)` and `SdkNative.nativeInit()`. If either throws (a transient split-install / library-availability race for `libdash_sdk_jni.so`, or an init failure surfaced through JNI), the flag remains `true`, so every subsequent `ensureLoaded()` call short-circuits and every SDK entry point jumps straight to a native method — producing misleading `UnsatisfiedLinkError`s / no-op initialization instead of a clean retry.
Flip the flag only after both operations succeed while still serializing concurrent callers, e.g. gate with `AtomicBoolean` inside a `synchronized(this)` block, or use a lazy `by lazy` holder that propagates the constructor exception and retries on the next access.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:626-636: Contact-crypto queue still has no FFI persister vtable slots (carried forward from prior-4)
`PlatformWalletChangeSet::pending_contact_crypto_added` / `pending_contact_crypto_cleared` still have no `on_persist_*` slots in `PersistenceCallbacks`, no matching JNI wiring in `build_vtable`, and are not fanned out from `FFIPersister::store`. The latest delta added explicit doc comments at persistence.rs:626-636 and changeset.rs:1189-1195 documenting the queue as process-lifetime only on FFI hosts, with the recurring sync sweep providing self-healing recovery after a restart — so this is no longer a silent data-loss risk and the framing is now honest.
Carrying forward at suggestion severity: the durable fix (add `on_persist_pending_contact_crypto_added_fn` / `_cleared_fn` slots plus Kotlin/Swift Room/SwiftData tables and load-side hydration) still matches every other fan-out field on `PlatformWalletChangeSet` and is worth landing before DashPay contact-crypto goes to production, because any RegisterReceiving / RegisterExternal / ContactInfoDecrypt / AutoAccept operation queued between two sweeps is delayed by one sweep interval after a kill — a UX drag on the contact-request encryption/decryption path, not a correctness regression.
ensureLoaded flipped the loaded flag via compareAndSet BEFORE running System.loadLibrary + nativeInit, so a failure (transient split-install race for the .so, or an init error) permanently poisoned the process — every later ensureLoaded short-circuited and every native entry point threw UnsatisfiedLinkError instead of cleanly retrying. Now uses double-checked locking over a @volatile flag set only after both operations succeed; a throw leaves it false so the next call retries. Repeat-call fast-path (volatile read) preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both suggestions addressed: NativeLoader poisoning — FIXED (342f45a). Contact-crypto queue vtable slots (prior-4) — deferred, as you framed it. Agreed this is suggestion-severity now that the deviation is honestly documented at all four sites and the recurring sweep bounds the impact to a one-sweep-interval delay (not data loss). The durable fix (add 🤖 Addressed by Claude Code |
…nd-example-app # Conflicts: # packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md # qa-contract/contract-id.testnet.json # qa-contract/schema/qa-contract.documents.json # qa-contract/src/codes.mjs # qa-contract/src/parse-test-plan.mjs
Platform-address transfer auto-selection sized its candidate inputs from the in-memory `account.address_credit_balance` cache, which reads 0 for a freshly-loaded wallet whose per-address credit balances were never hydrated. That made `auto_select_inputs` find no candidates and fail with `Insufficient balance: available 0 credits ... [DeductFromInput(0)]`, even though the same addresses were on-chain funded — while withdraw worked because `plan_withdrawal` already reads on-chain balances. Enumerate the account's derived addresses for the candidate SET only, then read authoritative on-chain balances via `AddressInfo::fetch_many` — the same proof query the transfer spend path re-fetches — dropping the wallet read lock before the network call. Mirrors `plan_withdrawal` exactly so the submit gate and spend path stay in lockstep and immune to a stale or zero cache. Empty-account case skips the fetch. Device-verified on testnet: with the account pool hydrated, the transfer finds the on-chain balance and submits successfully (funds move on-chain). 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
Cumulative review at c2f953b. The latest delta (~530 lines) is tightly scoped to two fixes: (1) SH-06 credit-restore — Kotlin now round-trips cached platform-address balances (with their ADDR-09 as_of_height pin) via a new PlatformAddressBalanceRestoreData[] and the JNI trampoline packs them into AddressBalanceEntryFFI (POD, freed with a single free_raw_slice); (2) double-register crash — load_from_persistor is now idempotent by computing the wallet id up front, checking wm.get_wallet(&wallet_id) inside the same write-lock scope as insert_wallet (TOCTOU-safe), and continueing on a hit without touching inserted_in_manager. Both fixes ship targeted regression tests. No new in-scope defects introduced. Two prior suggestion-level findings are carried forward because no code addressing them landed in this delta.
Source: reviewers claude general opus, codex general gpt-5.5, claude ffi-engineer opus, codex ffi-engineer gpt-5.5; verifier claude opus.
Reviewed queued commit c2f953bd. The PR head had already advanced to 8f333965 before posting, so this review is exact-SHA coverage for the dispatcher-assigned commit.
Prior Findings Reconciliation
prior-1: STILL VALID - NativeLoader.kt:24-29 unchanged at c2f953b —loaded.compareAndSet(false, true)still precedesSystem.loadLibrary(LIBRARY_NAME)andSdkNative.nativeInit(). Carried forward as a suggestion.prior-4: STILL VALID - persistence.rs:628-634 still carries only the deferral doc comment;PersistenceCallbackshas noon_persist_pending_contact_crypto_*_fnslots andFFIPersister::storedoes not fan them out.PlatformWalletChangeSet::pending_contact_crypto_*fields remain process-lifetime only on FFI hosts. Carried forward as a suggestion.
🟡 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/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativeLoader.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/NativeLoader.kt:24-29: NativeLoader flips `loaded` to true before load/init succeeds — a failed init permanently poisons the process
`ensureLoaded()` uses `loaded.compareAndSet(false, true)` and only then calls `System.loadLibrary(LIBRARY_NAME)` and `SdkNative.nativeInit()`. If either throws (a transient split-install / library-availability race for `libdash_sdk_jni.so`, or an init failure surfaced through JNI), the flag remains `true`, so every subsequent `ensureLoaded()` call short-circuits and every SDK entry point jumps straight to a native method — producing misleading `UnsatisfiedLinkError`s / no-op initialization instead of a clean retry.
Flip the flag only after both operations succeed while still serializing concurrent callers, e.g. gate with an `AtomicBoolean` inside a `synchronized(this)` block.
Carried forward from prior-1 — file unchanged at c2f953bd.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:628-634: Contact-crypto queue still has no FFI persister vtable slots (carried forward from prior-4)
`PlatformWalletChangeSet::pending_contact_crypto_added` / `pending_contact_crypto_cleared` still have no `on_persist_*` slots in `PersistenceCallbacks`, no matching JNI wiring in `build_vtable`, and are not fanned out from `FFIPersister::store`. The doc comment at persistence.rs:629-634 (and the mirror on `PlatformWalletChangeSet` in changeset.rs) documents the queue as process-lifetime only on FFI hosts, with the recurring sync sweep providing self-healing recovery after a restart — so this is documented deferral rather than silent data loss.
Carrying forward at suggestion severity: the durable fix (add `on_persist_pending_contact_crypto_added_fn` / `_cleared_fn` slots plus Kotlin Room / Swift SwiftData tables and load-side hydration) still matches every other fan-out field on `PlatformWalletChangeSet` and is worth landing before DashPay contact-crypto ships to production — any RegisterReceiving / RegisterExternal / ContactInfoDecrypt / AutoAccept operation queued between two sweeps is delayed by one sweep interval after a kill, a UX drag on the contact-request encryption / decryption path, not a correctness regression.
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.
|
Both carried-forward suggestions are already resolved on the current head — this review ran against the older queued commit
No code change needed this round — a re-review at 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review at 8f33396 (delta c2f953b..8f33396 is ~1.9k insertions, mostly Swift integration test scaffolding, qa-contract housekeeping, and a rust-dashcore rev bump). Prior reconciliation: prior-1 (NativeLoader poison flag) is FIXED — NativeLoader.kt:28-36 now uses @volatile + synchronized double-check and only sets loaded = true after both System.loadLibrary and SdkNative.nativeInit() succeed. Prior-2 (contact-crypto queue lacks FFI persister vtable slots) is STILL_VALID — persistence.rs:629-634 still documents the deferral, no on_persist_pending_contact_crypto_*_fn slots exist, and the queue remains process-lifetime on FFI hosts (self-healed by the recurring sweep). No new latest-delta defects identified. Carried-forward prior findings: 1 suggestion. New latest-delta findings: 0.
Source: reviewers claude/opus general, codex/gpt-5.5 general, claude/opus ffi-engineer, codex/gpt-5.5 ffi-engineer; verifier claude/opus.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Carried forward: contact-crypto queue still has no FFI persister vtable slots
Boundary: Rust `PlatformWalletChangeSet` → `rs-platform-wallet-ffi` `PersistenceCallbacks` C-vtable → Kotlin `NativePersistenceBridge` / Swift `PlatformWalletPersistenceHandler`.
`PlatformWalletChangeSet::pending_contact_crypto_added` / `pending_contact_crypto_cleared` still have no `on_persist_*` slots in `PersistenceCallbacks`, no matching JNI wiring in `build_vtable`, and are not fanned out from `FFIPersister::store` — only the deferral comment at persistence.rs:629-634 exists. The queue is process-lifetime only on FFI hosts and the recurring sync sweep re-enqueues after a restart, so this is documented deferral rather than silent data loss.
The durable fix (add `on_persist_pending_contact_crypto_added_fn` / `_cleared_fn` slots plus matching Kotlin Room DAOs / Swift SwiftData tables with load-side hydration) matches every other fan-out field on `PlatformWalletChangeSet` and is worth landing before DashPay contact-crypto ships to production. Any RegisterReceiving / RegisterExternal / ContactInfoDecrypt / AutoAccept operation queued between two sweeps is delayed by one sweep interval after a kill — a UX drag on the contact-request encryption / decryption path, not a correctness regression.
Status at 8f333965: STILL_VALID — the latest delta did not touch persistence.rs or the mirrored fields on `PlatformWalletChangeSet`.
DOC-02 was marked implemented in the test plan but had no create path — DocumentTypeDetailsScreen only exposed browse/count/sum queries. Wire up the real document-create state transition end to end: - rs-unified-sdk-jni: `documentCreate` JNI export over `platform_wallet_create_document_with_signer` (no signingKeyId — the Rust side selects an AUTHENTICATION + ECDSA key from the wallet's IdentityManager). - Kotlin SDK: `TransactionsNative.documentCreate` + `DocumentTransactions.create`. - App: schema-driven `CreateDocumentScreen` (owner picker + per-property editors parsed from the stored contract JSON), a "New Document" action on `DocumentTypeDetailsScreen`, and a `NewDocument` route. Port of iOS `CreateDocumentView` + `DocumentFieldsView`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Confirmed prior-1 (NativeLoader) reconciled as FIXED — thanks. On the remaining carried-forward suggestion (contact-crypto queue FFI persistence): keeping it as a tracked follow-up rather than landing it in this PR's tail, with a concrete reason beyond "it's a suggestion." Reading the full shape, a durable fix is larger than a normal fan-out field: I've scoped it as a dedicated follow-up (with the core-loader-hydration requirement and the flattened-op FFI struct spelled out) so it lands verified rather than rushed. It's worth doing before DashPay contact-crypto ships to production, as you note — just not as a tail addition to this Android-port PR. Every blocking finding across all rounds is resolved, and the deferral remains honestly documented at all four sites. 🤖 Addressed by Claude Code |
…election Right after a fresh app relaunch the in-memory derived address pool (`ManagedPlatformAccount.addresses.addresses`) is empty until a platform sync repopulates it, but `initialize_from_persisted` hydrates the per-address balance map via `set_address_credit_balance(.., None)`, whose `key_source=None` never populates the pool. Both `auto_select_inputs` (transfer) and `plan_withdrawal` (withdraw) enumerated only the empty pool for their candidate set, so input selection failed with "available 0 credits" / "No funded addresses available" immediately after launch even though the balances were persisted and on-chain. Build the candidate set from the UNION of the derived pool and the hydrated `address_balances` keys — the same source Platform Balance reads — so input selection works immediately after launch. Balances are still resolved on-chain via fetch_many; the union only decides which addresses to query. Add a regression test covering the empty-pool + hydrated-balance-map state. 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
Prior reconciliation: prior-1 is STILL_VALID at 8bd3a85; the FFI persistence layer, JNI vtable, Kotlin bridge, and Swift bridge still document that pending_contact_crypto_added/_cleared have no durable host persistence slots. Carried-forward prior findings: 1 suggestion. New latest-delta findings: 0; I also validated one cumulative JNI boundary hardening issue introduced earlier in this PR, not by the final transfer.rs delta.
Carried-forward prior findings: 1 suggestion. New latest-delta findings: 0. Cumulative current-head findings: 1 suggestion in JNI blob decoding that predates the 8f33396..8bd3a85 transfer.rs-only delta but is still introduced by this Android SDK PR surface.
Source: reviewers claude/opus general, codex/gpt-5.5 general, claude/opus ffi-engineer, codex/gpt-5.5 ffi-engineer; verifier codex/gpt-5.5.
🟡 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/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Carried forward: contact-crypto queue still has no FFI persister slots
`PlatformWalletChangeSet::pending_contact_crypto_added` and `pending_contact_crypto_cleared` still do not cross the FFI persistence ABI: `PersistenceCallbacks` has no `on_persist_pending_contact_crypto_*` slots, `rs-unified-sdk-jni::persistence::build_vtable` has no matching trampolines, and the Kotlin/Swift host bridges document the same non-durable behavior. The code is explicit that the recurring sweep re-enqueues after restart, so this is an intentional deferral rather than silent data loss, but any RegisterReceiving/RegisterExternal/ContactInfoDecrypt/AutoAccept work queued between sweeps can be delayed after process death. This remains worth tracking before relying on restart-immediate DashPay contact-crypto drains.
In `packages/rs-unified-sdk-jni/src/wallet_manager.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:853-854: Validate decoded row count before allocating transfer outputs
`decode_credit_outputs` trusts the first four bytes of `outputsBlob` as the row count and calls `Vec::with_capacity(count)` before verifying that the byte array contains that many fixed-size rows. The normal Kotlin wrapper builds this blob with `ManagedPlatformWallet.encodeCreditOutputs`, but the native boundary still receives a raw `ByteArray`; a malformed direct/reflected call with a huge count and short payload can force a large native allocation or abort before the truncation checks throw a `DashSDKException`. Add a length-before-allocation check here, and apply the same pattern to the funding recipients decoder that allocates from its blob row count.
decode_credit_outputs / decode_funding_recipients (wallet_manager.rs) and decode_credit_rows (credits.rs) trusted the blob's 4-byte header row count and called Vec::with_capacity(count) before validating the payload held that many rows. The normal Kotlin wrappers build well-formed blobs, but the raw ByteArray crosses the JNI boundary — a malformed direct/reflected call with a huge count and short body could force a large native allocation / abort before the per-row truncation checks throw. Added a length-before-allocation guard at each site: exact `count * 29` for the fixed-row decoders, `count <= remaining` for the variable-row recipients decoder. Throws DashSDKException on a mismatch; valid input unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both suggestions handled: Blob row-count allocation guard — FIXED (1fd86fb). Added a length-before-allocation check to all three JNI blob decoders ( Contact-crypto queue durability — tracked follow-up (unchanged from last round). Still a deferral by design (self-healing sweep, no data loss); the durable fix requires hydrating the in-memory queue inside the maintainer-owned core Note: the macOS 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Exact-SHA cumulative review for 6a979003 (previous reviewed commit 8bd3a858). Both prior findings remain STILL VALID at the checked-out head and are carried forward as suggestions. The latest delta adds the Kotlin document-create flow plus platform-address relaunch-candidate handling; no new latest-delta findings were verified.
Source: reviewers claude general opus (failed_usage_limit), codex general gpt-5.5, claude ffi-engineer opus (failed_usage_limit), codex ffi-engineer gpt-5.5; verifier codex gpt-5.5 (recovered_manual_json_after_cancelled_transcript).
Reviewed queued commit 6a979003. GitHub live head is currently 1fd86fba, so this is exact-SHA coverage for the dispatcher-assigned commit and the newer queue gate is left intact.
2 suggestion(s)
Prior Findings Reconciliation
prior-1: STILL VALID - contact-crypto queue persistence still has no FFI/JNI/Kotlin/Swift durable host callbacks.prior-2: STILL VALID -decode_credit_outputsstill allocates from the untrusted row count before validating payload length.
Carried-Forward Prior Findings
suggestion: Carried forward: contact-crypto queue still has no FFI persister slots
packages/rs-platform-wallet-ffi/src/persistence.rs lines 629-634
Boundary affected: Rust platform-wallet persistence changesets crossing into Swift/Kotlin host persistence callbacks. PlatformWalletChangeSet::pending_contact_crypto_added and pending_contact_crypto_cleared still do not cross the FFI persistence ABI: PersistenceCallbacks has no on_persist_pending_contact_crypto_* slots, the JNI vtable has no matching trampolines, and the Kotlin/Swift host bridges document the same non-durable behavior. The code explicitly says the recurring sweep re-enqueues after restart, so this is not silent data loss, but RegisterReceiving/RegisterExternal/ContactInfoDecrypt/AutoAccept work queued between sweeps can be delayed after process death.
Source: codex-general, codex-ffi-engineer
suggestion: Carried forward: validate decoded row count before allocating transfer outputs
packages/rs-unified-sdk-jni/src/wallet_manager.rs lines 853-854
Boundary affected: Kotlin/JNI ByteArray blobs crossing into Rust transfer/funding decoders. decode_credit_outputs trusts the first four bytes of outputsBlob as the row count and calls Vec::with_capacity(count) before verifying that the byte array contains that many fixed-size rows. The normal Kotlin wrapper builds this blob with ManagedPlatformWallet.encodeCreditOutputs, but the native boundary still receives a raw ByteArray; a malformed direct/reflected call with a huge count and short payload can force a large native allocation before the truncation checks throw a DashSDKException. The same defensive length-before-allocation check should also be applied to decode_funding_recipients at lines 745-746, which uses the same pattern for recipientsBlob.
Source: codex-general, codex-ffi-engineer
let count = u32::from_be_bytes(count_bytes.as_slice().try_into().ok()?) as usize;
let remaining = bytes.len().saturating_sub(cursor);
let max_rows = remaining / 29;
if count > max_rows {
throw_sdk_exception(
env,
1,
&format!("outputs blob row count {count} exceeds remaining payload ({remaining} bytes)"),
);
return None;
}
let mut outputs = Vec::with_capacity(count);
New Findings In Latest Delta
None. The 8bd3a858..6a979003 delta is the Kotlin document-create flow plus platform-address relaunch-candidate handling, and no new latest-delta defects were verified.
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/rs-platform-wallet-ffi/src/persistence.rs:629-634: Carried forward: contact-crypto queue still has no FFI persister slots
Boundary affected: Rust platform-wallet persistence changesets crossing into Swift/Kotlin host persistence callbacks. `PlatformWalletChangeSet::pending_contact_crypto_added` and `pending_contact_crypto_cleared` still do not cross the FFI persistence ABI: `PersistenceCallbacks` has no `on_persist_pending_contact_crypto_*` slots, the JNI vtable has no matching trampolines, and the Kotlin/Swift host bridges document the same non-durable behavior. The code explicitly says the recurring sweep re-enqueues after restart, so this is not silent data loss, but RegisterReceiving/RegisterExternal/ContactInfoDecrypt/AutoAccept work queued between sweeps can be delayed after process death.
- [SUGGESTION] packages/rs-unified-sdk-jni/src/wallet_manager.rs:853-854: Carried forward: validate decoded row count before allocating transfer outputs
Boundary affected: Kotlin/JNI `ByteArray` blobs crossing into Rust transfer/funding decoders. `decode_credit_outputs` trusts the first four bytes of `outputsBlob` as the row count and calls `Vec::with_capacity(count)` before verifying that the byte array contains that many fixed-size rows. The normal Kotlin wrapper builds this blob with `ManagedPlatformWallet.encodeCreditOutputs`, but the native boundary still receives a raw `ByteArray`; a malformed direct/reflected call with a huge count and short payload can force a large native allocation before the truncation checks throw a `DashSDKException`. The same defensive length-before-allocation check should also be applied to `decode_funding_recipients` at lines 745-746, which uses the same pattern for `recipientsBlob`.
Inline posting is skipped for this oversized PR; this body carries the verified exact-SHA findings.
|
Both are already resolved on the current head — this review ran against the older queued commit
No new code needed this round — a re-review at 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified against HEAD 1fd86fb. Prior reconciliation: prior-1 is STILL_VALID because the FFI persistence vtable still lacks contact-crypto queue slots; prior-2 is FIXED by the new length-before-allocation guards in the JNI row decoders. Carried-forward prior findings: prior-1. 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 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:629-634: Contact-crypto queue still has no FFI persister slots
The Rust FFI persister still documents that `PlatformWalletChangeSet::pending_contact_crypto_added` and `pending_contact_crypto_cleared` have no vtable slots, so Swift/Kotlin hosts cannot durably persist this deferred contact-crypto queue. The same limitation is documented in the Kotlin and Swift host bridges, while the SQLite persister does persist these fields. The recurring sweep can rediscover and re-enqueue the work after restart, so this is delayed work rather than permanent silent loss, but any queued RegisterReceiving/RegisterExternal/ContactInfoDecrypt/AutoAccept work can be lost across process death until that sweep runs again.
|
Investigated the contact-crypto queue durability in depth (4-agent scoping pass) to decide implement-vs-defer with evidence rather than another hand-wave. Findings split the problem cleanly, and the deciding factor is an upstream blocker your own SQLite persister shares:
Given that: implementing the Android store alone would write rows nothing ever reads (no user-visible change — the sweep self-heals exactly as now), while committing a premature Room migration whose schema should be co-designed with the restore consumer. So I'm deferring the coherent store + restore unit to land together once the core 🤖 Addressed by Claude Code |
|
Thanks for digging into the contact-crypto queue path. I agree store-only FFI/Room plumbing would not buy real durability until the core load/restore path can hydrate the in-memory queue; tracking the coherent store+restore work as a follow-up is fine for this PR from my side. |
|
Thanks — agreed. The coherent store+restore work is tracked as a follow-up with the full file-by-file plan and the upstream 🤖 Generated with Claude Code |
4 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