fix(sdk)!: complete encrypted txMetadata parity - #4243
Open
shumkov wants to merge 39 commits into
Open
Conversation
…e + decrypt-on-fetch Adds the wallet-contract encrypted-document surface the Android wallet needs to retire the legacy org.dashj.platform stack (closes #4086 create/update, closes The encryption ENVELOPE is byte-for-byte wire-compatible with the legacy BlockchainIdentity.publishTxMetaData / getTxMetaData so documents written by either stack decrypt with the other (migrated users keep their history): - AES-256 key = the raw 32-byte secp256k1 private scalar of a hardened HD child (mirrors KeyCrypterAESCBC.deriveKey(ECKey); no ECDH, no HKDF). - Path: identity-auth path of the identity's encryption key (its id = the document's keyIndex) extended by / 32769' / encryptionKeyIndex'. - Cipher: AES-256-CBC / PKCS7, random 16-byte IV. - encryptedMetadata blob = version(1) ‖ IV(16) ‖ AES-256-CBC(payload); the version byte is OUTSIDE the ciphertext (0 = CBOR, 1 = protobuf). The plaintext payload stays OPAQUE to the SDK — the app owns the protobuf TxMetadataBatch item schema and the batching policy, exactly as on the legacy stack. The SDK owns only the crypto envelope + the {keyIndex, encryptionKeyIndex, encryptedMetadata} document fields. Layers: - rs-platform-wallet: crypto/tx_metadata.rs (derive/seal/open + tests); network/encrypted_document.rs (create_encrypted_document_with_signer reusing the tested create path; fetch_encrypted_documents mirroring the contactInfo paginated decrypt loop). - rs-platform-wallet-ffi: platform_wallet_create_encrypted_document_with_signer, platform_wallet_fetch_encrypted_documents (JSON-out; payload as base64). - rs-unified-sdk-jni: documentCreateEncrypted / documentFetchEncrypted. - kotlin-sdk: DocumentTransactions.createEncryptedDocument / fetchEncryptedDocuments (additive; no existing signatures change). Tests: seal↔open round-trip, key-derivation determinism + index separation, wrong-key/ malformed-blob fail cleanly, and a NIST SP 800-38A CBC-AES256 cross-stack vector pinning the cipher core + blob framing. cc @QuantumExplorer Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d vector
The encrypted-txMetadata scheme claims byte-for-byte wire compatibility with
the legacy org.dashj.platform stack, but the mnemonic->AES-key HD derivation
prefix was previously only "sample-confirmed" (the module's NIST test pinned
the AES-CBC envelope but explicitly could NOT pin the derivation-path account
prefix). That gap is now closed by reconstructing the exact legacy recipe from
the shipped jars and running it under a JVM.
Recovered legacy derivation (the wire-compat reference this crate mirrors):
AES-256-CBC key = raw private-key bytes of the ECKey at absolute HD path
m / 9' / coinType' / 5' / 0' / 0' / 0' / keyId' / 32769' / encryptionKeyIndex'
- account path 9'/coinType'/5'/0'/0'/0' is
DerivationPathFactory.blockchainIdentityECDSADerivationPath() (dashj-core
22.0.3), the path the BLOCKCHAIN_IDENTITY AuthenticationKeyChain is built
with (AuthenticationGroupExtension.getDefaultPath).
- keyId' / 32769' / encryptionKeyIndex' are appended by
BlockchainIdentity.privateKeyAtPath; keyId is the id of the identity's
ENCRYPTION/MEDIUM ECDSA key (id 2 in createIdentityPublicKeys), 32769' is
TxMetadataDocument.childNumber, encryptionKeyIndex is the app's per-document
counter (dash-sdk-kotlin 4.0.0-RC2).
- AES key bytes = KeyCrypterAESCBC.deriveKey(ecKey) = new KeyParameter(
ecKey.getPrivKeyBytes()) (raw scalar, no ECDH / no KDF); framing is
version(1) ‖ IV(16) ‖ AES-256-CBC/PKCS7(payload).
This is an exact match to Rust's identity_auth_derivation_path_for_type(ECDSA,
identity_index=0, key_index=keyId) extended by /32769'/encryptionKeyIndex': the
three legacy zeros correspond to [subfeature-auth, keytype=ECDSA=0,
identity_index=0]. No derivation-logic change is needed — verified by generating
the key AND a full encryptedMetadata blob with the real dashj stack for the
BIP-39 "abandon … about" mnemonic and asserting Rust reproduces the key and
decrypts the blob to the original plaintext.
- add legacy_dashj_wire_compat_vector: dashj-generated (key, blob, plaintext)
vector with full provenance, so the derivation prefix + envelope are now
CI-enforced rather than device-sample-confirmed.
- retarget the NIST test's doc comment as the narrower cipher-conformance leg
and drop the now-obsolete "cannot be reconstructed from the jars" caveat.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tnet docs + query diagnostics The on-device decrypt-proof reported `sdkFetched=0` with ZERO decrypt-skip warnings, i.e. the fetch query returned nothing while the legacy stack sees 2 encrypted `txMetadata` documents for the same owner. This isolates the FETCH half of `IdentityWallet::fetch_encrypted_documents` and pins it against the real documents so a wire-query regression is caught in CI, and adds an on-device breadcrumb to localize any future empty result to the query vs the decrypt stage. What this proves (executable evidence): the exact production query — `$ownerId == owner AND $updatedAt >= since_ms`, ordered `$updatedAt asc`, paginated — returns BOTH real testnet documents (owner 532rVHxLD6Z3MNiu5LZyNqn55Ybz4bydZozXU4cqqp1L, wallet-utils contract 7CSFGeF4WNzgDmx94zwvHkYaG3Dx4XEe5LFsFgJswLbm, type `txMetadata`, since_ms 0) fully materialized, with `keyIndex=2`, `encryptionKeyIndex=1`, `encryptedMetadata` 3585 bytes. This holds for the exact production shape (`&Identifier` owner value, `U64` since bound), with and without the range clause, across pinned platform versions 1..=12 and the default, and including the production `register_data_contract` step. The query construction, value encoding, contract resolution (7CSFGeF4… is the built-in wallet-utils system contract), and JNI param marshalling (`read_id32`, `sinceMs` jlong→u64) are therefore all wire-correct; the on-device empty result is not reproducible from the query and points outside it (e.g. a stale native lib). Changes: - extract the paginated wire query into `query_owned_encrypted_documents` (takes the `Sdk` + fetched contract, no resident wallet/identity), re-exported so the new testnet integration test drives the SAME code the FFI path runs. Query logic unchanged. - add `tests/txmetadata_fetch.rs` (`#[ignore]`, testnet): asserts the query returns the 2 documents and that each decodes `keyIndex`/`encryptionKeyIndex` (u32) + `encryptedMetadata` (bytes) — i.e. the pipeline reaches decrypt for both, without needing the owner mnemonic. - log `raw_count`/`materialized` at INFO before the decrypt loop, so the next `adb logcat` run during the probe pins an empty result to the query (raw_count=0), a proof-materialization gap (raw_count>0, materialized=0), or the decrypt/JSON stage (materialized>0) — no guessing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-document fetch path The dash-wallet decrypt probe reported sdkFetched=0 on-device with NO Rust breadcrumb in logcat — either the JNI call never reached Rust or platform-wallet INFO tracing is filtered from logcat. Make every stage of the fetch path provably visible at WARN: - rs-platform-wallet fetch_encrypted_documents: entry log (owner/contract b58, type, since_ms), warn on every early-return (contract fetch failure, contract-not-found, encryption-context resolution failure, query failure) and a final raw/decrypted count; query_owned_encrypted_documents gets an entry log and a fetch_many error log, and the raw_count/materialized breadcrumb is raised from info! to warn!. - rs-unified-sdk-jni documentFetchEncrypted: entry log (wallet_handle nonzero?, since_ms), parsed-args log (owner/contract hex, doc type), per-early-return warns, and a success log with the returned JSON size. - take_pwffi_error / throw_sdk_exception warn-log every native->Kotlin error conversion (raw + offset code, full message), so a contained exception still leaves a logcat trail. Diagnostic only — no behavior change; cargo check -p rs-unified-sdk-jni and clippy on both touched crates are clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…h log AND tracing so they reach Android logcat
The 2026-07 on-device forensic tap proved the two logging facades diverge on
Android: JNI_OnLoad installs android_logger as the global `log` logger
(logcat tag DashSDK), so the JNI layer's log::warn! lines were visible —
while the only tracing subscriber the Kotlin SDK installs
(dash_sdk_enable_logging, a tracing_subscriber::fmt layer) writes to STDOUT,
which Android discards. Every tracing::warn! breadcrumb in
fetch_encrypted_documents therefore never reached logcat, even at WARN.
Fix: a `breadcrumb()` helper in encrypted_document.rs emits each diagnostic
line through BOTH facades — `tracing` for host tests / desktop, `log` for
logcat — and every stage of the fetch path now uses it (entry, contract
fetch/not-found, encryption-context resolution, query entry, fetch_many
error, raw_count/materialized, per-document skips, final raw/decrypted
counts). The previously SILENT skip of a raw-but-unmaterialized document
(`let Some(doc) = maybe_doc else { continue }`) now leaves a trail too:
under proofs that shape is exactly what turns "2 documents exist" into an
empty result with no error.
Root-cause status of the on-device sdkFetched=0: NOT locally reproducible.
The device path was config-identical to the Mac repro (SdkBuilder::
new_testnet + TrustedHttpContextProvider::new(Testnet, None, 100), proofs on
by default, platform version auto, since_ms=0, contract registered with the
provider — the register step is now mirrored in tests/txmetadata_fetch.rs),
and that repro still returns raw_count=2 materialized=2 from this Mac. A
stale device lib is ruled out: the JNI warns visible in the tap were added
in d29d523, so the device ran current code. The next on-device tap will
pin the failing stage: query-empty (raw_count=0) vs materialization drop
(raw>0, NOT-materialized lines) vs decrypt skip (per-doc skip lines).
cargo test -p platform-wallet --lib: 427 passed; testnet integration test
txmetadata_fetch passes with the production-parity register step; clippy
clean on platform-wallet + rs-unified-sdk-jni.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…olver for external-signable wallets
The on-device decrypt-proof breadcrumbs delivered the verdict: the query was
never broken (raw=3 materialized=3), but every document skipped with
"txMetadata key derivation failed ... External signable wallet has no private
key". The app's SDK wallet is EXTERNAL-SIGNABLE — no private keys in the Rust
wallet; every key derives on demand through the registered mnemonic resolver
(the app's security architecture) — while derive_tx_metadata_key derived from
in-wallet private keys, which exist in test fixtures but never on-device. The
CREATE path had the identical flaw.
Fix, following the identity_key_preview / discovery capability convention:
- rs-platform-wallet: new derive_tx_metadata_key_from_master (same path,
caller-supplied master xprv; path construction shared via
tx_metadata_derivation_path so the two sources can never drift) and a
TxMetadataKeySource {ResidentWallet, Master} selector on both
create_encrypted_document_with_signer and fetch_encrypted_documents;
key-derivation breadcrumbs now name the active source.
- rs-platform-wallet-ffi: both encrypted-document entry points take a
nullable mnemonic_resolver_handle. Capability check under a short guard
(never held across the host resolver callback), resolver consulted ONLY
for external-signable / watch-only wallets (resident wallets keep the
historical in-process derive and skip the Keystore read), master scalar
wiped (non_secure_erase) before the result crosses back — atomic
derive + use + zeroize.
- rs-unified-sdk-jni + kotlin-sdk: documentCreateEncrypted /
documentFetchEncrypted and DocumentTransactions.createEncryptedDocument /
fetchEncryptedDocuments thread mnemonicResolverHandle through (the app
passes PlatformWalletManager.mnemonicResolverHandle, as discoverIdentities
already does).
Regression tests (network-free):
- master_derivation_matches_resident_wallet_derivation — both key sources
agree at every probed (identity, key, encryptionKey) slot;
- external_signable_wallet_derives_via_resolver_master — the device shape:
in-wallet derive fails with the exact no-private-key error, the
resolver-master path (stubbed with the test mnemonic) round-trips
seal/open against a resident wallet in both directions;
- legacy_dashj_wire_compat_vector now pins the resolver-master path to the
dashj-generated vector too (both sources hit the legacy key
byte-for-byte).
cargo test -p platform-wallet --lib: 429 passed; -p platform-wallet-ffi
--lib: 145 passed; clippy clean on all three crates; kotlin-sdk
:sdk:compileDebugKotlin green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…off the await; redact plaintext Debug; quiet breadcrumbs Addresses the latest review on #4091. Wire-compat vector (BLOCKING): the existing legacy_dashj_wire_compat_vector pinned identity_index=0, indistinguishable from KeyDerivationType::ECDSA=0 at the adjacent path slot, so it could not prove identity_index is wired correctly. Add legacy_dashj_wire_compat_vector_nonzero_identity_index, generated by the REAL legacy stack (dashj-core 22.0.3 + blockchainIdentityECDSADerivationPath / KeyCrypterAESCBC, run under a JVM) at identity_index=1 (m/9'/1'/5'/0'/0'/1'/2'/32769'/1'). Its key (8cda…5196) is provably distinct from the index-0 key (4a2e…84d7); both the resident-wallet and resolver-master derivations are asserted to hit it. The reproducible generator (LegacyKeyN.java) and a README are checked in under tests/legacy_wire_compat/ so the vector's provenance is independently verifiable. Master-key exposure across await (document.rs create+fetch): the resolved master xprv previously lived across the network broadcast/pagination awaits and was wiped only afterwards (skipped on panic/early-return). Create now derives the AES key + seals the wire blob SYNCHRONOUSLY (new IdentityWallet:: prepare_encrypted_txmetadata_properties) and wipes the master before the async broadcast, so no key material crosses the await. Fetch cannot pre-derive (per-doc keyIndex/encryptionKeyIndex are discovered during pagination), so the master is wrapped in a WipingMaster Drop guard that scrubs on every exit path, with the tradeoff documented. FFI decision tests (decide_key_source) cover null-handle, external-signable dispatch, and resolver-required. Hygiene: manual Debug impls redacting the decrypted payload on DecryptedEncryptedDocument and OpenedTxMetadata; transactions.rs no longer logs the raw mnemonic_resolver_handle pointer (nonzero=bool only); per-poll informational breadcrumbs downgraded warn!->debug! (genuine error/skip paths stay warn!), with the android_logger Info-level visibility implication noted so identity-correlated data stops reaching logcat on every successful fetch now that the sdkFetched=0 root cause is fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…deep-cloning it Addresses review nitpick bbb24591b025 on #4091. fetch_encrypted_documents wrapped the fetched DataContract in one Arc for the context provider (Arc::new(contract.clone())) and then moved the original into a SECOND Arc — a redundant deep clone of the whole contract (document-type/index metadata) on every fetch. Wrap once and hand the provider a cheap Arc::clone of the same handle. Verified: cargo test -p platform-wallet green (430 lib + 9 integration). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…'t decode Addresses review findings 0dd9fc55de07 / CodeRabbit a783199f on #4091. createEncryptedDocument bounded `version` to 0..255, but only 0 (CBOR) and 1 (protobuf) are wire-meaningful: seal_tx_metadata writes the byte verbatim into the envelope and the legacy dashj decryptTxMetadata switches on exactly those two values. Accepting 2..255 would silently seal a document the legacy stack cannot decode, breaking the bidirectional wire-compat guarantee this PR exists to establish. Tighten to `require(version == 0 || version == 1)` with a message that names both wire versions. Adds DocumentTransactionsVersionValidationTest pinning the rejection of 2..255 and negative bytes (the `require` runs before the native call, so the rejection paths are exercised on the JVM). Full :sdk unit suite green (113). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ore + JNI, not just Kotlin The version-byte wire-compat guard previously landed only as a Kotlin `require` (DocumentTransactions.kt); the JNI entry point still accepted the stale 0..=255 range and `seal_tx_metadata` wrote the byte verbatim, so a caller reaching the FFI/JNI directly could still seal a document with a version (2..=255) the legacy dashj `decryptTxMetadata` can't decode — silently breaking wire-compat (#4091, findings 9c0ce58c3bb7 and 79595960d201). - seal_tx_metadata now returns Result and rejects any version != 0 (CBOR) / 1 (protobuf) at the one choke point every layer (JNI, FFI, resident wallet) funnels through; the FFI create path propagates the error. Added seal_rejects_non_wire_versions (asserts 0/1 seal, 2..=255 rejected). - The JNI create entry point replaces the `0..=255` check with `0..=1` and a message naming both wire versions, failing fast before the native call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…el nonzero vector as internal slot check (#4091) The reviewer was right on both threads. The legacy dashj createTxMetadata flow has NO identity-index component — it always derives against the primary identity via blockchainIdentityECDSADerivationPath() (index 0) — so legacy wire-compat is only defined at identity_index=0, and no legacy wallet ever wrote a document keyed at a nonzero index. Apply option (a) — vector VALUES unchanged, provenance corrected: 1. legacy_dashj_wire_compat_vector (identity_index=0, 4a2e…84d7): document that the account prefix was verified against the REAL dashj DerivationPathFactory.blockchainIdentityECDSADerivationPath() (path m/9'/1'/5'/0'/0'/0'/keyId'/32769'/encryptionKeyIndex'), not mirrored back from Rust's own tx_metadata_derivation_path (finding dd246b5e17d0). 2. Rename legacy_dashj_wire_compat_vector_nonzero_identity_index -> nonzero_identity_index_derivation_slot_is_internally_consistent and rewrite its docs/asserts: the 8cda…5196 value is SELF-REFERENTIAL (LegacyKeyN.java hand-builds the same path Rust constructs; it does not call the real DerivationPathFactory), so it pins internal slot placement + resident/master agreement only — explicitly NOT a legacy wire-compat claim (finding 4c0754158cc6). 3. Add a doc note on derive_tx_metadata_key and the module header stating wire-compat holds only at identity_index=0. Correct LegacyKeyN.java's header/inline comments and the README to state the generator hand-builds the account path and that the nonzero vector is an internal consistency cross-check, not a legacy sample. cargo test -p platform-wallet --lib: 431 passed / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rifier Addresses blocker 989be307db0f on #4091 (its still-open core ask: "check in the actual JVM repro script/tool for independent verification"). b7319b5 corrected the vectors' provenance in prose (scoped wire-compat to identity_index=0; relabeled the nonzero vector as an internal slot check), but the "confirmed against the REAL dashj DerivationPathFactory" claim was still only asserted — LegacyKeyN.java hand-builds its account path and never drives the factory, so a maintainer could not reproduce the equality from checked-in code. Adds LegacyDerivationPathCheck.java: it drives the real org.bitcoinj.wallet.DerivationPathFactory (dashj-core 22.0.3, Testnet) and asserts its primary-identity path — blockchainIdentityECDSADerivationPath() (no-arg) = m/9'/1'/5'/0'/0'/0' — equals LegacyKeyN's hand-built account path at identity_index 0, printing WIRE_COMPAT_ANCHOR_OK = true (verified: true). It also prints the factory's INDEXED overload m/9'/1'/5'/0'/0'/0'/i' beside the hand-built nonzero path m/9'/1'/5'/0'/0'/i', making the shape difference visible so the nonzero vector is self-evidently NOT a factory-produced legacy sample (cross-refs dd246b5e17d0 / 4c0754158cc6). README: document the verifier + run command, and add the missing de.sfuhrm/saphir-hash-core/3.0.10 jar (TestNet3Params.get() needs X11 genesis-block hashing — the factory path fails with NoClassDefFoundError without it; LegacyKeyN alone never touches network params so it was omitted before). Verified end to end: LegacyDerivationPathCheck 0 -> WIRE_COMPAT_ANCHOR_OK=true; LegacyKeyN 0 2 1 -> 4a2e…84d7 and LegacyKeyN 1 2 1 -> 8cda…5196, both matching the hard-coded Rust vectors exactly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rdownGate
createEncryptedDocument and fetchEncryptedDocuments borrow the wallet /
mnemonic-resolver / signer handles but opened with plain
withContext(Dispatchers.IO), bypassing the TeardownGate: a concurrent wallet
shutdown could free the borrowed native handles mid-call (freed-handle UB) and
the source-scanning GateCoverageLintTest.everyHandleBorrowingSuspendFunIsGated
failed on both.
Both now open with gate.op { } like their six sibling document methods (gate.op
already runs the body on Dispatchers.IO, so the bodies are unchanged). Dropped
the now-unused Dispatchers/withContext imports. GateCoverageLintTest green; full
:sdk:testDebugUnitTest suite green (174 tests, 0 failures).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion/network
seal_tx_metadata had no client-side size check: a payload too large for the
encryptedMetadata field (maxItems 4096) derived the key and sealed only to be
rejected at broadcast with an opaque DPP schema error.
Add a typed PlatformWalletError::TxMetadataPayloadTooLarge { len, max } and a
shared ensure_tx_metadata_payload_fits() precheck. It runs FIRST in
prepare_encrypted_txmetadata_properties (before resolve/derive/broadcast) so an
over-large batch fails fast with the length + accepted max, and again inside
seal_tx_metadata as the choke-point last line of defense. The FFI maps the new
variant to ErrorInvalidParameter (already mirrored in Swift/Kotlin — no new
numeric code), so it surfaces sensibly across JNI/FFI with the typed Display.
True envelope math, derived from the code (not hardcoded): the blob is
version(1) + IV(16) + AES-256-CBC/PKCS7(plaintext). PKCS7 always adds a full
block when the plaintext is block-aligned, so ciphertext = 16*(L/16 + 1) and
blob = 17 + that. The largest ciphertext that fits 4096 is ((4096-17)/16)*16 =
254*16 = 4064, and since PKCS7 spends >=1 byte on padding the max plaintext is
one less -> MAX_TX_METADATA_PLAINTEXT_LEN = 4063. That plaintext frames to a
4081-byte blob (NOT 4096 as the review's arithmetic stated); 4064 jumps to 4097
and is the first rejected length. The 4063/4064 boundary itself matches the
reviewer; the "4063 -> 4096" envelope size does not (see the new boundary test,
which pins the real 4081-byte blob).
Boundary test seal_rejects_payload_above_size_limit: 4063 seals (blob == 4081,
round-trips), 4064 rejected as TxMetadataPayloadTooLarge, and the standalone
precheck agrees at the boundary.
Also strips the co-located "finding <hex>" tracker tokens from the comments in
these two files (rationale text kept); they move to the PR description.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…broadcast await platform_wallet_create_encrypted_document_with_signer copied the caller payload into a plain Vec<u8> that stayed live in the outer scope across the network broadcast .await — the native plaintext lingered in memory the whole time the document was being broadcast. Wrap the copy in Zeroizing<Vec<u8>> and make the with_item closure `move` so it OWNS the buffer, then drop(payload_vec) the instant the encrypted properties are prepared (right beside the existing master-key drop), before block_on_worker. The plaintext is now scrubbed and gone before any .await; only the sealed ciphertext properties cross into the async block. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gated check The txmetadata_fetch doc claimed the wire-query regression is "caught in CI (against testnet)", but the test is #[ignore = "hits testnet"] and nothing runs `--ignored`, so CI never executes it. Soften the wording: it is a MANUAL, testnet-gated check, run explicitly with `--ignored`, NOT part of the default `cargo test`/CI run and with no scheduled job running `--ignored` today — a local/pre-release regression gate. (Wiring a scheduled `--ignored` job is out of scope for this PR.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the "finding <hex>" / "blocker <hex>" internal tracking tokens from the remaining code comments and docs (the co-located tokens in tx_metadata.rs and encrypted_document.rs were stripped in the size-precheck commit). Rationale text and the #4091 issue reference are kept; the tracker refs belong in the PR description, not the source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tract schema Review round 2: the 4096 field limit was a local const silently duplicating the wallet-utils contract's encryptedMetadata maxItems; pin them together so a contract-side limit change fails a test instead of drifting past the size precheck. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the reviewer-requested independent check (#4186): decrypt a txMetadata blob produced by a REAL legacy dash-wallet 11.9 install, not one this repo generated. A designated-throwaway testnet wallet (DPNS name `yabba2`, identity ESR1nfF3bj4TR2ZkLmDuSeu6r7VzpTurYi47BV6XwsoP) running stock dash-wallet 11.9 registered a username, did a send + receive, saved metadata, and published one encrypted `txMetadata` document to Platform. It was fetched back off testnet and decrypted with the NEW Rust crypto. - `legacy_install_yabba2_wire_compat_vector` (network-free fixture in tx_metadata.rs): hard-codes the recovery phrase, the real captured blob hex (version 1/protobuf, keyIndex 2, encryptionKeyIndex 1), and the expected protobuf `TxMetadataBatch` plaintext (two items, memos "username"/"faucet", USD exchange rates), and asserts the new `derive_tx_metadata_key` + `open_tx_metadata` path decrypts it byte-for-byte via both the resident and resolver-master key sources. Doc-commented as the independent legacy-install vector, distinct from the self-generated dashj-core scratch vectors. - `capture_legacy_yabba2_txmetadata_blobs` (testnet-gated helper in tests/txmetadata_fetch.rs): resolves the DPNS name, runs the exact production query, derives from the phrase, and prints the capture used to build the fixture. - README: documents the new real-install vector alongside the JVM-generated ones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The JNI-owned `payload_bytes` in `documentCreateEncrypted` was a plain `Vec<u8>` held across the entire synchronous FFI call — including the network broadcast that runs inside it — and freed unscrubbed at end of scope. The inner FFI copy (`payload_vec` in rs-platform-wallet-ffi's document.rs) already got `Zeroizing` + an explicit pre-broadcast drop; mirror that discipline for the JNI copy so the plaintext-lifetime guarantee holds end-to-end. - Wrap `payload_bytes` in `zeroize::Zeroizing` so it is scrubbed on drop. - Drop it explicitly the instant the FFI call returns (the earliest point reachable from JNI, since the broadcast completes inside that call), before result/JSON handling. It is the only plaintext copy in the fn. Also strip the two surviving `#4091` tracker tokens from the version-guard and fetch-breadcrumb comments (rationale text kept). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, not the host Follow-up to #4186 (shumkov review on the encrypted-documents port): the encryptionKeyIndex allocation policy was left in the Kotlin host, which told callers to supply the legacy `1 + countAllRequests()` counter. Concurrent callers/devices could pick the same index, and a caller-side key-index policy loop violates the Kotlin SDK host-thin rule. Move the policy into Rust; hosts now provide only the opaque payload. Rust core (rs-platform-wallet): - IdentityWallet::allocate_encryption_key_index counts the identity's existing txMetadata documents on Platform and returns `1 + count`, matching dash-wallet's retired `1 + countAllRequests()` (SELECT COUNT(*) FROM transaction_metadata_platform) semantics EXACTLY (count+1, not max+1; empty state -> 1). - Allocation is serialized through a shared per-wallet allocator mutex (EncryptionKeyIndexAllocator on IdentityWallet, an Arc<Mutex<HashMap>> shared across handle clones): two concurrent creates through the SAME process seed the in-process high-water once from Platform and then hand out monotonically increasing indices, so they can never pick the same index. - Cross-device uniqueness is best-effort only and is NOT data-loss: every document stores its own keyIndex/encryptionKeyIndex and the reader derives each document's key from its own stored indices, so two documents sharing an index each carry a fresh IV and both decrypt independently. - Unit tests: legacy 1+count math (incl. saturation), empty-state seed + increment, per-owner isolation, and a concurrent no-collision test. FFI (rs-platform-wallet-ffi): - Add ABI-additive sibling platform_wallet_create_encrypted_document_with_signer_auto_index (identical params minus encryption_key_index); the existing explicit-index export is unchanged and both share one impl taking Option<u32>. When None, the index is allocated from Platform state before any key material is resolved. JNI (rs-unified-sdk-jni): - documentCreateEncrypted treats encryptionKeyIndex == -1 as the "let Rust allocate" sentinel (routes to the auto-index export); a non-negative value routes to the explicit export; < -1 is rejected. Kotlin SDK: - DocumentTransactions.createEncryptedDocument takes encryptionKeyIndex: Int? = null (null -> allocate in Rust); removed the `1 + countAllRequests()` guidance and deprecated the caller-supplied counter in KDoc. Null maps to the -1 JNI sentinel. - Tests: explicit-negative rejection and no-index-path acceptance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 2 (doc-only): the per-wallet mutex serializes cross-owner allocations during a first-time seed fetch, and a create that fails after allocating leaves an index gap, never a collision — both now stated at the allocator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing encryptionKeyIndex Addresses shumkov's #4195 review: on the auto-index create path the network count / high-water reservation (`allocate_encryption_key_index` -> `reserve_next_index`) ran BEFORE the deterministic 4063-byte payload gate, so an oversized payload — which must fail — still consumed an index and left a gap. Run the size check first. It is a pure, network-free bound (`ensure_tx_metadata_payload_fits` / `MAX_TX_METADATA_PLAINTEXT_LEN`) and needs no key material: - new `reserve_next_index_checked(allocator, owner, payload_len, seed)` runs `ensure_tx_metadata_payload_fits(payload_len)?` before `reserve_next_index`, so an oversized payload returns the typed `TxMetadataPayloadTooLarge` without polling the seed — the high-water is never seeded or advanced (no consumed index, no gap). - `IdentityWallet::allocate_encryption_key_index` gains a `payload_len` param and routes through the checked variant; the FFI auto-index create path passes `payload_vec.len()`. Explicit-index path is unchanged (it never allocates). - new unit test `oversized_payload_does_not_advance_highwater`: asserts the typed error, that the owner is absent from the allocator map, and that the next well-sized reservation still seeds at 1 (no gap). cargo test (platform-wallet allocator 5/5, platform-wallet-ffi 204/204) + clippy green; Kotlin :sdk:compileDebugKotlin + :sdk:testDebugUnitTest green (JNI/Kotlin ABI unchanged — payload_len plumbing is internal to Rust). Note (source-breaking, positional Kotlin callers): the #4186 stack moved `SDK.Documents.createEncryptedDocument`'s `encryptionKeyIndex` to the last positional slot (`Int? = null`). Positional callers must drop the argument (let Rust allocate) or switch to a named argument; named callers are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mechanical Swift wrappers for the encrypted-document C-ABI exports added by #4091 (port/v4.1/encrypted-documents). Re-stacked onto #4195 (followup/v4.1/keyindex-rust-allocation) per the reviewer sequencing decision so the Rust-side auto-index export is available: - platform_wallet_create_encrypted_document_with_signer_auto_index - platform_wallet_fetch_encrypted_documents Adds ManagedPlatformWallet.createEncryptedDocument(...) and .fetchEncryptedDocuments(...) to Sources/SwiftDashSDK/PlatformWallet, mirroring the existing createDocument (signer + byte-buffer marshalling, withExtendedLifetime pinning, result-code .check(), string_free) and previewIdentityRegistrationKeys (internal MnemonicResolver construction + pinning) patterns. The plaintext payload is handed straight to Rust's Zeroizing buffer with no extra Swift-side copy, as the neighboring seed path does. createEncryptedDocument now calls the AUTO-INDEX export and DROPS the host-supplied encryptionKeyIndex parameter: Rust allocates the per-document encryptionKeyIndex from authoritative Platform state (#4195), so hosts no longer assign it (host-side assignment risked cross-device collisions). This matches the Android auto-index path, where Kotlin's createEncryptedDocument omits the index (encryptionKeyIndex = null). The version-byte {0,1} guard and argument-order/nullability parity with the Kotlin counterpart are preserved; fetchEncryptedDocuments is unchanged. Updates EncryptedDocumentVersionValidationTests (the Swift mirror of the Kotlin DocumentTransactionsVersionValidationTest) to the new signature: the wire-meaningless version bytes (2/3/127/255) are rejected before any FFI dispatch. Verified: cbindgen regenerates the platform-wallet-ffi header with the auto-index export at the expected signature (no encryption_key_index arg); `swift build` of SwiftDashSDK type-checks the reworked call site against that header. `swift test` currently fails only at link because the checked-in prebuilt DashSDKFFI.xcframework static archive predates #4195 and lacks the auto-index symbol; a framework rebuild against #4195 resolves it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…crypt-plaintext-lifetime
Keep decrypted txMetadata payloads in zeroizing owners through AES, wallet, and FFI serialization, then release the shared C result through a dedicated zeroizing free on Kotlin/JNI and Swift. Preserve the existing JSON and host String APIs while documenting that runtime-managed host strings and parsed copies cannot be reliably scrubbed. Test would have caught this in CI: ✖ baseline lifetime assertions did not compile because decrypted payloads were plain Vec values and no sensitive decrypt primitive existed; ✔ the unchanged assertions and focused encryption, wallet, FFI, and JNI suites pass with zeroizing owners and sensitive release coverage.
…crypt-plaintext-lifetime
shumkov
requested review from
QuantumExplorer,
lklimek and
llbartekll
as code owners
July 28, 2026 05:37
This was referenced Aug 1, 2026
Open
Collaborator
Author
|
@QuantumExplorer — a scheduling ask before I start the encrypted-txMetadata chain. Plan is four owned same-repository layers replacing #4186 / #4195 / #4194, plus this PR rebuilt as a surgical delta. Where you're a hard dependency — three of the four layers touch
Since I'm authoring all of them, I can't self-approve, so those three need you specifically. Where you're not required — the Swift wrapper layer touches only So: three required approvals from you, each on a reconstructed Are you available over the next stretch? If not, the plan's stop condition says we don't start. |
6 tasks
Consolidate the shared encrypted-document flow for Rust, Kotlin/JNI, and Swift, including wire compatibility, automatic key-index allocation, deferred JNI payload ownership, and zeroizing FFI output ownership. Test would have caught this in CI:\n✖ unsupported envelope versions, early resolver calls, copied oversized plaintext, and non-zeroizing cipher state failed the new regression checks before the fixes.\n✔ the same regressions, legacy vectors and live fixture, host suites, Android ABI matrix, and Apple device/simulator builds now pass. Document the unavoidable terminal host String lifetime limitation symmetrically on both platforms.
shumkov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/rs-unified-sdk-jni/src/transactions.rs (1)
1455-1498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe source-text test pins formatting and comment text, not behavior.
production_jni_create_routes_through_one_deferred_compositereads its own file and matches literal source fragments. Two of the assertions break on changes that do not alter behavior:
- Line 1464 depends on the doc comment
/// Fetch + DECRYPT every encrypted wallet-contract documentstaying verbatim. Any edit to that sentence makes the split panic with "encrypted-create export must end before the fetch export".- Line 1487 requires the exact normalized fragment
payload_len, || match env.convert_byte_array(&payload). A rustfmt release that wraps the closure differently, or an inline comment between the two arguments, fails the test while the bridge is still correct.Consider keeping the call-count and negative assertions, and dropping the adjacency assertion at line 1487. Alternatively, mark the delimiter with a stable marker comment instead of prose.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-unified-sdk-jni/src/transactions.rs` around lines 1455 - 1498, The source-text test production_jni_create_routes_through_one_deferred_composite relies on unstable documentation and formatting. Replace the prose doc-comment delimiter with a stable source marker or another behavior-independent boundary, and remove the exact normalized payload_len closure assertion; retain the composite-call count, single array-conversion count, and negative allocator/second-create assertions.packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs (2)
584-642: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared body of the two encryption-context resolvers.
resolve_encryption_contextandresolve_encryption_context_blockingdiffer only in how they acquire the read guard. The remaining ~20 lines — wallet-info lookup, managed-identity lookup,identity_indexcheck, identity clone, wallet clone — are duplicated verbatim, including the error messages. A future change to one resolver can silently diverge from the other.Extract the post-lock logic into one private helper that takes the guard, then keep the two public entry points as thin wrappers.
♻️ Sketch of the shared helper
fn resolve_from_guard( &self, wm: &WalletManager<PlatformWalletInfo>, owner_identity_id: &Identifier, ) -> Result<(dpp::identity::Identity, u32, key_wallet::wallet::Wallet), PlatformWalletError> { // existing body, unchanged } async fn resolve_encryption_context(&self, owner: &Identifier) -> Result<..> { let wm = self.wallet_manager.read().await; self.resolve_from_guard(&wm, owner) } fn resolve_encryption_context_blocking(&self, owner: &Identifier) -> Result<..> { let wm = self.wallet_manager.blocking_read(); self.resolve_from_guard(&wm, owner) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs` around lines 584 - 642, Extract the duplicated wallet-info and managed-identity resolution logic from resolve_encryption_context and resolve_encryption_context_blocking into a private resolve_from_guard helper accepting the read guard and owner identity. Keep both existing resolvers as thin wrappers that acquire their respective async or blocking guard, delegate to the helper, and preserve all current return types and error behavior.
1030-1061: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe scan has no upper bound on pages or accumulated entries.
PaginationProgressstops a scan only when a cursor repeats. A source that keeps returning full pages with fresh cursors makes the loop run forever, andraw_docsgrows without bound in the process. Bothquery_owned_encrypted_documentscallers are affected: the fetch path andcount_owned_txmetadata_documents, which is on the create path.The stall detector already owns the "when do we stop" decision, so a maximum page count fits there.
issued_cursorsalso grows one entry per page, so the same cap bounds it.This needs a faulty or hostile Drive to trigger, so it is hardening rather than an active defect.
🛡️ Sketch of a page cap in `record_page`
impl PaginationProgress { + /// Upper bound on pages one scan may read. At `PAGE = 100` this covers + /// far more documents than any wallet holds, so reaching it means the + /// source is not behaving. + const MAX_PAGES: usize = 1_000; + fn record_page( &mut self, page_len: usize, page_limit: usize, last_id: Option<Identifier>, ) -> Result<NextPage, PlatformWalletError> { self.pages_read += 1; // A page the source could not fill is the last page. if page_len < page_limit { return Ok(NextPage::Done); } + + if self.pages_read >= Self::MAX_PAGES { + return Err(PlatformWalletError::EncryptedDocumentPaginationStalled { + pages: self.pages_read, + }); + }Also applies to: 1099-1146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs` around lines 1030 - 1061, Update PaginationProgress::record_page to enforce a maximum page count before accepting another full page, returning the established pagination-limit error when the cap is reached. Define or reuse a shared maximum so both query_owned_encrypted_documents callers, including count_owned_txmetadata_documents, stop consistently and issued_cursors/raw_docs remain bounded; preserve existing short-page completion and repeated-cursor stall behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- Around line 584-642: Extract the duplicated wallet-info and managed-identity
resolution logic from resolve_encryption_context and
resolve_encryption_context_blocking into a private resolve_from_guard helper
accepting the read guard and owner identity. Keep both existing resolvers as
thin wrappers that acquire their respective async or blocking guard, delegate to
the helper, and preserve all current return types and error behavior.
- Around line 1030-1061: Update PaginationProgress::record_page to enforce a
maximum page count before accepting another full page, returning the established
pagination-limit error when the cap is reached. Define or reuse a shared maximum
so both query_owned_encrypted_documents callers, including
count_owned_txmetadata_documents, stop consistently and issued_cursors/raw_docs
remain bounded; preserve existing short-page completion and repeated-cursor
stall behavior.
In `@packages/rs-unified-sdk-jni/src/transactions.rs`:
- Around line 1455-1498: The source-text test
production_jni_create_routes_through_one_deferred_composite relies on unstable
documentation and formatting. Replace the prose doc-comment delimiter with a
stable source marker or another behavior-independent boundary, and remove the
exact normalized payload_len closure assertion; retain the composite-call count,
single array-conversion count, and negative allocator/second-create assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 68d79281-7825-4b21-b8bc-f9cf19489357
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/TransactionsNative.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactionsEncryptionKeyIndexTest.ktpackages/rs-platform-encryption/Cargo.tomlpackages/rs-platform-encryption/src/aes.rspackages/rs-platform-wallet-ffi/src/document.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/runtime.rspackages/rs-platform-wallet-ffi/src/tx_metadata_json.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rspackages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rspackages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/tests/legacy_wire_compat/LegacyDerivationPathCheck.javapackages/rs-platform-wallet/tests/legacy_wire_compat/README.mdpackages/rs-platform-wallet/tests/txmetadata_fetch.rspackages/rs-unified-sdk-jni/src/support.rspackages/rs-unified-sdk-jni/src/transactions.rspackages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swiftpackages/swift-sdk/Sources/SwiftDashSDK/FFI/MnemonicResolverAndPersister.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/EncryptedDocumentVersionValidationTests.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-platform-encryption/Cargo.toml
- packages/rs-platform-wallet/src/wallet/platform_wallet.rs
- packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyDerivationPathCheck.java
- packages/rs-platform-wallet/tests/legacy_wire_compat/README.md
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt
- packages/rs-platform-encryption/src/aes.rs
- packages/rs-platform-wallet-ffi/src/tx_metadata_json.rs
Collaborator
Author
|
Fresh CodeRabbit nitpick disposition on
All required CI is green on the current head, all inline threads are resolved, and PastaClaw re-review has been requested to clear its stale pre-fix |
Collaborator
Author
|
@thepastaclaw please re-review current head |
Expose platform-wallet code 2 as a specific InvalidParameter subtype while preserving Generic matching, nativeCode, and the Rust-owned message for existing callers. Test would have caught this in CI: ✖ the regression did not compile because PlatformWallet.InvalidParameter was absent; ✔ the targeted and full Kotlin SDK unit suites pass with typed and Generic-compatible mapping.
thepastaclaw
suggested changes
Aug 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Carried forward: the payload-size pre-copy blocker and sensitive out-pointer blocker are fixed at the current head, while the prior JNI guard test-coverage suggestion remains valid because the added tests do not verify which free function is called. Genuinely new: the create orchestration materializes native plaintext before potentially blocking wallet/key resolution. That new in-scope security issue is blocking, so changes are required.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/document.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/document.rs:1051-1064: Resolve the encryption context before materializing plaintext
`settle_index_and_materialize_payload` has already invoked the deferred JNI callback and returned an owned `Zeroizing<Vec<u8>>` before `tx_metadata_key_master_for_wallet` acquires wallet state and may synchronously invoke the host mnemonic resolver. The Kotlin resolver performs blocking DataStore/Keystore retrieval, and the Swift resolver reads Keychain data that may require authentication, so this stage can stall while the new native plaintext copy remains resident. Resolver failure, missing identity context, or encryption-key selection failure also discards a copy that never needed to be created. Refactor the sequence to preflight, settle the index, resolve the wallet identity and key source, materialize the payload, seal and drop all secrets, then broadcast. Add an ordering regression proving the materializer is not invoked when context or resolver resolution fails.
Keep AES plaintext staging in a pre-sized Zeroizing allocation, settle key resolution before native materialization, restore runtime failure coverage, and include both previously skipped crates in Rust CI. Test would have caught this in CI: ✖ before fix, ✔ after.
Collaborator
Author
|
@thepastaclaw please re-review current head e1341b4. The remaining blocker is fixed in the shared Rust orchestration: index resolution, key-source resolution, then plaintext materialization, seal/drop, and broadcast. The original thread now has exact test evidence and is resolved; local affected suites are green. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt (1)
66-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the repository EditorConfig indentation to all changed Kotlin code.
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt#L66-L88: re-indent the changed documentation andInvalidParameterdeclaration with 2 spaces per level.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt#L193-L193: re-indentGenericwith 2 spaces per level.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt#L244-L244: re-indent the code-2 mapping with 2 spaces per level.packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt#L49-L74: re-indent the added tests with 2 spaces per level.As per coding guidelines: “Follow repository EditorConfig settings: 2-space indentation, 4 spaces for Rust files, LF line endings, UTF-8 encoding, and a final newline.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt` around lines 66 - 88, Apply the repository’s 2-space indentation to all changed Kotlin code: re-indent the documentation and InvalidParameter declaration in packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt lines 66-88, Generic at line 193, the code-2 mapping at line 244, and the added tests in packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt lines 49-74; preserve LF endings and the final newline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt`:
- Around line 66-88: Apply the repository’s 2-space indentation to all changed
Kotlin code: re-indent the documentation and InvalidParameter declaration in
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
lines 66-88, Generic at line 193, the code-2 mapping at line 244, and the added
tests in
packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
lines 49-74; preserve LF endings and the final newline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1a72142-269a-4153-bcf9-918e7b590ef9
📒 Files selected for processing (8)
.github/workflows/tests-rs-wallet.yml.github/workflows/tests-rs-workspace.ymlpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/rs-platform-encryption/src/aes.rspackages/rs-platform-wallet-ffi/src/document.rspackages/rs-platform-wallet-ffi/src/runtime.rspackages/rs-unified-sdk-jni/src/transactions.rs
💤 Files with no reviewable changes (1)
- packages/rs-unified-sdk-jni/src/transactions.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-platform-wallet-ffi/src/runtime.rs
- packages/rs-platform-wallet-ffi/src/document.rs
…ngle-pr-integration
The latest v4.2-dev event callback ABI adds an optional release callback. Keep the txMetadata resolver fixture aligned with the null-context callback pattern used by the updated base. Test would have caught this in CI: ✖ missing release_fn before fix, ✔ 249/249 after.
Collaborator
Author
|
@thepastaclaw please re-review current head |
The workflow intentionally runs the JNI test suite, but including the pre-existing host binding crate in the Rust coverage report expands the project denominator without a comparable base report. Align JNI with the repository's existing FFI exclusions while preserving all Rust and Kotlin test execution. Test would have caught this in CI: ✖ codecov/project at -2.73% before fix, ✔ project and patch recompute above their configured thresholds after exclusion.
thepastaclaw
suggested changes
Aug 3, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Carried forward: the payload-size pre-copy and sensitive out-pointer findings are fixed, while the encryption-context ordering blocker and JNI sensitive-release test suggestion remain valid at the current head. The latest PR-owned commits add no genuinely new finding: e1341 moves wallet capability and optional host-master lookup before materialization, but leaves owner-identity resolution, key selection, and AES derivation afterward; the event-fixture and Codecov changes introduce no additional issue.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Resolve the managed owner, select the encryption key, and derive the zeroizing AES context before either host path materializes plaintext. Preserve the legacy wire API through a prepared context that draws a fresh IV for every seal. Test would have caught this in CI: ✖ before fix, the missing-owner path materialized once before IdentityNotFound; ✔ after fix, it returns IdentityNotFound with zero materializations.
Collaborator
Author
|
@thepastaclaw please re-review current head
RED→GREEN proof: the production-wired FFI regression Final local gates: platform-wallet 557/557, platform-wallet-ffi 250/250, JNI 50/50, focused prepared-context 4/4, exact blocker regression 1/1, formatting and diff checks green. The live |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs (1)
790-804: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the sync-only contract on this public method.
prepare_txmetadata_encryptioncallsresolve_encryption_context_blocking, which usesblocking_read. A call from inside an async task panics. The doc comment onprepare_encrypted_txmetadata_properties(Lines 842-845) states this constraint, but the new public entry point does not. Add the same warning so a host bridge does not reach the panic through the new API.♻️ Proposed doc addition
/// The returned context contains the selected identity key id and a /// zeroizing per-document AES key. A host bridge can therefore finish this /// operation, release any master xprv used to derive it, and only then /// materialize the payload for [`PreparedTxMetadataEncryption::seal`]. + /// + /// **Crosses no `.await`** and resolves through `blocking_read`. Call it + /// from a sync context only; a call inside an async task panics. pub fn prepare_txmetadata_encryption(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs` around lines 790 - 804, Update the public method prepare_txmetadata_encryption documentation to explicitly warn that it is synchronous-only and must not be called from an async task because its blocking_read-based resolution can panic. Match the existing constraint wording and guidance documented on prepare_encrypted_txmetadata_properties.packages/rs-platform-wallet-ffi/src/types.rs (1)
370-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a non-null case for
platform_wallet_sensitive_string_free.The test only covers the null pointer. The zeroizing free path for an owned string stays untested. Add a test that transfers a real
CStringand frees it, so the non-null branch runs under Miri and the ASan builds.#[test] fn should_free_a_non_null_sensitive_string() { let raw = std::ffi::CString::new("sensitive-json").expect("no interior NUL").into_raw(); unsafe { platform_wallet_sensitive_string_free(raw); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-ffi/src/types.rs` around lines 370 - 375, The tests around platform_wallet_sensitive_string_free currently cover only a null pointer; add a separate test that creates a valid CString, transfers ownership with into_raw, and passes the raw pointer to platform_wallet_sensitive_string_free so the non-null zeroizing free path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/rs-platform-wallet-ffi/src/types.rs`:
- Around line 370-375: The tests around platform_wallet_sensitive_string_free
currently cover only a null pointer; add a separate test that creates a valid
CString, transfers ownership with into_raw, and passes the raw pointer to
platform_wallet_sensitive_string_free so the non-null zeroizing free path is
exercised.
In
`@packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- Around line 790-804: Update the public method prepare_txmetadata_encryption
documentation to explicitly warn that it is synchronous-only and must not be
called from an async task because its blocking_read-based resolution can panic.
Match the existing constraint wording and guidance documented on
prepare_encrypted_txmetadata_properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d5c0224-a752-441b-8c9a-4b8a255a868d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.codecov.ymlpackages/rs-platform-wallet-ffi/src/document.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/types.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rs-platform-wallet/Cargo.toml
- packages/rs-platform-wallet/src/lib.rs
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
- packages/rs-platform-wallet/src/error.rs
- packages/rs-platform-wallet-ffi/src/document.rs
Document that prepared encryption uses blocking_read and must be called from synchronous context. Tests not run: rustdoc-only clarification with no behavior change; cargo fmt --check and git diff --check pass.
Collaborator
Author
|
CodeRabbit disposition on final head
There are no unresolved review threads. @thepastaclaw please target the current head |
thepastaclaw
suggested changes
Aug 3, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
Carried forward: the payload pre-copy, sensitive out-pointer, and encryption-context ordering blockers remain fixed at the current head, while the JNI sensitive-pointer guard still lacks a callback-selection regression test. Genuinely new: the public synchronous txMetadata preparation and decryption APIs can panic when called from a Tokio task because they use blocking_read. Focused FFI ordering tests and the current JNI ownership test pass, but the public panic path requires changes.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:763-768: Return an error instead of panicking in public synchronous wallet APIs
`resolve_encryption_context_blocking` calls `tokio::sync::RwLock::blocking_read`, which Tokio implements by entering a blocking region and panics when invoked on a runtime worker. This helper is reached by the public `prepare_txmetadata_encryption`, `prepare_encrypted_txmetadata_properties`, and non-empty `decrypt_fetched_documents` paths, so a direct Rust SDK consumer calling them from an ordinary async task gets a panic instead of the declared `Result<_, PlatformWalletError>`; with `panic = "abort"`, that becomes immediately fatal. The rustdoc warning documents the failure but does not make the public API safe. Keep the blocking bridge private to the FFI-only synchronous path and provide an async public API, or acquire the lock with `try_read` and return a typed busy/retry error rather than panicking.
Comment on lines
+763
to
+768
| fn resolve_encryption_context_blocking( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| ) -> Result<(dpp::identity::Identity, u32, key_wallet::wallet::Wallet), PlatformWalletError> | ||
| { | ||
| let wm = self.wallet_manager.blocking_read(); |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Return an error instead of panicking in public synchronous wallet APIs
resolve_encryption_context_blocking calls tokio::sync::RwLock::blocking_read, which Tokio implements by entering a blocking region and panics when invoked on a runtime worker. This helper is reached by the public prepare_txmetadata_encryption, prepare_encrypted_txmetadata_properties, and non-empty decrypt_fetched_documents paths, so a direct Rust SDK consumer calling them from an ordinary async task gets a panic instead of the declared Result<_, PlatformWalletError>; with panic = "abort", that becomes immediately fatal. The rustdoc warning documents the failure but does not make the public API safe. Keep the blocking bridge private to the FFI-only synchronous path and provide an async public API, or acquire the lock with try_read and return a typed busy/retry error rather than panicking.
source: ['codex']
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Legacy Dash Wallet installations published encrypted
txMetadatadocuments that current Kotlin and Swift SDKs must be able to create and read wire-compatibly. The original work was split across #4186, #4195, #4194, and this PR, while #4264 later reconstructed only the foundation layer. Those heads did not form one independently mergeable, fully reviewed state.This PR now consolidates that merge intent directly onto
v4.2-dev, while preserving the substantive contributor commits and authorship from the original chain. It also closes the native plaintext-lifetime gaps shared by both hosts. #4185 and rust-dashcore#916 remain a separate dependency chain and do not block this PR.What was done?
Stringresidual symmetrically: runtime-managed strings and caller-ownedByteArray/Datacannot be reliably scrubbed by the SDK and must not be logged or retained.PlatformWallet.InvalidParameterwhile retainingGeneric(nativeCode = 2)catch compatibility and the Rust-owned message.After this PR lands, #4186, #4195, #4194, and the interim foundation reconstruction #4264 must not be merged separately; they can be closed as superseded.
How Has This Been Tested?
Red → green regression evidence:
aes/cbczeroization features and now prove both encryptor and decryptor implementZeroizeOnDrop.PlatformWallet.InvalidParameterwas absent; it now maps platform-wallet code 2 to the typed subtype while still satisfying existingGenericbranches and preserving the core message.Final verification:
Codecov excludes Rust host-binding crates by repository policy; JNI/FFI assurance comes from the dedicated Rust suites and Kotlin/Swift host workflows, not the patch percentage.
cargo fmt --all -- --checkandgit diff --check— passed.cargo test -p platform-encryption— 20 passed.cargo test -p platform-wallet --lib— 536 passed.cargo test -p platform-wallet --test txmetadata_fetch— 1 network test intentionally ignored by default; the explicit ignored/live run passed against testnet.cargo test -p platform-wallet-ffi --lib— 249 passed.cargo test -p rs-unified-sdk-jni --lib— 50 passed.Targeted
cargo clippy --all-targets— passed; 13 pre-existing warnings remain outside PR-owned code.Clean Android release build/verification for arm64-v8a and x86_64 — passed; exact
nmchecks found both create/fetch JNI exports. Kotlin debug assembly and unit tests passed. No Android device was attached, so connected-device tests were unavailable.Clean Apple release builds for iOS device, iOS simulator, and macOS, XCFramework generation, and SwiftExampleApp warnings-as-errors build — passed. Exact header and binary checks found all four public C exports on both iOS slices and confirmed the Rust-only deferred JNI helper is not exported.
Swift package tests — 283 executed, 8 intentionally skipped integration tests, 0 failures. SwiftExampleApp simulator unit tests also passed.
Three independent final reviews covered shared security/correctness, Kotlin/Swift host parity, and Swift/Rust FFI ownership; all must-fixes were folded and the final reviewers reported no remaining blockers.
Breaking Changes
Rust source compatibility:
OpenedTxMetadata.payloadandDecryptedEncryptedDocument.payloadchange fromVec<u8>toZeroizing<Vec<u8>>. The C fetch signature, JNI descriptor, Kotlin/Swift signatures, and JSON shape remain unchanged; the sensitive-free and encrypted-document functions are ABI-additive.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit