feat(kotlin-sdk): wire-compatible encrypted txMetadata document create + decrypt-on-fetch#4091
Conversation
bfoss765
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds legacy-compatible txMetadata encryption and decryption, paginated encrypted-document retrieval, platform-wallet FFI operations, JNI bindings, and Kotlin suspend APIs with validation, JSON serialization, secure key handling, and compatibility tests. ChangesEncrypted document transaction stack
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 0c26a84) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR adds a wire-compatible encrypted txMetadata create + decrypt-on-fetch surface (Rust crypto envelope, IdentityWallet network layer, platform-wallet-ffi, JNI, Kotlin) that reuses established patterns for document creation, FFI marshaling, and pagination. The AES-256-CBC envelope is correctly implemented and pinned to a NIST vector; no memory-safety, panic-across-true-FFI-boundary (for this PR's specific additions beyond the existing pattern), or consensus-impacting defect was found. The main gaps are test coverage: the network-layer key-selection/fetch/decrypt logic that carries the actual wire-compatibility contract is entirely untested, and the mnemonic→AES-key HD derivation-path prefix — the single fact this whole migration depends on — is verified only against itself, never against a real legacy-written document (a gap the PR body already discloses).
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/general=claude-sonnet-5(completed_parseable_output_with_nonzero_wrapper_exit); opus/general=claude-opus-4-8(completed); codex/security-auditor=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/security-auditor=claude-sonnet-5(completed); opus/security-auditor=claude-opus-4-8(completed); codex/rust-quality=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/rust-quality=claude-sonnet-5(completed); opus/rust-quality=claude-opus-4-8(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); opus/ffi-engineer=claude-opus-4-8(completed); verifier=verifier-sonnet5-4091-1783720228=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🟡 7 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:95-126: HD derivation-path prefix that determines cross-stack key agreement is unpinned
`derive_tx_metadata_key` builds the AES key from `identity_auth_derivation_path_for_type(ECDSA, identity_index, key_index) / 32769' / encryption_key_index'`. Wire-compatibility with the legacy `org.dashj.platform` stack depends entirely on this path reproducing dashj's `blockchainIdentityECDSADerivationPath(keyIndex)/32769'/encryptionKeyIndex'` byte-for-byte. The only tests that touch this (`key_derivation_is_deterministic_and_index_separated` and `nist_cbc_aes256_cross_stack_vector`) check determinism/index-separation and pin the AES-CBC *cipher + framing* respectively — neither asserts the derived key against a legacy-computed value. If the account-prefix or key-type mapping diverges even slightly, every derived key silently mismatches: `open_tx_metadata` fails cleanly and the caller just skips the document (by design), so there is no runtime signal — migrated users' entire memo/tax-category history quietly disappears, which is exactly the failure this PR exists to prevent. The PR body already discloses this gap and requests a real legacy-written sample; that sample (mnemonic, identity index, keyIndex, encryptionKeyIndex, encryptedMetadata hex) should become a fixed cross-stack test vector before this path is relied on for the dash-wallet migration.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:144-152: `OpenedTxMetadata` also derives `Debug` over the raw decrypted payload
Same concern as `DecryptedEncryptedDocument`: this intermediate struct returned by `open_tx_metadata` holds the decrypted plaintext bytes and derives `Debug`, making it trivial to accidentally print a user's decrypted financial metadata during future troubleshooting (e.g. `tracing::debug!("{:?}", opened)`).
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:135-142: `seal_tx_metadata` doesn't validate the version byte it writes into the SDK-owned envelope
The module doc comment defines the version byte as part of the envelope the SDK owns (only `0` = CBOR and `1` = protobuf are meaningful to the legacy `decryptTxMetadata`, which switches on it), yet `seal_tx_metadata` writes the caller-supplied byte verbatim. The Kotlin layer only bounds `version` to `0..255` (`DocumentTransactions.kt:174`, `require(version in 0..255)`), not to the two legacy-meaningful values, so a caller can seal a document with a version byte the legacy stack can't interpret — silently breaking the exact wire-compatibility guarantee this code exists to uphold, with nothing else to catch it since the payload itself is opaque. Confidence kept low because the PR may deliberately treat `version` as forward-extensible and both known-version constants are exported for callers to use correctly.
In `packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:67-351: Network-layer create/fetch logic — the actual wire-compat contract — has zero test coverage
`crypto/tx_metadata.rs` (the envelope) is unit-tested, but this file has no tests at all, despite containing the pieces most load-bearing for wire-compatibility and most likely to silently regress: `select_encryption_key_id`'s ENCRYPTION/MEDIUM → AUTHENTICATION/HIGH fallback (the exact mirror of legacy `createTxMetadata` key selection), `resolve_encryption_context`'s watch-only-identity error path, the paginated `$ownerId`/`$updatedAt` query + `StartAfter` cursor loop, and the skip-on-malformed/skip-on-wrong-key behavior the doc comments call out as an explicit design requirement. The PR description states a since-timestamp query-shape test is included, but no such test exists in the diff. This crate's `TestPlatformBuilder`/mock-identity conventions are already established in sibling files (`contact_info.rs`, `profile.rs`), so a test asserting the key-selection fallback order and a mocked multi-page fetch (equal `$updatedAt` at a page boundary, a mixed-in malformed/wrong-key document) would meaningfully lock in this contract.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:47-65: `DecryptedEncryptedDocument` derives `Debug` over a user's decrypted financial payload
`DecryptedEncryptedDocument` derives `Debug` and carries `payload: Vec<u8>` — the decrypted plaintext of a user's tx-metadata batch (memos, tax categories, exchange-rate records, gift cards per the PR description). Elsewhere in this same module tree, `DerivedIdentityAuthKey` (identity_handle.rs) deliberately omits `Debug` specifically because it carries secret key material that must not be printable — confirmed by its doc comment and the absence of a `#[derive(Debug...)]` above the struct. This decrypted payload is arguably more sensitive (real financial/PII data), yet derives `Debug` freely, so a future `{:?}` in a log line or `dbg!()` would leak a user's financial memos into logs.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:214-240: `fetch_encrypted_documents` unconditionally re-fetches and re-proof-verifies the contract from the network on every call
`DataContract::fetch(&self.sdk, *contract_id)` runs on every invocation, paying a network round-trip + Merkle proof verification each time. This crate already has a caching precedent (`network/mod.rs::dashpay_contract()`, a `OnceLock`-cached bundled system contract, added specifically because 'the previous per-call `load_system_data_contract` re-deserialized the contract on every profile / contactInfo op'). That precedent only avoids re-parsing a local bundle; this function's cost is a genuine network fetch, which is more expensive to repeat — so the case for caching here is at least as strong. A wallet sync loop polling for new tx-metadata will pay this cost every call. Since `contract_id` is caller-supplied rather than a single well-known constant, the cache would need to key by id (e.g. `Mutex<HashMap<Identifier, Arc<DataContract>>>`).
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:250-348: Fetch buffers every encrypted document across all pages before decrypting any of them
The paginated loop accumulates every raw `Document` into `raw_docs` (`raw_docs.extend(page)`) and only decrypts after the entire scan completes. For a first migration fetch (`since_ms == 0`) with a large txMetadata history, this holds the full encrypted document set in memory, then separately allocates the full decrypted `DecryptedEncryptedDocument` set (and, at the FFI layer, its JSON serialization) in the same scope — roughly doubling peak memory on a mobile target. Since the whole result set is inherently returned to the caller anyway, this is a bounded improvement rather than a correctness issue: decrypting each page as it arrives (and dropping the raw page) would keep peak memory proportional to one page + the output.
| pub fn derive_tx_metadata_key( | ||
| wallet: &Wallet, | ||
| network: Network, | ||
| identity_index: u32, | ||
| key_index: u32, | ||
| encryption_key_index: u32, | ||
| ) -> Result<Zeroizing<[u8; 32]>, PlatformWalletError> { | ||
| let root_path = identity_auth_derivation_path_for_type( | ||
| network, | ||
| KeyDerivationType::ECDSA, | ||
| identity_index, | ||
| key_index, | ||
| )?; | ||
|
|
||
| let path = root_path.extend([ | ||
| ChildNumber::from_hardened_idx(TX_METADATA_ENCRYPTION_CHILD).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Invalid txMetadata encryption child index: {e}" | ||
| )) | ||
| })?, | ||
| ChildNumber::from_hardened_idx(encryption_key_index).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Invalid txMetadata encryptionKeyIndex: {e}" | ||
| )) | ||
| })?, | ||
| ]); | ||
|
|
||
| let ext = wallet.derive_extended_private_key(&path).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!("Failed to derive txMetadata key: {e}")) | ||
| })?; | ||
| Ok(Zeroizing::new(ext.private_key.secret_bytes())) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: HD derivation-path prefix that determines cross-stack key agreement is unpinned
derive_tx_metadata_key builds the AES key from identity_auth_derivation_path_for_type(ECDSA, identity_index, key_index) / 32769' / encryption_key_index'. Wire-compatibility with the legacy org.dashj.platform stack depends entirely on this path reproducing dashj's blockchainIdentityECDSADerivationPath(keyIndex)/32769'/encryptionKeyIndex' byte-for-byte. The only tests that touch this (key_derivation_is_deterministic_and_index_separated and nist_cbc_aes256_cross_stack_vector) check determinism/index-separation and pin the AES-CBC cipher + framing respectively — neither asserts the derived key against a legacy-computed value. If the account-prefix or key-type mapping diverges even slightly, every derived key silently mismatches: open_tx_metadata fails cleanly and the caller just skips the document (by design), so there is no runtime signal — migrated users' entire memo/tax-category history quietly disappears, which is exactly the failure this PR exists to prevent. The PR body already discloses this gap and requests a real legacy-written sample; that sample (mnemonic, identity index, keyIndex, encryptionKeyIndex, encryptedMetadata hex) should become a fixed cross-stack test vector before this path is relied on for the dash-wallet migration.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 729636e — HD derivation-path prefix that determines cross-stack key agreement is unpinned no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| impl IdentityWallet { | ||
| /// Select the identity's encryption key id (the document's `keyIndex` | ||
| /// field): an `ECDSA_SECP256K1` `Purpose::ENCRYPTION` / `MEDIUM` key, falling | ||
| /// back to an `AUTHENTICATION` / `HIGH` key — mirroring the legacy | ||
| /// `BlockchainIdentity.createTxMetadata` selection | ||
| /// (`getFirstPublicKey(ENCRYPTION, MEDIUM)` → `getHighAuthenticationKey`). | ||
| fn select_encryption_key_id( | ||
| identity: &dpp::identity::Identity, | ||
| ) -> Result<u32, PlatformWalletError> { | ||
| identity | ||
| .get_first_public_key_matching( | ||
| Purpose::ENCRYPTION, | ||
| [SecurityLevel::MEDIUM].into(), | ||
| [KeyType::ECDSA_SECP256K1].into(), | ||
| false, | ||
| ) | ||
| .or_else(|| { | ||
| identity.get_first_public_key_matching( | ||
| Purpose::AUTHENTICATION, | ||
| [SecurityLevel::HIGH].into(), | ||
| [KeyType::ECDSA_SECP256K1].into(), | ||
| false, | ||
| ) | ||
| }) | ||
| .map(|k| k.id()) | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData( | ||
| "Identity has no ECDSA_SECP256K1 ENCRYPTION (MEDIUM) or AUTHENTICATION \ | ||
| (HIGH) key to derive the txMetadata encryption key" | ||
| .to_string(), | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| /// Resolve `(identity, identity_index, wallet)` for `owner_identity_id` | ||
| /// from the in-process wallet manager — the inputs the tx-metadata key | ||
| /// derivation needs. Errors for a watch-only / out-of-wallet identity (no | ||
| /// resident HD slot); the dash-wallet migration uses a resident mnemonic | ||
| /// wallet. | ||
| async fn resolve_encryption_context( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| ) -> Result<(dpp::identity::Identity, u32, key_wallet::wallet::Wallet), PlatformWalletError> { | ||
| let wm = self.wallet_manager.read().await; | ||
| let info = wm | ||
| .get_wallet_info(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; | ||
| let managed = info | ||
| .identity_manager | ||
| .managed_identity(owner_identity_id) | ||
| .ok_or(PlatformWalletError::IdentityNotFound(*owner_identity_id))?; | ||
| let identity_index = managed.identity_index.ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Identity {owner_identity_id} is watch-only (no resident HD slot); \ | ||
| cannot derive its txMetadata encryption key in-process" | ||
| )) | ||
| })?; | ||
| let identity = managed.identity.clone(); | ||
| let wallet = wm | ||
| .get_wallet(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))? | ||
| .clone(); | ||
| Ok((identity, identity_index, wallet)) | ||
| } | ||
|
|
||
| /// Create + broadcast an ENCRYPTED `txMetadata`-style document on | ||
| /// `contract_id`'s `document_type_name`, owned by `owner_identity_id`. | ||
| /// | ||
| /// The SDK derives the identity encryption key, seals `payload` into the | ||
| /// wire-compatible `version ‖ IV ‖ AES-256-CBC` blob, and writes the | ||
| /// `{keyIndex, encryptionKeyIndex, encryptedMetadata}` document — the exact | ||
| /// shape the legacy `publishTxMetaData` wrote, so the legacy stack decrypts | ||
| /// it and vice versa. | ||
| /// | ||
| /// The caller supplies: | ||
| /// - `encryption_key_index`: the per-document index (dash-wallet's | ||
| /// monotonic `1 + countAllRequests()` counter). Batching stays app-side. | ||
| /// - `version`: the payload version byte (`1` = protobuf, as the wallet | ||
| /// writes). | ||
| /// - `payload`: the already-serialized opaque plaintext (a protobuf | ||
| /// `TxMetadataBatch`) — the SDK does not parse it. | ||
| /// | ||
| /// The `keyIndex` field (the identity encryption key id) is selected | ||
| /// SDK-side to match the legacy stack. Returns the confirmed `Document`. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn create_encrypted_document_with_signer<S>( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| encryption_key_index: u32, | ||
| version: u8, | ||
| payload: &[u8], | ||
| signer: &S, | ||
| ) -> Result<Document, PlatformWalletError> | ||
| where | ||
| S: Signer<IdentityPublicKey> + Send + Sync, | ||
| { | ||
| use dashcore::secp256k1::rand::{thread_rng, RngCore}; | ||
|
|
||
| let (identity, identity_index, wallet) = | ||
| self.resolve_encryption_context(owner_identity_id).await?; | ||
| let key_index = Self::select_encryption_key_id(&identity)?; | ||
|
|
||
| // Derive the AES key and seal the payload into the wire blob. | ||
| let aes_key = derive_tx_metadata_key( | ||
| &wallet, | ||
| self.sdk.network, | ||
| identity_index, | ||
| key_index, | ||
| encryption_key_index, | ||
| )?; | ||
| let mut iv = [0u8; 16]; | ||
| thread_rng().fill_bytes(&mut iv); | ||
| let blob = seal_tx_metadata(&aes_key, version, &iv, payload); | ||
|
|
||
| // Reuse the generic create path: it fetches the contract, sanitizes the | ||
| // hex `encryptedMetadata` into `Bytes` against the schema, auto-selects | ||
| // the AUTHENTICATION signing key, and broadcasts on the 8 MB worker | ||
| // stack. Byte-array fields are accepted as hex strings there. | ||
| let properties_json = serde_json::json!({ | ||
| FIELD_KEY_INDEX: key_index, | ||
| FIELD_ENCRYPTION_KEY_INDEX: encryption_key_index, | ||
| FIELD_ENCRYPTED_METADATA: hex::encode(&blob), | ||
| }) | ||
| .to_string(); | ||
|
|
||
| self.create_document_with_signer( | ||
| owner_identity_id, | ||
| contract_id, | ||
| document_type_name, | ||
| &properties_json, | ||
| signer, | ||
| ) | ||
| .await | ||
| } | ||
|
|
||
| /// Fetch every encrypted `txMetadata`-style document owned by | ||
| /// `owner_identity_id` on `contract_id`'s `document_type_name` updated at or | ||
| /// after `since_ms`, and DECRYPT each with the identity's derived key. | ||
| /// | ||
| /// Mirrors the legacy `getTxMetaData(sinceTime, key)`: the query is | ||
| /// `$ownerId == owner AND $updatedAt >= since_ms` ordered by `$updatedAt` | ||
| /// ascending, paginated so a wallet with many documents isn't truncated. A | ||
| /// document whose key can't be derived or whose blob doesn't decrypt is | ||
| /// SKIPPED with a warning (a malformed document must not abort the sync), | ||
| /// matching the resident `contactInfo` sweep. | ||
| pub async fn fetch_encrypted_documents( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| since_ms: u64, | ||
| ) -> Result<Vec<DecryptedEncryptedDocument>, PlatformWalletError> { | ||
| use dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v0::Start; | ||
| use dash_sdk::drive::query::{OrderClause, WhereClause, WhereOperator}; | ||
| use dash_sdk::platform::{ContextProvider, Fetch, FetchMany}; | ||
| use dpp::platform_value::platform_value; | ||
|
|
||
| // Fetch the contract and register it so `fetch_many`'s proof | ||
| // verification can resolve it through the context provider (the mobile | ||
| // provider never fetches contracts itself). | ||
| let contract = DataContract::fetch(&self.sdk, *contract_id) | ||
| .await | ||
| .map_err(PlatformWalletError::Sdk)? | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Data contract {contract_id} not found on Platform; cannot fetch documents" | ||
| )) | ||
| })?; | ||
| if let Some(provider) = self.sdk.context_provider() { | ||
| provider.register_data_contract(Arc::new(contract.clone())); | ||
| } | ||
| let contract = Arc::new(contract); | ||
|
|
||
| let (_identity, identity_index, wallet) = | ||
| self.resolve_encryption_context(owner_identity_id).await?; | ||
|
|
||
| // Paginated owner-scoped, since-timestamp scan. The `$updatedAt >=` | ||
| // where-clause + `$updatedAt asc` order-by bind to the contract's | ||
| // `($ownerId, $updatedAt)` index (the same query shape the legacy | ||
| // `TxMetadata.get(userId, since)` builder used). | ||
| const PAGE: u32 = 100; | ||
| let mut raw_docs: Vec<(Identifier, Option<Document>)> = Vec::new(); | ||
| let mut start: Option<Start> = None; | ||
| 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(&self.sdk, query) | ||
| .await | ||
| .map_err(PlatformWalletError::Sdk)?; | ||
| 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, | ||
| } | ||
| } | ||
|
|
||
| let mut out = Vec::new(); | ||
| for (doc_id, maybe_doc) in raw_docs.iter() { | ||
| let Some(doc) = maybe_doc else { continue }; | ||
| let props = doc.properties(); | ||
| let (Some(key_index), Some(encryption_key_index)) = ( | ||
| props | ||
| .get(FIELD_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| props | ||
| .get(FIELD_ENCRYPTION_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| ) else { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, "encrypted document missing key indices; skipping"); | ||
| continue; | ||
| }; | ||
| let Some(blob) = props | ||
| .get(FIELD_ENCRYPTED_METADATA) | ||
| .and_then(|v: &Value| v.to_binary_bytes().ok()) | ||
| else { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, "encrypted document missing encryptedMetadata; skipping"); | ||
| continue; | ||
| }; | ||
|
|
||
| let aes_key = match derive_tx_metadata_key( | ||
| &wallet, | ||
| self.sdk.network, | ||
| identity_index, | ||
| key_index, | ||
| encryption_key_index, | ||
| ) { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, error = %e, "txMetadata key derivation failed; skipping"); | ||
| continue; | ||
| } | ||
| }; | ||
| let opened = match open_tx_metadata(&aes_key, &blob) { | ||
| Ok(o) => o, | ||
| Err(e) => { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, error = %e, "txMetadata decrypt failed; skipping"); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| out.push(DecryptedEncryptedDocument { | ||
| document_id: *doc_id, | ||
| owner_id: doc.owner_id(), | ||
| key_index, | ||
| encryption_key_index, | ||
| version: opened.version, | ||
| updated_at_ms: doc.updated_at(), | ||
| payload: opened.payload, | ||
| }); | ||
| } | ||
| Ok(out) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Network-layer create/fetch logic — the actual wire-compat contract — has zero test coverage
crypto/tx_metadata.rs (the envelope) is unit-tested, but this file has no tests at all, despite containing the pieces most load-bearing for wire-compatibility and most likely to silently regress: select_encryption_key_id's ENCRYPTION/MEDIUM → AUTHENTICATION/HIGH fallback (the exact mirror of legacy createTxMetadata key selection), resolve_encryption_context's watch-only-identity error path, the paginated $ownerId/$updatedAt query + StartAfter cursor loop, and the skip-on-malformed/skip-on-wrong-key behavior the doc comments call out as an explicit design requirement. The PR description states a since-timestamp query-shape test is included, but no such test exists in the diff. This crate's TestPlatformBuilder/mock-identity conventions are already established in sibling files (contact_info.rs, profile.rs), so a test asserting the key-selection fallback order and a mocked multi-page fetch (equal $updatedAt at a page boundary, a mixed-in malformed/wrong-key document) would meaningfully lock in this contract.
source: ['claude', 'codex']
There was a problem hiding this comment.
Still valid at 9a60fa0: the ignored testnet test still does not exercise key-selection fallback, watch-only handling, create/decrypt behavior, or malformed/wrong-key skip paths in CI. The latest delta only registers the fetched contract with the context provider.
Corrected in place: the automatic title/hash reconciler incorrectly labeled this carried-forward finding resolved.
| #[derive(Debug, Clone)] | ||
| pub struct DecryptedEncryptedDocument { | ||
| /// Canonical 32-byte document id. | ||
| pub document_id: Identifier, | ||
| /// Document owner ($ownerId). | ||
| pub owner_id: Identifier, | ||
| /// The document's `keyIndex` field (the identity's ENCRYPTION key id used | ||
| /// to derive the decryption key). | ||
| pub key_index: u32, | ||
| /// The document's `encryptionKeyIndex` field (the app's per-document index). | ||
| pub encryption_key_index: u32, | ||
| /// The blob's leading version byte (0 = CBOR, 1 = protobuf). | ||
| pub version: u8, | ||
| /// $updatedAt in epoch-millis, if the document carries it. The app tracks | ||
| /// this as its since-timestamp high-water mark for the next fetch. | ||
| pub updated_at_ms: Option<u64>, | ||
| /// The decrypted, opaque payload bytes. | ||
| pub payload: Vec<u8>, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: DecryptedEncryptedDocument derives Debug over a user's decrypted financial payload
DecryptedEncryptedDocument derives Debug and carries payload: Vec<u8> — the decrypted plaintext of a user's tx-metadata batch (memos, tax categories, exchange-rate records, gift cards per the PR description). Elsewhere in this same module tree, DerivedIdentityAuthKey (identity_handle.rs) deliberately omits Debug specifically because it carries secret key material that must not be printable — confirmed by its doc comment and the absence of a #[derive(Debug...)] above the struct. This decrypted payload is arguably more sensitive (real financial/PII data), yet derives Debug freely, so a future {:?} in a log line or dbg!() would leak a user's financial memos into logs.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — DecryptedEncryptedDocument derives Debug over a user's decrypted financial payload no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// The plaintext recovered from a stored `encryptedMetadata` blob. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct OpenedTxMetadata { | ||
| /// The blob's leading version byte (0 = CBOR, 1 = protobuf). The app | ||
| /// dispatches its payload parse on this. | ||
| pub version: u8, | ||
| /// The decrypted, PKCS7-unpadded payload bytes — opaque to this crate. | ||
| pub payload: Vec<u8>, | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: OpenedTxMetadata also derives Debug over the raw decrypted payload
Same concern as DecryptedEncryptedDocument: this intermediate struct returned by open_tx_metadata holds the decrypted plaintext bytes and derives Debug, making it trivial to accidentally print a user's decrypted financial metadata during future troubleshooting (e.g. tracing::debug!("{:?}", opened)).
source: ['claude']
There was a problem hiding this comment.
Resolved in 729636e — OpenedTxMetadata also derives Debug over the raw decrypted payload no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| pub async fn fetch_encrypted_documents( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| since_ms: u64, | ||
| ) -> Result<Vec<DecryptedEncryptedDocument>, PlatformWalletError> { | ||
| use dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v0::Start; | ||
| use dash_sdk::drive::query::{OrderClause, WhereClause, WhereOperator}; | ||
| use dash_sdk::platform::{ContextProvider, Fetch, FetchMany}; | ||
| use dpp::platform_value::platform_value; | ||
|
|
||
| // Fetch the contract and register it so `fetch_many`'s proof | ||
| // verification can resolve it through the context provider (the mobile | ||
| // provider never fetches contracts itself). | ||
| let contract = DataContract::fetch(&self.sdk, *contract_id) | ||
| .await | ||
| .map_err(PlatformWalletError::Sdk)? | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Data contract {contract_id} not found on Platform; cannot fetch documents" | ||
| )) | ||
| })?; | ||
| if let Some(provider) = self.sdk.context_provider() { | ||
| provider.register_data_contract(Arc::new(contract.clone())); | ||
| } | ||
| let contract = Arc::new(contract); |
There was a problem hiding this comment.
🟡 Suggestion: fetch_encrypted_documents unconditionally re-fetches and re-proof-verifies the contract from the network on every call
DataContract::fetch(&self.sdk, *contract_id) runs on every invocation, paying a network round-trip + Merkle proof verification each time. This crate already has a caching precedent (network/mod.rs::dashpay_contract(), a OnceLock-cached bundled system contract, added specifically because 'the previous per-call load_system_data_contract re-deserialized the contract on every profile / contactInfo op'). That precedent only avoids re-parsing a local bundle; this function's cost is a genuine network fetch, which is more expensive to repeat — so the case for caching here is at least as strong. A wallet sync loop polling for new tx-metadata will pay this cost every call. Since contract_id is caller-supplied rather than a single well-known constant, the cache would need to key by id (e.g. Mutex<HashMap<Identifier, Arc<DataContract>>>).
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — fetch_encrypted_documents unconditionally re-fetches and re-proof-verifies the contract from the network on every call no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let mut raw_docs: Vec<(Identifier, Option<Document>)> = Vec::new(); | ||
| let mut start: Option<Start> = None; | ||
| 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(&self.sdk, query) | ||
| .await | ||
| .map_err(PlatformWalletError::Sdk)?; | ||
| 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, | ||
| } | ||
| } | ||
|
|
||
| let mut out = Vec::new(); | ||
| for (doc_id, maybe_doc) in raw_docs.iter() { | ||
| let Some(doc) = maybe_doc else { continue }; | ||
| let props = doc.properties(); | ||
| let (Some(key_index), Some(encryption_key_index)) = ( | ||
| props | ||
| .get(FIELD_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| props | ||
| .get(FIELD_ENCRYPTION_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| ) else { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, "encrypted document missing key indices; skipping"); | ||
| continue; | ||
| }; | ||
| let Some(blob) = props | ||
| .get(FIELD_ENCRYPTED_METADATA) | ||
| .and_then(|v: &Value| v.to_binary_bytes().ok()) | ||
| else { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, "encrypted document missing encryptedMetadata; skipping"); | ||
| continue; | ||
| }; | ||
|
|
||
| let aes_key = match derive_tx_metadata_key( | ||
| &wallet, | ||
| self.sdk.network, | ||
| identity_index, | ||
| key_index, | ||
| encryption_key_index, | ||
| ) { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, error = %e, "txMetadata key derivation failed; skipping"); | ||
| continue; | ||
| } | ||
| }; | ||
| let opened = match open_tx_metadata(&aes_key, &blob) { | ||
| Ok(o) => o, | ||
| Err(e) => { | ||
| tracing::warn!(owner = %owner_identity_id, doc = %doc_id, error = %e, "txMetadata decrypt failed; skipping"); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| out.push(DecryptedEncryptedDocument { | ||
| document_id: *doc_id, | ||
| owner_id: doc.owner_id(), | ||
| key_index, | ||
| encryption_key_index, | ||
| version: opened.version, | ||
| updated_at_ms: doc.updated_at(), | ||
| payload: opened.payload, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Fetch buffers every encrypted document across all pages before decrypting any of them
The paginated loop accumulates every raw Document into raw_docs (raw_docs.extend(page)) and only decrypts after the entire scan completes. For a first migration fetch (since_ms == 0) with a large txMetadata history, this holds the full encrypted document set in memory, then separately allocates the full decrypted DecryptedEncryptedDocument set (and, at the FFI layer, its JSON serialization) in the same scope — roughly doubling peak memory on a mobile target. Since the whole result set is inherently returned to the caller anyway, this is a bounded improvement rather than a correctness issue: decrypting each page as it arrives (and dropping the raw page) would keep peak memory proportional to one page + the output.
source: ['claude', 'codex']
There was a problem hiding this comment.
Still valid at 9a60fa0: query_owned_encrypted_documents still drains every page into raw_docs before the caller begins decrypting. The latest delta adds logging and test-context registration; it does not change this buffering behavior.
Corrected in place: the automatic title/hash reconciler incorrectly labeled this carried-forward finding resolved.
| if let Some(provider) = self.sdk.context_provider() { | ||
| provider.register_data_contract(Arc::new(contract.clone())); | ||
| } | ||
| let contract = Arc::new(contract); |
There was a problem hiding this comment.
💬 Nitpick: Avoidable full-contract clone before Arc-wrapping
contract.clone() deep-clones the entire DataContract (document-type/index metadata) just to register it with the context provider, and then the original is separately moved into an Arc on the next line. Wrapping in the Arc first and cloning the cheap Arc handle avoids the duplicate allocation. This is a cold path (once per fetch call), so it's a minor cleanup, not a hot-path concern.
| if let Some(provider) = self.sdk.context_provider() { | |
| provider.register_data_contract(Arc::new(contract.clone())); | |
| } | |
| let contract = Arc::new(contract); | |
| let contract = std::sync::Arc::new(contract); | |
| if let Some(provider) = self.sdk.context_provider() { | |
| provider.register_data_contract(std::sync::Arc::clone(&contract)); | |
| } |
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — Avoidable full-contract clone before Arc-wrapping no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| pub fn seal_tx_metadata(key: &[u8; 32], version: u8, iv: &[u8; 16], payload: &[u8]) -> Vec<u8> { | ||
| let ciphertext = platform_encryption::encrypt_aes_256_cbc(key, iv, payload); | ||
| let mut blob = Vec::with_capacity(BLOB_HEADER_LEN + ciphertext.len()); | ||
| blob.push(version); | ||
| blob.extend_from_slice(iv); | ||
| blob.extend_from_slice(&ciphertext); | ||
| blob | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: seal_tx_metadata doesn't validate the version byte it writes into the SDK-owned envelope
The module doc comment defines the version byte as part of the envelope the SDK owns (only 0 = CBOR and 1 = protobuf are meaningful to the legacy decryptTxMetadata, which switches on it), yet seal_tx_metadata writes the caller-supplied byte verbatim. The Kotlin layer only bounds version to 0..255 (DocumentTransactions.kt:174, require(version in 0..255)), not to the two legacy-meaningful values, so a caller can seal a document with a version byte the legacy stack can't interpret — silently breaking the exact wire-compatibility guarantee this code exists to uphold, with nothing else to catch it since the payload itself is opaque. Confidence kept low because the PR may deliberately treat version as forward-extensible and both known-version constants are exported for callers to use correctly.
source: ['claude']
There was a problem hiding this comment.
Resolved in 729636e — seal_tx_metadata doesn't validate the version byte it writes into the SDK-owned envelope no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Code Review
Source: reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit), sonnet5/general=claude-sonnet-5(completed), codex/security-auditor=gpt-5.6-sol(completed), sonnet5/security-auditor=claude-sonnet-5(completed), codex/rust-quality=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit), sonnet5/rust-quality=claude-sonnet-5(completed), codex/ffi-engineer=gpt-5.6-sol(completed), sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (orchestration-only, not a reviewer/verifier).
This PR adds a wire-compatible encrypted-txMetadata create/decrypt surface for the Kotlin SDK migration off the legacy org.dashj.platform stack. Prior Reconciliation: 7 of 8 prior findings (network-layer test coverage, two Debug-derive-on-plaintext concerns, repeated contract fetch, unbounded page buffering, an avoidable contract clone, and unchecked version byte) are unchanged and STILL VALID — encrypted_document.rs and the non-test portions of tx_metadata.rs have zero diff between 98ad84b and 729636e. The 8th, prior-hdpath-95 (HD derivation-path unpinned against the legacy stack), is contested among sub-agents: I independently verified the PR description (fetched live) is unmodified and still states the derivation prefix is 'not yet confirmed against a real legacy-written document' and explicitly asks a human maintainer (@QuantumExplorer) for a sample; the commit that adds the new test is co-authored by an AI agent, not that maintainer; git diff confirms no reproducible JVM script/artifact was checked in; and the new vector uses identity_index=0, numerically indistinguishable from KeyDerivationType::ECDSA=0 in the derived path, so it cannot actually prove identity_index sits in the claimed path position. I side with STILL VALID (not FIXED) despite majority-lane disagreement. Carried-Forward Prior Findings: all 8 re-anchored below. New Findings In Latest Delta: the identity_index=0 degeneracy is folded into the re-elevated hdpath finding rather than raised separately. Additional Cumulative Findings: codex-security-auditor raised a blocking UAF (encrypted-create FFI export retains a raw signer pointer across async network work that a concurrent manager swap could free). I verified document.rs is a pre-existing file and the six sibling document operations (replace/delete/transfer/set-price/purchase, and the original create) already use the identical raw-pointer-across-block_on_worker shape predating this PR — so this PR does not introduce or worsen the pattern; per the scope-gating rules this is moved to out_of_scope_findings as a valid systemic follow-up rather than a blocking in-PR finding. The catch_unwind-at-FFI-boundary gap is likewise a confirmed pre-existing crate-wide pattern (8 block_on_worker call sites, all unguarded) and stays out of scope.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1, trigger new_push, prior head 98ad84b): reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); codex/rust-quality=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/rust-quality=claude-sonnet-5(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-4091-1783774124=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(s)
Prior Reconciliation
prior-hdpath-95: STILL VALID — Contested. Six sub-agent lanes classified this FIXED based on the newlegacy_dashj_wire_compat_vectortest; I disagree after independent verification. The live PR description (fetched fresh, not from cache) is byte-for-byte unchanged and still says the derivation prefix is 'not yet confirmed against a real legacy-written document,' explicitly requesting a sample from @QuantumExplorer.git diff 98ad84b7..729636ed --statshows only the test module oftx_metadata.rschanged — no reproducible JVM script, log, or artifact was checked in to let a maintainer independently regenerate the claimed vector. The commit is co-authored by an AI agent ('Claude Fable 5'), not the requested human. Additionally, the new vector usesidentity_index=0, which is numerically identical toKeyDerivationType::ECDSA=0at path positionkey_type_index, so the test cannot distinguish correctidentity_indexplacement from it being dropped, swapped, or misordered — the exact failure class (silent data loss for non-primary identities) the finding originally warned about remains empirically unverified. Kept as blocking.prior-nettest-67: STILL VALID —encrypted_document.rshas zero tests; confirmed unchanged between 98ad84b and 729636e by direct file read and diff.prior-debugderive-doc-47: STILL VALID —DecryptedEncryptedDocument(encrypted_document.rs:47) still derivesDebugoverpayload: Vec<u8>, the decrypted financial plaintext. Unchanged.prior-debugderive-meta-144: STILL VALID —OpenedTxMetadata(tx_metadata.rs:145) still derivesDebugover the decrypted payload. Unchanged (outside the delta's diff hunk).prior-refetch-214: STILL VALID —fetch_encrypted_documentsstill unconditionally callsDataContract::fetch(encrypted_document.rs:229) on every invocation. Unchanged.prior-bufferall-250: STILL VALID — The pagination loop still accumulates all raw documents intoraw_docsbefore decrypting any (encrypted_document.rs:250-293). Unchanged.prior-clone-237: STILL VALID —contract.clone()(encrypted_document.rs:238) is still a full deep clone performed before the original is separately wrapped inArcon the next line. Unchanged.prior-versionbyte-135: STILL VALID —seal_tx_metadatastill writes the caller-supplied version byte verbatim with no bounds check against the two legacy-meaningful values (0/1); Kotlin'sDocumentTransactions.kt:174only bounds to0..255. Unchanged.
Carried-Forward Prior Findings
blocking: The 'dashj-generated' wire-compat vector has unverifiable provenance and doesn't actually test the identity_index path component
packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:95-400
The new legacy_dashj_wire_compat_vector test claims its hardcoded key/blob were produced by 'the ACTUAL legacy stack ... run under a JVM,' but no script, log, or artifact is checked in anywhere in this delta (confirmed: only this test module changed) to let a maintainer independently regenerate or verify it. The PR description — fetched live, not from cache, and unmodified since the prior review — still says the derivation-path prefix 'is not yet confirmed against a real legacy-written document' and explicitly asks a human maintainer (@QuantumExplorer) for one; that request was never answered by a human here. Instead, the commit that adds this test is co-authored by an AI agent asserting the exact fact the PR was requesting external confirmation for. Separately, and independently of the provenance question: the vector calls derive_tx_metadata_key(&wallet, Testnet, identity_index=0, key_index=2, encryption_key_index=1). KeyDerivationType::ECDSA is numerically 0 and occupies the path position immediately before identity_index, so with identity_index also 0 the two components .../0'/0'/2'/... are indistinguishable — the test would pass identically even if identity_index were silently dropped, swapped, or misplaced in the path. Because open_tx_metadata fails silently and the caller just skips undecryptable documents by design, a wrong mapping would surface as a migrated user's second/third Platform identity silently losing all tx-metadata history with no error — precisely the failure this PR exists to prevent. Before treating this as closing the gate for real migration: check in the actual JVM repro script/tool for independent verification, add a second vector with identity_index != 0 to actually exercise that path component, and reconcile the PR body (withdraw the request to @QuantumExplorer if this is considered sufficient, or keep it if not).
suggestion: Network-layer create/fetch logic — the actual wire-compat contract — has zero test coverage
packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:67-351
The crypto envelope now has strong test coverage, but this file — which contains select_encryption_key_id's ENCRYPTION/MEDIUM → AUTHENTICATION/HIGH fallback (the exact mirror of legacy createTxMetadata key selection), resolve_encryption_context's watch-only-identity error path, the paginated $ownerId/$updatedAt query + StartAfter cursor loop, and the skip-on-malformed/skip-on-wrong-key behavior — has no tests at all. These are independently load-bearing for wire-compatibility: selecting or reading the wrong document field fails even if the crypto vector passes. This crate's TestPlatformBuilder/mock-identity conventions are already established in sibling files (contact_info.rs, profile.rs); a test asserting the key-selection fallback order and a mocked multi-page fetch (equal $updatedAt at a page boundary, a mixed-in malformed/wrong-key document) would meaningfully lock in this contract.
suggestion: DecryptedEncryptedDocument derives Debug over a user's decrypted financial payload
packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:47-65
This public result type derives Debug while payload: Vec<u8> holds the decrypted plaintext of a user's tx-metadata batch (memos, tax categories, exchange-rate records, gift cards, per the PR description). Sibling type DerivedIdentityAuthKey (identity_handle.rs) deliberately omits Debug specifically because it carries secret material — this decrypted payload is arguably more sensitive (real financial/PII data), yet derives Debug freely. A future {:?} or dbg!() in a log line or tracing statement would leak a user's financial memos.
suggestion: OpenedTxMetadata also derives Debug over the raw decrypted payload
packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:144-152
Same concern as DecryptedEncryptedDocument: this intermediate struct returned by open_tx_metadata holds the decrypted plaintext bytes and derives Debug, making it trivial to accidentally print a user's decrypted financial metadata during future troubleshooting (e.g. tracing::debug!("{:?}", opened)).
suggestion: fetch_encrypted_documents unconditionally re-fetches and re-proof-verifies the contract on every call
packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:214-240
DataContract::fetch(&self.sdk, *contract_id) (line 229) runs on every invocation, paying a network round-trip plus Merkle proof verification each time. This crate already has a caching precedent for bundled system contracts (network/mod.rs::dashpay_contract()); a wallet sync loop polling for new tx-metadata will pay this cost every call. Since contract_id is caller-supplied, a cache would need to key by id (e.g. Mutex<HashMap<Identifier, Arc<DataContract>>>).
suggestion: Fetch buffers every encrypted document across all pages before decrypting any of them
packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:250-348
The paginated loop accumulates every raw Document into raw_docs and only decrypts after the entire scan completes, roughly doubling peak memory (raw docs + decrypted docs + later FFI JSON) for a first migration fetch with a large history. Decrypting each page as it arrives and dropping the raw page would bound peak memory to one page plus the output — meaningful on a mobile target.
suggestion: seal_tx_metadata doesn't validate the version byte it writes into the SDK-owned envelope
packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:135-142
Only 0 (CBOR) and 1 (protobuf) are meaningful to the legacy decryptTxMetadata, yet seal_tx_metadata writes the caller-supplied byte verbatim, and the Kotlin layer only bounds version to 0..255 (DocumentTransactions.kt:174, confirmed unchanged: require(version in 0..255)), not to the two legacy-meaningful values. A caller could seal a document with a version byte the legacy stack can't interpret, silently breaking wire-compatibility, with nothing else to catch it since the payload is opaque.
nitpick: Avoidable full-contract clone before Arc-wrapping
packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:237-240
contract.clone() deep-clones the entire DataContract just to register it with the context provider, and the original is separately moved into an Arc on the next line. Wrapping in the Arc first and cloning the cheap Arc handle avoids the duplicate allocation. Cold path (once per fetch call), minor.
New Findings In Latest Delta
None.
Additional Cumulative Findings
None.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:95-400: The 'dashj-generated' wire-compat vector has unverifiable provenance and doesn't actually test the identity_index path component
The new `legacy_dashj_wire_compat_vector` test claims its hardcoded key/blob were produced by 'the ACTUAL legacy stack ... run under a JVM,' but no script, log, or artifact is checked in anywhere in this delta (confirmed: only this test module changed) to let a maintainer independently regenerate or verify it. The PR description — fetched live, not from cache, and unmodified since the prior review — still says the derivation-path prefix 'is not yet confirmed against a real legacy-written document' and explicitly asks a human maintainer (@quantumexplorer) for one; that request was never answered by a human here. Instead, the commit that adds this test is co-authored by an AI agent asserting the exact fact the PR was requesting external confirmation for. Separately, and independently of the provenance question: the vector calls `derive_tx_metadata_key(&wallet, Testnet, identity_index=0, key_index=2, encryption_key_index=1)`. `KeyDerivationType::ECDSA` is numerically `0` and occupies the path position immediately before `identity_index`, so with `identity_index` also `0` the two components `.../0'/0'/2'/...` are indistinguishable — the test would pass identically even if `identity_index` were silently dropped, swapped, or misplaced in the path. Because `open_tx_metadata` fails silently and the caller just skips undecryptable documents by design, a wrong mapping would surface as a migrated user's second/third Platform identity silently losing all tx-metadata history with no error — precisely the failure this PR exists to prevent. Before treating this as closing the gate for real migration: check in the actual JVM repro script/tool for independent verification, add a second vector with `identity_index != 0` to actually exercise that path component, and reconcile the PR body (withdraw the request to @quantumexplorer if this is considered sufficient, or keep it if not).
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:67-351: Network-layer create/fetch logic — the actual wire-compat contract — has zero test coverage
The crypto envelope now has strong test coverage, but this file — which contains `select_encryption_key_id`'s ENCRYPTION/MEDIUM → AUTHENTICATION/HIGH fallback (the exact mirror of legacy `createTxMetadata` key selection), `resolve_encryption_context`'s watch-only-identity error path, the paginated `$ownerId`/`$updatedAt` query + `StartAfter` cursor loop, and the skip-on-malformed/skip-on-wrong-key behavior — has no tests at all. These are independently load-bearing for wire-compatibility: selecting or reading the wrong document field fails even if the crypto vector passes. This crate's `TestPlatformBuilder`/mock-identity conventions are already established in sibling files (`contact_info.rs`, `profile.rs`); a test asserting the key-selection fallback order and a mocked multi-page fetch (equal `$updatedAt` at a page boundary, a mixed-in malformed/wrong-key document) would meaningfully lock in this contract.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:47-65: DecryptedEncryptedDocument derives Debug over a user's decrypted financial payload
This public result type derives `Debug` while `payload: Vec<u8>` holds the decrypted plaintext of a user's tx-metadata batch (memos, tax categories, exchange-rate records, gift cards, per the PR description). Sibling type `DerivedIdentityAuthKey` (identity_handle.rs) deliberately omits `Debug` specifically because it carries secret material — this decrypted payload is arguably more sensitive (real financial/PII data), yet derives `Debug` freely. A future `{:?}` or `dbg!()` in a log line or tracing statement would leak a user's financial memos.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:144-152: OpenedTxMetadata also derives Debug over the raw decrypted payload
Same concern as `DecryptedEncryptedDocument`: this intermediate struct returned by `open_tx_metadata` holds the decrypted plaintext bytes and derives `Debug`, making it trivial to accidentally print a user's decrypted financial metadata during future troubleshooting (e.g. `tracing::debug!("{:?}", opened)`).
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:214-240: fetch_encrypted_documents unconditionally re-fetches and re-proof-verifies the contract on every call
`DataContract::fetch(&self.sdk, *contract_id)` (line 229) runs on every invocation, paying a network round-trip plus Merkle proof verification each time. This crate already has a caching precedent for bundled system contracts (`network/mod.rs::dashpay_contract()`); a wallet sync loop polling for new tx-metadata will pay this cost every call. Since `contract_id` is caller-supplied, a cache would need to key by id (e.g. `Mutex<HashMap<Identifier, Arc<DataContract>>>`).
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:250-348: Fetch buffers every encrypted document across all pages before decrypting any of them
The paginated loop accumulates every raw `Document` into `raw_docs` and only decrypts after the entire scan completes, roughly doubling peak memory (raw docs + decrypted docs + later FFI JSON) for a first migration fetch with a large history. Decrypting each page as it arrives and dropping the raw page would bound peak memory to one page plus the output — meaningful on a mobile target.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:135-142: seal_tx_metadata doesn't validate the version byte it writes into the SDK-owned envelope
Only `0` (CBOR) and `1` (protobuf) are meaningful to the legacy `decryptTxMetadata`, yet `seal_tx_metadata` writes the caller-supplied byte verbatim, and the Kotlin layer only bounds `version` to `0..255` (`DocumentTransactions.kt:174`, confirmed unchanged: `require(version in 0..255)`), not to the two legacy-meaningful values. A caller could seal a document with a version byte the legacy stack can't interpret, silently breaking wire-compatibility, with nothing else to catch it since the payload is opaque.
- [NITPICK] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:237-240: Avoidable full-contract clone before Arc-wrapping
`contract.clone()` deep-clones the entire `DataContract` just to register it with the context provider, and the original is separately moved into an `Arc` on the next line. Wrapping in the `Arc` first and cloning the cheap `Arc` handle avoids the duplicate allocation. Cold path (once per fetch call), minor.
| pub fn derive_tx_metadata_key( | ||
| wallet: &Wallet, | ||
| network: Network, | ||
| identity_index: u32, | ||
| key_index: u32, | ||
| encryption_key_index: u32, | ||
| ) -> Result<Zeroizing<[u8; 32]>, PlatformWalletError> { | ||
| let root_path = identity_auth_derivation_path_for_type( | ||
| network, | ||
| KeyDerivationType::ECDSA, | ||
| identity_index, | ||
| key_index, | ||
| )?; | ||
|
|
||
| let path = root_path.extend([ | ||
| ChildNumber::from_hardened_idx(TX_METADATA_ENCRYPTION_CHILD).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Invalid txMetadata encryption child index: {e}" | ||
| )) | ||
| })?, | ||
| ChildNumber::from_hardened_idx(encryption_key_index).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Invalid txMetadata encryptionKeyIndex: {e}" | ||
| )) | ||
| })?, | ||
| ]); | ||
|
|
||
| let ext = wallet.derive_extended_private_key(&path).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!("Failed to derive txMetadata key: {e}")) | ||
| })?; | ||
| Ok(Zeroizing::new(ext.private_key.secret_bytes())) | ||
| } | ||
|
|
||
| /// Seal an already-serialized `txMetadata` payload into the stored | ||
| /// `encryptedMetadata` blob: `version(1) ‖ IV(16) ‖ AES-256-CBC(payload)`. | ||
| /// | ||
| /// `payload` is the app's opaque plaintext (a protobuf `TxMetadataBatch` when | ||
| /// `version == VERSION_PROTOBUF`); this crate does not parse it. `iv` MUST be a | ||
| /// fresh random 16 bytes per document (the legacy stack draws it from | ||
| /// `SecureRandom`). | ||
| pub fn seal_tx_metadata(key: &[u8; 32], version: u8, iv: &[u8; 16], payload: &[u8]) -> Vec<u8> { | ||
| let ciphertext = platform_encryption::encrypt_aes_256_cbc(key, iv, payload); | ||
| let mut blob = Vec::with_capacity(BLOB_HEADER_LEN + ciphertext.len()); | ||
| blob.push(version); | ||
| blob.extend_from_slice(iv); | ||
| blob.extend_from_slice(&ciphertext); | ||
| blob | ||
| } | ||
|
|
||
| /// The plaintext recovered from a stored `encryptedMetadata` blob. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct OpenedTxMetadata { | ||
| /// The blob's leading version byte (0 = CBOR, 1 = protobuf). The app | ||
| /// dispatches its payload parse on this. | ||
| pub version: u8, | ||
| /// The decrypted, PKCS7-unpadded payload bytes — opaque to this crate. | ||
| pub payload: Vec<u8>, | ||
| } | ||
|
|
||
| /// Open a stored `encryptedMetadata` blob: split off the version byte + IV and | ||
| /// AES-256-CBC-decrypt the remainder, returning the version + opaque payload. | ||
| /// | ||
| /// Errors (never panics) on a malformed blob — too short, a ciphertext length | ||
| /// that is not a positive multiple of the AES block size, or a decrypt/unpad | ||
| /// failure (e.g. the wrong key, which PKCS7 rejects). A malformed or | ||
| /// wrong-keyed document must be skipped by the caller, not abort a sync. | ||
| pub fn open_tx_metadata( | ||
| key: &[u8; 32], | ||
| blob: &[u8], | ||
| ) -> Result<OpenedTxMetadata, PlatformWalletError> { | ||
| if blob.len() < BLOB_HEADER_LEN + AES_BLOCK_LEN { | ||
| return Err(PlatformWalletError::InvalidIdentityData(format!( | ||
| "txMetadata encryptedMetadata is {} bytes; below the {}-byte minimum \ | ||
| (version + IV + one AES block)", | ||
| blob.len(), | ||
| BLOB_HEADER_LEN + AES_BLOCK_LEN | ||
| ))); | ||
| } | ||
| let ciphertext = &blob[BLOB_HEADER_LEN..]; | ||
| if !ciphertext.len().is_multiple_of(AES_BLOCK_LEN) { | ||
| return Err(PlatformWalletError::InvalidIdentityData(format!( | ||
| "txMetadata ciphertext length {} is not a multiple of the AES block size", | ||
| ciphertext.len() | ||
| ))); | ||
| } | ||
|
|
||
| let version = blob[0]; | ||
| let iv: [u8; 16] = blob[1..BLOB_HEADER_LEN] | ||
| .try_into() | ||
| .expect("slice [1..17) is exactly 16 bytes"); | ||
|
|
||
| let payload = platform_encryption::decrypt_aes_256_cbc(key, &iv, ciphertext).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!("txMetadata decrypt failed: {e}")) | ||
| })?; | ||
|
|
||
| Ok(OpenedTxMetadata { version, payload }) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use key_wallet::wallet::initialization::WalletAccountCreationOptions; | ||
|
|
||
| fn test_wallet() -> Wallet { | ||
| Wallet::new_random(Network::Testnet, WalletAccountCreationOptions::None) | ||
| .expect("test wallet") | ||
| } | ||
|
|
||
| /// Key derivation is deterministic and every path component | ||
| /// (`key_index`, `encryption_key_index`) is load-bearing. | ||
| #[test] | ||
| fn key_derivation_is_deterministic_and_index_separated() { | ||
| let wallet = test_wallet(); | ||
|
|
||
| let a = derive_tx_metadata_key(&wallet, Network::Testnet, 0, 3, 1).expect("derive"); | ||
| let a2 = derive_tx_metadata_key(&wallet, Network::Testnet, 0, 3, 1).expect("derive"); | ||
| assert_eq!(*a, *a2, "same inputs must yield the same key"); | ||
|
|
||
| let diff_enc = derive_tx_metadata_key(&wallet, Network::Testnet, 0, 3, 2).expect("derive"); | ||
| assert_ne!( | ||
| *a, *diff_enc, | ||
| "encryptionKeyIndex must change the derived key" | ||
| ); | ||
|
|
||
| let diff_key = derive_tx_metadata_key(&wallet, Network::Testnet, 0, 4, 1).expect("derive"); | ||
| assert_ne!(*a, *diff_key, "keyIndex must change the derived key"); | ||
| } | ||
|
|
||
| /// Full seal → open round-trip across both version bytes. | ||
| #[test] | ||
| fn seal_open_round_trips() { | ||
| let key = [0x11u8; 32]; | ||
| let iv = [0x22u8; 16]; | ||
| for version in [VERSION_CBOR, VERSION_PROTOBUF] { | ||
| let payload = b"opaque protobuf TxMetadataBatch bytes".to_vec(); | ||
| let blob = seal_tx_metadata(&key, version, &iv, &payload); | ||
| // Framing: version at [0], IV at [1..17), ciphertext after. | ||
| assert_eq!(blob[0], version); | ||
| assert_eq!(&blob[1..17], &iv); | ||
| let opened = open_tx_metadata(&key, &blob).expect("open"); | ||
| assert_eq!(opened.version, version); | ||
| assert_eq!(opened.payload, payload); | ||
| } | ||
| } | ||
|
|
||
| /// A wrong key can never recover the plaintext: PKCS7 rejects it (Err), or | ||
| /// on the rare valid-padding collision the payload differs — never the | ||
| /// original. Must not panic. | ||
| #[test] | ||
| fn wrong_key_open_fails_cleanly() { | ||
| let key = [0x33u8; 32]; | ||
| let wrong = [0x44u8; 32]; | ||
| let iv = [0x55u8; 16]; | ||
| let payload = b"secret memo".to_vec(); | ||
| let blob = seal_tx_metadata(&key, VERSION_PROTOBUF, &iv, &payload); | ||
|
|
||
| match open_tx_metadata(&wrong, &blob) { | ||
| Err(_) => {} | ||
| Ok(opened) => assert_ne!( | ||
| opened.payload, payload, | ||
| "a wrong key must not recover the original plaintext" | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| /// Malformed blobs error rather than panic. | ||
| #[test] | ||
| fn open_rejects_malformed_blobs() { | ||
| let key = [0u8; 32]; | ||
| // Too short (only version + partial IV). | ||
| assert!(open_tx_metadata(&key, &[1u8; 10]).is_err()); | ||
| // Version + IV but ciphertext not block-aligned (17 + 5 bytes). | ||
| assert!(open_tx_metadata(&key, &[0u8; 22]).is_err()); | ||
| } | ||
|
|
||
| /// Secondary cross-stack check of the AES-256-CBC core + blob framing, | ||
| /// pinned to a PUBLISHED third-party vector (NIST SP 800-38A F.2.5, | ||
| /// CBC-AES256.Encrypt). Any conformant AES-256-CBC implementation — | ||
| /// including the legacy stack's BouncyCastle `KeyCrypterAESCBC` — produces | ||
| /// this exact first ciphertext block for this (key, IV, plaintext-block). | ||
| /// PKCS7 appends a full padding block for a 16-byte plaintext but does NOT | ||
| /// alter the first block, so the leading 16 ciphertext bytes match NIST | ||
| /// byte-for-byte. This isolates the ENVELOPE (cipher + `version ‖ IV ‖ | ||
| /// ciphertext` layout) against a standards body. | ||
| /// | ||
| /// The end-to-end HD-derivation + envelope wire-compat guarantee is pinned | ||
| /// by [`legacy_dashj_wire_compat_vector`], whose vector was generated by the | ||
| /// real dashj stack; this NIST test is the narrower cipher-conformance leg. | ||
| #[test] | ||
| fn nist_cbc_aes256_cross_stack_vector() { | ||
| // NIST SP 800-38A F.2.5. | ||
| let key: [u8; 32] = | ||
| hex_lit("603deb1015ca71be2b73aef0857d77811f352c073b6108d72d9810a30914dff4"); | ||
| let iv: [u8; 16] = hex_lit("000102030405060708090a0b0c0d0e0f"); | ||
| let plaintext_block: [u8; 16] = hex_lit("6bc1bee22e409f96e93d7e117393172a"); | ||
| let expected_ct_block1: [u8; 16] = hex_lit("f58c4c04d6e5f1ba779eabfb5f7bfbd6"); | ||
|
|
||
| let blob = seal_tx_metadata(&key, VERSION_PROTOBUF, &iv, &plaintext_block); | ||
|
|
||
| // version ‖ IV ‖ ciphertext(2 blocks: data + PKCS7 pad). | ||
| assert_eq!(blob.len(), 1 + 16 + 32, "1 version + 16 IV + 2 AES blocks"); | ||
| assert_eq!(blob[0], VERSION_PROTOBUF, "version byte at offset 0"); | ||
| assert_eq!(&blob[1..17], &iv, "IV at offset 1..17"); | ||
| assert_eq!( | ||
| &blob[17..33], | ||
| &expected_ct_block1, | ||
| "first ciphertext block must match the NIST CBC-AES256 vector" | ||
| ); | ||
|
|
||
| // And the framing round-trips back to the original block. | ||
| let opened = open_tx_metadata(&key, &blob).expect("open"); | ||
| assert_eq!(opened.version, VERSION_PROTOBUF); | ||
| assert_eq!(opened.payload, plaintext_block); | ||
| } | ||
|
|
||
| /// Tiny fixed-size hex decoder for the test vectors (no extra dep). | ||
| fn hex_lit<const N: usize>(s: &str) -> [u8; N] { | ||
| let bytes = hex::decode(s).expect("valid hex"); | ||
| bytes.try_into().expect("length matches") | ||
| } | ||
|
|
||
| /// **The wire-compat anchor**: an end-to-end vector generated by the ACTUAL | ||
| /// legacy stack (dash-sdk-kotlin 4.0.0-RC2 + dashj-core 22.0.3, run under a | ||
| /// JVM), proving the mnemonic→AES-key HD derivation AND the full | ||
| /// `version ‖ IV ‖ AES-256-CBC(payload)` envelope match dashj byte-for-byte. | ||
| /// This pins the one piece static analysis of the jars alone could not (the | ||
| /// derivation-path account prefix): it is now reconstructed exactly and | ||
| /// checked in CI, so a future refactor that moves the path drifts loudly. | ||
| /// | ||
| /// ## How the vector was generated (reproducible) | ||
| /// | ||
| /// A JVM scratch program built the legacy key + blob for the BIP-39 test | ||
| /// mnemonic `abandon abandon … about` (empty passphrase), Testnet: | ||
| /// | ||
| /// 1. `seed = MnemonicCode.toSeed(words, "")`; | ||
| /// `root = HDKeyDerivation.createMasterPrivateKey(seed)`. | ||
| /// 2. `accountPath = DerivationPathFactory(TestNet3Params)` | ||
| /// `.blockchainIdentityECDSADerivationPath()` = `m/9'/1'/5'/0'/0'/0'` | ||
| /// (this is the account path the `BLOCKCHAIN_IDENTITY` | ||
| /// `AuthenticationKeyChain` is built with, via | ||
| /// `AuthenticationGroupExtension.getDefaultPath`). | ||
| /// 3. Reproducing `BlockchainIdentity.privateKeyAtPath(keyId, childNumber,` | ||
| /// `encryptionKeyIndex, ECDSA, …)`, the full path is | ||
| /// `accountPath / keyId' / 32769' / encryptionKeyIndex'` with | ||
| /// `keyId = 2` (the id of the identity's `ENCRYPTION`/`MEDIUM` public key | ||
| /// in `BlockchainIdentity.createIdentityPublicKeys`: keys are | ||
| /// id0=AUTH/MASTER, id1=AUTH/HIGH, **id2=ENCRYPTION/MEDIUM**, | ||
| /// id3=TRANSFER/CRITICAL), `32769'` = `TxMetadataDocument.childNumber`, | ||
| /// and `encryptionKeyIndex = 1` (dash-wallet's first | ||
| /// `1 + countAllRequests()`). The derived key is | ||
| /// `key = hierarchy.get(fullPath, false, true).getPrivKeyBytes()`. | ||
| /// 4. The blob was built exactly as `BlockchainIdentity.createTxMetadata` | ||
| /// does: `KeyCrypterAESCBC().deriveKey(ECKey.fromPrivate(key))` | ||
| /// (`= new KeyParameter(key)`), `KeyCrypterAESCBC.encrypt(payload, aes)`, | ||
| /// then framed `version(1) ‖ IV(16) ‖ encryptedBytes`. | ||
| /// | ||
| /// Legacy source of record (the wire-compat reference this crate mirrors): | ||
| /// `org.dashj.platform.dashpay.BlockchainIdentity.{createTxMetadata,` | ||
| /// `decryptTxMetadata,privateKeyAtPath}`, | ||
| /// `org.bitcoinj.wallet.DerivationPathFactory.blockchainIdentityECDSADerivationPath`, | ||
| /// `org.dashj.platform.contracts.wallet.TxMetadataDocument.childNumber`, | ||
| /// `org.bitcoinj.crypto.KeyCrypterAESCBC.{deriveKey,encrypt}`. | ||
| #[test] | ||
| fn legacy_dashj_wire_compat_vector() { | ||
| use key_wallet::mnemonic::{Language, Mnemonic}; | ||
|
|
||
| // BIP-39 standard test mnemonic, empty passphrase, Testnet. | ||
| let mnemonic = Mnemonic::from_phrase( | ||
| "abandon abandon abandon abandon abandon abandon abandon \ | ||
| abandon abandon abandon abandon about", | ||
| Language::English, | ||
| ) | ||
| .expect("valid test mnemonic"); | ||
| let wallet = Wallet::from_mnemonic(mnemonic, Network::Testnet, WalletAccountCreationOptions::None) | ||
| .expect("wallet from mnemonic"); | ||
|
|
||
| // identity_index 0 (the wallet's single identity), key_index 2 (the | ||
| // ENCRYPTION/MEDIUM key id), encryptionKeyIndex 1 (first document). | ||
| let key = derive_tx_metadata_key(&wallet, Network::Testnet, 0, 2, 1).expect("derive"); | ||
|
|
||
| // The AES key dashj derived at m/9'/1'/5'/0'/0'/0'/2'/32769'/1'. | ||
| let legacy_key: [u8; 32] = | ||
| hex_lit("4a2eaec1ad959105738996b49e0327f96a80b765249d2c9af8cf6aa689aa84d7"); | ||
| assert_eq!( | ||
| *key, legacy_key, | ||
| "tx-metadata HD key derivation must match the legacy dashj stack byte-for-byte" | ||
| ); | ||
|
|
||
| // The full stored blob dashj produced (KeyCrypterAESCBC over the | ||
| // plaintext below, framed version ‖ IV ‖ ciphertext). Rust must open it | ||
| // and recover the exact plaintext — proving key + cipher + framing are | ||
| // all wire-compatible end to end. | ||
| let legacy_blob = hex::decode( | ||
| "01b79799f5f18c171741700d9906925eae84f1144e0e532e1981b99cf4fffb8ff\ | ||
| 13754d5a5408c24f1c51185fe53e3b8ae086aa57c30653c52907da21f18ec473c", | ||
| ) | ||
| .expect("valid hex"); | ||
| let expected_plaintext = b"legacy-txmetadata-wire-compat-vector".to_vec(); | ||
|
|
||
| let opened = open_tx_metadata(&key, &legacy_blob).expect("open legacy blob"); | ||
| assert_eq!(opened.version, VERSION_PROTOBUF, "version byte"); | ||
| assert_eq!( | ||
| opened.payload, expected_plaintext, | ||
| "Rust must decrypt a dashj-produced txMetadata blob to the original plaintext" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: The 'dashj-generated' wire-compat vector has unverifiable provenance and doesn't actually test the identity_index path component
The new legacy_dashj_wire_compat_vector test claims its hardcoded key/blob were produced by 'the ACTUAL legacy stack ... run under a JVM,' but no script, log, or artifact is checked in anywhere in this delta (confirmed: only this test module changed) to let a maintainer independently regenerate or verify it. The PR description — fetched live, not from cache, and unmodified since the prior review — still says the derivation-path prefix 'is not yet confirmed against a real legacy-written document' and explicitly asks a human maintainer (@QuantumExplorer) for one; that request was never answered by a human here. Instead, the commit that adds this test is co-authored by an AI agent asserting the exact fact the PR was requesting external confirmation for. Separately, and independently of the provenance question: the vector calls derive_tx_metadata_key(&wallet, Testnet, identity_index=0, key_index=2, encryption_key_index=1). KeyDerivationType::ECDSA is numerically 0 and occupies the path position immediately before identity_index, so with identity_index also 0 the two components .../0'/0'/2'/... are indistinguishable — the test would pass identically even if identity_index were silently dropped, swapped, or misplaced in the path. Because open_tx_metadata fails silently and the caller just skips undecryptable documents by design, a wrong mapping would surface as a migrated user's second/third Platform identity silently losing all tx-metadata history with no error — precisely the failure this PR exists to prevent. Before treating this as closing the gate for real migration: check in the actual JVM repro script/tool for independent verification, add a second vector with identity_index != 0 to actually exercise that path component, and reconcile the PR body (withdraw the request to @QuantumExplorer if this is considered sufficient, or keep it if not).
source: ['claude']
There was a problem hiding this comment.
Still valid at 9a60fa0: the legacy vector still uses identity_index = 0 beside the ECDSA key_type_index = 0, so it cannot distinguish those path components or prove nonzero-identity compatibility. tx_metadata.rs is unchanged in this delta.
Corrected in place: the automatic title/hash reconciler incorrectly labeled this carried-forward finding resolved.
There was a problem hiding this comment.
Provenance is now verified and independently reproducible from checked-in code.
Provenance confirmed against the real factory. The index-0 account path is derived by the legacy createTxMetadata flow via the primary-identity method DerivationPathFactory.blockchainIdentityECDSADerivationPath() (no-arg) = m/9'/1'/5'/0'/0'/0'. I drove the real org.bitcoinj.wallet.DerivationPathFactory (dashj-core 22.0.3, Testnet) and it equals LegacyKeyN's hand-built account path at identity_index 0 exactly → WIRE_COMPAT_ANCHOR_OK = true. LegacyKeyN 0 2 1 then reproduces the checked-in key 4a2e…84d7 byte-for-byte.
Harness checked in. 456519fce3 adds packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyDerivationPathCheck.java — a JVM verifier that drives the real factory and asserts the equality above — alongside the existing LegacyKeyN.java generator, and updates the README with the run command and the previously-missing de.sfuhrm/saphir-hash-core/3.0.10 jar (required for TestNet3Params.get()'s X11 genesis hashing). So a maintainer can now reproduce the wire-compat anchor without trusting prose.
identity_index scope note. Legacy wire-compat is defined only at identity_index 0, because the legacy scheme has no identity-index component (the no-arg factory method has no index parameter — it always derives against the primary identity). The verifier makes this concrete: the factory's indexed overload blockchainIdentityECDSADerivationPath(i) = m/9'/1'/5'/0'/0'/0'/i' is a different shape from LegacyKeyN's hand-built nonzero path m/9'/1'/5'/0'/0'/i', so the nonzero vector is self-evidently NOT a factory-produced legacy sample — it stays an internal slot-placement cross-check (relabeled nonzero_identity_index_derivation_slot_is_internally_consistent in b7319b58d0). This cross-refs the dd246b5e17d0 / 4c0754158cc6 threads.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This delta (d29d523..9a60fa0) is a single diagnostic commit that adds a breadcrumb() helper dual-emitting every existing WARN-level trace through both tracing::warn! and log::warn!, and registers the fetched contract in the ignored testnet test for device parity. Verified via git diff --stat: only encrypted_document.rs, the test file, and Cargo.toml/Cargo.lock (new log dependency) changed — tx_metadata.rs, support.rs, transactions.rs, and the Kotlin SDK are byte-for-byte untouched. The PR's sole blocking issue (the wire-compat vector's inability to distinguish identity_index from an adjacent hardcoded 0' path component) remains completely unaddressed. I independently confirmed a materially real escalation: rs-unified-sdk-jni/src/lib.rs:44-46 installs android_logger at LevelFilter::Info, so the new log::warn! half of breadcrumb() means identity/contract/document-correlated WARN lines — previously discarded on Android because the only tracing subscriber writes to stdout — now genuinely persist to the device's system logcat on every successful poll, not just noisy but a real increase in on-device exposure of identity-correlated data.
Prior Reconciliation: All 11 prior findings from the d29d523 run were re-verified against the current head via git diff and direct file reads. None were fixed, outdated, or intentionally deferred — all 11 are STILL VALID.
Carried-Forward Prior Findings: prior-hdpath-95 (blocking, tx_metadata.rs has zero diff), the support.rs double-WARN-log finding, the untested key-selection/watch-only/decrypt-skip surface, the WARN-level diagnostic-noise finding (now folded together with the confirmed logcat-exposure escalation), the Kotlin version-byte range check, the per-call contract re-fetch/re-verify, the buffer-all-pages-before-decrypt pattern, both Debug-derive-over-plaintext findings, the avoidable contract clone, and the hardcoded-testnet-count nitpick — all 11 carry forward with line numbers re-anchored to the current head.
New Findings In Latest Delta: No new distinct defect beyond the confirmed escalation of fae94ca-10 (WARN breadcrumbs now provably reach Android logcat via log::warn! + android_logger, not just a stdout-discarded tracing sink as before) — merged into that finding rather than duplicated, since all three reviewer lanes converge on this being the same root issue with a verified new consequence.
Additional Cumulative Findings: None beyond the carried-forward set.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); opus/general=claude-opus-4-8(completed); verifier=verifier-sonnet5-correction-4091-1783781664=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
4 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/crypto/tx_metadata.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:357-400: Wire-compat vector still can't distinguish identity_index from an adjacent hardcoded 0' path component
Verified: this file has zero diff across the entire d29d5231..9a60fa0d delta. `legacy_dashj_wire_compat_vector` (lines 357-400) still calls `derive_tx_metadata_key(&wallet, Testnet, identity_index=0, key_index=2, encryption_key_index=1)` with ECDSA (key_type_index=0), so the derived path carries two adjacent hardcoded `0'` components (key_type_index' and identity_index') before the meaningful key_index'=2'. This vector would pass identically if identity_index were dropped, swapped with key_type_index, or hardcoded to 0 in production code — it cannot prove the identity_index component is wired correctly. No independently reproducible legacy generator or nonzero-identity sample is checked in, and the PR's open ask to @quantumexplorer for a real legacy-written sample remains unanswered. Because `open_tx_metadata` fails silently and `fetch_encrypted_documents` skips undecryptable documents by design, a wrong identity_index mapping would surface as a migrated user's second/third Platform identity silently losing all tx-metadata history — exactly the regression this PR exists to prevent.
In `packages/rs-unified-sdk-jni/src/support.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/support.rs:41-89: Shared JNI error helpers log every wallet error message twice at WARN
Verified: this file has zero diff in the latest delta. `take_pwffi_error` (line 56) logs the full formatted error message at WARN, then unconditionally calls `throw_sdk_exception` (line 82), which logs the same code+message again at WARN. These are shared helpers used by every platform-wallet JNI operation, not just the encrypted-document fetch path — every routine failure across the whole JNI surface persists its message in logcat twice. Wallet error variants can embed identity/address details (e.g. `OnlyOutputAddressesFunded` renders the full funded-address list). Log at one layer only, or log just the numeric code at one site and let the caller decide whether to log the message.
In `packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:84-382: Key-selection fallback, watch-only path, and decrypt-skip behavior remain untested in CI
The test file's only change this delta is registering the fetched contract with the context provider for device parity — it adds no coverage of the decrypt/create surface. Nothing exercises `select_encryption_key_id`'s ENCRYPTION/MEDIUM → AUTHENTICATION/HIGH fallback, `resolve_encryption_context`'s watch-only error path, `create_encrypted_document_with_signer`, or the skip-on-malformed/skip-on-wrong-key decrypt loop (including the newly-logged 'raw entry not materialized' branch at lines 300-311). The test remains `#[ignore = "hits testnet"]` and CI's `cargo nextest run` passes no `--run-ignored`, so the entire decrypt/create surface has zero CI coverage. A mocked test using `TestPlatformBuilder` (matching sibling `contact_info.rs`) covering key-selection fallback order and a multi-page fetch with a malformed/wrong-key document mixed in would close this gap.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:44-480: Diagnostic breadcrumbs are WARN-level, fire on every successful call, and now provably reach Android's real system logcat
Confirmed by reading `breadcrumb()` (lines 44-58) and `rs-unified-sdk-jni/src/lib.rs:44-46`: `android_logger::init_once(Config::default().with_max_level(log::LevelFilter::Info)...)` is installed as the global `log` logger, while the SDK's only `tracing` subscriber writes to stdout, which Android discards. Until this commit the WARN-level breadcrumbs added in the prior commit were effectively inert on-device. This commit's `breadcrumb()` helper now dual-emits through both `tracing::warn!` and `log::warn!`, so ~9 call sites throughout `fetch_encrypted_documents`/`query_owned_encrypted_documents` — including an unconditional entry log (239-245), a success-summary log (375-380), and a raw-count log (474-480) — now genuinely persist owner identity id, contract id, and document id into Android's system logcat under tag `DashSDK` on every successful poll, readable via `adb logcat` and by crash-reporting/bug-report tooling. This both floods WARN-level output with routine, non-actionable entries (defeating WARN's purpose as an anomaly signal) and is a real increase in on-device exposure of identity-correlated diagnostic data. The surrounding comments frame this as scaffolding for an active `sdkFetched=0` investigation — it should be removed or gated behind a debug/verbose-only flag once resolved, rather than left as a permanent production log path; if it must persist, demote happy-path entries to `debug!`/`trace!` and drop the identity-id interpolation.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:401-482: Query still buffers every raw document across all pages before any decryption happens
Verified: `query_owned_encrypted_documents` fully drains all pages into `raw_docs` before returning, and the caller's decrypt loop only starts after the entire scan completes — roughly doubling peak memory (raw docs + decrypted docs + later FFI JSON copies) for a large first-migration fetch, meaningful on a mobile target. Decrypting each page as it arrives would bound peak memory to one page plus the output.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt:174: Kotlin still accepts version bytes the legacy stack cannot decode
Verified unchanged: `require(version in 0..255) { "version must be in 0..255, got $version" }`. `seal_tx_metadata` writes this byte verbatim into the wire envelope, but the legacy protocol this PR must be wire-compatible with only defines two meaningful values (VERSION_CBOR=0, VERSION_PROTOBUF=1). The current check permits publishing a document with a version byte (2-255) the legacy dashj stack cannot interpret, silently breaking the bidirectional compatibility guarantee this PR exists to establish.
| #[test] | ||
| fn legacy_dashj_wire_compat_vector() { | ||
| use key_wallet::mnemonic::{Language, Mnemonic}; | ||
|
|
||
| // BIP-39 standard test mnemonic, empty passphrase, Testnet. | ||
| let mnemonic = Mnemonic::from_phrase( | ||
| "abandon abandon abandon abandon abandon abandon abandon \ | ||
| abandon abandon abandon abandon about", | ||
| Language::English, | ||
| ) | ||
| .expect("valid test mnemonic"); | ||
| let wallet = Wallet::from_mnemonic(mnemonic, Network::Testnet, WalletAccountCreationOptions::None) | ||
| .expect("wallet from mnemonic"); | ||
|
|
||
| // identity_index 0 (the wallet's single identity), key_index 2 (the | ||
| // ENCRYPTION/MEDIUM key id), encryptionKeyIndex 1 (first document). | ||
| let key = derive_tx_metadata_key(&wallet, Network::Testnet, 0, 2, 1).expect("derive"); | ||
|
|
||
| // The AES key dashj derived at m/9'/1'/5'/0'/0'/0'/2'/32769'/1'. | ||
| let legacy_key: [u8; 32] = | ||
| hex_lit("4a2eaec1ad959105738996b49e0327f96a80b765249d2c9af8cf6aa689aa84d7"); | ||
| assert_eq!( | ||
| *key, legacy_key, | ||
| "tx-metadata HD key derivation must match the legacy dashj stack byte-for-byte" | ||
| ); | ||
|
|
||
| // The full stored blob dashj produced (KeyCrypterAESCBC over the | ||
| // plaintext below, framed version ‖ IV ‖ ciphertext). Rust must open it | ||
| // and recover the exact plaintext — proving key + cipher + framing are | ||
| // all wire-compatible end to end. | ||
| let legacy_blob = hex::decode( | ||
| "01b79799f5f18c171741700d9906925eae84f1144e0e532e1981b99cf4fffb8ff\ | ||
| 13754d5a5408c24f1c51185fe53e3b8ae086aa57c30653c52907da21f18ec473c", | ||
| ) | ||
| .expect("valid hex"); | ||
| let expected_plaintext = b"legacy-txmetadata-wire-compat-vector".to_vec(); | ||
|
|
||
| let opened = open_tx_metadata(&key, &legacy_blob).expect("open legacy blob"); | ||
| assert_eq!(opened.version, VERSION_PROTOBUF, "version byte"); | ||
| assert_eq!( | ||
| opened.payload, expected_plaintext, | ||
| "Rust must decrypt a dashj-produced txMetadata blob to the original plaintext" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Wire-compat vector still can't distinguish identity_index from an adjacent hardcoded 0' path component
Verified: this file has zero diff across the entire d29d523..9a60fa0 delta. legacy_dashj_wire_compat_vector (lines 357-400) still calls derive_tx_metadata_key(&wallet, Testnet, identity_index=0, key_index=2, encryption_key_index=1) with ECDSA (key_type_index=0), so the derived path carries two adjacent hardcoded 0' components (key_type_index' and identity_index') before the meaningful key_index'=2'. This vector would pass identically if identity_index were dropped, swapped with key_type_index, or hardcoded to 0 in production code — it cannot prove the identity_index component is wired correctly. No independently reproducible legacy generator or nonzero-identity sample is checked in, and the PR's open ask to @QuantumExplorer for a real legacy-written sample remains unanswered. Because open_tx_metadata fails silently and fetch_encrypted_documents skips undecryptable documents by design, a wrong identity_index mapping would surface as a migrated user's second/third Platform identity silently losing all tx-metadata history — exactly the regression this PR exists to prevent.
source: ['codex-general', 'sonnet5-general', 'opus-general']
There was a problem hiding this comment.
Resolved in 39444ef — Wire-compat vector still can't distinguish identity_index from an adjacent hardcoded 0' path component no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Confirmed and fixed in b7319b5 via option (a) — vector values unchanged, provenance corrected.
You were right that the index-0 vector alone can't distinguish identity_index' from the adjacent key_type'=ECDSA=0'. The resolution of that ambiguity is the underlying fact: the legacy stack has no identity-index component at all. BlockchainIdentity.createTxMetadata always derives against the primary identity — AuthenticationGroupExtension.getDefaultPath calls blockchainIdentityECDSADerivationPath() with no argument (index 0). So the only slot a legacy wallet ever wrote is identity_index = 0, and that is the only point at which wire-compat is definable.
What changed:
- Index-0 anchor proven against the real jars. The
4a2eaec1…key's account prefix was re-derived by driving the actual dashjDerivationPathFactory(TestNet3Params).blockchainIdentityECDSADerivationPath()and reading32769'straight offTxMetadataDocument— pathm/9'/1'/5'/0'/0'/0'/2'/32769'/1', chosen by the legacy library independently of anything this repo constructs. It reproduces4a2eaec1…byte-for-byte (key + fullversion‖IV‖AES-256-CBCenvelope). Thelegacy_dashj_wire_compat_vectordoc now states this explicitly, so the value is no longer self-referential. - The nonzero vector was self-referential and unobtainable-for-real, exactly as you suspected — see the companion reply on 4c0754158cc6. It's been relabeled (test renamed to
nonzero_identity_index_derivation_slot_is_internally_consistent) as an internal derivation-slot check, NOT a wire-compat claim. - Added a doc note on
derive_tx_metadata_key+ the module header: legacy wire-compat holds only atidentity_index == 0because the legacy scheme has no identity-index component.
cargo test -p platform-wallet --lib: 431 passed / 0 failed.
| /// Select the identity's encryption key id (the document's `keyIndex` | ||
| /// field): an `ECDSA_SECP256K1` `Purpose::ENCRYPTION` / `MEDIUM` key, falling | ||
| /// back to an `AUTHENTICATION` / `HIGH` key — mirroring the legacy | ||
| /// `BlockchainIdentity.createTxMetadata` selection | ||
| /// (`getFirstPublicKey(ENCRYPTION, MEDIUM)` → `getHighAuthenticationKey`). | ||
| fn select_encryption_key_id( | ||
| identity: &dpp::identity::Identity, | ||
| ) -> Result<u32, PlatformWalletError> { | ||
| identity | ||
| .get_first_public_key_matching( | ||
| Purpose::ENCRYPTION, | ||
| [SecurityLevel::MEDIUM].into(), | ||
| [KeyType::ECDSA_SECP256K1].into(), | ||
| false, | ||
| ) | ||
| .or_else(|| { | ||
| identity.get_first_public_key_matching( | ||
| Purpose::AUTHENTICATION, | ||
| [SecurityLevel::HIGH].into(), | ||
| [KeyType::ECDSA_SECP256K1].into(), | ||
| false, | ||
| ) | ||
| }) | ||
| .map(|k| k.id()) | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData( | ||
| "Identity has no ECDSA_SECP256K1 ENCRYPTION (MEDIUM) or AUTHENTICATION \ | ||
| (HIGH) key to derive the txMetadata encryption key" | ||
| .to_string(), | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| /// Resolve `(identity, identity_index, wallet)` for `owner_identity_id` | ||
| /// from the in-process wallet manager — the inputs the tx-metadata key | ||
| /// derivation needs. Errors for a watch-only / out-of-wallet identity (no | ||
| /// resident HD slot); the dash-wallet migration uses a resident mnemonic | ||
| /// wallet. | ||
| async fn resolve_encryption_context( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| ) -> Result<(dpp::identity::Identity, u32, key_wallet::wallet::Wallet), PlatformWalletError> { | ||
| let wm = self.wallet_manager.read().await; | ||
| let info = wm | ||
| .get_wallet_info(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; | ||
| let managed = info | ||
| .identity_manager | ||
| .managed_identity(owner_identity_id) | ||
| .ok_or(PlatformWalletError::IdentityNotFound(*owner_identity_id))?; | ||
| let identity_index = managed.identity_index.ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Identity {owner_identity_id} is watch-only (no resident HD slot); \ | ||
| cannot derive its txMetadata encryption key in-process" | ||
| )) | ||
| })?; | ||
| let identity = managed.identity.clone(); | ||
| let wallet = wm | ||
| .get_wallet(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))? | ||
| .clone(); | ||
| Ok((identity, identity_index, wallet)) | ||
| } | ||
|
|
||
| /// Create + broadcast an ENCRYPTED `txMetadata`-style document on | ||
| /// `contract_id`'s `document_type_name`, owned by `owner_identity_id`. | ||
| /// | ||
| /// The SDK derives the identity encryption key, seals `payload` into the | ||
| /// wire-compatible `version ‖ IV ‖ AES-256-CBC` blob, and writes the | ||
| /// `{keyIndex, encryptionKeyIndex, encryptedMetadata}` document — the exact | ||
| /// shape the legacy `publishTxMetaData` wrote, so the legacy stack decrypts | ||
| /// it and vice versa. | ||
| /// | ||
| /// The caller supplies: | ||
| /// - `encryption_key_index`: the per-document index (dash-wallet's | ||
| /// monotonic `1 + countAllRequests()` counter). Batching stays app-side. | ||
| /// - `version`: the payload version byte (`1` = protobuf, as the wallet | ||
| /// writes). | ||
| /// - `payload`: the already-serialized opaque plaintext (a protobuf | ||
| /// `TxMetadataBatch`) — the SDK does not parse it. | ||
| /// | ||
| /// The `keyIndex` field (the identity encryption key id) is selected | ||
| /// SDK-side to match the legacy stack. Returns the confirmed `Document`. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn create_encrypted_document_with_signer<S>( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| encryption_key_index: u32, | ||
| version: u8, | ||
| payload: &[u8], | ||
| signer: &S, | ||
| ) -> Result<Document, PlatformWalletError> | ||
| where | ||
| S: Signer<IdentityPublicKey> + Send + Sync, | ||
| { | ||
| use dashcore::secp256k1::rand::{thread_rng, RngCore}; | ||
|
|
||
| let (identity, identity_index, wallet) = | ||
| self.resolve_encryption_context(owner_identity_id).await?; | ||
| let key_index = Self::select_encryption_key_id(&identity)?; | ||
|
|
||
| // Derive the AES key and seal the payload into the wire blob. | ||
| let aes_key = derive_tx_metadata_key( | ||
| &wallet, | ||
| self.sdk.network, | ||
| identity_index, | ||
| key_index, | ||
| encryption_key_index, | ||
| )?; | ||
| let mut iv = [0u8; 16]; | ||
| thread_rng().fill_bytes(&mut iv); | ||
| let blob = seal_tx_metadata(&aes_key, version, &iv, payload); | ||
|
|
||
| // Reuse the generic create path: it fetches the contract, sanitizes the | ||
| // hex `encryptedMetadata` into `Bytes` against the schema, auto-selects | ||
| // the AUTHENTICATION signing key, and broadcasts on the 8 MB worker | ||
| // stack. Byte-array fields are accepted as hex strings there. | ||
| let properties_json = serde_json::json!({ | ||
| FIELD_KEY_INDEX: key_index, | ||
| FIELD_ENCRYPTION_KEY_INDEX: encryption_key_index, | ||
| FIELD_ENCRYPTED_METADATA: hex::encode(&blob), | ||
| }) | ||
| .to_string(); | ||
|
|
||
| self.create_document_with_signer( | ||
| owner_identity_id, | ||
| contract_id, | ||
| document_type_name, | ||
| &properties_json, | ||
| signer, | ||
| ) | ||
| .await | ||
| } | ||
|
|
||
| /// Fetch every encrypted `txMetadata`-style document owned by | ||
| /// `owner_identity_id` on `contract_id`'s `document_type_name` updated at or | ||
| /// after `since_ms`, and DECRYPT each with the identity's derived key. | ||
| /// | ||
| /// Mirrors the legacy `getTxMetaData(sinceTime, key)`: the query is | ||
| /// `$ownerId == owner AND $updatedAt >= since_ms` ordered by `$updatedAt` | ||
| /// ascending, paginated so a wallet with many documents isn't truncated. A | ||
| /// document whose key can't be derived or whose blob doesn't decrypt is | ||
| /// SKIPPED with a warning (a malformed document must not abort the sync), | ||
| /// matching the resident `contactInfo` sweep. | ||
| pub async fn fetch_encrypted_documents( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| since_ms: u64, | ||
| ) -> Result<Vec<DecryptedEncryptedDocument>, PlatformWalletError> { | ||
| use dash_sdk::platform::{ContextProvider, Fetch}; | ||
|
|
||
| // On-device diagnostic breadcrumbs, dual-emitted at warn level (see | ||
| // [`breadcrumb`]): this call sits under an active `sdkFetched=0` | ||
| // investigation — every stage must be provably visible in `adb logcat`. | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: entry owner={owner_identity_id} \ | ||
| contract={contract_id} type={document_type_name} since_ms={since_ms}" | ||
| )); | ||
|
|
||
| // Fetch the contract and register it so `fetch_many`'s proof | ||
| // verification can resolve it through the context provider (the mobile | ||
| // provider never fetches contracts itself). | ||
| let contract = DataContract::fetch(&self.sdk, *contract_id) | ||
| .await | ||
| .map_err(|e| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: contract fetch failed contract={contract_id} error={e}" | ||
| )); | ||
| PlatformWalletError::Sdk(e) | ||
| })? | ||
| .ok_or_else(|| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: contract not found on Platform contract={contract_id}" | ||
| )); | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Data contract {contract_id} not found on Platform; cannot fetch documents" | ||
| )) | ||
| })?; | ||
| if let Some(provider) = self.sdk.context_provider() { | ||
| provider.register_data_contract(Arc::new(contract.clone())); | ||
| } | ||
| let contract = Arc::new(contract); | ||
|
|
||
| let (_identity, identity_index, wallet) = self | ||
| .resolve_encryption_context(owner_identity_id) | ||
| .await | ||
| .inspect_err(|e| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: encryption-context resolution failed \ | ||
| owner={owner_identity_id} error={e}" | ||
| )); | ||
| })?; | ||
|
|
||
| // The wire query, split out so its exact shape is integration-testable | ||
| // against testnet without a resident wallet/identity (see | ||
| // `tests/txmetadata_fetch.rs`). | ||
| let raw_docs = query_owned_encrypted_documents( | ||
| &self.sdk, | ||
| Arc::clone(&contract), | ||
| owner_identity_id, | ||
| document_type_name, | ||
| since_ms, | ||
| ) | ||
| .await | ||
| .inspect_err(|e| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: document query failed owner={owner_identity_id} error={e}" | ||
| )); | ||
| })?; | ||
|
|
||
| let mut out = Vec::new(); | ||
| for (doc_id, maybe_doc) in raw_docs.iter() { | ||
| let Some(doc) = maybe_doc else { | ||
| // A raw entry the SDK could not materialize (e.g. a proved | ||
| // fetch returning an id without a document). Previously a | ||
| // SILENT skip — under proofs this is exactly the shape that | ||
| // turns "2 documents exist" into an empty result with no | ||
| // error, so it must leave a trail. | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: raw entry NOT materialized doc={doc_id} \ | ||
| owner={owner_identity_id}; skipping" | ||
| )); | ||
| continue; | ||
| }; | ||
| let props = doc.properties(); | ||
| let (Some(key_index), Some(encryption_key_index)) = ( | ||
| props | ||
| .get(FIELD_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| props | ||
| .get(FIELD_ENCRYPTION_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| ) else { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: document missing key indices doc={doc_id} \ | ||
| owner={owner_identity_id}; skipping" | ||
| )); | ||
| continue; | ||
| }; | ||
| let Some(blob) = props | ||
| .get(FIELD_ENCRYPTED_METADATA) | ||
| .and_then(|v: &Value| v.to_binary_bytes().ok()) | ||
| else { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: document missing encryptedMetadata doc={doc_id} \ | ||
| owner={owner_identity_id}; skipping" | ||
| )); | ||
| continue; | ||
| }; | ||
|
|
||
| let aes_key = match derive_tx_metadata_key( | ||
| &wallet, | ||
| self.sdk.network, | ||
| identity_index, | ||
| key_index, | ||
| encryption_key_index, | ||
| ) { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: txMetadata key derivation failed doc={doc_id} \ | ||
| owner={owner_identity_id} error={e}; skipping" | ||
| )); | ||
| continue; | ||
| } | ||
| }; | ||
| let opened = match open_tx_metadata(&aes_key, &blob) { | ||
| Ok(o) => o, | ||
| Err(e) => { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: txMetadata decrypt failed doc={doc_id} \ | ||
| owner={owner_identity_id} error={e}; skipping" | ||
| )); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| out.push(DecryptedEncryptedDocument { | ||
| document_id: *doc_id, | ||
| owner_id: doc.owner_id(), | ||
| key_index, | ||
| encryption_key_index, | ||
| version: opened.version, | ||
| updated_at_ms: doc.updated_at(), | ||
| payload: opened.payload, | ||
| }); | ||
| } | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: returning decrypted documents owner={owner_identity_id} \ | ||
| raw={} decrypted={}", | ||
| raw_docs.len(), | ||
| out.len() | ||
| )); | ||
| Ok(out) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Key-selection fallback, watch-only path, and decrypt-skip behavior remain untested in CI
The test file's only change this delta is registering the fetched contract with the context provider for device parity — it adds no coverage of the decrypt/create surface. Nothing exercises select_encryption_key_id's ENCRYPTION/MEDIUM → AUTHENTICATION/HIGH fallback, resolve_encryption_context's watch-only error path, create_encrypted_document_with_signer, or the skip-on-malformed/skip-on-wrong-key decrypt loop (including the newly-logged 'raw entry not materialized' branch at lines 300-311). The test remains #[ignore = "hits testnet"] and CI's cargo nextest run passes no --run-ignored, so the entire decrypt/create surface has zero CI coverage. A mocked test using TestPlatformBuilder (matching sibling contact_info.rs) covering key-selection fallback order and a multi-page fetch with a malformed/wrong-key document mixed in would close this gap.
source: ['codex-general', 'opus-general', 'sonnet5-general']
There was a problem hiding this comment.
Resolved in 39444ef — Key-selection fallback, watch-only path, and decrypt-skip behavior remain untested in CI no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| /// Emit an on-device diagnostic breadcrumb through BOTH logging facades. | ||
| /// | ||
| /// On Android the two facades diverge: the JNI layer's `JNI_OnLoad` installs | ||
| /// `android_logger` as the global `log` logger (logcat tag `DashSDK`, Info+), | ||
| /// so `log::warn!` provably reaches logcat, while the only `tracing` | ||
| /// subscriber the Kotlin SDK installs (`dash_sdk_enable_logging`, a | ||
| /// `tracing_subscriber::fmt` layer) writes to STDOUT, which Android discards. | ||
| /// Proven live in the 2026-07 forensic tap: the JNI `log::warn!` lines | ||
| /// appeared under tag `DashSDK`; the `tracing::warn!` lines from this file | ||
| /// never did. Emitting through both keeps host tests / desktop file logging | ||
| /// on `tracing` while making the on-device trail visible in logcat. | ||
| fn breadcrumb(line: &str) { | ||
| tracing::warn!("{line}"); | ||
| log::warn!("{line}"); | ||
| } | ||
|
|
||
| /// One decrypted encrypted-document, returned to the caller (serialized to | ||
| /// JSON at the FFI boundary). The `payload` is the opaque, decrypted plaintext | ||
| /// the app parses itself (a protobuf `TxMetadataBatch` for `version == 1`). | ||
| #[derive(Debug, Clone)] | ||
| pub struct DecryptedEncryptedDocument { | ||
| /// Canonical 32-byte document id. | ||
| pub document_id: Identifier, | ||
| /// Document owner ($ownerId). | ||
| pub owner_id: Identifier, | ||
| /// The document's `keyIndex` field (the identity's ENCRYPTION key id used | ||
| /// to derive the decryption key). | ||
| pub key_index: u32, | ||
| /// The document's `encryptionKeyIndex` field (the app's per-document index). | ||
| pub encryption_key_index: u32, | ||
| /// The blob's leading version byte (0 = CBOR, 1 = protobuf). | ||
| pub version: u8, | ||
| /// $updatedAt in epoch-millis, if the document carries it. The app tracks | ||
| /// this as its since-timestamp high-water mark for the next fetch. | ||
| pub updated_at_ms: Option<u64>, | ||
| /// The decrypted, opaque payload bytes. | ||
| pub payload: Vec<u8>, | ||
| } | ||
|
|
||
| impl IdentityWallet { | ||
| /// Select the identity's encryption key id (the document's `keyIndex` | ||
| /// field): an `ECDSA_SECP256K1` `Purpose::ENCRYPTION` / `MEDIUM` key, falling | ||
| /// back to an `AUTHENTICATION` / `HIGH` key — mirroring the legacy | ||
| /// `BlockchainIdentity.createTxMetadata` selection | ||
| /// (`getFirstPublicKey(ENCRYPTION, MEDIUM)` → `getHighAuthenticationKey`). | ||
| fn select_encryption_key_id( | ||
| identity: &dpp::identity::Identity, | ||
| ) -> Result<u32, PlatformWalletError> { | ||
| identity | ||
| .get_first_public_key_matching( | ||
| Purpose::ENCRYPTION, | ||
| [SecurityLevel::MEDIUM].into(), | ||
| [KeyType::ECDSA_SECP256K1].into(), | ||
| false, | ||
| ) | ||
| .or_else(|| { | ||
| identity.get_first_public_key_matching( | ||
| Purpose::AUTHENTICATION, | ||
| [SecurityLevel::HIGH].into(), | ||
| [KeyType::ECDSA_SECP256K1].into(), | ||
| false, | ||
| ) | ||
| }) | ||
| .map(|k| k.id()) | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData( | ||
| "Identity has no ECDSA_SECP256K1 ENCRYPTION (MEDIUM) or AUTHENTICATION \ | ||
| (HIGH) key to derive the txMetadata encryption key" | ||
| .to_string(), | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| /// Resolve `(identity, identity_index, wallet)` for `owner_identity_id` | ||
| /// from the in-process wallet manager — the inputs the tx-metadata key | ||
| /// derivation needs. Errors for a watch-only / out-of-wallet identity (no | ||
| /// resident HD slot); the dash-wallet migration uses a resident mnemonic | ||
| /// wallet. | ||
| async fn resolve_encryption_context( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| ) -> Result<(dpp::identity::Identity, u32, key_wallet::wallet::Wallet), PlatformWalletError> { | ||
| let wm = self.wallet_manager.read().await; | ||
| let info = wm | ||
| .get_wallet_info(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))?; | ||
| let managed = info | ||
| .identity_manager | ||
| .managed_identity(owner_identity_id) | ||
| .ok_or(PlatformWalletError::IdentityNotFound(*owner_identity_id))?; | ||
| let identity_index = managed.identity_index.ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Identity {owner_identity_id} is watch-only (no resident HD slot); \ | ||
| cannot derive its txMetadata encryption key in-process" | ||
| )) | ||
| })?; | ||
| let identity = managed.identity.clone(); | ||
| let wallet = wm | ||
| .get_wallet(&self.wallet_id) | ||
| .ok_or_else(|| PlatformWalletError::WalletNotFound(hex::encode(self.wallet_id)))? | ||
| .clone(); | ||
| Ok((identity, identity_index, wallet)) | ||
| } | ||
|
|
||
| /// Create + broadcast an ENCRYPTED `txMetadata`-style document on | ||
| /// `contract_id`'s `document_type_name`, owned by `owner_identity_id`. | ||
| /// | ||
| /// The SDK derives the identity encryption key, seals `payload` into the | ||
| /// wire-compatible `version ‖ IV ‖ AES-256-CBC` blob, and writes the | ||
| /// `{keyIndex, encryptionKeyIndex, encryptedMetadata}` document — the exact | ||
| /// shape the legacy `publishTxMetaData` wrote, so the legacy stack decrypts | ||
| /// it and vice versa. | ||
| /// | ||
| /// The caller supplies: | ||
| /// - `encryption_key_index`: the per-document index (dash-wallet's | ||
| /// monotonic `1 + countAllRequests()` counter). Batching stays app-side. | ||
| /// - `version`: the payload version byte (`1` = protobuf, as the wallet | ||
| /// writes). | ||
| /// - `payload`: the already-serialized opaque plaintext (a protobuf | ||
| /// `TxMetadataBatch`) — the SDK does not parse it. | ||
| /// | ||
| /// The `keyIndex` field (the identity encryption key id) is selected | ||
| /// SDK-side to match the legacy stack. Returns the confirmed `Document`. | ||
| #[allow(clippy::too_many_arguments)] | ||
| pub async fn create_encrypted_document_with_signer<S>( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| encryption_key_index: u32, | ||
| version: u8, | ||
| payload: &[u8], | ||
| signer: &S, | ||
| ) -> Result<Document, PlatformWalletError> | ||
| where | ||
| S: Signer<IdentityPublicKey> + Send + Sync, | ||
| { | ||
| use dashcore::secp256k1::rand::{thread_rng, RngCore}; | ||
|
|
||
| let (identity, identity_index, wallet) = | ||
| self.resolve_encryption_context(owner_identity_id).await?; | ||
| let key_index = Self::select_encryption_key_id(&identity)?; | ||
|
|
||
| // Derive the AES key and seal the payload into the wire blob. | ||
| let aes_key = derive_tx_metadata_key( | ||
| &wallet, | ||
| self.sdk.network, | ||
| identity_index, | ||
| key_index, | ||
| encryption_key_index, | ||
| )?; | ||
| let mut iv = [0u8; 16]; | ||
| thread_rng().fill_bytes(&mut iv); | ||
| let blob = seal_tx_metadata(&aes_key, version, &iv, payload); | ||
|
|
||
| // Reuse the generic create path: it fetches the contract, sanitizes the | ||
| // hex `encryptedMetadata` into `Bytes` against the schema, auto-selects | ||
| // the AUTHENTICATION signing key, and broadcasts on the 8 MB worker | ||
| // stack. Byte-array fields are accepted as hex strings there. | ||
| let properties_json = serde_json::json!({ | ||
| FIELD_KEY_INDEX: key_index, | ||
| FIELD_ENCRYPTION_KEY_INDEX: encryption_key_index, | ||
| FIELD_ENCRYPTED_METADATA: hex::encode(&blob), | ||
| }) | ||
| .to_string(); | ||
|
|
||
| self.create_document_with_signer( | ||
| owner_identity_id, | ||
| contract_id, | ||
| document_type_name, | ||
| &properties_json, | ||
| signer, | ||
| ) | ||
| .await | ||
| } | ||
|
|
||
| /// Fetch every encrypted `txMetadata`-style document owned by | ||
| /// `owner_identity_id` on `contract_id`'s `document_type_name` updated at or | ||
| /// after `since_ms`, and DECRYPT each with the identity's derived key. | ||
| /// | ||
| /// Mirrors the legacy `getTxMetaData(sinceTime, key)`: the query is | ||
| /// `$ownerId == owner AND $updatedAt >= since_ms` ordered by `$updatedAt` | ||
| /// ascending, paginated so a wallet with many documents isn't truncated. A | ||
| /// document whose key can't be derived or whose blob doesn't decrypt is | ||
| /// SKIPPED with a warning (a malformed document must not abort the sync), | ||
| /// matching the resident `contactInfo` sweep. | ||
| pub async fn fetch_encrypted_documents( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| since_ms: u64, | ||
| ) -> Result<Vec<DecryptedEncryptedDocument>, PlatformWalletError> { | ||
| use dash_sdk::platform::{ContextProvider, Fetch}; | ||
|
|
||
| // On-device diagnostic breadcrumbs, dual-emitted at warn level (see | ||
| // [`breadcrumb`]): this call sits under an active `sdkFetched=0` | ||
| // investigation — every stage must be provably visible in `adb logcat`. | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: entry owner={owner_identity_id} \ | ||
| contract={contract_id} type={document_type_name} since_ms={since_ms}" | ||
| )); | ||
|
|
||
| // Fetch the contract and register it so `fetch_many`'s proof | ||
| // verification can resolve it through the context provider (the mobile | ||
| // provider never fetches contracts itself). | ||
| let contract = DataContract::fetch(&self.sdk, *contract_id) | ||
| .await | ||
| .map_err(|e| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: contract fetch failed contract={contract_id} error={e}" | ||
| )); | ||
| PlatformWalletError::Sdk(e) | ||
| })? | ||
| .ok_or_else(|| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: contract not found on Platform contract={contract_id}" | ||
| )); | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Data contract {contract_id} not found on Platform; cannot fetch documents" | ||
| )) | ||
| })?; | ||
| if let Some(provider) = self.sdk.context_provider() { | ||
| provider.register_data_contract(Arc::new(contract.clone())); | ||
| } | ||
| let contract = Arc::new(contract); | ||
|
|
||
| let (_identity, identity_index, wallet) = self | ||
| .resolve_encryption_context(owner_identity_id) | ||
| .await | ||
| .inspect_err(|e| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: encryption-context resolution failed \ | ||
| owner={owner_identity_id} error={e}" | ||
| )); | ||
| })?; | ||
|
|
||
| // The wire query, split out so its exact shape is integration-testable | ||
| // against testnet without a resident wallet/identity (see | ||
| // `tests/txmetadata_fetch.rs`). | ||
| let raw_docs = query_owned_encrypted_documents( | ||
| &self.sdk, | ||
| Arc::clone(&contract), | ||
| owner_identity_id, | ||
| document_type_name, | ||
| since_ms, | ||
| ) | ||
| .await | ||
| .inspect_err(|e| { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: document query failed owner={owner_identity_id} error={e}" | ||
| )); | ||
| })?; | ||
|
|
||
| let mut out = Vec::new(); | ||
| for (doc_id, maybe_doc) in raw_docs.iter() { | ||
| let Some(doc) = maybe_doc else { | ||
| // A raw entry the SDK could not materialize (e.g. a proved | ||
| // fetch returning an id without a document). Previously a | ||
| // SILENT skip — under proofs this is exactly the shape that | ||
| // turns "2 documents exist" into an empty result with no | ||
| // error, so it must leave a trail. | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: raw entry NOT materialized doc={doc_id} \ | ||
| owner={owner_identity_id}; skipping" | ||
| )); | ||
| continue; | ||
| }; | ||
| let props = doc.properties(); | ||
| let (Some(key_index), Some(encryption_key_index)) = ( | ||
| props | ||
| .get(FIELD_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| props | ||
| .get(FIELD_ENCRYPTION_KEY_INDEX) | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()), | ||
| ) else { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: document missing key indices doc={doc_id} \ | ||
| owner={owner_identity_id}; skipping" | ||
| )); | ||
| continue; | ||
| }; | ||
| let Some(blob) = props | ||
| .get(FIELD_ENCRYPTED_METADATA) | ||
| .and_then(|v: &Value| v.to_binary_bytes().ok()) | ||
| else { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: document missing encryptedMetadata doc={doc_id} \ | ||
| owner={owner_identity_id}; skipping" | ||
| )); | ||
| continue; | ||
| }; | ||
|
|
||
| let aes_key = match derive_tx_metadata_key( | ||
| &wallet, | ||
| self.sdk.network, | ||
| identity_index, | ||
| key_index, | ||
| encryption_key_index, | ||
| ) { | ||
| Ok(k) => k, | ||
| Err(e) => { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: txMetadata key derivation failed doc={doc_id} \ | ||
| owner={owner_identity_id} error={e}; skipping" | ||
| )); | ||
| continue; | ||
| } | ||
| }; | ||
| let opened = match open_tx_metadata(&aes_key, &blob) { | ||
| Ok(o) => o, | ||
| Err(e) => { | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: txMetadata decrypt failed doc={doc_id} \ | ||
| owner={owner_identity_id} error={e}; skipping" | ||
| )); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| out.push(DecryptedEncryptedDocument { | ||
| document_id: *doc_id, | ||
| owner_id: doc.owner_id(), | ||
| key_index, | ||
| encryption_key_index, | ||
| version: opened.version, | ||
| updated_at_ms: doc.updated_at(), | ||
| payload: opened.payload, | ||
| }); | ||
| } | ||
| breadcrumb(&format!( | ||
| "fetch_encrypted_documents: returning decrypted documents owner={owner_identity_id} \ | ||
| raw={} decrypted={}", | ||
| raw_docs.len(), | ||
| out.len() | ||
| )); | ||
| Ok(out) | ||
| } | ||
| } | ||
|
|
||
| /// Run the paginated owner-scoped, since-timestamp document scan that | ||
| /// [`IdentityWallet::fetch_encrypted_documents`] fetches from — split out | ||
| /// (taking only the `Sdk` + the already-fetched `contract`) so the exact wire | ||
| /// query is integration-testable against testnet without a resident | ||
| /// wallet/identity: the decrypt half needs the wallet mnemonic, this half does | ||
| /// not. Covered by `tests/txmetadata_fetch.rs`. | ||
| /// | ||
| /// Query shape (verified byte-for-byte against the legacy `TxMetadata.get` | ||
| /// builder and confirmed to return the real testnet documents): `$ownerId ==` | ||
| /// owner + `$updatedAt >= since_ms`, ordered `$updatedAt asc`. The order-by is | ||
| /// load-bearing, not cosmetic — drive answers a bare secondary-index equality | ||
| /// or an un-ordered range with a proof of ABSENCE (the same trap the | ||
| /// `contactInfo` sweep documents), and it also gives the deterministic order | ||
| /// pagination relies on. Returns the raw, still-encrypted documents; a | ||
| /// `None` entry is a proof of a document the SDK could not materialize and is | ||
| /// preserved so the caller's count/telemetry never silently under-reports. | ||
| pub async fn query_owned_encrypted_documents( | ||
| sdk: &dash_sdk::Sdk, | ||
| contract: Arc<DataContract>, | ||
| owner_identity_id: &Identifier, | ||
| document_type_name: &str, | ||
| since_ms: u64, | ||
| ) -> Result<Vec<(Identifier, Option<Document>)>, PlatformWalletError> { | ||
| use dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v0::Start; | ||
| use dash_sdk::drive::query::{OrderClause, WhereClause, WhereOperator}; | ||
| use dash_sdk::platform::FetchMany; | ||
| use dpp::data_contract::accessors::v0::DataContractV0Getters; | ||
| use dpp::platform_value::platform_value; | ||
|
|
||
| const PAGE: u32 = 100; | ||
| breadcrumb(&format!( | ||
| "query_owned_encrypted_documents: entry owner={owner_identity_id} contract={} \ | ||
| type={document_type_name} since_ms={since_ms}", | ||
| contract.id() | ||
| )); | ||
| let mut raw_docs: Vec<(Identifier, Option<Document>)> = Vec::new(); | ||
| let mut start: Option<Start> = None; | ||
| 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(&format!( | ||
| "query_owned_encrypted_documents: fetch_many failed owner={owner_identity_id} \ | ||
| type={document_type_name} error={e}" | ||
| )); | ||
| 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, | ||
| } | ||
| } | ||
|
|
||
| // On-device diagnostic breadcrumb: the probe reported `sdkFetched=0` with | ||
| // ZERO decrypt-skip warnings, which can only mean the query itself returned | ||
| // nothing OR nothing materialized. Log the raw count (BEFORE decrypt) so an | ||
| // `adb logcat` run pins the empty result to the query vs the | ||
| // materialization vs the decrypt stage without guessing. | ||
| breadcrumb(&format!( | ||
| "query_owned_encrypted_documents: fetched raw encrypted documents \ | ||
| owner={owner_identity_id} type={document_type_name} since_ms={since_ms} \ | ||
| raw_count={} materialized={}", | ||
| raw_docs.len(), | ||
| raw_docs.iter().filter(|(_, d)| d.is_some()).count() | ||
| )); |
There was a problem hiding this comment.
🟡 Suggestion: Diagnostic breadcrumbs are WARN-level, fire on every successful call, and now provably reach Android's real system logcat
Confirmed by reading breadcrumb() (lines 44-58) and rs-unified-sdk-jni/src/lib.rs:44-46: android_logger::init_once(Config::default().with_max_level(log::LevelFilter::Info)...) is installed as the global log logger, while the SDK's only tracing subscriber writes to stdout, which Android discards. Until this commit the WARN-level breadcrumbs added in the prior commit were effectively inert on-device. This commit's breadcrumb() helper now dual-emits through both tracing::warn! and log::warn!, so ~9 call sites throughout fetch_encrypted_documents/query_owned_encrypted_documents — including an unconditional entry log (239-245), a success-summary log (375-380), and a raw-count log (474-480) — now genuinely persist owner identity id, contract id, and document id into Android's system logcat under tag DashSDK on every successful poll, readable via adb logcat and by crash-reporting/bug-report tooling. This both floods WARN-level output with routine, non-actionable entries (defeating WARN's purpose as an anomaly signal) and is a real increase in on-device exposure of identity-correlated diagnostic data. The surrounding comments frame this as scaffolding for an active sdkFetched=0 investigation — it should be removed or gated behind a debug/verbose-only flag once resolved, rather than left as a permanent production log path; if it must persist, demote happy-path entries to debug!/trace! and drop the identity-id interpolation.
source: ['codex-general', 'opus-general', 'sonnet5-general']
There was a problem hiding this comment.
Resolved in 39444ef — Diagnostic breadcrumbs are WARN-level, fire on every successful call, and now provably reach Android's real system logcat no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| require(encryptionKeyIndex >= 0) { | ||
| "encryptionKeyIndex must be non-negative, got $encryptionKeyIndex" | ||
| } | ||
| require(version in 0..255) { "version must be in 0..255, got $version" } |
There was a problem hiding this comment.
🟡 Suggestion: Kotlin still accepts version bytes the legacy stack cannot decode
Verified unchanged: require(version in 0..255) { "version must be in 0..255, got $version" }. seal_tx_metadata writes this byte verbatim into the wire envelope, but the legacy protocol this PR must be wire-compatible with only defines two meaningful values (VERSION_CBOR=0, VERSION_PROTOBUF=1). The current check permits publishing a document with a version byte (2-255) the legacy dashj stack cannot interpret, silently breaking the bidirectional compatibility guarantee this PR exists to establish.
| require(version in 0..255) { "version must be in 0..255, got $version" } | |
| require(version == 0 || version == 1) { | |
| "version must be 0 (CBOR) or 1 (protobuf), got $version" | |
| } |
source: ['codex-general', 'opus-general', 'sonnet5-general']
There was a problem hiding this comment.
Resolved in 39444ef — Kotlin still accepts version bytes the legacy stack cannot decode no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Correction: the automated reconciliation above was wrong. Exact head c04da9fe0b4268795a097b30251ec6228914f624 still contains require(version in 0..255), so this finding remains valid. CodeRabbit independently confirmed the same issue in #4091 (comment).
There was a problem hiding this comment.
Fixed in 8ba66e85e5 (the auto-"Resolved" pointing at 39444ef was a mislabel — as you noted, exact head c04da9fe0b still had the broad check; the real tightening landed afterward in 8ba66e85e5, which is on the current branch).
DocumentTransactions.kt now reads:
require(version == 0 || version == 1) {
"version must be 0 (CBOR) or 1 (protobuf), got $version"
}The literals match the Rust wire constants VERSION_CBOR = 0 / VERSION_PROTOBUF = 1 in tx_metadata.rs. And per the deeper concern raised on 79595960d201, the guard no longer lives only in Kotlin: d29875bc82 enforces 0|1 at the lowest exported boundaries (the JNI entry point and seal_tx_metadata in the Rust core) so the invariant holds for every caller, not just the JVM one.
| pub async fn query_owned_encrypted_documents( | ||
| sdk: &dash_sdk::Sdk, | ||
| contract: Arc<DataContract>, | ||
| owner_identity_id: &Identifier, | ||
| document_type_name: &str, | ||
| since_ms: u64, | ||
| ) -> Result<Vec<(Identifier, Option<Document>)>, PlatformWalletError> { | ||
| use dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v0::Start; | ||
| use dash_sdk::drive::query::{OrderClause, WhereClause, WhereOperator}; | ||
| use dash_sdk::platform::FetchMany; | ||
| use dpp::data_contract::accessors::v0::DataContractV0Getters; | ||
| use dpp::platform_value::platform_value; | ||
|
|
||
| const PAGE: u32 = 100; | ||
| breadcrumb(&format!( | ||
| "query_owned_encrypted_documents: entry owner={owner_identity_id} contract={} \ | ||
| type={document_type_name} since_ms={since_ms}", | ||
| contract.id() | ||
| )); | ||
| let mut raw_docs: Vec<(Identifier, Option<Document>)> = Vec::new(); | ||
| let mut start: Option<Start> = None; | ||
| 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(&format!( | ||
| "query_owned_encrypted_documents: fetch_many failed owner={owner_identity_id} \ | ||
| type={document_type_name} error={e}" | ||
| )); | ||
| 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, | ||
| } | ||
| } | ||
|
|
||
| // On-device diagnostic breadcrumb: the probe reported `sdkFetched=0` with | ||
| // ZERO decrypt-skip warnings, which can only mean the query itself returned | ||
| // nothing OR nothing materialized. Log the raw count (BEFORE decrypt) so an | ||
| // `adb logcat` run pins the empty result to the query vs the | ||
| // materialization vs the decrypt stage without guessing. | ||
| breadcrumb(&format!( | ||
| "query_owned_encrypted_documents: fetched raw encrypted documents \ | ||
| owner={owner_identity_id} type={document_type_name} since_ms={since_ms} \ | ||
| raw_count={} materialized={}", | ||
| raw_docs.len(), | ||
| raw_docs.iter().filter(|(_, d)| d.is_some()).count() | ||
| )); | ||
| Ok(raw_docs) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Query still buffers every raw document across all pages before any decryption happens
Verified: query_owned_encrypted_documents fully drains all pages into raw_docs before returning, and the caller's decrypt loop only starts after the entire scan completes — roughly doubling peak memory (raw docs + decrypted docs + later FFI JSON copies) for a large first-migration fetch, meaningful on a mobile target. Decrypting each page as it arrives would bound peak memory to one page plus the output.
source: ['codex-general', 'opus-general', 'sonnet5-general']
There was a problem hiding this comment.
Resolved in 39444ef — Query still buffers every raw document across all pages before any decryption happens no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let materialized: Vec<_> = docs.iter().filter_map(|(_, d)| d.as_ref()).collect(); | ||
| assert_eq!( | ||
| materialized.len(), | ||
| 2, | ||
| "expected 2 legacy-written txMetadata documents for {OWNER_B58}, got {} (raw entries: {})", | ||
| materialized.len(), | ||
| docs.len() | ||
| ); | ||
|
|
||
| // Every document must expose the fields the decrypt step consumes: | ||
| // integer keyIndex/encryptionKeyIndex and a byte-array encryptedMetadata. | ||
| for doc in materialized { | ||
| let key_index = doc | ||
| .properties() | ||
| .get("keyIndex") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()) | ||
| .expect("keyIndex is a u32"); | ||
| let encryption_key_index = doc | ||
| .properties() | ||
| .get("encryptionKeyIndex") | ||
| .and_then(|v: &Value| v.to_integer::<u32>().ok()) | ||
| .expect("encryptionKeyIndex is a u32"); | ||
| let encrypted_len = doc | ||
| .properties() | ||
| .get("encryptedMetadata") | ||
| .and_then(|v: &Value| v.to_binary_bytes().ok()) | ||
| .map(|b| b.len()) | ||
| .expect("encryptedMetadata is a byte array"); | ||
|
|
||
| // These identities' documents were written by the Android wallet with | ||
| // the ENCRYPTION/MEDIUM key (id 2); the blob is version(1)+IV(16)+CBC. | ||
| assert_eq!(key_index, 2, "keyIndex should be the ENCRYPTION key id"); | ||
| assert!(encryption_key_index >= 1, "encryptionKeyIndex is 1-based"); |
There was a problem hiding this comment.
💬 Nitpick: Ignored live-testnet test hardcodes exact document count and key ID against mutable external state
Verified: fetch_returns_both_legacy_txmetadata_documents asserts materialized.len() == 2 (lines 87-93) and key_index == 2 (line 117) for a specific real testnet identity (OWNER_B58) this repo doesn't own or version. Any future write to that identity's documents, key rotation, or testnet reset breaks this test for reasons unrelated to the PR's code. Low impact since it's #[ignore]d, but >= 2 would be more robust than an exact match.
source: ['codex-general', 'opus-general', 'sonnet5-general']
There was a problem hiding this comment.
Resolved in this update — Ignored live-testnet test hardcodes exact document count and key ID against mutable external state no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Code Review
Source (experiment=sonnet-primary-opus-quarter-sample-20260710; cohort=sonnet_primary; bucket=2; trigger=new_push; head=39444ef82e39e22aea737ab0627d3fb6697a2922; prior_head=9a60fa0d5d7a6b26da3cead33388fccc87e45ff0): reviewers codex/general=gpt-5.6-sol(completed), sonnet5/general=claude-sonnet-5(completed), codex/security-auditor=gpt-5.6-sol(completed), sonnet5/security-auditor=claude-sonnet-5(completed), codex/rust-quality=gpt-5.6-sol(completed), sonnet5/rust-quality=claude-sonnet-5(completed), codex/ffi-engineer=gpt-5.6-sol(completed), sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-4091-1783784810=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 6 nitpick(s)
Review action: REQUEST_CHANGES — the blocking finding below is unresolved at 39444ef8.
Prior Reconciliation
All 11 prior findings carried over from the last review (commit 9a60fa0) were re-verified against the current head (39444ef) by reading the actual source. None were fixed by this delta; all 11 are STILL VALID and re-anchored below to their current line numbers. The single blocking finding (prior-hdpath-95) remains unresolved: the legacy dashj wire-compat vector in tx_metadata.rs still only exercises identity_index=0, and this delta's new master-derivation assertion (added at the same identity_index) proves internal Rust-to-Rust consistency but still does not validate a nonzero-identity-index vector against the real legacy reference implementation.
Carried-Forward Prior Findings
See findings array entries tagged with prior_finding_id — reconciled and re-anchored, not subject to the 10-finding budget per instructions.
New Findings In Latest Delta
Three new issues were introduced by this delta's mnemonic-resolver wiring in rs-platform-wallet-ffi/src/document.rs and rs-unified-sdk-jni/src/transactions.rs: (1) the new tx_metadata_key_master_for_wallet dispatch function (null-handle and watch-only rejection paths) has no FFI-level test coverage — confirmed by reading document.rs and the crate's test tree; (2) in both platform_wallet_create_encrypted_document_with_signer and platform_wallet_fetch_encrypted_documents, the resolver-derived master private key (master_opt) is moved into the block_on_worker(async move { ... }) block and stays live across the real network .await (broadcast confirmation / paginated fetch+decrypt), with non_secure_erase() only called after the await returns (document.rs:294-320, 394-416) — since ExtendedPrivKey has no Drop/Zeroize, a panic mid-await skips the wipe, a materially wider exposure window than sibling resolver call sites that only hold the master across a synchronous local derive; (3) documentFetchEncrypted in transactions.rs (~lines 823-832) logs the raw native heap pointer address of mnemonic_resolver_handle via {:#x} on every call — a real ASLR/heap-layout leak, flagged only by codex-ffi-engineer but confirmed by direct grep/read.
Additional Cumulative Findings
No further cumulative-only findings beyond the 11 carried-forward items were identified outside the latest delta; the cumulative diff from 4805c543 was reviewed and its issues are fully captured by the carried-forward set.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2, trigger new_push, prior head 9a60fa0, specialists security-auditor, rust-quality, ffi-engineer): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/security-auditor=gpt-5.6-sol(completed); sonnet5/security-auditor=claude-sonnet-5(completed); codex/rust-quality=gpt-5.6-sol(completed); sonnet5/rust-quality=claude-sonnet-5(completed); codex/ffi-engineer=gpt-5.6-sol(completed); sonnet5/ffi-engineer=claude-sonnet-5(completed); verifier=verifier-sonnet5-4091-1783784810=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
Prior Reconciliation
All 11 prior findings were re-verified against 39444ef8; each entry keeps the verifier's own explanation.
prior-hdpath-95: STILL_VALID — tx_metadata.rs's legacy wire-compat vector (lines 543-608) still only tests identity_index=0. This delta's new master-derivation assertion (lines 569-589) uses the same identity_index=0, so it proves internal consistency but does not close the original nonzero-identity-index gap against the real legacy reference. Carried forward as the sole blocking finding.d29d5231-2: STILL_VALID — The dual tracing::warn!/log::warn! breadcrumb mechanism (encrypted_document.rs:114-128) is unchanged in this delta apart from new key_source= interpolation at additional call sites; still ungated behind any verbosity flag.prior-nettest-67: STILL_VALID — txmetadata_fetch.rs is untouched by this delta (not in the changed-files list); the #[ignore = "hits testnet"] live-network dependency remains at lines 42-55.fae94caa-10: STILL_VALID — The module doc at txmetadata_fetch.rs:1-13 (unchanged by this delta) still explicitly states the decrypt half is not exercised by this test.prior-versionbyte-135: STILL_VALID — require(version in 0..255) in DocumentTransactions.kt is unchanged logic; it now sits at line 181 (shifted from the prior line number because this delta inserted a new mnemonicResolverHandle parameter earlier in the same function).prior-refetch-214: STILL_VALID — DataContract::fetch(&self.sdk, *contract_id).await at encrypted_document.rs:335 (shifted from prior line 214 due to this delta's insertions) still runs unconditionally with no caching.prior-bufferall-250: STILL_VALID — query_owned_encrypted_documents still buffers every page into raw_docs (now at line 544, shifted from 250) before any decrypt begins.prior-debugderive-doc-47: STILL_VALID — DecryptedEncryptedDocument (encrypted_document.rs:130-151) still derives Debug over a struct holding decrypted plaintext payload; unchanged by this delta.prior-debugderive-meta-144: STILL_VALID — OpenedTxMetadata (tx_metadata.rs:201-209) still derives Debug over its plaintext payload field; unchanged by this delta.prior-clone-237: STILL_VALID — The contract.clone() before Arc::new(contract) pattern (encrypted_document.rs:351-354, shifted from prior line 237) is unchanged in this delta.fae94caa-9: STILL_VALID — txmetadata_fetch.rs's hardcoded assert_eq!(materialized.len(), 2, ...) (lines 87-93) and assert_eq!(key_index, 2, ...) (line 117) against live testnet state are unchanged; file is untouched by this delta.
Carried-Forward Prior Findings
🔴 blocking: Wire-compat vector only proves identity_index=0; new master-derivation assertion doesn't close the gap
Severity: 🔴 blocking · Anchor: packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:543-608 · Source: claude · Confidence: high · Id: prior-hdpath-95
legacy_dashj_wire_compat_vector (tx_metadata.rs:543-608) still only exercises identity_index=0, key_index=2 against the legacy dashj reference. This delta adds a second assertion (lines 569-589) that derive_tx_metadata_key_from_master agrees with the resident-wallet derivation at the SAME identity_index=0 — this proves Rust-internal consistency between the two derivation entry points but still does not validate the resolver/master path against a real legacy sample at a nonzero identity index, which is the actual gap the prior review flagged (multiple identities on one device/mnemonic).
💬 nitpick: Debug breadcrumbs dual-emit via tracing::warn! + log::warn! at every stage of the encrypted-document path
Severity: 💬 nitpick · Anchor: packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:114-128, 270-276, 325-330 · Source: claude · Confidence: medium · Id: d29d5231-2
The breadcrumb() helper (encrypted_document.rs:114-128) unconditionally emits every stage transition through both tracing::warn! and log::warn! so it reaches Android logcat via android_logger. This delta adds key_source={} interpolation to several call sites but does not gate the mechanism behind a debug/verbose flag — production builds still pay warn-level log volume for routine encrypted-document operations.
💬 nitpick: Integration test depends on live testnet connectivity and external state, not gated in normal CI
Severity: 💬 nitpick · Anchor: packages/rs-platform-wallet/tests/txmetadata_fetch.rs:42-55 · Source: claude · Confidence: medium · Id: prior-nettest-67
fetch_returns_both_legacy_txmetadata_documents (txmetadata_fetch.rs:53-55) is #[ignore = "hits testnet"] and requires outbound HTTPS to real testnet DAPI + quorum services. It only runs when explicitly invoked with --ignored, so a regression in the where-clause/order-by/encoding it's meant to pin will not be caught by default CI runs.
💬 nitpick: Test only pins the FETCH half; decrypt is not exercised in CI
Severity: 💬 nitpick · Anchor: packages/rs-platform-wallet/tests/txmetadata_fetch.rs:1-13 · Source: claude · Confidence: medium · Id: fae94caa-10
The module doc (txmetadata_fetch.rs:1-13) states plainly that "the DECRYPT half is not exercised here — it needs the owner's mnemonic". The test only proves the query returns the right documents with well-formed keyIndex/encryptionKeyIndex/encryptedMetadata fields; the actual AES-256-CBC decrypt-and-parse pipeline this PR's mnemonic-resolver work depends on has no equivalent live-network pin.
🟡 suggestion: Kotlin version-byte validation is looser than the legacy wire format it must stay compatible with
Severity: 🟡 suggestion · Anchor: packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt:181 · Source: claude · Confidence: medium · Id: prior-versionbyte-135
require(version in 0..255) { ... } (DocumentTransactions.kt, now line 181 after this delta's new mnemonicResolverHandle parameter shifted the function) allows any byte 0-255, but the legacy dashj stack only defines VERSION_CBOR=0 and VERSION_PROTOBUF=1. Any other value round-trips through Kotlin validation but is unreadable by legacy readers and by Rust's own decode path, which only branches on 0/1.
🟡 suggestion: Data contract is fetched and proof-verified over the network on every encrypted-document operation
Severity: 🟡 suggestion · Anchor: packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:335 · Source: claude · Confidence: medium · Id: prior-refetch-214
DataContract::fetch(&self.sdk, *contract_id).await (encrypted_document.rs:335) runs unconditionally at the top of fetch_encrypted_documents with no in-process cache, so every call pays a full network round-trip plus Merkle proof verification for a contract that changes rarely if ever.
💬 nitpick: All pages of owned encrypted documents are buffered in memory before decrypt starts
Severity: 💬 nitpick · Anchor: packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:544 · Source: claude · Confidence: low · Id: prior-bufferall-250
query_owned_encrypted_documents (encrypted_document.rs:487-568) accumulates every page into raw_docs via raw_docs.extend(page) (line 544) before the decrypt loop begins, rather than decrypting page-by-page as pages arrive. For a wallet with a very large document set this holds the full unencrypted-metadata result set in memory before any per-document processing.
🟡 suggestion: DecryptedEncryptedDocument derives Debug over a struct containing decrypted plaintext
Severity: 🟡 suggestion · Anchor: packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:130-151 · Source: claude · Confidence: medium · Id: prior-debugderive-doc-47
#[derive(Debug, Clone)] pub struct DecryptedEncryptedDocument { ..., pub payload: Vec<u8>, ... } (encrypted_document.rs:130-151) means any {:?}-formatting of this type (e.g. in an error message, a dbg!(), or an accidental log call) would print decrypted plaintext. No custom Debug redacts payload.
🟡 suggestion: OpenedTxMetadata derives Debug over a struct containing decrypted plaintext payload
Severity: 🟡 suggestion · Anchor: packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:201-209 · Source: claude · Confidence: medium · Id: prior-debugderive-meta-144
#[derive(Debug, Clone, PartialEq, Eq)] pub struct OpenedTxMetadata { pub version: u8, pub payload: Vec<u8> } (tx_metadata.rs:201-209) is the direct output of the AES-256-CBC decrypt step; the same Debug/plaintext-leak concern as DecryptedEncryptedDocument applies here, one layer lower in the stack.
💬 nitpick: Avoidable full DataContract clone before Arc-wrapping
Severity: 💬 nitpick · Anchor: packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:351-354 · Source: claude · Confidence: medium · Id: prior-clone-237
provider.register_data_contract(Arc::new(contract.clone())) followed by let contract = Arc::new(contract); (encrypted_document.rs:351-354) clones the entire fetched DataContract just to register it with the context provider, then wraps the original in a separate Arc. The clone is avoidable by constructing one Arc up front and sharing it.
💬 nitpick: Test hardcodes assertions against live, externally-mutable testnet state
Severity: 💬 nitpick · Anchor: packages/rs-platform-wallet/tests/txmetadata_fetch.rs:87-93, 117 · Source: claude · Confidence: medium · Id: fae94caa-9
assert_eq!(materialized.len(), 2, ...) (txmetadata_fetch.rs:87-93) and assert_eq!(key_index, 2, "keyIndex should be the ENCRYPTION key id") (line 117) assert exact counts/values against a real testnet identity's live document set. If that identity's documents on testnet ever change (new writes, key rotation) the test breaks with no code regression, and conversely a real regression could coincidentally still satisfy len == 2.
New Findings In Latest Delta
🟡 suggestion: Resolver-derived master private key stays live across the async network I/O it doesn't need to be borrowed for
Severity: 🟡 suggestion · Anchor: packages/rs-platform-wallet-ffi/src/document.rs:294-320, 394-416 · Source: claude · Confidence: high · Id: new-master-key-live-across-await
In both platform_wallet_create_encrypted_document_with_signer (document.rs:294-320) and platform_wallet_fetch_encrypted_documents (document.rs:394-416), master_opt is resolved synchronously then moved into block_on_worker(async move { ... }), where key_source borrows it for the duration of a real network .await (broadcast confirmation, or paginated fetch+decrypt). master.private_key.non_secure_erase() only runs after the await returns (lines 319-320, 415-416). ExtendedPrivKey has no Drop/Zeroize impl, so a panic during the await window (network error unwind, decode panic, etc.) skips the wipe entirely, leaving the raw master scalar resident in memory. This is a materially wider exposure window than the sibling resolver call sites (identity_key_preview.rs, identity_discovery.rs), which only hold the master across a synchronous local derive loop, not a network round-trip.
🟡 suggestion: New tx_metadata_key_master_for_wallet dispatch function has no FFI-level test coverage
Severity: 🟡 suggestion · Anchor: packages/rs-platform-wallet-ffi/src/document.rs:46-81 · Source: claude+codex · Confidence: medium · Id: new-no-ffi-test-coverage-resolver-dispatch
tx_metadata_key_master_for_wallet (document.rs:46-81) — the function that decides whether a wallet needs resolver-based master derivation and rejects a null resolver handle for external-signable/watch-only wallets — is new in this delta but is exercised only indirectly through unit tests inside platform-wallet's tx_metadata.rs (external_signable_wallet_derives_via_resolver_master). No test in rs-platform-wallet-ffi calls the actual FFI entry points (platform_wallet_create_encrypted_document_with_signer, platform_wallet_fetch_encrypted_documents) with a null resolver handle against a watch-only/external-signable wallet handle to confirm the ErrorWalletOperation rejection path at the FFI boundary.
🟡 suggestion: Raw native heap pointer address of mnemonic_resolver_handle logged on every fetch call
Severity: 🟡 suggestion · Anchor: packages/rs-unified-sdk-jni/src/transactions.rs:823-832 · Source: codex · Confidence: medium · Id: new-mnemonic-handle-pointer-logged
documentFetchEncrypted (transactions.rs, log::warn! block ~lines 823-832) logs mnemonic_resolver_handle={:#x} — the raw jlong/native pointer value of the Box<MnemonicResolverHandle> — on every invocation. Unlike wallet_handle, which is a logical map key into HandleStorage, this value (depending on the resolver-handle allocation scheme) may be or derive from an actual heap address, so writing it to logcat at warn-level in production builds leaks heap-layout information and weakens ASLR. The (nonzero={}) boolean already present in the same log line is sufficient for the stated debugging purpose (confirming the JVM call reached native code with a non-null handle) without printing the raw address.
Additional Cumulative Findings
None beyond the carried-forward set above; the cumulative diff was reviewed and its issues are fully captured there.
…off the await; redact plaintext Debug; quiet breadcrumbs Addresses the latest review on dashpay#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>
|
On-device wire-compat proof: PROVEN. Root cause of the earlier on-device failures, in sequence: (1) the fetch query was never broken — with visible breadcrumbs the device showed
Final verification on a real device (Galaxy S22, testnet): the Android wallet fetched its identity's legacy-written encrypted txMetadata documents through the new API and decrypted every one — |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
BLOCKING addressed with a real nonzero-identity vector: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs (1)
227-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resolve_encryption_contextandresolve_encryption_context_blockingare near-duplicates.Both bodies are identical apart from
self.wallet_manager.read().awaitvs.blocking_read(). Factoring the shared lookup logic into a helper taking the already-acquired guard would remove the duplication and the risk of the two paths silently diverging (e.g. differing error messages).♻️ Extract shared lookup logic
+ fn context_from_wallet_info( + info: &WalletInfo, // whatever the wallet_manager entry type is + wallet_id: &[u8; 32], + owner_identity_id: &Identifier, + ) -> Result<(dpp::identity::Identity, u32), PlatformWalletError> { + let managed = info + .identity_manager + .managed_identity(owner_identity_id) + .ok_or(PlatformWalletError::IdentityNotFound(*owner_identity_id))?; + let identity_index = managed.identity_index.ok_or_else(|| { + PlatformWalletError::InvalidIdentityData(format!( + "Identity {owner_identity_id} is watch-only (no resident HD slot); \ + cannot derive its txMetadata encryption key in-process" + )) + })?; + Ok((managed.identity.clone(), identity_index)) + }Then both
resolve_encryption_contextandresolve_encryption_context_blockingbecome thin wrappers that just acquire the guard differently and call this helper plus theget_walletlookup.🤖 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 227 - 283, Extract the duplicated wallet and managed-identity lookup from resolve_encryption_context and resolve_encryption_context_blocking into a shared helper that accepts the already-acquired wallet-manager guard and owner_identity_id, preserving the existing errors and return values. Keep each wrapper responsible only for acquiring read versus blocking_read access, then invoke the helper so both paths share identical lookup behavior.packages/rs-platform-wallet/tests/txmetadata_fetch.rs (1)
53-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffIgnored test performs real network calls against testnet, contradicting the test-guidelines' "no network calls; mock instead" rule.
This is presumably intentional — the whole point is pinning the wire query against real legacy-written testnet documents, and it's gated behind
#[ignore = "hits testnet"]so it never runs in CI. Worth keeping in mind if a mocked companion test is ever added to close the CI-coverage gap already flagged forfetch_encrypted_documents.As per coding guidelines: "Unit and integration tests should not perform network calls; mock dependencies instead."
🤖 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/tests/txmetadata_fetch.rs` around lines 53 - 124, The ignored test fetch_returns_both_legacy_txmetadata_documents performs real testnet network calls, violating the test guidelines. Replace its live SDK, contract fetch, context-provider registration, and document query with mocked dependencies and fixture documents that preserve the existing assertions; keep the legacy metadata validation coverage without requiring testnet access.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.
Inline comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt`:
- Line 181: Update the version validation in the DocumentTransactions sealing
flow to accept only the supported wire versions 0 and 1, using the existing
VERSION_CBOR and VERSION_PROTOBUF symbols rather than the broad byte-range
check. Keep writing the validated version byte unchanged into the envelope.
---
Nitpick comments:
In
`@packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- Around line 227-283: Extract the duplicated wallet and managed-identity lookup
from resolve_encryption_context and resolve_encryption_context_blocking into a
shared helper that accepts the already-acquired wallet-manager guard and
owner_identity_id, preserving the existing errors and return values. Keep each
wrapper responsible only for acquiring read versus blocking_read access, then
invoke the helper so both paths share identical lookup behavior.
In `@packages/rs-platform-wallet/tests/txmetadata_fetch.rs`:
- Around line 53-124: The ignored test
fetch_returns_both_legacy_txmetadata_documents performs real testnet network
calls, violating the test guidelines. Replace its live SDK, contract fetch,
context-provider registration, and document query with mocked dependencies and
fixture documents that preserve the existing assertions; keep the legacy
metadata validation coverage without requiring testnet access.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e960b2b-8674-4b08-98c6-23496fa8c219
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
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/rs-platform-wallet-ffi/Cargo.tomlpackages/rs-platform-wallet-ffi/src/document.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/wallet/identity/crypto/mod.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/mod.rspackages/rs-platform-wallet/tests/legacy_wire_compat/LegacyKeyN.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.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior Reconciliation
All 14 prior findings were rechecked: 2 remain STILL VALID, 5 are FIXED, 3 are INTENTIONALLY DEFERRED, and 4 are OUTDATED after scope and context verification.
Carried-Forward Prior Findings
The legacy nonzero-index vector still manually assumes the disputed path instead of invoking the legacy platform implementation. Actual FFI resolver integration remains untested despite the new pure dispatch tests.
New Findings In Latest Delta
One documentation nitpick: the fetch call-site comment still describes WARN-level logging and an active investigation after the helper was changed to DEBUG and the investigation declared resolved.
Additional Cumulative Findings
None. The targeted platform-wallet tests passed 9/9 and platform-wallet-ffi document tests passed 4/4.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2, trigger new_push, prior head 39444ef, specialists none): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); verifier=verifier-codex-fallback-4091-1783795359=gpt-5.6-sol fallback; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyKeyN.java`:
- [BLOCKING] packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyKeyN.java:27-43: Nonzero-index vector still assumes the disputed legacy account path
The generator manually constructs `9'/1'/5'/0'/0'/identityIndex'` and appends the txMetadata children. It imports only dashj-core's generic BIP32 and AES classes; it never invokes `DerivationPathFactory.blockchainIdentityECDSADerivationPath`, `BlockchainIdentity.privateKeyAtPath`, or another legacy platform API that chooses this path. Its documented classpath also contains no legacy platform artifact. The resulting vector therefore proves that Rust and dashj-core derive the same value for a path supplied by this PR, but it does not independently prove that the legacy wallet selected that path for a nonzero identity—the compatibility uncertainty identified by the prior finding. This is especially material because the unchanged PR description still says the prefix cannot be reconstructed from the legacy jars alone and requests a real legacy-written sample. Use the actual legacy path/create flow or a real legacy-written document from a nonzero identity before relying on this migration-critical compatibility claim.
In `packages/rs-platform-wallet-ffi/src/document.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/document.rs:62-97: Resolver integration remains untested through the FFI boundary
The new tests cover only the extracted `decide_key_source` truth table. No test invokes `tx_metadata_key_master_for_wallet` or either exported encrypted-document function with resident and external-signable wallets. Consequently, the wallet capability lookup, missing-wallet mapping, resolver callback, null-handle `ErrorWalletOperation`, and entry-point wiring remain unverified. This is the integration path that previously failed on-device, so the pure branch tests do not fully resolve the prior FFI-level coverage concern.
| // blockchainIdentityECDSADerivationPath(testnet) for identity `identityIndex`: | ||
| // FEATURE_PURPOSE=9', coinType(testnet)=1', FEATURE_PURPOSE_IDENTITIES=5', | ||
| // 0' (subfeature), 0' (keyType=ECDSA), identityIndex' | ||
| List<ChildNumber> accountPath = new ArrayList<>(); | ||
| accountPath.add(new ChildNumber(9, true)); | ||
| accountPath.add(new ChildNumber(1, true)); | ||
| accountPath.add(new ChildNumber(5, true)); | ||
| accountPath.add(new ChildNumber(0, true)); | ||
| accountPath.add(new ChildNumber(0, true)); // keyType = ECDSA = 0 | ||
| accountPath.add(new ChildNumber(identityIndex, true)); // identity index | ||
|
|
||
| int txMetaChild = 32769; // TxMetadataDocument.childNumber | ||
|
|
||
| List<ChildNumber> full = new ArrayList<>(accountPath); | ||
| full.add(new ChildNumber(keyId, true)); | ||
| full.add(new ChildNumber(txMetaChild, true)); | ||
| full.add(new ChildNumber(encryptionKeyIndex, true)); |
There was a problem hiding this comment.
🔴 Blocking: Nonzero-index vector still assumes the disputed legacy account path
The generator manually constructs 9'/1'/5'/0'/0'/identityIndex' and appends the txMetadata children. It imports only dashj-core's generic BIP32 and AES classes; it never invokes DerivationPathFactory.blockchainIdentityECDSADerivationPath, BlockchainIdentity.privateKeyAtPath, or another legacy platform API that chooses this path. Its documented classpath also contains no legacy platform artifact. The resulting vector therefore proves that Rust and dashj-core derive the same value for a path supplied by this PR, but it does not independently prove that the legacy wallet selected that path for a nonzero identity—the compatibility uncertainty identified by the prior finding. This is especially material because the unchanged PR description still says the prefix cannot be reconstructed from the legacy jars alone and requests a real legacy-written sample. Use the actual legacy path/create flow or a real legacy-written document from a nonzero identity before relying on this migration-critical compatibility claim.
source: ['codex-general']
There was a problem hiding this comment.
Resolved in this update — Nonzero-index vector still assumes the disputed legacy account path no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Confirmed — your read of LegacyKeyN.java is exactly correct, and it's fixed in b7319b5 (option (a)).
The generator manually constructs 9'/1'/5'/0'/0'/identityIndex' and never invokes DerivationPathFactory.blockchainIdentityECDSADerivationPath / BlockchainIdentity.privateKeyAtPath. So for a nonzero index it only re-derives, under dashj-core's raw HDKeyDerivation, the very path this crate's tx_metadata_derivation_path already constructs — it proves Rust ⟷ dashj-core agree on the key for a path this PR chose, not that the legacy platform selects that path. And it can't: the legacy createTxMetadata flow has no identity-index component (always the primary identity via blockchainIdentityECDSADerivationPath() = index 0), so no legacy-written document is keyed at identity_index > 0 — a real nonzero sample is unobtainable because it never existed.
Applied:
- Relabeled the vector/test:
legacy_dashj_wire_compat_vector_nonzero_identity_index→nonzero_identity_index_derivation_slot_is_internally_consistent. Its doc and assert messages now state plainly that8cda…5196is a self-referential internal slot-placement + resident/master-agreement pin, NOT a legacy wire-compat claim. Value unchanged (it's still a useful regression pin on Rust's own slot wiring). - Corrected
LegacyKeyN.java(this file): the class/inline comments now flag that it hand-builds the account path and does not call the real factory; at index 0 the hand-built path coincides with the factory output (independently confirmed), at >0 it's self-referential. - Corrected the README to split the two vectors accordingly.
- The genuine cross-stack guarantee now rests solely on the index-0
legacy_dashj_wire_compat_vector, whose path was verified against the real dashjDerivationPathFactoryand matches4a2eaec1…byte-for-byte (details on the companion thread dd246b5e17d0).
cargo test -p platform-wallet --lib: 431 passed / 0 failed.
| unsafe fn tx_metadata_key_master_for_wallet( | ||
| wallet: &platform_wallet::PlatformWallet, | ||
| mnemonic_resolver_handle: *mut MnemonicResolverHandle, | ||
| ) -> Result<Option<ExtendedPrivKey>, PlatformWalletFFIResult> { | ||
| // Phase 1 — short capability-check guard, dropped before any resolver | ||
| // interaction. | ||
| let wallet_has_resident_keys = { | ||
| let wm = wallet.wallet_manager().blocking_read(); | ||
| match wm.get_wallet(&wallet.wallet_id()) { | ||
| Some(kw) => !kw.is_external_signable() && !kw.is_watch_only(), | ||
| None => { | ||
| return Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidHandle, | ||
| "Wallet not found in wallet manager", | ||
| )); | ||
| } | ||
| } | ||
| }; | ||
| match decide_key_source(wallet_has_resident_keys, mnemonic_resolver_handle.is_null()) { | ||
| KeySourceDecision::ResidentWallet => Ok(None), | ||
| KeySourceDecision::ResolverRequired => Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorWalletOperation, | ||
| "this wallet has no resident private keys (external-signable / watch-only); \ | ||
| a mnemonic resolver handle is required to derive its txMetadata encryption keys", | ||
| )), | ||
| KeySourceDecision::ResolveMaster => { | ||
| let wallet_id = wallet.wallet_id(); | ||
| // SAFETY: handle is non-null (the decision proves it) and the | ||
| // caller's safety contract guarantees it came from | ||
| // `dash_sdk_mnemonic_resolver_create`. | ||
| let master = unsafe { | ||
| resolve_master_from_resolver(mnemonic_resolver_handle, &wallet_id, wallet.network())? | ||
| }; | ||
| Ok(Some(master)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Resolver integration remains untested through the FFI boundary
The new tests cover only the extracted decide_key_source truth table. No test invokes tx_metadata_key_master_for_wallet or either exported encrypted-document function with resident and external-signable wallets. Consequently, the wallet capability lookup, missing-wallet mapping, resolver callback, null-handle ErrorWalletOperation, and entry-point wiring remain unverified. This is the integration path that previously failed on-device, so the pure branch tests do not fully resolve the prior FFI-level coverage concern.
source: ['codex-general']
There was a problem hiding this comment.
Resolved in this update — Resolver integration remains untested through the FFI boundary no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // On-device diagnostic breadcrumbs, dual-emitted at warn level (see | ||
| // [`breadcrumb`]): this call sits under an active `sdkFetched=0` | ||
| // investigation — every stage must be provably visible in `adb logcat`. | ||
| breadcrumb(&format!( |
There was a problem hiding this comment.
💬 Nitpick: Fetch breadcrumb comment describes obsolete WARN-level diagnostics
This comment still says the breadcrumb is emitted at WARN level for an active sdkFetched=0 investigation. The same delta changed breadcrumb to DEBUG and documents that the root cause is fixed. The stale description incorrectly implies that this stage remains visible in Android logcat.
| // On-device diagnostic breadcrumbs, dual-emitted at warn level (see | |
| // [`breadcrumb`]): this call sits under an active `sdkFetched=0` | |
| // investigation — every stage must be provably visible in `adb logcat`. | |
| breadcrumb(&format!( | |
| // Informational fetch-stage breadcrumb, dual-emitted at DEBUG level. | |
| // Genuine failure and skip paths use `breadcrumb_error` at WARN level. | |
source: ['sonnet5-general']
There was a problem hiding this comment.
Resolved in this update — Fetch breadcrumb comment describes obsolete WARN-level diagnostics no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…deep-cloning it Addresses review nitpick bbb24591b025 on dashpay#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>
|
Suggestion round addressed in
|
…'t decode Addresses review findings 0dd9fc55de07 / CodeRabbit a783199f on dashpay#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>
|
Fixed in |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior Reconciliation: All 3 prior findings from the ad7f32f review remain STILL VALID — none of their files were touched anywhere in the ad7f32f..8ba66e8 delta or the wider cumulative diff since they were first added. Carried-Forward Prior Findings: (1) LegacyKeyN.java's nonzero-index vector still hand-builds the disputed legacy HD path with generic bitcoinj primitives rather than a real legacy platform API; (2) tx_metadata_key_master_for_wallet's resolver dispatch is still only exercised via the pure decide_key_source truth table (4 tests, none constructing a live PlatformWallet); (3) the fetch breadcrumb comment in encrypted_document.rs still claims WARN-level logging under an 'active investigation' that contradicts the DEBUG-level breadcrumb() implementation and its own up-to-date doc comment. New Findings In Latest Delta: this delta adds a Kotlin-only require(version == 0 || version == 1) guard, but verification shows the wire-compat invariant it protects is not enforced anywhere else in the stack — seal_tx_metadata, prepare_encrypted_txmetadata_properties, the exported C FFI platform_wallet_create_encrypted_document_with_signer, and, most concretely, the JNI binding Java_..._documentCreateEncrypted (rs-unified-sdk-jni/src/transactions.rs:747-750) still explicitly accepts the full stale 0..=255 range. This is a real, currently-inconsistent security boundary directly exposed by this PR's own fix. Additional Cumulative Findings: none beyond the above.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 3, trigger new_push, prior head ad7f32f, specialists none): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); verifier=verifier-sonnet5-4091-1783819508=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking
3 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-unified-sdk-jni/src/transactions.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/transactions.rs:747-750: Version-byte wire-compat guard only reaches Kotlin — the JNI, FFI, and Rust core layers still accept the stale 0..=255 range
This commit's stated goal is closing a hole where a wire-incompatible version byte (2-255) could be sealed into a document the legacy dashj stack can't decode. The fix lands only in `DocumentTransactions.kt` as a JVM-side `require`. Verified directly against the checked-out head: this JNI entry point still runs `if !(0..=255).contains(&version)` — the exact stale range the Kotlin guard was written to replace — before casting to `u8` and forwarding to `platform_wallet_create_encrypted_document_with_signer`. That exported C function (`rs-platform-wallet-ffi/src/document.rs:307`) takes `version: u8` with no validation, and `seal_tx_metadata` (`rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:192`) and `prepare_encrypted_txmetadata_properties` write the byte verbatim into the envelope with no check either. Any caller entering through the JNI/FFI boundary directly — a future non-Kotlin JVM caller, a Rust-only integration, or the JNI guard silently drifting out of sync with the Kotlin one — can still publish a document the legacy stack cannot decode, which is precisely the bidirectional guarantee this PR exists to protect. Validate 0/1 at the lowest exported boundary (the FFI `platform_wallet_create_encrypted_document_with_signer` or `seal_tx_metadata` itself) so the invariant holds for every caller, and update this JNI check to match rather than leaving it as the stale `0..=255`.
| if !(0..=255).contains(&version) { | ||
| throw_sdk_exception(env, 1, "version must be in 0..=255"); | ||
| return ptr::null_mut(); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Version-byte wire-compat guard only reaches Kotlin — the JNI, FFI, and Rust core layers still accept the stale 0..=255 range
This commit's stated goal is closing a hole where a wire-incompatible version byte (2-255) could be sealed into a document the legacy dashj stack can't decode. The fix lands only in DocumentTransactions.kt as a JVM-side require. Verified directly against the checked-out head: this JNI entry point still runs if !(0..=255).contains(&version) — the exact stale range the Kotlin guard was written to replace — before casting to u8 and forwarding to platform_wallet_create_encrypted_document_with_signer. That exported C function (rs-platform-wallet-ffi/src/document.rs:307) takes version: u8 with no validation, and seal_tx_metadata (rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs:192) and prepare_encrypted_txmetadata_properties write the byte verbatim into the envelope with no check either. Any caller entering through the JNI/FFI boundary directly — a future non-Kotlin JVM caller, a Rust-only integration, or the JNI guard silently drifting out of sync with the Kotlin one — can still publish a document the legacy stack cannot decode, which is precisely the bidirectional guarantee this PR exists to protect. Validate 0/1 at the lowest exported boundary (the FFI platform_wallet_create_encrypted_document_with_signer or seal_tx_metadata itself) so the invariant holds for every caller, and update this JNI check to match rather than leaving it as the stale 0..=255.
| if !(0..=255).contains(&version) { | |
| throw_sdk_exception(env, 1, "version must be in 0..=255"); | |
| return ptr::null_mut(); | |
| } | |
| if !(version == 0 || version == 1) { | |
| throw_sdk_exception(env, 1, "version must be 0 (CBOR) or 1 (protobuf)"); | |
| return ptr::null_mut(); | |
| } | |
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in d29875b — Version-byte wire-compat guard only reaches Kotlin — the JNI, FFI, and Rust core layers still accept the stale 0..=255 range no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…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 (dashpay#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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior Reconciliation: All 4 prior findings from 8ba66e85 reconciled against d29875bc. prior-1 is FIXED — verified in the diff: seal_tx_metadata (tx_metadata.rs) now returns Result<Vec<u8>, PlatformWalletError> and rejects any version byte other than 0/1 at the shared choke point every binding funnels through, its sole caller prepare_encrypted_txmetadata_properties propagates via ? with no signature change needed, and JNI's stale 0..=255 check is independently tightened to 0..=1 as defense-in-depth. A new test seal_rejects_non_wire_versions pins rejection of all 254 invalid bytes (2..=255). The other 3 prior findings are STILL VALID — confirmed by git diff showing zero changes to LegacyKeyN.java and rs-platform-wallet-ffi/src/document.rs in this delta, and the breadcrumb comment in encrypted_document.rs unchanged in content (only shifted by 3 lines due to this delta's unrelated insertion). Carried-Forward Prior Findings: (1) LegacyKeyN.java's nonzero-index vector (lines 27-43) still hand-builds the disputed 9'/1'/5'/0'/0'/identityIndex' legacy HD path using only generic bitcoinj/bouncycastle primitives, never a real legacy platform API — confirmed by direct read, imports are org.bitcoinj.crypto.*/org.bouncycastle.crypto.* only. (2) tx_metadata_key_master_for_wallet (document.rs:62-98) still performs a live wallet-manager lookup and resolver-callback dispatch untested beyond the extracted pure decide_key_source truth table — confirmed by reading the test module (lines 934-980), which only exercises decide_key_source. (3) The fetch breadcrumb comment (now confirmed at encrypted_document.rs:378-380) still claims 'dual-emitted at warn level' under an 'active investigation,' contradicting the breadcrumb() helper (lines 129-132) which is DEBUG and explicitly documents the root cause as fixed. New Findings In Latest Delta: None — the delta is a correct, complete, minimal fix for prior-1. Additional Cumulative Findings: None — the cumulative file set (verified via git diff stat against the PR base) is unchanged from the prior review.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2, trigger new_push, prior head 8ba66e8, specialists none): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); verifier=verifier-sonnet5-4091-1783864813=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🟡 1 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/document.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/document.rs:62-98: Resolver dispatch in `tx_metadata_key_master_for_wallet` remains untested through the actual FFI boundary
This function performs a live wallet-manager lookup, missing-wallet error mapping (`ErrorInvalidHandle`), and resolver-callback dispatch — but the test module in this file (lines 934-980, confirmed by direct read) only exercises the extracted pure `decide_key_source` truth table. None of the three tests construct a live `PlatformWallet`/wallet manager or invoke the resolver callback, so the missing-wallet mapping, the null-handle `ErrorWalletOperation` path, and the actual resolver-callback invocation remain unverified at the FFI entry point — the layer that previously failed on-device. This file is untouched by the current delta.
| unsafe fn tx_metadata_key_master_for_wallet( | ||
| wallet: &platform_wallet::PlatformWallet, | ||
| mnemonic_resolver_handle: *mut MnemonicResolverHandle, | ||
| ) -> Result<Option<ExtendedPrivKey>, PlatformWalletFFIResult> { | ||
| // Phase 1 — short capability-check guard, dropped before any resolver | ||
| // interaction. | ||
| let wallet_has_resident_keys = { | ||
| let wm = wallet.wallet_manager().blocking_read(); | ||
| match wm.get_wallet(&wallet.wallet_id()) { | ||
| Some(kw) => !kw.is_external_signable() && !kw.is_watch_only(), | ||
| None => { | ||
| return Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidHandle, | ||
| "Wallet not found in wallet manager", | ||
| )); | ||
| } | ||
| } | ||
| }; | ||
| match decide_key_source(wallet_has_resident_keys, mnemonic_resolver_handle.is_null()) { | ||
| KeySourceDecision::ResidentWallet => Ok(None), | ||
| KeySourceDecision::ResolverRequired => Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorWalletOperation, | ||
| "this wallet has no resident private keys (external-signable / watch-only); \ | ||
| a mnemonic resolver handle is required to derive its txMetadata encryption keys", | ||
| )), | ||
| KeySourceDecision::ResolveMaster => { | ||
| let wallet_id = wallet.wallet_id(); | ||
| // SAFETY: handle is non-null (the decision proves it) and the | ||
| // caller's safety contract guarantees it came from | ||
| // `dash_sdk_mnemonic_resolver_create`. | ||
| let master = unsafe { | ||
| resolve_master_from_resolver(mnemonic_resolver_handle, &wallet_id, wallet.network())? | ||
| }; | ||
| Ok(Some(master)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Resolver dispatch in tx_metadata_key_master_for_wallet remains untested through the actual FFI boundary
This function performs a live wallet-manager lookup, missing-wallet error mapping (ErrorInvalidHandle), and resolver-callback dispatch — but the test module in this file (lines 934-980, confirmed by direct read) only exercises the extracted pure decide_key_source truth table. None of the three tests construct a live PlatformWallet/wallet manager or invoke the resolver callback, so the missing-wallet mapping, the null-handle ErrorWalletOperation path, and the actual resolver-callback invocation remain unverified at the FFI entry point — the layer that previously failed on-device. This file is untouched by the current delta.
source: ['codex-general', 'sonnet5-general']
There was a problem hiding this comment.
Resolved in this update — Resolver dispatch in tx_metadata_key_master_for_wallet remains untested through the actual FFI boundary no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Correction: this finding remains valid at 456519fce37e1767466463fb4c68f2559d60794c and is carried in the current review as Resolver dispatch remains untested through the live FFI boundary. The preceding auto-reconciliation message was caused by title/hash drift and should be disregarded.
There was a problem hiding this comment.
Correction: Still valid and carried forward at c7f91a50: no current-head test invokes tx_metadata_key_master_for_wallet through an encrypted-document export with a fake resolver.
Canonical preliminary review: #4091 (review)
|
Version validation moved Rust-side ( The wire-compat-vector thread (dd246b5e17d0 / 4c0754158cc6) I'm holding for a separate reply with a decision — a harness against the real legacy jars shows the legacy tx-metadata key path has no identity-index component, so a "nonzero identity_index" vector has no legacy counterpart. More shortly. |
…el nonzero vector as internal slot check (dashpay#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 dashpay#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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The wire-compatibility implementation and latest DerivationPathFactory provenance verifier are consistent with dashj-core 22.0.3, and the targeted txMetadata tests pass. No blocking defect remains, but the new JNI/FFI path can still abort on worker panic or race signer destruction during shutdown. Boundary-level resolver coverage, fetch/decrypt orchestration coverage, and one diagnostic comment also remain incomplete.
Source (experiment sonnet-primary-opus-quarter-sample-20260710; cohort sonnet_opus_sample; bucket 0): reviewers — codex general/security-auditor/ffi-engineer = gpt-5.6-sol (completed); sonnet5 general/security-auditor/ffi-engineer = claude-sonnet-5 (authentication failed, no evidence); opus general/security-auditor/ffi-engineer = claude-opus-4-8 (authentication failed, no evidence). Verifier — primary claude-sonnet-5 failed authentication; fallback gpt-5.6-sol completed. Orchestrator — openai/gpt-5.6-sol, high reasoning, orchestration-only.
🟡 4 suggestion(s) | 💬 1 nitpick(s)
| let result: Result<(Identifier, String), PlatformWalletError> = | ||
| block_on_worker(async move { | ||
| let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); | ||
| // Generic create path (no key material in scope): fetches the | ||
| // contract, sanitizes the hex `encryptedMetadata` into `Bytes`, | ||
| // auto-selects the AUTHENTICATION signing key, and broadcasts on | ||
| // the 8 MB worker stack. | ||
| let confirmed: Document = identity_wallet | ||
| .create_document_with_signer( | ||
| &owner_id_for_async, | ||
| &contract_id_for_async, | ||
| &document_type_str, | ||
| &properties_json, | ||
| signer, | ||
| ) | ||
| .await?; | ||
| let json_string = confirmed_document_to_json(&confirmed)?; | ||
| Ok::<_, PlatformWalletError>((confirmed.id(), json_string)) | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: Nested C ABI prevents the JNI panic guard from handling worker panics
The JNI entry point runs under support::guard, but it invokes this extern "C" export, which calls block_on_worker. That helper uses expect when constructing the runtime and joining the spawned task. Under the Android unwind profiles, a worker panic therefore begins unwinding inside a non-unwind C ABI function and aborts the process before the outer JNI catch_unwind can translate it into a Java exception. The fetch export repeats the same path at lines 465–491. Return a structured FFI error for runtime/join failures, or have JNI call a non-C Rust implementation while still inside its guard.
source: ['codex-ffi-engineer']
There was a problem hiding this comment.
Correction: Still valid and carried forward at c7f91a50: the nested non-unwind C ABI still prevents the outer JNI panic guard from catching block_on_worker panics.
Canonical preliminary review: #4091 (review)
| let signer_addr = signer_handle as usize; | ||
| let owner_id_for_async = owner_id; | ||
| let contract_id_for_async = contract_id_value; | ||
|
|
||
| let option = PLATFORM_WALLET_STORAGE.with_item(wallet_handle, |wallet| { | ||
| let identity_wallet = wallet.identity().clone(); | ||
|
|
||
| // 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); | ||
|
|
||
| // Derive the AES key + seal the wire blob SYNCHRONOUSLY, then wipe the | ||
| // master BEFORE any network `.await`: the master xprv never crosses the | ||
| // broadcast await (dashpay/platform#4091). Only the sealed properties | ||
| // (ciphertext, no key material) cross into the async block below. | ||
| let key_source = match master_opt.as_ref() { | ||
| Some(master) => TxMetadataKeySource::Master(&master.0), | ||
| None => TxMetadataKeySource::ResidentWallet, | ||
| }; | ||
| let properties_json = identity_wallet | ||
| .prepare_encrypted_txmetadata_properties( | ||
| &owner_id_for_async, | ||
| encryption_key_index, | ||
| version, | ||
| &payload_vec, | ||
| key_source, | ||
| ) | ||
| .map_err(PlatformWalletFFIResult::from)?; | ||
| // Scrub the master now — it is not needed for the broadcast. | ||
| drop(master_opt); | ||
|
|
||
| let result: Result<(Identifier, String), PlatformWalletError> = | ||
| block_on_worker(async move { | ||
| let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); | ||
| // Generic create path (no key material in scope): fetches the | ||
| // contract, sanitizes the hex `encryptedMetadata` into `Bytes`, | ||
| // auto-selects the AUTHENTICATION signing key, and broadcasts on | ||
| // the 8 MB worker stack. | ||
| let confirmed: Document = identity_wallet | ||
| .create_document_with_signer( | ||
| &owner_id_for_async, | ||
| &contract_id_for_async, | ||
| &document_type_str, | ||
| &properties_json, | ||
| signer, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🟡 Suggestion: Manager shutdown can free the signer while encrypted create is awaiting DAPI
The export converts the caller-owned signer pointer to an integer, reconstructs &VTableSigner on the worker, and retains it through create_document_with_signer. That method awaits the contract fetch before eventually invoking the signer during broadcast. DocumentTransactions.createEncryptedDocument runs in a caller-owned coroutine, while PlatformWalletManager.close() only cancels its own scope and then destroys the shared KeystoreSigner; it does not join these document calls. A concurrent manager replacement or shutdown can therefore free the signer, vtable, and callback context before the worker uses them, causing undefined behavior and a wallet-process crash. The manager's detached drain path already recognizes this lifecycle race and creates coroutine-owned handles. Give document creation an equivalent in-flight signer lease or make shutdown wait for active document operations.
source: ['codex-security-auditor', 'codex-ffi-engineer']
There was a problem hiding this comment.
Correction: Still valid and carried forward at c7f91a50; current-head verification found the same lifetime root cause at the Kotlin call sites, where both encrypted-document methods bypass TeardownGate and can race signer/resolver destruction.
Canonical preliminary review: #4091 (review)
| unsafe fn tx_metadata_key_master_for_wallet( | ||
| wallet: &platform_wallet::PlatformWallet, | ||
| mnemonic_resolver_handle: *mut MnemonicResolverHandle, | ||
| ) -> Result<Option<ExtendedPrivKey>, PlatformWalletFFIResult> { | ||
| // Phase 1 — short capability-check guard, dropped before any resolver | ||
| // interaction. | ||
| let wallet_has_resident_keys = { | ||
| let wm = wallet.wallet_manager().blocking_read(); | ||
| match wm.get_wallet(&wallet.wallet_id()) { | ||
| Some(kw) => !kw.is_external_signable() && !kw.is_watch_only(), | ||
| None => { | ||
| return Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidHandle, | ||
| "Wallet not found in wallet manager", | ||
| )); | ||
| } | ||
| } | ||
| }; | ||
| match decide_key_source(wallet_has_resident_keys, mnemonic_resolver_handle.is_null()) { | ||
| KeySourceDecision::ResidentWallet => Ok(None), | ||
| KeySourceDecision::ResolverRequired => Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorWalletOperation, | ||
| "this wallet has no resident private keys (external-signable / watch-only); \ | ||
| a mnemonic resolver handle is required to derive its txMetadata encryption keys", | ||
| )), | ||
| KeySourceDecision::ResolveMaster => { | ||
| let wallet_id = wallet.wallet_id(); | ||
| // SAFETY: handle is non-null (the decision proves it) and the | ||
| // caller's safety contract guarantees it came from | ||
| // `dash_sdk_mnemonic_resolver_create`. | ||
| let master = unsafe { | ||
| resolve_master_from_resolver(mnemonic_resolver_handle, &wallet_id, wallet.network())? | ||
| }; | ||
| Ok(Some(master)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Resolver dispatch remains untested through the live FFI boundary
tx_metadata_key_master_for_wallet performs the live wallet-manager lookup, maps a missing wallet to ErrorInvalidHandle, detects external/watch-only wallets, maps a missing resolver to ErrorWalletOperation, and invokes the host resolver callback. The tests at lines 934–979 exercise only the extracted decide_key_source truth table. No test constructs a live wallet, invokes a MnemonicResolverHandle, verifies callback/error ownership, or calls either encrypted-document export. This is the boundary that previously failed on-device, so its wiring can regress while the current tests continue to pass.
source: ['codex-general', 'codex-ffi-engineer']
There was a problem hiding this comment.
Correction: Still valid and carried forward at c7f91a50: the encrypted-document FFI path still lacks a live external-signable wallet/resolver dispatch test.
Canonical preliminary review: #4091 (review)
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| #[ignore = "hits testnet"] | ||
| async fn fetch_returns_both_legacy_txmetadata_documents() { |
There was a problem hiding this comment.
🟡 Suggestion: The fetch/query/decrypt pipeline has no automated regression test
The only test reaching query_owned_encrypted_documents is ignored because it calls live testnet, uses since_ms = 0, and returns only two documents. Normal CI therefore does not exercise the owner/since/order query, the StartAfter pagination branch, per-document decryption, malformed-document skipping, or timestamp propagation as one pipeline. The cryptographic unit tests cover sealing and opening but not this orchestration. Add a network-free mocked test with multiple pages and malformed entries.
source: ['codex-general']
There was a problem hiding this comment.
Correction: Still valid and carried forward at c7f91a50: the only production-query test remains ignored/live-testnet and does not exercise the normal-CI pagination + decrypt pipeline.
Canonical preliminary review: #4091 (review)
| // On-device diagnostic breadcrumbs, dual-emitted at warn level (see | ||
| // [`breadcrumb`]): this call sits under an active `sdkFetched=0` | ||
| // investigation — every stage must be provably visible in `adb logcat`. |
There was a problem hiding this comment.
💬 Nitpick: Fetch breadcrumb comment describes obsolete WARN-level diagnostics
The comment says breadcrumb is emitted at WARN during an active sdkFetched=0 investigation. The helper now emits at DEBUG, its documentation says that investigation's root cause is fixed, and only breadcrumb_error emits at WARN.
| // On-device diagnostic breadcrumbs, dual-emitted at warn level (see | |
| // [`breadcrumb`]): this call sits under an active `sdkFetched=0` | |
| // investigation — every stage must be provably visible in `adb logcat`. | |
| // On-device diagnostic breadcrumbs, dual-emitted at DEBUG level (see | |
| // [`breadcrumb`]); genuine failures use [`breadcrumb_error`] (WARN) | |
| // so they remain visible on-device. | |
source: ['codex-general']
…e + decrypt-on-fetch Adds the wallet-contract encrypted-document surface the Android wallet needs to retire the legacy org.dashj.platform stack (closes dashpay#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 dashpay#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 dashpay#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 dashpay#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 (dashpay#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 (dashpay#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 dashpay#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>
bfoss765
force-pushed
the
feat/kotlin-sdk-encrypted-documents
branch
from
456519f to
c7f91a5
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Preliminary Codex-only verification at exact head c7f91a5 confirms one blocking lifecycle defect: the two encrypted-document methods bypass the manager teardown gate, and the repository's gate-coverage scanner flags both. Four additional carried-forward findings remain valid; no distinct latest-delta finding was identified. Targeted Rust tests passed, while Kotlin execution was unavailable because Java is not installed.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt:293-352: Encrypted document calls bypass the native-handle teardown gate
Both new methods use plain `withContext(Dispatchers.IO)` even though `PlatformWalletManager` constructs this facade with its `TeardownGate`. Manager shutdown waits only for operations registered through `gate.op`, then destroys the non-refcounted mnemonic-resolver and signer boxes. Create retains and dereferences the signer after awaiting DAPI, while both calls can invoke the resolver callback, so concurrent shutdown can free a borrowed handle and cause undefined behavior or a process crash. The checked-in `GateCoverageLintTest` has an empty allowlist, and its exact scanner reports these two methods as the only violations. Wrap both method bodies in `gate.op`.
In `packages/rs-platform-wallet-ffi/src/document.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/document.rs:62-98: Resolver dispatch is not tested through the encrypted-document FFI path
`tx_metadata_key_master_for_wallet` combines the live wallet lookup, external-signable capability check, wallet-id/network selection, resolver callback, and FFI error mapping. The document tests exercise only the extracted boolean decision table, while the cryptographic tests start with an already-created master key. Other tests cover generic resolver mechanics, but none runs this new wallet-dispatch glue or either encrypted-document export with a fake resolver. Add a test using an external-signable wallet and resolver handle so the Android path that motivated this fix cannot regress independently of the pure decision logic.
In `packages/rs-platform-wallet/tests/txmetadata_fetch.rs`:
- [SUGGESTION] packages/rs-platform-wallet/tests/txmetadata_fetch.rs:53-55: The fetch, pagination, and decrypt pipeline lacks a normal-CI regression test
The only test reaching the production document query is an ignored live-testnet test that returns two documents and explicitly does not decrypt them. Normal CI therefore does not exercise the owner/since/order query together with `StartAfter` pagination, per-document derivation and decryption, malformed-document skipping, or `updatedAt` propagation. The ten passing cryptographic tests cover sealing and opening in isolation, not this orchestration. Add a network-free multi-page test seam with valid and malformed entries.
| ): String = withContext(Dispatchers.IO) { | ||
| require(ownerId.size == 32) { "ownerId must be 32 bytes" } | ||
| require(contractId.size == 32) { "contractId must be 32 bytes" } | ||
| require(encryptionKeyIndex >= 0) { | ||
| "encryptionKeyIndex must be non-negative, got $encryptionKeyIndex" | ||
| } | ||
| // Only 0 (CBOR) and 1 (protobuf) are wire-meaningful: `seal_tx_metadata` | ||
| // writes this byte verbatim into the envelope and the legacy dashj stack | ||
| // (decryptTxMetadata) switches on exactly those two values. Accepting 2..255 | ||
| // would silently seal a document the legacy stack can't decode, breaking the | ||
| // bidirectional wire-compat guarantee (dashpay/platform#4091). | ||
| require(version == 0 || version == 1) { | ||
| "version must be 0 (CBOR) or 1 (protobuf), got $version" | ||
| } | ||
| mapNativeErrors { | ||
| TransactionsNative.documentCreateEncrypted( | ||
| walletHandle, | ||
| mnemonicResolverHandle, | ||
| ownerId, | ||
| contractId, | ||
| documentType, | ||
| encryptionKeyIndex, | ||
| version, | ||
| payload, | ||
| signerHandle, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Fetch + DECRYPT every encrypted wallet-contract document owned by | ||
| * [ownerId] on [contractId]'s [documentType] updated at or after [sinceMs] | ||
| * (epoch-millis). Implements the read half of the legacy | ||
| * `BlockchainIdentity.getTxMetaData(since, key)` retirement | ||
| * (dashpay/platform#4087): the SDK fetches the owner-scoped, since-timestamp | ||
| * documents and decrypts each with the identity's derived key. Documents | ||
| * that fail to decrypt are skipped Rust-side (a bad document never aborts | ||
| * the fetch). | ||
| * | ||
| * @return a JSON array; each element is `{ "id", "ownerId" (base58), | ||
| * "keyIndex", "encryptionKeyIndex", "version", "updatedAt" (number|null), | ||
| * "payload" (base64 of the decrypted opaque plaintext) }`. The caller | ||
| * parses each `payload` itself (a protobuf `TxMetadataBatch` for | ||
| * `version == 1`) and reconciles memo / taxCategory / exchangeRate / | ||
| * service / giftCard fields into its local store. | ||
| * | ||
| * [mnemonicResolverHandle] is the host mnemonic-resolver handle | ||
| * ([org.dashfoundation.dashsdk.wallet.PlatformWalletManager.mnemonicResolverHandle]): | ||
| * required for external-signable wallets (the app's shape — the AES key | ||
| * derives on demand through the resolver), ignored for wallets with | ||
| * resident private keys. | ||
| */ | ||
| suspend fun fetchEncryptedDocuments( | ||
| walletHandle: Long, | ||
| mnemonicResolverHandle: Long, | ||
| ownerId: ByteArray, | ||
| contractId: ByteArray, | ||
| documentType: String, | ||
| sinceMs: Long, | ||
| ): String = withContext(Dispatchers.IO) { |
There was a problem hiding this comment.
🔴 Blocking: Encrypted document calls bypass the native-handle teardown gate
Both new methods use plain withContext(Dispatchers.IO) even though PlatformWalletManager constructs this facade with its TeardownGate. Manager shutdown waits only for operations registered through gate.op, then destroys the non-refcounted mnemonic-resolver and signer boxes. Create retains and dereferences the signer after awaiting DAPI, while both calls can invoke the resolver callback, so concurrent shutdown can free a borrowed handle and cause undefined behavior or a process crash. The checked-in GateCoverageLintTest has an empty allowlist, and its exact scanner reports these two methods as the only violations. Wrap both method bodies in gate.op.
source: ['codex']
There was a problem hiding this comment.
Resolved in 0c26a84 — Encrypted document calls bypass the native-handle teardown gate no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| unsafe fn tx_metadata_key_master_for_wallet( | ||
| wallet: &platform_wallet::PlatformWallet, | ||
| mnemonic_resolver_handle: *mut MnemonicResolverHandle, | ||
| ) -> Result<Option<ExtendedPrivKey>, PlatformWalletFFIResult> { | ||
| // Phase 1 — short capability-check guard, dropped before any resolver | ||
| // interaction. | ||
| let wallet_has_resident_keys = { | ||
| let wm = wallet.wallet_manager().blocking_read(); | ||
| match wm.get_wallet(&wallet.wallet_id()) { | ||
| Some(kw) => !kw.is_external_signable() && !kw.is_watch_only(), | ||
| None => { | ||
| return Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorInvalidHandle, | ||
| "Wallet not found in wallet manager", | ||
| )); | ||
| } | ||
| } | ||
| }; | ||
| match decide_key_source(wallet_has_resident_keys, mnemonic_resolver_handle.is_null()) { | ||
| KeySourceDecision::ResidentWallet => Ok(None), | ||
| KeySourceDecision::ResolverRequired => Err(PlatformWalletFFIResult::err( | ||
| PlatformWalletFFIResultCode::ErrorWalletOperation, | ||
| "this wallet has no resident private keys (external-signable / watch-only); \ | ||
| a mnemonic resolver handle is required to derive its txMetadata encryption keys", | ||
| )), | ||
| KeySourceDecision::ResolveMaster => { | ||
| let wallet_id = wallet.wallet_id(); | ||
| // SAFETY: handle is non-null (the decision proves it) and the | ||
| // caller's safety contract guarantees it came from | ||
| // `dash_sdk_mnemonic_resolver_create`. | ||
| let master = unsafe { | ||
| resolve_master_from_resolver(mnemonic_resolver_handle, &wallet_id, wallet.network())? | ||
| }; | ||
| Ok(Some(master)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Resolver dispatch is not tested through the encrypted-document FFI path
tx_metadata_key_master_for_wallet combines the live wallet lookup, external-signable capability check, wallet-id/network selection, resolver callback, and FFI error mapping. The document tests exercise only the extracted boolean decision table, while the cryptographic tests start with an already-created master key. Other tests cover generic resolver mechanics, but none runs this new wallet-dispatch glue or either encrypted-document export with a fake resolver. Add a test using an external-signable wallet and resolver handle so the Android path that motivated this fix cannot regress independently of the pure decision logic.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Resolver dispatch is not tested through the encrypted-document FFI path no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| #[ignore = "hits testnet"] | ||
| async fn fetch_returns_both_legacy_txmetadata_documents() { |
There was a problem hiding this comment.
🟡 Suggestion: The fetch, pagination, and decrypt pipeline lacks a normal-CI regression test
The only test reaching the production document query is an ignored live-testnet test that returns two documents and explicitly does not decrypt them. Normal CI therefore does not exercise the owner/since/order query together with StartAfter pagination, per-document derivation and decryption, malformed-document skipping, or updatedAt propagation. The ten passing cryptographic tests cover sealing and opening in isolation, not this orchestration. Add a network-free multi-page test seam with valid and malformed entries.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — The fetch, pagination, and decrypt pipeline lacks a normal-CI regression test no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
bfoss765
changed the title
…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>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Prior Reconciliation: At the exact current head 0c26a84, I independently re-verified all five prior findings against source. prior-2 is FIXED — DocumentTransactions.kt's createEncryptedDocument (line 290) and fetchEncryptedDocuments (line 349) now open with gate.op instead of bare withContext(Dispatchers.IO), and DocumentTransactions is only ever constructed by PlatformWalletManager with its live, non-null teardownGate (PlatformWalletManager.kt:465), so closeInternal's teardownGate.closeAndAwait() (line 1931) now blocks until both encrypted operations drain before the signer/mnemonic-resolver native boxes are freed at lines 1945-1946 — the use-after-free race is closed. prior-1, prior-3, prior-4, and prior-5 remain STILL VALID; none was touched by this delta. Carried-Forward Prior Findings: (1) the nested non-unwind extern "C" create/fetch exports in document.rs still call block_on_worker, which still has .expect("Failed to create tokio runtime...") (runtime.rs:38) and .expect("tokio worker panicked") (runtime.rs:60) with no inner panic boundary, so a panic still aborts before the outer JNI guard can translate it; (2) tx_metadata_key_master_for_wallet (document.rs:62-98) is still exercised only by the pure decide_key_source truth table (document.rs:938-979), not by a live external-signable wallet + resolver callback + either encrypted-document export; (3) fetch_returns_both_legacy_txmetadata_documents (txmetadata_fetch.rs:54-55) is still #[ignore = "hits testnet"] and explicitly stops short of decryption, so normal CI still has no pipeline test for query+pagination+decrypt+malformed-skip; (4) the call-site comment above breadcrumb(...) in fetch_encrypted_documents (encrypted_document.rs:378-380) still says "dual-emitted at warn level" during an "active sdkFetched=0 investigation," while breadcrumb() itself (lines 129-131) emits at DEBUG and only breadcrumb_error (line 140-141) emits at WARN. New Findings In Latest Delta: None — the single-commit delta is a minimal, correct fix scoped exactly to the teardown-gate gap; no new defect was introduced. Additional Cumulative Findings: None found across the full 09340b8..0c26a84 range beyond the four carried-forward items above; both Codex and Sonnet coverage (general, security-auditor, ffi-engineer, six lanes total) converge on the same four residual items and the same fix.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed)
🟡 1 suggestion(s)
3 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`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/document.rs:372-390: Nested C ABI still defeats the JNI panic guard for encrypted create/fetch
The JNI entry point runs under `support::guard`, but it invokes this non-unwind `extern "C"` export, which calls `block_on_worker` (runtime.rs:32-60). That helper still uses `.expect("Failed to create tokio runtime for platform-wallet-ffi")` on first-use runtime construction (runtime.rs:38) and `.expect("tokio worker panicked")` when joining the spawned task (runtime.rs:60) — confirmed unchanged at the current head. Neither `.expect()` is caught inside this C-ABI function, so a panic during runtime construction or while awaiting the create/fetch future unwinds directly into a non-unwind `extern "C" fn`, aborting the process before the outer JNI `catch_unwind` (documented in kotlin-sdk/CLAUDE.md as required at every export) gets a chance to translate it into a Java exception. The fetch export at document.rs:429-491 repeats the identical pattern. This is a process-abort DoS vector triggerable by a panic-inducing malformed/adversarial response or corrupted local state, not just an isolated-operation failure.
| let result: Result<(Identifier, String), PlatformWalletError> = | ||
| block_on_worker(async move { | ||
| let signer: &VTableSigner = &*(signer_addr as *const VTableSigner); | ||
| // Generic create path (no key material in scope): fetches the | ||
| // contract, sanitizes the hex `encryptedMetadata` into `Bytes`, | ||
| // auto-selects the AUTHENTICATION signing key, and broadcasts on | ||
| // the 8 MB worker stack. | ||
| let confirmed: Document = identity_wallet | ||
| .create_document_with_signer( | ||
| &owner_id_for_async, | ||
| &contract_id_for_async, | ||
| &document_type_str, | ||
| &properties_json, | ||
| signer, | ||
| ) | ||
| .await?; | ||
| let json_string = confirmed_document_to_json(&confirmed)?; | ||
| Ok::<_, PlatformWalletError>((confirmed.id(), json_string)) | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: Nested C ABI still defeats the JNI panic guard for encrypted create/fetch
The JNI entry point runs under support::guard, but it invokes this non-unwind extern "C" export, which calls block_on_worker (runtime.rs:32-60). That helper still uses .expect("Failed to create tokio runtime for platform-wallet-ffi") on first-use runtime construction (runtime.rs:38) and .expect("tokio worker panicked") when joining the spawned task (runtime.rs:60) — confirmed unchanged at the current head. Neither .expect() is caught inside this C-ABI function, so a panic during runtime construction or while awaiting the create/fetch future unwinds directly into a non-unwind extern "C" fn, aborting the process before the outer JNI catch_unwind (documented in kotlin-sdk/CLAUDE.md as required at every export) gets a chance to translate it into a Java exception. The fetch export at document.rs:429-491 repeats the identical pattern. This is a process-abort DoS vector triggerable by a panic-inducing malformed/adversarial response or corrupted local state, not just an isolated-operation failure.
source: ['claude', 'codex']
2 participants
Closes #4086 (encrypted-document create/update) and #4087 (decrypt-on-fetch) for the wallet
txMetadatacontract in the Kotlin SDK. Context: dash-wallet migration plan Phase 3h step 3 (dashpay/dash-wallet#1507).The single hard requirement is wire-compatibility with the legacy
org.dashj.platformstack (BlockchainIdentity.publishTxMetaData/getTxMetaData): the Android wallet already publishes per-identity encryptedTxMetadatadocuments through the legacy library, so documents written by the legacy stack must decrypt with this implementation and vice versa — otherwise migrated users lose their memos / tax categories / exchange-rate records / gift cards.Recovered legacy scheme (byte-level)
Recovered by decompiling the legacy jars (
dash-sdk-kotlin4.0.0-RC2-SNAPSHOT —BlockchainIdentity,TxMetadataDocument,TxMetadata) andorg.bitcoinj.crypto.KeyCrypterAESCBC(dashj-core22.0.3), cross-checked against the dash-walletPlatformSyncServicefetch/publish loop.Key derivation — NOT ECDH, NOT HKDF, no hashing. The AES-256 key is literally the raw 32-byte secp256k1 private scalar of a hardened HD child.
KeyCrypterAESCBC.deriveKey(ECKey)isnew KeyParameter(ecKey.getPrivKeyBytes()). The EC key is derived at:ChildNumber(32769, hardened)(0x8001, fromTxMetadataDocument's static init).keyIndex= the identity'sgetFirstPublicKey(ENCRYPTION, MEDIUM).getId()(fallbackAUTHENTICATION, HIGH);encryptionKeyIndex= the caller-supplied per-document index.decryptTxMetadatareads both back from the document and re-derives symmetrically. (The naming is counter-intuitive but internally consistent: document fieldkeyIndexholds the encryption key id, fieldencryptionKeyIndexholds the app's index.)encryptionKeyIndex = 1 + countAllRequests()(a monotonic per-document counter) andversion = VERSION_PROTOBUF (1).Cipher: AES-256-CBC / PKCS7 (BouncyCastle
PaddedBufferedBlockCipher(CBCBlockCipher(AESEngine))), random 16-byte IV fromSecureRandom. No HMAC / tag / GCM.Stored
encryptedMetadatablob layout (the authoritativecreateTxMetadata/decryptTxMetadataframing — the separateTxMetadataDocument.decrypthelper uses a different, unused layout and was NOT followed):The version byte is outside the ciphertext. Plaintext for version 1 is a protobuf
WalletUtils.TxMetadataBatch; for version 0 a CBOR list — the wallet'sTxMetadataItemschema.Contract / document schema: contract id
7CSFGeF4WNzgDmx94zwvHkYaG3Dx4XEe5LFsFgJswLbm(testnet; owner[0u8;32]), appwallet-utils, document typetxMetadata— already present in this repo aspackages/wallet-utils-contract. Fields:keyIndex(int),encryptionKeyIndex(int),encryptedMetadata(byteArray, 32..=4096). Query on($ownerId asc, $updatedAt asc); the wallet's fetch is$ownerId == owner AND $updatedAt >= since, ordered by$updatedAt.Payload boundary
The SDK owns only the encryption envelope (key derivation,
version ‖ IV ‖ ciphertext, AES-CBC) and the three document fields. The decrypted payload is opaque to the SDK — the app keeps ownership of the protobufTxMetadataBatchitem schema and the batching policy, exactly as on the legacy stack (batching stays app-side per the issue). Create takes already-serialized payload bytes + version; fetch returns decrypted payload bytes (base64 in JSON) + version.Implementation
rs-platform-wallet—crypto/tx_metadata.rs:derive_tx_metadata_key(identity_auth_derivation_path_for_type(ECDSA, identity_index, keyIndex)extended by[32769', encryptionKeyIndex'], raw scalar as the AES key — the same base-path machinery a registered identity's keys use, the same extend-by-two-hardened-children shape ascontactInfo);seal_tx_metadata/open_tx_metadatafor the blob.network/encrypted_document.rs:create_encrypted_document_with_signer(selects the encryption key id to match legacy, seals, and reuses the testedcreate_document_with_signerpath) andfetch_encrypted_documents(paginated owner-scoped since-timestamp scan mirroring thecontactInfodecrypt loop; undecryptable docs skipped, never aborting).rs-platform-wallet-ffi—platform_wallet_create_encrypted_document_with_signer,platform_wallet_fetch_encrypted_documents(JSON out; payload base64).rs-unified-sdk-jni—documentCreateEncrypted,documentFetchEncrypted.kotlin-sdk—DocumentTransactions.createEncryptedDocument(...)/fetchEncryptedDocuments(...). Additive only — no existing signature changes.Wire-compat test status (stated honestly)
Included Rust tests: seal↔open round-trip (both version bytes); key-derivation determinism +
keyIndex/encryptionKeyIndexseparation; wrong-key and malformed-blob fail cleanly (no panic); since-timestamp query shape; and a NIST SP 800-38A F.2.5 CBC-AES256 cross-stack vector that pins the AES-256-CBC core + theversion ‖ IV ‖ ciphertextframing to a published third-party vector (the same bytes BouncyCastle'sKeyCrypterAESCBCproduces).What is fully pinned: the encryption envelope — cipher, PKCS7, IV/version framing, and the field layout.
What still needs a real legacy sample: the mnemonic→key HD derivation-path account prefix cannot be reconstructed from the legacy jars alone (it lives in dashj wallet config, not in the tx-metadata code). Analysis shows Rust's
identity_auth_derivation_path_for_type(ECDSA, identity_index=0, key_index=N)reproduces dashj'sblockchainIdentityECDSADerivationPath(N)(m/9'/coin'/5'/0'/0'/0'/N') for the primary identity, so appending/ 32769' / encryptionKeyIndex'should reconstruct the exact legacy key — but this is not yet confirmed against a real legacy-written document. Requesting one legacy-writtentxMetadatasample (itskeyIndex,encryptionKeyIndex, andencryptedMetadatahex, plus the identity's index) to close the end-to-end derivation vector before this is relied on for migration.cc @QuantumExplorer
Summary by CodeRabbit