feat(platform-wallet): allocate txMetadata encryptionKeyIndex in Rust, not the host (stacked on #4186)

[bfoss765]

Stacked on #4186 (encrypted documents) — do not merge before it.

Addresses shumkov's #4186 review: the encryptionKeyIndex allocation policy
was left in the Kotlin host, which asked callers to supply the legacy
1 + countAllRequests() counter. Concurrent callers/devices could pick the
same index, and a caller-side key-index loop breaks the SDK host-thin rule.
This moves the policy into Rust; hosts now pass only the opaque payload.

What changed

• IdentityWallet::allocate_encryption_key_index derives the next index from
  authoritative Platform state — it counts the identity's existing txMetadata
  documents and returns 1 + count, matching dash-wallet's retired
  1 + countAllRequests() (SELECT COUNT(*) FROM transaction_metadata_platform)
  exactly in FORMULA (count+1, not max+1; empty → 1) — but the count SOURCE is now
  authoritative Platform state rather than the local Room table, so produced numbers
  legitimately diverge from legacy under restore-from-seed (new is strictly SAFER:
  legacy restarted at 1 and collided) and under publish-lag (safe; collisions are
  non-lossy per the IV argument above). Coverage note: the Platform-count seed path
  itself is testnet-integration-only; unit tests inject the seed.
• Allocation is serialized under a shared per-wallet mutex, so two concurrent
  creates through the same process can never pick the same index (seed once
  from Platform, then hand out increasing indices in-process).
• FFI: ABI-additive sibling
  platform_wallet_create_encrypted_document_with_signer_auto_index (existing
  explicit-index export unchanged); both share one impl over Option<u32>.
• JNI: encryptionKeyIndex == -1 is the "allocate in Rust" sentinel.
• Kotlin: createEncryptedDocument(encryptionKeyIndex: Int? = null) — null
  lets Rust allocate; removed the 1 + countAllRequests() guidance and
  deprecated the caller-supplied counter. Explicit index kept for migration/tests.

Cross-device caveat (documented in code): the index is best-effort unique
per device. A cross-device duplicate is not data-loss: each document stores
its own keyIndex/encryptionKeyIndex and the reader derives each document's
key from its own stored indices, so both documents decrypt independently.

Tests: Rust unit tests for the legacy 1+count math, empty-state seeding,
per-owner isolation, and concurrent no-collision; Kotlin tests for the no-index
path and explicit-negative rejection. cargo test/clippy green on the three
touched crates; :sdk:compileDebugKotlin + :sdk:testDebugUnitTest green.

[coderabbitai]

Warning

Review limit reached

@bfoss765, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f1c56214-819e-4b9c-91ff-ccea73ca1bbf

📥 Commits

Reviewing files that changed from the base of the PR and between 267e1cb and 4f2eb06.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (22)

• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactions.kt
• packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/TransactionsNative.kt
• packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/documents/DocumentTransactionsVersionValidationTest.kt
• packages/rs-platform-wallet-ffi/Cargo.toml
• packages/rs-platform-wallet-ffi/src/document.rs
• packages/rs-platform-wallet-ffi/src/error.rs
• packages/rs-platform-wallet/Cargo.toml
• packages/rs-platform-wallet/src/error.rs
• packages/rs-platform-wallet/src/lib.rs
• packages/rs-platform-wallet/src/wallet/identity/crypto/mod.rs
• packages/rs-platform-wallet/src/wallet/identity/crypto/tx_metadata.rs
• packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs
• packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
• packages/rs-platform-wallet/src/wallet/identity/network/mod.rs
• packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
• packages/rs-platform-wallet/src/wallet/platform_wallet.rs
• packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyDerivationPathCheck.java
• packages/rs-platform-wallet/tests/legacy_wire_compat/LegacyKeyN.java
• packages/rs-platform-wallet/tests/legacy_wire_compat/README.md
• packages/rs-platform-wallet/tests/txmetadata_fetch.rs
• packages/rs-unified-sdk-jni/src/support.rs
• packages/rs-unified-sdk-jni/src/transactions.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Review complete (commit 4f2eb06)
Last checked: 2026-07-23 21:30 UTC

[shumkov]

Reviewed (two independent passes). The allocation design is sound and legacy-compatible: 1 + Platform-count reproduces dash-wallet's formula while fixing the restore-from-seed collision, the per-wallet mutex correctly serializes the seed fetch (race-tested with a genuinely discriminating test), offline fails loud, and the cross-device index-reuse analysis (CBC + fresh IV, readers derive from each document's stored index) is honest. Two items:

