Skip to content

feat(sdk): add hardened encrypted txMetadata documents - #4264

Open
shumkov wants to merge 22 commits into
v4.2-devfrom
fix/txmetadata-encrypted-document-foundation
Open

feat(sdk): add hardened encrypted txMetadata documents#4264
shumkov wants to merge 22 commits into
v4.2-devfrom
fix/txmetadata-encrypted-document-foundation

Conversation

@shumkov

@shumkov shumkov commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

Android clients can now create and fetch legacy-compatible encrypted txMetadata through the shared Rust wallet path without depending on the retired dashj Platform stack. This is the owned replacement for #4186 because its contributor branch is not maintainable by us; the first 21 commits preserve bfoss765 as author and replay source head 6f7abba onto v4.2-dev.

Related: #4086, #4091, #4186, #4195

What was done?

  • Added the txMetadata envelope, key derivation, create/broadcast, paginated fetch/decrypt, C FFI, JNI, and host-thin Kotlin APIs. The Rust core remains the source of truth for supported wire versions and payload bounds.
  • Made external-signable wallets derive through the mnemonic resolver while scrubbing resolved master material and plaintext native copies at the earliest safe point. Teardown gating prevents borrowed native handles from being freed during Kotlin calls.
  • Hardened the boundary behavior: invalid inputs fail before signer, resolver, allocation, or network work; output pointers are null-published before fallible work; runtime startup can be retried; worker failures use bounded stable messages; and Kotlin's typed invalid-parameter error remains compatible with callers matching the existing Generic fallback.
  • Kept successful-device logs free of identity-correlating identifiers, caller-controlled document types, raw handles, plaintext, and raw native error bodies.
  • Pinned wire compatibility with network-free dashj/JVM vectors and a captured encrypted blob produced by a stock dash-wallet 11.9 testnet install. The manual testnet fetch helper no longer contains the throwaway recovery phrase or prints decrypted material.

Stack sequencing: #4195's allocator work will be replaced by U3 on top of this branch, followed by the Swift surface and the shared decrypt-output lifetime fix. Merge the stack in order and do not cut a v4.2 release between this foundation and the decrypt-output fix; this PR still returns decrypted payloads as base64 JSON through a normal CString.

How Has This Been Tested?

  • Red-to-green regressions demonstrated that the previous code reached resolver/context work before rejecting unsupported versions, retained failed runtime initialization, copied oversized JNI payloads, and broke Kotlin Generic fallback matching. Each now fails before the protected operation or preserves compatibility as intended.
  • platform-wallet library tests: 511 passed.
  • platform-wallet-ffi library tests: 221 passed.
  • rs-unified-sdk-jni library tests: 49 passed.
  • Kotlin debug assembly and unit tests: 188 passed, with no failures or skips.
  • cargo fmt --all -- --check and git diff --check passed.
  • cargo clippy for all targets in the three touched Rust crates completed successfully; 13 existing warnings remain in unrelated code and none are introduced in the touched U2 files.
  • Android NDK r28b x86_64 dev build with ABI verification passed: 227 JNI exports, one anchored C ABI export, and zero misaligned LOAD segments.
  • The ignored testnet integration target compiles, and the real-install legacy vector runs network-free in the library suite. The live testnet check was not run.
  • Android instrumented tests were not run because this machine has no installed AVD or connected device.

Independent correctness, FFI/security, and Kotlin compatibility reviewers approved the final diff after their must-fixes were folded in.

Breaking Changes

None. The new encrypted-document surface and error subtype are additive; PlatformWallet.InvalidParameter continues to satisfy existing PlatformWallet.Generic matching and reports native code 2.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • New Features
    • Added support for creating and retrieving encrypted documents.
    • Encrypted document retrieval decrypts matching records and supports timestamp-based queries.
    • Added compatible transaction-metadata encryption with wallet-based key handling.
  • Bug Fixes
    • Added clearer invalid-parameter errors for unsupported versions and oversized payloads.
    • Improved handling of runtime and worker failures without exposing sensitive data.
  • Compatibility
    • Added legacy-compatible encryption and derivation behavior for existing transaction metadata.