1. Validate payload size before allocating the index. The auto-index path performs the network count/allocation before the deterministic 4063-byte payload validation, so an oversized payload — which must fail — still consumes an index (leaving a gap). Run the size check first; it needs no network.

2. Coordinate with feat(swift-sdk): wrappers for the txMetadata encrypted-document FFI exports (stacked on #4186) #4194: that PR currently wraps only the explicit-index export this PR deprecates (no *_auto_index wrapper in Swift) — see the comment there; the two should land in an agreed order so iOS doesn't re-diverge on the exact policy this PR unifies.

Minor: encryptionKeyIndex moved to the last positional slot in Kotlin — source-breaking for positional callers, worth a changelog line when the #4186 stack ships.

[bfoss765]

Fixed: the size gate now runs before index allocation on the auto-index path — an oversized payload returns TxMetadataPayloadTooLarge without ever seeding or advancing the allocator's high-water (no consumed index, no gap), covered by a new test that asserts the owner is absent from the allocator map and the next well-sized reservation still seeds at 1. Added the release note that encryptionKeyIndex's move to the last positional Kotlin slot is source-breaking for positional callers.

Sequencing per your decision: this PR (#4195) lands first; #4194 has been reworked onto the new auto-index export.

[shumkov]

Round-3 verification (two independent passes, reconciled): the fix is verified — the payload size gate runs strictly before the allocator mutex and before any Platform seed polling on the auto-index path (and before derivation on the explicit path), all three gates share the single computed constant via one function (defense-in-depth, not drift), and the new oversized_payload_does_not_advance_highwater test genuinely proves the seed future is never polled and the allocator stays unseeded.

Code-ready. Two asks before merge:

• A clean Swift CI rerun — the current red is the mac-runner keychain regressing again (keychainError(-61) in unrelated IdentityResolverSignIntegrationTests), not this PR.
• Body refresh: this body doesn't mention the resolved feat(swift-sdk): wrappers for the txMetadata encrypted-document FFI exports (stacked on #4186) #4194 sequencing (re-stack), and both stacked bodies still describe the old ordering. Nit: fix-round narration in a few new comments — provenance to the description.

[shumkov]

Covered by the same-repo stack dispatch run (29994291221) — full Rust workspace suite green, including the oversized-allocator test. No further asks beyond the earlier body-refresh notes; merge order #4195 → #4194 stands.

[thepastaclaw]

Final validation — Codex + Sonnet

This PR correctly moves txMetadata encryptionKeyIndex allocation from the Kotlin host into Rust: the tokio::sync::Mutex-based allocator is properly serialized across awaits, the size-gate-before-allocation fix is sound and unit-tested, and the duplicate-index-is-safe cryptographic reasoning holds up. Remaining issues are non-blocking: the FFI boundary copies the plaintext payload before the deterministic size check runs, the authoritative Platform-count seed path lacks automated test coverage, two locks (the FFI handle-storage RwLock and the allocator's own tokio Mutex) are now held across longer network round trips, and there is a duplicated contract-fetch block plus a couple of nitpicks.

Corrected in place after the final gate was rerun against the checkpointed Codex canonical output; three Codex-only findings already closed by the precheck verifier were retracted.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (completed), gpt-5.6-sol — rust-quality (completed), gpt-5.6-sol — ffi-engineer (completed)
• Verifier: claude-sonnet-5 — final-verifier
• Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — security-auditor (failed), claude-sonnet-5 — rust-quality (completed), claude-sonnet-5 — ffi-engineer (completed), claude-sonnet-5 — general (failed), claude-sonnet-5 — security-auditor (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — security-auditor (failed), claude-sonnet-5 — security-auditor (failed), claude-sonnet-5 — security-auditor (completed)

🟡 7 suggestion(s) | 💬 2 nitpick(s)

Nitpick details

• Comment edit merged two unrelated sentences onto one run-on line — packages/rs-unified-sdk-jni/src/transactions.rs:928-931
  Removing the '(#4091)' reference collapsed two separate comment thoughts onto a single line: '...so these no longer need to be loud. Android visibility: JNI_OnLoad installs android_logger at'. cargo fmt does not reflow comments, so this run-on will persist as-is; split it back across lines for readability.
• Explicit-index create path bypasses the allocator entirely, so the no-collision guarantee only holds when a process never mixes both entry points — packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:401-429
allocate_encryption_key_index/reserve_next_index_checked only runs when the host omits the index. The explicit-index export (platform_wallet_create_encrypted_document_with_signer) never touches enc_key_index_allocator, so a caller mixing the explicit-index (migration/tests) and auto-index paths for the SAME owner in the SAME process can still race: an in-flight explicit create at index N doesn't update the in-process high-water, and a concurrent auto-index call whose Platform-count seed hasn't yet observed N's unconfirmed broadcast can allocate N again. Per the PR's own design (duplicate indices are safe since each document stores and decrypts from its own indices), this isn't exploitable for data loss or key compromise, but the advertised guarantee ('two concurrent creates through the SAME wallet process can never pick the same index') only holds when all callers in that process exclusively use the auto-index path — worth a one-line doc note on the explicit-index export making that scope limit explicit.

🤖 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:435-445: Oversized payload is copied into an owned buffer before the deterministic size gate rejects it
  create_encrypted_document_impl unconditionally copies `payload_len` bytes via `slice::from_raw_parts(payload, payload_len).to_vec()` (wrapped in `Zeroizing`) before any size validation runs. The network-free `ensure_tx_metadata_payload_fits` check only executes later — inside `allocate_encryption_key_index`/`reserve_next_index_checked` on the auto-index path, or inside `prepare_encrypted_txmetadata_properties` on the explicit-index path — both of which run strictly after this copy. A caller passing an oversized `payload_len` (host bug, fuzzed input, or a misbehaving JNI caller) forces a full allocation and memcopy proportional to the invalid input before the typed `TxMetadataPayloadTooLarge` error is returned; an allocation failure for a very large length aborts the process rather than surfacing the typed error. The length is known before any bytes are touched, so hoist the pure length check ahead of the copy — and mirror it in the JNI wrapper's `convert_byte_array` call so a Java caller doesn't pay for the doomed copy on its side either.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/document.rs:454-538: Auto-index path performs a second network round trip (plus a reentrant host callback) inside the shared handle-storage read lock
  The whole `create_encrypted_document_impl` body runs inside `PLATFORM_WALLET_STORAGE.with_item(wallet_handle, move |wallet| {...})`, which holds a `parking_lot::RwLock` read guard over the entire global `HashMap<Handle, T>` (see `HandleStorage::with_item` in handle.rs) for the whole closure. Before this PR that closure did one blocking network round trip (the broadcast). Now, on the host-omitted-index path, it does a second one first — `allocate_encryption_key_index`, which itself may paginate through every existing txMetadata document for the owner on the first call — followed by a synchronous reentrant call into the host's mnemonic resolver for external-signable wallets. Any thread needing the write half of this lock for the same handle (`with_item_mut`/`remove`, e.g. during wallet teardown) now blocks for roughly double the previous worst case, plus however long the resolver callback (Keychain/biometric prompt) takes. This extends a pre-existing accepted pattern rather than introducing a new bug class, but since `Arc<PlatformWallet>` is already cheaply cloneable, cloning it and dropping the guard before any network/resolver work would remove the exposure rather than lengthen it.

In `packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:329-425: The authoritative Platform-count seed path has no automated test coverage
  `allocator_tests` thoroughly pins `next_encryption_key_index_from_count`, `reserve_next_index`, and `reserve_next_index_checked` by injecting the seed as a bare future — none of them call `count_owned_txmetadata_documents` or `allocate_encryption_key_index`, so the pagination loop, contract-fetch/register step, contract-not-found error path, raw-entry counting (including un-materialized `None` entries), and the `u32::try_from` conversion are never exercised by `cargo test`. Per the PR description this path is only covered by a manual/testnet-gated integration test. Add a mock-SDK test that drives the real first-allocation path end to end (including a multi-page and a `None`-entry case) and asserts the resulting index equals `raw_count + 1`, plus that a second allocation for the same owner reuses the cached high-water without a second query. This is genuinely new logic introduced by this PR and is exactly the kind of regression CI won't currently catch.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:81-97: Wallet-wide allocator mutex held across an un-timeboxed network round trip
  `reserve_next_index` acquires `allocator.lock().await` and holds it across the entire first-time seed future — which, via `allocate_encryption_key_index` → `count_owned_txmetadata_documents`, does a `DataContract::fetch` plus a paginated `Document::fetch_many` against Platform. `enc_key_index_allocator` is a single mutex shared across every owner identity in the wallet process, and rs-sdk's default `RequestSettings` leaves `timeout`/`connect_timeout` unset. A slow or unresponsive DAPI node can therefore stall the seed indefinitely, and because the lock is held for the duration, every OTHER identity's auto-index encrypted-document create in the same wallet process blocks behind it too — a single bad node turns into a wallet-wide freeze of the encrypted-document-create feature rather than a failure scoped to the identity that triggered the fetch. Scoping the lock per-owner (e.g. a `HashMap<Identifier, Mutex<u32>>` or an entry-level lock) or bounding the seed future with a timeout would contain the blast radius to the one owner actually waiting on Platform.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:329-354: count_owned_txmetadata_documents duplicates the fetch+register-contract block from fetch_encrypted_documents
  `count_owned_txmetadata_documents` (lines 337-349: `DataContract::fetch` → `Arc::new` → `context_provider().register_data_contract`) is nearly a line-for-line copy of the same three steps in `fetch_encrypted_documents` (lines 604-626) further down in the same `impl` block. This PR adds a third instance, in spirit, of a pattern already duplicated once — and the two existing copies already drift slightly (different 'contract not found' message text, and only one emits a `breadcrumb_error` on fetch failure). Extract a small helper, e.g. `async fn fetch_and_register_contract(&self, contract_id: &Identifier) -> Result<Arc<DataContract>, PlatformWalletError>`, to stop the duplication and the wording drift.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/encrypted_document.rs:43-44: Allocator high-water map is keyed only by owner identity, not by (owner, contract, document type)
  `EncryptionKeyIndexAllocator` is `HashMap<Identifier, u32>`, keyed solely by `owner_identity_id`. `allocate_encryption_key_index` takes `contract_id` and `document_type_name` as first-class parameters and seeds the high-water from a count scoped to that specific contract/document-type pair — but once seeded, the stored high-water is shared across ANY subsequent call for that owner regardless of `contract_id`/`document_type_name`. Today there is exactly one wallet-contract document type (`txMetadata`), so this is dormant, but the API is deliberately generalized (both the FFI exports and the Kotlin surface accept an arbitrary `document_type_name`). If this is ever reused for a second encrypted-document type on the same identity, the second type's allocation would silently continue from the first type's high-water instead of reseeding from `1 + count` for its own contract/type — producing an `encryptionKeyIndex` that no longer matches the documented `1 + count` semantics for that type.

In `packages/rs-unified-sdk-jni/src/transactions.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/transactions.rs:809-818: Zeroization effort covers only the native-side plaintext copies, not the originating JVM byte[]
  `documentCreateEncrypted` wraps its native copy of the txMetadata plaintext in `Zeroizing` and drops it as soon as the FFI call returns, and `create_encrypted_document_impl` does the same for its own copy — both explicitly documented as guarding against the plaintext lingering across the broadcast await. But the actual origin of that plaintext is the Kotlin-side `payload: ByteArray` (`DocumentTransactions.kt createEncryptedDocument`), which is passed straight through to `TransactionsNative.documentCreateEncrypted`. `env.convert_byte_array(&payload)` copies out of the JVM array into the Rust `Vec`, but the JVM array itself — and any Kotlin-side protobuf-serialization buffers that produced it — remain resident in the JVM heap indefinitely, subject to ordinary GC rather than scrubbing, with no wipe performed by or requested of the Kotlin caller after the native call returns. Given how much care this PR takes to scrub the two Rust-side copies, the asymmetry is worth a one-line doc note on `createEncryptedDocument` so future auditors don't assume the whole cross-language plaintext lifecycle is scrubbed.

[thepastaclaw]

🟡 Suggestion: Oversized txMetadata payloads are copied twice across the JNI/FFI boundary before the size limit is enforced

documentCreateEncrypted calls env.convert_byte_array(&payload) to copy the full JVM byte[] into a native Vec before any size check runs. The downstream FFI call (rs-platform-wallet-ffi/src/document.rs ~line 440-445, create_encrypted_document_impl) copies it a second time via slice::from_raw_parts(...).to_vec(). Only after both copies exist does ensure_tx_metadata_payload_fits (invoked from allocate_encryption_key_index/prepare_encrypted_txmetadata_properties) reject payloads over the 4063-byte limit. A caller supplying a large valid buffer (a compromised host component, or a direct C/JNI caller bypassing normal app logic) forces two full native allocations before receiving the typed TxMetadataPayloadTooLarge/ErrorInvalidParameter error, risking an OOM abort of the host process instead of a clean typed failure. Check the Java array length via env.get_array_length (already used elsewhere in this file, e.g. line 195) before convert_byte_array, and check payload_len against the same limit at the C FFI entry point before constructing the slice, so non-JNI callers get the same protection.

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: Auto-index create path extends how long the global wallet-handle-storage lock is held, now across a network round trip and a reentrant host callback

The entire create body runs inside the closure passed to PLATFORM_WALLET_STORAGE.with_item(wallet_handle, ...), which holds a parking_lot::RwLock read guard over the process-wide HashMap<Handle, Arc<PlatformWallet>> shared by every wallet handle. Before this PR, that closure performed one network round trip (the broadcast). This PR adds two more phases inside the held guard on the None-index path: allocate_encryption_key_index (a full network fetch — contract lookup plus a possibly-paginated document count on the first call per owner) and, for external-signable/watch-only wallets, a synchronous reentrant call into the host's Kotlin mnemonic resolver via tx_metadata_key_master_for_wallet (which may block on Keychain/Keystore or a biometric prompt). Any thread needing the write half of this lock for ANY handle (wallet dispose/create via with_item_mut/insert/remove) now blocks for the duration of all three phases plus the broadcast. Since Arc<PlatformWallet> is cheap to clone, the guard could be dropped immediately after cloning it, before any network or resolver work, removing the exposure. This extends a pre-existing accepted pattern (also present unmodified in identity_discovery.rs/identity_key_preview.rs) rather than introducing it, but it measurably lengthens the window during which unrelated wallet-lifecycle calls can stall (a potential ANR on Android).

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: The authoritative Platform-derived allocator seed path has no automated regression coverage

allocator_tests (module starting ~line 842) thoroughly pins next_encryption_key_index_from_count, reserve_next_index, and reserve_next_index_checked — but every one of those tests injects the seed as a bare async { Ok(N) } future, never calling allocate_encryption_key_index or count_owned_txmetadata_documents (lines 329-354, 397-424), the code that actually fetches/registers the contract, runs the paginated owner-scoped query, and turns the raw result into 1 + count. Per the PR description, this path is only exercised by a manual/testnet-gated integration test that is #[ignore]d in CI and doesn't invoke the allocator. A regression in the query construction, pagination, contract registration, or the count→index wiring (e.g. an off-by-one, a missed page, or miscounting un-materialized entries) would ship undetected. A mock-SDK test that returns a known owner-scoped count (including a multi-page case and a None document entry) and asserts the resulting allocated index would close this gap without a live network.

_{source: ['claude', 'codex']}

[thepastaclaw]

Retracted after final-gate correction. The Codex precheck verifier had already refuted this finding: the deployed wallet contract's $ownerId,$updatedAt index is neither countable nor rangeCountable, while DocumentCount::fetch requires a matching countable/range-countable index. Those flags are immutable after contract creation, so the suggested aggregate query would fail rather than improve this path.

[thepastaclaw]

Retracted after final-gate correction. The precheck verifier correctly classified this as pre-existing, out-of-scope decrypt-side memory-hygiene debt. The result types, AES helper behavior, and fetch-side serialization copies predate this allocator PR; fixing the full ownership/zeroization pipeline belongs in a separate redesign.

[thepastaclaw]

🟡 Suggestion: count_owned_txmetadata_documents duplicates the fetch-and-register-contract block already present in fetch_encrypted_documents

Lines 337-349 (DataContract::fetch → Arc::new → context_provider().register_data_contract) are nearly a line-for-line copy of the same three steps in fetch_encrypted_documents at lines 604-626 further down in the same impl block. The two copies have already drifted: only the fetch_encrypted_documents copy emits a breadcrumb_error on fetch failure and includes the identifying context in its 'not found' message, while the new copy in count_owned_txmetadata_documents has a slightly different message text and no breadcrumb. Extracting a shared async fn fetch_and_register_contract(&self, contract_id: &Identifier) -> Result<Arc<DataContract>, PlatformWalletError> would remove the duplication this PR adds and stop the error-message/breadcrumb wording from drifting further between the two call sites.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: Allocator high-water map is keyed only by owner identity, not by (owner, contract, document type)

EncryptionKeyIndexAllocator is HashMap<Identifier, u32>, keyed solely by owner_identity_id. allocate_encryption_key_index takes contract_id and document_type_name as first-class parameters and seeds the high-water from a count that IS scoped to a specific contract/document-type pair — but once seeded, the stored high-water is reused for ANY subsequent call for that owner regardless of contract_id/document_type_name. Today there is exactly one wallet-contract document type (txMetadata), so this is dormant. But both the FFI exports and the Kotlin surface already accept an arbitrary document_type_name, generalizing the API; if a second encrypted-document type is ever added for the same identity, its allocation would silently continue from the first type's high-water instead of reseeding from 1 + count for its own contract/type, producing an index that no longer matches the documented 1 + count semantics for that type.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: Wallet-wide allocator mutex is held across an un-timeboxed network round trip, letting one slow node stall every identity's encrypted-document creates

reserve_next_index acquires allocator.lock().await and holds it across the entire Platform fetch used to seed the high-water on a caller's first allocation (seed.await? at line 96). enc_key_index_allocator is a single Arc<tokio::sync::Mutex<..>> shared across every identity in the wallet process (cloned via Arc::clone in identity_handle.rs), and rs-sdk's default RequestSettings sets timeout: None / connect_timeout: None (packages/rs-sdk/src/sdk.rs:80-82), so nothing at the SDK layer bounds this await. A DAPI node that accepts the connection but never completes the response can stall the seed indefinitely, and because the mutex is held for the duration, EVERY OTHER identity's auto-index create in the same wallet process blocks behind it too — a single unresponsive node turns into a wallet-wide freeze of the encrypted-document-create feature, not just a failure for the identity that triggered the fetch. Scoping the lock per-owner, or wrapping the seed future with a bounded timeout, would contain the blast radius to the affected identity.

_{source: ['claude']}

[thepastaclaw]

Retracted after final-gate correction. This synchronous preparation API and its blocking_read implementation predate the allocator change, its rustdoc explicitly requires a synchronous context, and the production FFI call site satisfies that precondition before entering the broadcast future. It is not an in-scope defect introduced by this PR.

[thepastaclaw]

🟡 Suggestion: Plaintext-zeroization effort covers only the native-side copies, not the originating JVM byte[]

documentCreateEncrypted wraps its native copy of the txMetadata plaintext in Zeroizing and drops it as soon as the FFI call returns, and create_encrypted_document_impl does the same for its own copy — both explicitly guard against the plaintext lingering across the broadcast await. But the actual origin of that plaintext is the Kotlin-side payload: ByteArray, which is passed straight through from DocumentTransactions.createEncryptedDocument to the native call. env.convert_byte_array copies OUT of the JVM array into the Rust Vec, but the JVM array itself — and any Kotlin-side protobuf-serialization buffers that produced it — remain resident in the JVM heap subject to ordinary GC, not scrubbing, with no wipe requested of the Kotlin caller after the native call returns. Given how carefully the two Rust-side copies are scrubbed, the JVM-side copy is the one that actually lives longest and is entirely outside Rust's control for a heap-dump/memory-scraping threat model. If that boundary is intentionally out of scope, a one-line doc note on createEncryptedDocument would prevent future auditors from assuming the whole cross-language plaintext lifecycle is scrubbed.

_{source: ['claude']}