bfoss765 and others added 22 commits August 1, 2026 17:43
…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>
(cherry picked from commit ee6546a)
…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>
(cherry picked from commit 2571bbb)
…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>
(cherry picked from commit dfc3c23)
…-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>
(cherry picked from commit edaa67f)
…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>
(cherry picked from commit bcafc60)
…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>
(cherry picked from commit 8b4290b)
…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>
(cherry picked from commit ebfc9c4)
…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>
(cherry picked from commit 988368a)
…'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>
(cherry picked from commit 87b6f37)
…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>
(cherry picked from commit 7f8ac4d)
…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>
(cherry picked from commit 8a74912)
…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>
(cherry picked from commit 5e5220d)
…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>
(cherry picked from commit 194ec86)
…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>
(cherry picked from commit d456822)
…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>
(cherry picked from commit 0c5527a)
…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>
(cherry picked from commit c8063e4)
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>
(cherry picked from commit 9af3ece)
…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>
(cherry picked from commit 5accc5d)
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>
(cherry picked from commit 64bec68)
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>
(cherry picked from commit 983154d)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 6f7abba)
Make the shared Rust/FFI path reject invalid inputs before signer access, host copies, resolver/network work, and recover cleanly after runtime startup failures. Preserve Kotlin Generic fallback compatibility while exposing typed invalid-parameter failures, and keep the legacy wallet vector network-free.

Test would have caught this in CI: ✖ the old path resolved or signed before rejecting unsupported versions, retained a sticky runtime-init failure, copied oversized JNI payloads, and broke Generic fallback matching; ✔ regression tests pass after shared early gates, retryable runtime initialization, pre-copy bounds checks, and compatible subtype mapping.
@github-actions github-actions Bot added this to the v4.2.0 milestone Aug 1, 2026
@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds legacy-compatible encrypted transaction-metadata documents. It implements key derivation, encryption, decryption, paginated queries, Rust FFI, JNI and Kotlin APIs, typed validation errors, and failure-handling tests.

Changes

Encrypted document support

Layer / File(s) Summary
Transaction metadata cryptography
packages/rs-platform-wallet/src/wallet/identity/crypto/..., packages/rs-platform-wallet/tests/legacy_wire_compat/*
Adds HD key derivation, AES-256-CBC sealing and opening, version and payload validation, redacted metadata types, and legacy compatibility checks.
Encrypted document wallet operations
packages/rs-platform-wallet/src/wallet/identity/network/..., packages/rs-platform-wallet/tests/txmetadata_fetch.rs
Adds encrypted document preparation, decryption, pagination, cursor handling, exports, and a Testnet integration test.
Native FFI and runtime handling
packages/rs-platform-wallet-ffi/src/*, packages/rs-platform-wallet-ffi/Cargo.toml
Adds encrypted document FFI operations, base64 JSON payloads, sensitive-data cleanup, typed validation mapping, and fallible worker execution.
JNI and Kotlin integration
packages/rs-unified-sdk-jni/src/*, packages/kotlin-sdk/sdk/src/main/kotlin/...
Adds encrypted document JNI methods, argument validation, JSON conversion, Kotlin transaction methods, and bounded error breadcrumbs.
Typed Kotlin error mapping
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/*, packages/kotlin-sdk/sdk/src/test/kotlin/...
Adds PlatformWallet.InvalidParameter and tests its native-code, inheritance, and message behavior.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant KotlinClient
  participant TransactionsNative
  participant JNITransactions
  participant PlatformWalletFFI
  participant IdentityWallet
  KotlinClient->>TransactionsNative: createEncryptedDocument or fetchEncryptedDocuments
  TransactionsNative->>JNITransactions: invoke encrypted JNI method
  JNITransactions->>PlatformWalletFFI: validate and call encrypted document FFI
  PlatformWalletFFI->>IdentityWallet: create or query encrypted documents
  IdentityWallet-->>PlatformWalletFFI: encrypted or decrypted document data
  PlatformWalletFFI-->>JNITransactions: JSON result
  JNITransactions-->>TransactionsNative: canonical JSON
  TransactionsNative-->>KotlinClient: document JSON
Loading

Possibly related issues

Possibly related PRs

  • dashpay/platform#4243 — Overlaps in encrypted txMetadata document fetching, decryption, FFI serialization, and cleanup.

Suggested reviewers: quantumexplorer, lklimek

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the PR's main change: hardened encrypted txMetadata document support in the SDK.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/txmetadata-encrypted-document-foundation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

⛔ Blockers found — Sonnet deferred (commit bf88c92)
Canonical validated blockers: 1

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs (1)

118-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the review-conversation references from the permanent doc.

Lines 121-122 and the test doc at lines 477-481 cite "the reviewer's figure" and "the review". A reader of the merged code has no access to that conversation. State the arithmetic without referring to the review thread.

📝 Proposed doc edit
 /// Note the envelope for the maximum plaintext is 4081 bytes, not 4096: the
 /// gap 4082..=4096 is unreachable because the next plaintext byte (4064) forces
-/// a fresh padding block that jumps straight to 4097. (The 4063/4064 boundary
-/// itself matches the reviewer's figure; the "4063 → 4096" envelope size in the
-/// review does not — see the module tests, which pin the real 4081-byte blob.)
+/// a fresh padding block that jumps straight to 4097. The module tests pin the
+/// real 4081-byte blob.
🤖 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/crypto/tx_metadata.rs` around
lines 118 - 122, Update the documentation comment near the maximum plaintext
envelope and the related test documentation to remove references to the
reviewer, review, or review conversation. Preserve the arithmetic explanation
and the stated 4081-byte maximum, expressing the 4063/4064 boundary and
unreachable 4082..=4096 range as standalone technical facts.
packages/rs-unified-sdk-jni/src/support.rs (1)

101-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider pub(crate) for the breadcrumb helpers.

platform_wallet_error_breadcrumb and thrown_exception_breadcrumb are internal diagnostic seams. Only this module and its tests call them. pub adds them to the crate's public surface, and the tests reach private items through super:: already, as net_from_ord shows at Line 184.

♻️ Proposed visibility narrowing
-pub fn platform_wallet_error_breadcrumb(platform_wallet_code: i32, _message: &str) -> String {
+pub(crate) fn platform_wallet_error_breadcrumb(platform_wallet_code: i32, _message: &str) -> String {
-pub fn thrown_exception_breadcrumb(code: i32, _message: &str) -> String {
+pub(crate) fn thrown_exception_breadcrumb(code: i32, _message: &str) -> String {
🤖 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/support.rs` around lines 101 - 116, Change
the visibility of platform_wallet_error_breadcrumb and
thrown_exception_breadcrumb from pub to pub(crate), keeping their signatures and
behavior unchanged so they remain accessible within the crate and existing
tests.
🤖 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.

Inline comments:
In
`@packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- Around line 597-639: Bound the pagination loop in the document-fetching flow
around Document::fetch_many by tracking the number of pages and enforcing a
maximum page limit, while also verifying that the next StartAfter cursor
advances beyond the previous cursor. Stop or return an appropriate error when
either guard detects no progress or the page limit is reached, preventing
repeated full pages from growing raw_docs indefinitely.

---

Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs`:
- Around line 118-122: Update the documentation comment near the maximum
plaintext envelope and the related test documentation to remove references to
the reviewer, review, or review conversation. Preserve the arithmetic
explanation and the stated 4081-byte maximum, expressing the 4063/4064 boundary
and unreachable 4082..=4096 range as standalone technical facts.

In `@packages/rs-unified-sdk-jni/src/support.rs`:
- Around line 101-116: Change the visibility of platform_wallet_error_breadcrumb
and thrown_exception_breadcrumb from pub to pub(crate), keeping their signatures
and behavior unchanged so they remain accessible within the crate and existing
tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 48d089a2-0ca2-415f-9837-b19462d17093

📥 Commits

Reviewing files that changed from the base of the PR and between ed4116b and bf88c92.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (22)
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/TransactionsNative.kt
  • packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt
  • packages/rs-platform-wallet-ffi/Cargo.toml
  • packages/rs-platform-wallet-ffi/src/document.rs
  • packages/rs-platform-wallet-ffi/src/error.rs
  • packages/rs-platform-wallet-ffi/src/lib.rs
  • packages/rs-platform-wallet-ffi/src/runtime.rs
  • packages/rs-platform-wallet/Cargo.toml
  • packages/rs-platform-wallet/src/error.rs
  • packages/rs-platform-wallet/src/lib.rs
  • packages/rs-platform-wallet/src/wallet/identity/crypto/mod.rs
  • packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
  • packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyDerivationPathCheck.java
  • packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyKeyN.java
  • packages/rs-platform-wallet/tests/legacy_wire_compat/README.md
  • packages/rs-platform-wallet/tests/txmetadata_fetch.rs
  • packages/rs-unified-sdk-jni/src/support.rs
  • packages/rs-unified-sdk-jni/src/transactions.rs

Comment on lines +597 to +639
loop {
let query = dash_sdk::platform::DocumentQuery {
select: dash_sdk::drive::query::SelectProjection::documents(),
data_contract: Arc::clone(&contract),
document_type_name: document_type_name.to_string(),
where_clauses: vec![
WhereClause {
field: "$ownerId".to_string(),
operator: WhereOperator::Equal,
value: platform_value!(owner_identity_id),
},
WhereClause {
field: "$updatedAt".to_string(),
operator: WhereOperator::GreaterThanOrEquals,
value: platform_value!(since_ms),
},
],
group_by: vec![],
having: vec![],
order_by_clauses: vec![OrderClause {
field: "$updatedAt".to_string(),
ascending: true,
}],
limit: PAGE,
start: start.clone(),
};

let page = Document::fetch_many(sdk, query).await.map_err(|e| {
breadcrumb_error("query_owned_encrypted_documents: fetch_many failed error_kind=sdk");
PlatformWalletError::Sdk(e)
})?;
let page_len = page.len();
let last_id = page.keys().last().copied();
raw_docs.extend(page);

if page_len < PAGE as usize {
break;
}
match last_id {
Some(id) => start = Some(Start::StartAfter(id.to_buffer().to_vec())),
None => break,
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Bound the pagination loop.

The loop continues while a page returns exactly PAGE entries and its last key is Some. The cursor advances only if the server honors StartAfter. If a node returns the same full page repeatedly, the loop never ends and raw_docs grows without limit, which hangs the call and exhausts memory.

Add a progress check on the cursor and a maximum page count.

🛡️ Proposed guard
     const PAGE: u32 = 100;
+    // A node that ignores `StartAfter` would otherwise spin forever and grow
+    // `raw_docs` without limit.
+    const MAX_PAGES: usize = 1_000;
     breadcrumb(&format!(
         "query_owned_encrypted_documents: entry since_ms={since_ms}"
     ));
     let mut raw_docs: Vec<(Identifier, Option<Document>)> = Vec::new();
     let mut start: Option<Start> = None;
+    let mut pages = 0usize;
     loop {
+        pages += 1;
+        if pages > MAX_PAGES {
+            breadcrumb_error(
+                "query_owned_encrypted_documents: page limit reached; stopping the scan",
+            );
+            break;
+        }
         match last_id {
-            Some(id) => start = Some(Start::StartAfter(id.to_buffer().to_vec())),
+            Some(id) => {
+                let next = Start::StartAfter(id.to_buffer().to_vec());
+                if Some(&next) == start.as_ref() {
+                    breadcrumb_error(
+                        "query_owned_encrypted_documents: cursor did not advance; stopping the scan",
+                    );
+                    break;
+                }
+                start = Some(next);
+            }
             None => break,
         }
🤖 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 597 - 639, Bound the pagination loop in the document-fetching flow
around Document::fetch_many by tracking the number of pages and enforcing a
maximum page limit, while also verifying that the next StartAfter cursor
advances beyond the previous cursor. Stop or return an appropriate error when
either guard detects no progress or the page limit is reached, preventing
repeated full pages from growing raw_docs indefinitely.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preliminary review — Codex only

The txMetadata foundation is well layered and its core cryptographic compatibility is extensively tested, but the C ABI still lets a non-derivable high-bit encryption key index copy plaintext and invoke the host mnemonic resolver before failing. Three non-blocking issues also remain around read-side version validation, secret residency during network fetches, and missing successful end-to-end fetch/decrypt coverage.

Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (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 | 🟡 3 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/document.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/document.rs:474-478: Reject non-derivable encryption key indices before resolver access
  The C ABI accepts every `u32` for `encryption_key_index`, but derivation eventually passes it to `ChildNumber::from_hardened_idx`, which rejects indices with bit 31 set. A request such as `0x8000_0000` therefore cannot succeed, yet this boundary copies the plaintext at lines 490-495 and, for an external-signable wallet, invokes the host mnemonic resolver at lines 511-513 before derivation rejects it. This violates the early-validation boundary established for payload length and wire version and can trigger a user-visible keychain prompt for an impossible request. The resulting `InvalidIdentityData` also maps to `ErrorUnknown` rather than the typed invalid-parameter result. Add a shared core/FFI range check before payload materialization, wallet lookup, or resolver access, and cover `0x8000_0000` with a regression test.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/document.rs:665-701: Acquire txMetadata key material after network fetching
  The external-signable path resolves the mnemonic-derived master xprv before fetching the contract and all document pages, then retains it across every network await. The resident path similarly clones the key-bearing wallet before the document query. This unnecessarily prompts the resolver for missing contracts or empty results and lets a slow or stalled network extend secret residency indefinitely. The comment stating that derivation occurs between page fetches does not match the implementation: `query_owned_encrypted_documents` completes all pagination before the synchronous decrypt loop starts. Split raw document retrieval from decryption and acquire the resolver master or resident wallet only after the network phase returns materialized documents that require decryption.

In `packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:379-380: Reject unsupported versions when opening txMetadata
  The create path declares versions 0 and 1 as the only supported wire formats, but `open_tx_metadata` accepts any leading byte. Because the version byte is outside the ciphertext and IV, changing a valid blob's first byte to 2 does not affect CBC decryption or padding, so the fetch path can return successfully decrypted data tagged with an unsupported version. Validate the version before decryption so malformed or unsupported documents follow the documented skip-on-error path instead of producing a result outside the API's supported domain.

In `packages/rs-platform-wallet/tests/txmetadata_fetch.rs`:
- [SUGGESTION] packages/rs-platform-wallet/tests/txmetadata_fetch.rs:15-17: Exercise the successful fetch-to-decrypt path in an automated test
  No automated test successfully calls `IdentityWallet::fetch_encrypted_documents`. The network-free tests exercise crypto directly, while this ignored test calls only `query_owned_encrypted_documents` and then independently reimplements field extraction. It therefore does not verify the production orchestration that maps document properties, derives per-document keys, skips malformed entries, propagates the version, or maps IDs and timestamps into `DecryptedEncryptedDocument`. Add a mock-SDK test with a managed wallet, one valid sealed document, and one malformed document, then drive the public fetch method and assert the complete valid result plus skip behavior.

Comment on lines +474 to +478
// The wire version is likewise decidable from the argument alone. Rejecting
// it here keeps an unsealable request from reaching the payload pointer, the
// wallet, or the host key resolver — the last of which drives a device
// keychain round-trip that can prompt the user.
unwrap_result_or_return!(ensure_tx_metadata_version_supported(version));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Reject non-derivable encryption key indices before resolver access

The C ABI accepts every u32 for encryption_key_index, but derivation eventually passes it to ChildNumber::from_hardened_idx, which rejects indices with bit 31 set. A request such as 0x8000_0000 therefore cannot succeed, yet this boundary copies the plaintext at lines 490-495 and, for an external-signable wallet, invokes the host mnemonic resolver at lines 511-513 before derivation rejects it. This violates the early-validation boundary established for payload length and wire version and can trigger a user-visible keychain prompt for an impossible request. The resulting InvalidIdentityData also maps to ErrorUnknown rather than the typed invalid-parameter result. Add a shared core/FFI range check before payload materialization, wallet lookup, or resolver access, and cover 0x8000_0000 with a regression test.

source: ['codex']

Comment on lines +379 to +380
let version = blob[0];
let iv: [u8; 16] = blob[1..BLOB_HEADER_LEN]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Reject unsupported versions when opening txMetadata

The create path declares versions 0 and 1 as the only supported wire formats, but open_tx_metadata accepts any leading byte. Because the version byte is outside the ciphertext and IV, changing a valid blob's first byte to 2 does not affect CBC decryption or padding, so the fetch path can return successfully decrypted data tagged with an unsupported version. Validate the version before decryption so malformed or unsupported documents follow the documented skip-on-error path instead of producing a result outside the API's supported domain.

Suggested change
let version = blob[0];
let iv: [u8; 16] = blob[1..BLOB_HEADER_LEN]
let version = blob[0];
ensure_tx_metadata_version_supported(version)?;
let iv: [u8; 16] = blob[1..BLOB_HEADER_LEN]

source: ['codex']

Comment on lines +665 to +701
// Key-source selection by wallet capability (may synchronously call
// back into the host mnemonic resolver for external-signable
// wallets — see `tx_metadata_key_master_for_wallet`). The resolved
// master is wrapped in a Drop-wiping guard.
let master_opt =
unsafe { tx_metadata_key_master_for_wallet(wallet, mnemonic_resolver_handle) }?
.map(WipingMaster);

// Fallible worker entry: a runtime that cannot be built, or a worker
// that does not complete, becomes a value this export maps instead of a
// panic that would reach the C frame.
let result: Result<Vec<platform_wallet::DecryptedEncryptedDocument>, PlatformWalletError> =
try_block_on_worker(async move {
// TRADEOFF: unlike create, a document's
// (keyIndex, encryptionKeyIndex) are only known AFTER its page is
// fetched, so the master cannot be fully pre-derived before the
// network work. It therefore stays resident across the pagination
// awaits — but inside the `WipingMaster` Drop guard, so a panic or
// early return still scrubs its scalar (a manual post-await erase
// would be skipped on those paths). Per-document key derivation is
// itself synchronous, between page fetches (see
// `fetch_encrypted_documents`).
let key_source = match master_opt.as_ref() {
Some(master) => TxMetadataKeySource::Master(&master.0),
None => TxMetadataKeySource::ResidentWallet,
};
let fetched = identity_wallet
.fetch_encrypted_documents(
&owner_id_for_async,
&contract_id_for_async,
&document_type_str,
since_ms,
key_source,
)
.await;
drop(master_opt); // scrub as soon as the fetch completes
fetched

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Acquire txMetadata key material after network fetching

The external-signable path resolves the mnemonic-derived master xprv before fetching the contract and all document pages, then retains it across every network await. The resident path similarly clones the key-bearing wallet before the document query. This unnecessarily prompts the resolver for missing contracts or empty results and lets a slow or stalled network extend secret residency indefinitely. The comment stating that derivation occurs between page fetches does not match the implementation: query_owned_encrypted_documents completes all pagination before the synchronous decrypt loop starts. Split raw document retrieval from decryption and acquire the resolver master or resident wallet only after the network phase returns materialized documents that require decryption.

source: ['codex']

Comment on lines +15 to +17
//! DECRYPT half is not exercised here — it needs the owner's mnemonic — but the
//! per-document field extraction that feeds decrypt IS asserted, proving the
//! pipeline reaches the decrypt step for both documents.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Exercise the successful fetch-to-decrypt path in an automated test

No automated test successfully calls IdentityWallet::fetch_encrypted_documents. The network-free tests exercise crypto directly, while this ignored test calls only query_owned_encrypted_documents and then independently reimplements field extraction. It therefore does not verify the production orchestration that maps document properties, derives per-document keys, skips malformed entries, propagates the version, or maps IDs and timestamps into DecryptedEncryptedDocument. Add a mock-SDK test with a managed wallet, one valid sealed document, and one malformed document, then drive the public fetch method and assert the complete valid result plus skip behavior.

source: ['codex']

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants