Repository navigation
feat(wasm-sdk): seal and scan DashPay private contact requests - #5811
Conversation
The prepared-keys test also opens an entry with the first key of the set, so a scan skipping earlier keys would fail it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The SDK can now append entries to a private document type's store, paid from the shielded pool (type 29), and find a wallet's entries in it: - `PrivateDocumentCreateFromShieldedPool` builds, structure-checks and broadcasts the transition from a proven bundle, and waits for its nullifiers to be proven spent. - `PrivateDocumentScanKeys` and `Sdk::scan_private_document_page` sort a proved page of a store into what the wallet's identities received (their keys open it), what they sent (their outgoing viewing keys recover it) and what opened but is refused, with the cursor to scan from next. - DashPay: `PrivateContactRequest` with the schema's bounds, `seal_private_contact_request` (the sender's encryption key, a deniable MAC, and the outgoing viewing key derived from that key), and `Sdk::scan_private_contact_requests`, which decodes each request, checks its sender against the identity key it names and keeps the first request per sender, recipient and account reference. platform-encryption: prepared recipient keys check a sender with the key that opened the entry, so a scan never copies secret keys out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… contact requests wasm-sdk binds the SDK's private document and DashPay private contact request helpers: - `getPrivateDocumentEntries(dataContractId, documentTypeName, startPosition)`: a proved page of a private type's store. - `sealPrivateContactRequest(options)`: the entry, sealed from the sender's encryption key to one of the recipient's DashPay keys. - `scanPrivateContactRequests(options)`: one page of DashPay's store for the wallet's identities: received requests with their senders checked, sent requests recovered with the identities' encryption keys, and the entries refused, with the cursor to scan from next. evo-sdk exposes them as `sdk.privateDocuments.entries`, `sealContactRequest` and `scanContactRequests`. Appending a sealed entry needs an Orchard proof, which this build cannot make yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A wallet restoring its sent requests needs the encryption key each was sealed with to rebuild its local record of the request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A private contact request carried a deniable MAC keyed by the static ECDH of the sender's encryption key and the recipient's key. Anyone holding the recipient's key could compute it, so a stolen recipient key forged requests from any sender, including rotations that redirect the recipient's payments. The request is now signed by one of the sender's signing keys, as its public request's transition is: an enabled AUTHENTICATION key at a level DashPay's documents are signed at (CRITICAL or HIGH), ECDSA_SECP256K1 or ECDSA_HASH160. The entry still names the encryption key, which the pair's shared key and the outgoing viewing key come from; the reader recovers the signing key from the signature. Entries with a MAC are refused. Envelope (signature mode, nothing deployed): the signature now signs the double SHA-256 of the signing data, as identity signers sign data, so a wallet signs with the signer it signs transitions with. The caller passes the signature; `private_document_signing_data`, `sign_private_document_signing_data`, `recover_private_document_signer` and `OpenedPrivateDocumentEntry::recover_signer` are new. Prepared recipient keys are wiped on drop. Reading also checks, at the entry's block time, the recipient key it is encrypted to, and that a sent entry names the identity and key its outgoing viewing key comes from and is signed by that identity. The sender is checked before the payload is decoded. `PrivateDocumentScanKeys::verify_sender` tries every key set of the identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
`sealPrivateContactRequest` takes the `signer: IdentitySigner` the sender
signs its transitions with, which signs the request with the first of the
sender's signing keys it holds. The encryption key's private key is now
`senderEncryptionPrivateKey`.
`scanPrivateContactRequests` takes each sender's encryption keys with their
ids (`encryptionKeys: [{ keyId, privateKey }]`), which a sent entry must
name. Sent results carry `senderKeyId`, and the reasons `notSigned` and
`recipientKeyNotUsable` are mapped. Private keys read from the options are
wiped once used.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 38 minutes. View limit details
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. |
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit c5d08bb. Normal review starts when eligible; priority review starts as soon as a slot is available. |
Review fixes: - The envelope's authenticated data names the recipient key `ser(R)`, so a body re-encrypted to another key of the same identity (an old key's holder re-targeting a signed request) authenticates no sender. `private_document_signing_data` takes the recipient. The MAC and signature vectors are re-pinned; nothing is deployed. - A signing key bound to a contract signs only inside its bounds: DashPay, or its `privateContactRequest` type. A contract group bound signs none, as a client cannot read the group's members as they were. The seal and the reader apply the same rule; `private_contact_request_signing_keys` lists the keys that qualify. - `PrivateContactRequest::from_properties` drops a null only under an optional property's name, so an unknown property set to null is refused. - Recovered sent requests are deduplicated per sender, recipient and account reference, apart from the received ones. - Senders added with the same outgoing viewing key: an entry recovers as the one it names. - Docs: the transition's proof shows the nullifiers spent, not that its entries were appended; fetching a sender right after its page tells the serving node which identity the wallet looked up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the findings against head 888068a. Two blocking cold-start defects remain: uncached custom contracts cannot verify private-document pages, and private contact operations can resolve DashPay before discovering protocol version 15. This was a static review; current CI contains successful policy/review checks but no build or test results, and the PastaClaw review is pending.
🔴 2 blocking | 🟡 4 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change adds substantial WASM marshalling and facade code, including private-key conversion and cleanup in private_contact_requests.rs, but delegates cryptographic and signature decisions to existing SDK calls rather than introducing intricate critical-surface logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/wasm-sdk/src/private_contact_requests.rs`:
- [BLOCKING] packages/wasm-sdk/src/private_contact_requests.rs:452-455: Load the contract before fetching a private document page
The first query for an uncached custom contract fails even when the server returns a valid proof. PrivateDocumentEntries::FromProof requires provider.get_data_contract() to supply the contract, while the trusted and verified WASM providers consult their known-contract caches and compiled system contracts rather than fetching missing custom contracts. Neither this binding nor the delegated Fetch implementation loads that dependency. Call the existing get_or_fetch_contract helper before fetching the page, as the other contract-dependent bindings do, and cover an initial query for an uncached non-system contract.
- [BLOCKING] packages/wasm-sdk/src/private_contact_requests.rs:506-508: Discover protocol support before resolving the private DashPay type
With no persisted protocol version or cached DashPay override, an unpinned mainnet/testnet SDK starts at version 13. Sdk::seal_private_contact_request immediately resolves DashPay through the context at that version, obtaining a contract without privateContactRequest; signing-key lookup then fails without any network request that could discover version 15. Sdk::scan_private_contact_requests captures the same old contract before its page fetch discovers protocol support, then passes that stale contract to private_contact_requests_from_scan, so the first scan fails too. Locally held identity wrappers make sealing reachable without an earlier proved read, and scanning needs no earlier identity fetch. Fix the ordering in the owning Rust SDK methods by requiring/discovering protocol version 15 before resolving DashPay, following fetch_decision_proposals_contract, and add cold-start coverage for both operations.
- [SUGGESTION] packages/wasm-sdk/src/private_contact_requests.rs:496-504: Make temporary secret-key cleanup cover every exit
After encryption_secret_key is constructed, failures parsing signer, recipientIdentity, recipientKeyId, or request return through ? without reaching non_secure_erase(). The recipient parser has the same cleanup gap at lines 395–406: an invalid later element drops previously collected SecretKey values before the erase loop runs. The pinned secp256k1 0.33.1 SecretKey is Copy and has no erasing destructor, so ordinary cleanup leaves these binding-owned scalar temporaries unwiped. This is defense in depth rather than an exposed-key exploit, but it contradicts the stated cleanup guarantee. Use an owning cleanup guard for temporary scalars and partially constructed collections; parsing all other seal options before deriving the scalar also shortens its lifetime. Preserve the caller-owned PrivateKey objects.
- [SUGGESTION] packages/wasm-sdk/src/private_contact_requests.rs:522-525: Expose the scan's identity-lookup privacy warning
The delegated scanner immediately fetches the identities needed to authenticate entries that opened for the wallet, including their senders and the wallet's recipient/sender identities. A node serving both the page and those lookups can correlate them, revealing information the encrypted store alone does not disclose. The native SDK explicitly documents this tradeoff and offers a lower-level scan/read path with caller-supplied identities, but neither this method's documentation nor the evo-sdk facade carries that warning. Document the correlation risk on both public surfaces and make clear that this convenience API performs those lookups automatically; the native mitigation is not currently exposed by these bindings.
- [SUGGESTION] packages/wasm-sdk/src/private_contact_requests.rs:346-357: Reuse the existing key-option and fixed-byte conversion helpers
The new helpers duplicate conversions already available in the project: encrypted_for::secret_key_from_options performs the same PrivateKey wrapper extraction and scalar conversion, and wasm_dpp2::utils::try_to_fixed_bytes performs the same Uint8Array-to-array conversion. The local fixed_bytes helper already introduces different length-error wording. Reuse the shared fixed-byte helper and the existing key-option helper at the option-reading call sites, or extract a shared wrapper-to-secret conversion if both forms are needed. Keep PrivateKey wrapper semantics intact rather than replacing extraction with a raw-byte parser. This is a helper-reuse suggestion, not a business-logic ownership violation.
In `packages/js-evo-sdk/tests/unit/facades/private-documents.spec.ts`:
- [SUGGESTION] packages/js-evo-sdk/tests/unit/facades/private-documents.spec.ts:19-28: Test the actual WASM option and result conversions
These tests stub all three new WASM methods, so they verify facade forwarding but never execute the Rust binding this PR adds. Native SDK tests likewise start after JavaScript options have been converted. Add local, deterministic binding-level tests using real PrivateKey, Identity, and IdentitySigner wrappers, plus representative nonempty page and scan fixtures. Assert optional request fields, rejection mapping, malformed fixed-size inputs, Uint8Array outputs, and exact bigint positions/counts above Number.MAX_SAFE_INTEGER. Keep the existing tests as the separate facade-forwarding contract.
…y heights Review fixes: - Requests are checked against their type's schema with dpp's document validation (`DataContract::validate_document_properties`), before encoding when sealing and after decoding when reading, as the payload codec asks. The SDK's own copies of the label and proof bounds are gone; `to_properties` is a typed conversion. The SDK enables dpp's `validation` feature. `seal_private_contact_request` and `private_contact_request_signing_data` take `platform_version` last. - The reader takes each identity with the height its proof is of (`ProvedIdentity`). `private_contact_request_identities` lists, per identity, the latest block whose entry it checks; a snapshot proved before it, which a node may serve within the SDK's height tolerance, or one not given, fails the read instead of judging the entry against keys as they were before it. Only signed entries need identities. - `Sdk::fetch_private_contact_request_identities` fetches them four at a time with their metadata and fetches a stale one once more. - Both SDK helpers establish protocol version 15 before resolving DashPay, so an unpinned SDK starting below it does not read a DashPay without the private type. - `PrivateDocumentScanKeys::derive` builds scan keys from key paths with a derivation callback, wiping what it derives, for signers that keep keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
…ry exit Review fixes (with the SDK changes of #5809 merged in, which also resolve DashPay only once protocol version 15 is established): - `getPrivateDocumentEntries` loads the contract (`get_or_fetch_contract`) before fetching a page, as the page's proof is checked against it and the context provider holds a custom contract only once loaded. - Secret keys read from the options are held in a guard that wipes them when dropped, so an error parsing a later option wipes them too. The seal reads the encryption key after every other option. - The scan's docs, and the evo-sdk facade's, say it fetches the identities the page's requests are checked against right after the page, which a node serving both can correlate. - The shared `secret_key_from_options` and `try_to_fixed_bytes` replace the local helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Five prior findings are fixed, but a retained pre-upgrade DashPay contract can still prevent the new sealing and scanning APIs from working after protocol discovery. The binding-conversion coverage suggestion also remains open. This was a static review of head 9d347a5; current CI shows no build/test results or failed code checks, and the review check remains pending.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The substantial new WASM binding module packages/wasm-sdk/src/private_contact_requests.rs directly implements private-key ingestion, lifetime and erasure management, and outgoing viewing-key derivation in ReadSecretKeys and scan_keys_from_options, meeting the intricate key-handling surface criterion despite delegating cryptographic primitives to the SDK. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/wasm-sdk/src/private_contact_requests.rs`:
- [BLOCKING] packages/wasm-sdk/src/private_contact_requests.rs:540-543: Refresh retained DashPay contracts that predate private requests
A browser wallet that previously fetched DashPay v2 can retain that snapshot across the protocol-15 upgrade. WasmSdkBuilder::build reloads persisted contracts into the context cache, and both TrustedHttpContextProvider::get_data_contract and VerifiedHttpContextProvider::get_data_contract return known contracts before consulting the protocol-selected system contract. The delegated SDK helper now establishes protocol 15, but fetch_dashpay_contract → fetch_system_data_contract still accepts the cached v2 contract, which lacks privateContactRequest. Sealing then fails during signing-key/type lookup, and scanning fails when the page verifier resolves that missing type. Neither protocol refresh nor restarting clears this retained snapshot. This breaks the new APIs for an existing-wallet upgrade state, independently of the fixed empty-cache discovery case. Resolve system-contract freshness in the owning SDK/context-provider path so an older retained contract cannot shadow the protocol's required generation, while preserving newer proved overrides. Add regression coverage for a protocol-15 client hydrated with a protocol-14 DashPay snapshot.
In `packages/js-evo-sdk/tests/unit/facades/private-documents.spec.ts`:
- [SUGGESTION] packages/js-evo-sdk/tests/unit/facades/private-documents.spec.ts:19-27: Test the actual WASM option and result conversions
(existing thread: https://github.com/dashpay/platform/pull/5811#discussion_r4506329231)
These stubs bypass all three new bindings, so the tests establish facade forwarding but not request_from_options, scan_keys_from_options, rejection-name mapping, or EntriesPage/ScanResult serialization. The native SDK tests begin with typed Rust inputs and cannot verify those JavaScript-boundary behaviors. Although a WASM runtime is required, a new functional harness is not: wasm-sdk already has wasm-bindgen-test library tests, the reusable tests-rs-wasm.yml workflow runs them under Node, and state_transitions/document.rs demonstrates testing private option-conversion helpers directly. Add deterministic binding tests using real PrivateKey, Identity, and IdentitySigner wrappers and nonempty result fixtures. Cover optional fields, malformed 69/43-byte inputs, rejection reasons, Uint8Array outputs, and exact bigint positions/counts above Number.MAX_SAFE_INTEGER. Keep these forwarding tests separately. If this coverage is deferred to the functional suite, link a tracking issue or follow-up PR; the supplied discussion contains no tracking reference.
…he private type Review fixes: - A signing key past its `expires_at` signs nothing: the reader checks it at the entry's block time with dpp's `is_expired_at` (the expiry instant is expired), and the key list and the seal check it at this device's clock, so an expired key is not offered. - `private_document_create_from_shielded_pool` establishes protocol version 15 before building the transition, which an older SDK's limits would refuse for any entries. - A DashPay the context provider holds from before the private type is refetched from the network and registered with the provider, which the page's proof is verified against. - The reader refuses an identity given under another identity's id, and a recipient key id that is not the key the entry is encrypted to. - The identity fetch iterates owned pairs, so its future is `Send` (rust-lang/rust#100013), and its retry logic is a function tests drive with scripted proofs. - An OS RNG failure is an error, not a panic. - The wasm-sdk contract fixture gives its indexed `rarity` string the `maxLength` dpp's now enabled validation requires of an indexed string. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
… the refreshed DashPay A private contact request's encryption keys, the sender's and the recipient's, are now used only inside their contract bounds: unbound, bound to DashPay, or bound to DashPay's contactRequest or privateContactRequest. contactRequest is allowed because DashPay requires bound keys for it and apps register their DashPay keys bound to it. Keys bound to another contract, another DashPay type or a contract group are refused by the seal and the reader. dpp decides the document-type scope once, in ContractBounds::check_document_type (Allowed, Denied, or needing the group's members, which a client refuses). The signing key check and encrypted_for's key check use it too. After registering a refreshed DashPay, the helper reads it back from the context provider and fails if the provider still lacks the private type, as every page's proof would. The reader's docs no longer claim a key was the identity's when the entry was appended. Tests: encryption key bounds for both roles in the seal and the reader; the DashPay refresh on an unpinned SDK, the already-current shortcut, and a refresh the provider does not take; a send on an unpinned SDK learns the network's version before it builds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
…validate wasm contracts with their tokens
A DashPay fetched from the network because the context provider held none
was returned without being registered, so every page's proof, which is
verified against the provider's contract, failed. The helper now gives the
provider any DashPay it does not hold with the private type, whether it held
none or an older one, and fails if the provider still does not hold it.
Enabling dpp's validation in rs-sdk made wasm-dpp2's DataContract
constructor check document token costs, but it parsed the document types
before adding the contract's tokens, so a cost in the contract's own token
was refused ("No maximum token position limit is set"). The tokens are now
part of the parsed contract.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
… test a multibyte label The contract format reads token positions as text keys, but the constructor handed its tokens to the parser keyed by integer, so every DataContract built with tokens failed to deserialize. Positions now go in as strings. The bounds test also checks that the account label's bound is in bytes: 32 two-byte characters (64 bytes) are refused when sealing and when reading. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
On a platform version that builds data contracts of structure version 0 (protocol version 8 and earlier), the constructor dropped the tokens it was given without saying so. It now refuses them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
…equests An entry opens only for the store it was sealed for, as its commitment binds the contract and type, but `private_contact_requests_from_scan` took any scan: a caller handing it a scan of another store's page would have that store's entries read as DashPay requests. An opened entry now keeps the store it was opened for (`contract_id`, `document_type_name`), and the reader refuses a scan holding an entry of any store but DashPay's privateContactRequest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ts proof binds Drive reads the store's chunk power from the store element a page's proof binds, to settle the page, but dropped it from the result, so the SDK walk learned the chunk size from page lengths with up to two pages read one at a time first. - `PrivateDocumentEntries` (Drive's type, which the proof verifier and the SDK return) gains `chunk_power: Option<u8>`. Drive's read and its proof verification fill it; a page read without a proof leaves it `None`, as the response carries no chunk power (the server's unproved response would need a new proto field). The mock encodes it. - The walk takes a proved page's chunk size: every page after the first is fetched at once from the first's end. A page proved with another chunk size than the walk's ends the walk with an error. Without a chunk power, the walk learns the size from page lengths as before. A client-side read result: no consensus change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
…w carries A page literal in the native regression sets `chunk_power: None`, as a page read without a proof has it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 75279c6 and confirmed that all nine prior findings are fixed. One non-blocking performance suggestion remains: warm-cache page reads repeat contract serialization and synchronous browser-cache processing. This was a static review only; current checks show successful policy/title checks and a pending automated review, with no build or test results available.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The substantial new binding in packages/wasm-sdk/src/private_contact_requests.rs directly handles private encryption keys, outgoing viewing-key derivation, signing integration and secret erasure, meeting both the complexity and critical key-handling criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/wasm-sdk/src/private_contact_requests.rs`:
- [SUGGESTION] packages/wasm-sdk/src/private_contact_requests.rs:486-490: Avoid repeating contract cloning and persistence checks for every page
Both entries and contact-request scanning call this helper for every page, even when fetch_private_document_contract returns the already-held Arc<DataContract>. This unconditionally deep-clones the contract and calls cache_contract. With an attached context on mainnet or testnet, cache_contract_in_context then calls store_if_newer, which serializes the candidate, synchronously reads and parses the network's entire contract-cache JSON, and decodes the stored contract before recognizing that its version is unchanged. A warm-cache store traversal therefore repeats schema allocation and browser-storage processing on every page. Preserve the native freshness check, but track which contract generation has already been persisted so unchanged pages can skip the clone and persistence path. Cover initial persistence, upgrades, and repeated unchanged pages with a regression test.
…nce a page Reading a private document store page fetched the contract declaring the type and, every page, deep-cloned it and stored it for the next page load. With a context on mainnet or testnet, storing serializes the contract, reads and parses the network's whole stored contract map synchronously and decodes the stored one, only to find its version unchanged, so a store walk did that for every page of a warm cache. The SDK now records the version of each contract a private read last stored, and a page whose contract is at that version neither copies nor stores it; a new version is stored as before. The record is forgotten wherever the SDK removes or replaces the stored contract (an explicit removal, a stale-contract drop, a seeded contract), so the next page stores it again. The SDK's freshness check of the contract is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…current limit The reader checks a signing key's expiry against the entry's block time using the expiry the identity given carries now, not the one the key had when the entry was appended. An identity may extend a key's expiry, reviving an expired key, so an entry signed after the key's first expiry is refused against an identity read before the extension and read against one read after it. The read's documentation now says so, beside its note on historical key membership, and a test pins the expired, extended, read-again sequence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and-scan' into feat/wasm-sdk-private-contact-requests
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At head 699693f, the complete six-file diff preserves SDK ownership of contract preparation, cryptographic operations, and proof verification; all ten prior findings are fixed, and no additional actionable in-scope defects were confirmed. Validation was static only: current checks show successful policy/title checks and a pending review check, but no build or test results.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The substantial WASM implementation in packages/wasm-sdk/src/private_contact_requests.rs introduces private-key parsing, lifetime and erasure management, outgoing viewing-key derivation, and signer integration, making the diff itself intricate key-handling work despite delegating cryptographic primitives to the Rust SDK. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
…n older one added or hydrated `add_known_contract`, `add_known_contracts` and `with_known_contracts` replaced a held contract whatever its version, so an application restoring an older cached contract while a page verification ran could put back a contract without the private type the verification had resolved, and the page would then be refused. They now keep a held contract of a higher version, as `register_data_contract` does, through one rule both providers share; an equal version still replaces. An explicit replacement stays possible: `replace_known_contract` puts a contract in place of whatever is held, which the wasm SDK's `addKnownContract` seed now calls, keeping its documented replacement with an older or equal version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd's activation refusal An unpinned SDK below protocol version 15 refreshes its version before a private document send, and refuses when it is still below. The refresh swallowed its failure, so the refusal carried no cause and `is_transient_protocol_version_refusal` read it as final: a caller stopped retrying although the network may serve version 15 already, the refresh having failed only for want of an answer. The send now keeps the refresh's failure as the refusal's cause, as a failed probe is the cause of the gate's own refusal; the pin and the network's version read as before. `refresh_protocol_version` keeps its signature. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…promoted during the fetch A private page read fetches the contract declaring the type when the provider holds none that does. A fetched contract without the type was refused at once, although another operation may have given the provider a newer contract declaring it while the fetch ran; the read then failed for a contract the provider already held. The provider is now asked again first, and a newer contract declaring the type is used; otherwise the refusal, and its protocol-version form below version 15, stand. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t hold or probe `network_proven_below` answered, for an SDK that cannot ask the network (no context provider, or no gate enforced), whether any version had been verified below the one asked. A version verified earlier, perhaps by a clone, does not show what the network runs now: after an upgrade it still read as below, so a caller took a refusal as "not active yet". That branch now answers no; the network reads as proven below only while a probe's epoch hold shows it, or once a probe does. A test's `mock_verified_protocol_version` now also sets such a hold, outlasting the test, so a mock network set below a version still reads as proven there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests # Conflicts: # packages/wasm-sdk/src/sdk.rs
…gher version `WasmTrustedContext::register_data_contract` said a registered contract replaces the one the cache held. The inner provider now keeps a held contract of a higher version, which later lookups resolve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An unpinned SDK below protocol version 15 refreshed its version before a private document send, and refused when it was still below. With proofs off the refresh asks nothing, so the send was refused without asking the network, though the gate's probe asks for a proof whatever the proof setting; and the refresh ignored the send's request settings, so a stalled epoch query could wait and retry longer than the caller set. The send now asks the gate first, under the caller's request settings. Its probe raises an unpinned SDK's version, and it lets the send through only once a verified answer shows the network serves version 15 and the SDK speaks it; otherwise it refuses with the structured activation error, the probe's failure as its cause. The version check and refresh stay for an SDK that enforces no gate (a mock), and the refresh now runs under the caller's settings too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… tests The protocol gate vectors proved a network at 13 or 14, or at a version newer than any build. The private document requests need 15, so the SDK could not show a proved answer at 15 lifting a refusal. The exporter now also writes `epoch-probe-pv15.bin`, the genesis epoch proved by a protocol 15 state and signed at 15, and the vector check verifies it under the rules of 13, 14 and 15. Re-running the exporter rewrites the other vectors byte for byte. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e requests too DashPay's private contact request scan and seal resolved their contract after `require_protocol_version`, which only refreshes the SDK's version: with proofs off the refresh asks nothing, so an unpinned SDK below 15 failed with a generic error without asking the network, although the gate's probe asks for a proof whatever the proof setting. The send's order is now one helper, `Sdk::establish_protocol_requirement`: the gate first, under the caller's request settings, then, only for an SDK that enforces no gate (a mock) and is still below, a refresh under the same settings, then the structured refusal. The send and DashPay's contract resolution both use it, so the scan and seal refuse below 15 with `ProtocolVersionNotActive` rather than a generic error. `require_protocol_version` is unchanged for the features that use it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-sdk-private-contact-requests
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete six-file diff and relevant SDK, context-provider, proof-verification, and upstream implementations at head 6a9fea5. All ten prior findings are addressed, and no additional actionable in-scope defects were confirmed. This was a static review: no builds or tests were run; current checks show passing policy/title checks and the review pending, but no build or test results.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — packages/wasm-sdk/src/private_contact_requests.rs adds substantial cross-language parsing, ownership, erasure and derivation of private keys for signing, sealing and scanning requests, directly changing a key-handling surface despite delegating cryptographic primitives to Rust SDK calls. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
…t to 1 MiB A private store's chunk power is clamp(floor(log2(target / stored entry size)), 4, 11), committed into the store when it is created. At a 512 KiB target DashPay's privateContactRequest store (589-byte entries, 605 bytes stored) took chunk power 9: 512 entries, about 302 KiB a page, half the bytes of a 2048-note shielded chunk (2048 x 312 bytes, about 624 KiB). SYSTEM_LIMITS_V5.private_document_store_target_chunk_bytes is now 1 MiB: DashPay's pages hold 1024 entries (about 605 KiB), so reading its store takes half the requests, and 4096-byte entries take 128 to a chunk instead of 64. Protocol version 15 is unreleased, so the table is edited in place and no store exists under the old target. Drive's tests follow the new sizes: - the empty store, the layout and the chunk power tests expect power 10 for 601-byte stored entries and 7 for 4112-byte ones; - the paging fixture's entry count is a u16, so the paging test still covers two compacted chunks and a buffered tail at 128 to a chunk; - the changed-limits test compares 1 MiB with 2 MiB; - the fixed-fee test appends 134 entries per size instead of 70, so it still crosses a compaction at 4096-byte entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and-scan' into feat/wasm-sdk-private-contact-requests
…vate-contact-requests v5.1-dev now carries #5809 (squash a8b4b49), #5817 and the v5.0-dev merge #5818. Both conflicts were #5809 arriving twice: - packages/wasm-sdk/src/sdk.rs keeps this branch's version, which is #5809's unchanged file plus this branch's own changes; - packages/rs-sdk/src/mock/provider.rs takes v5.1-dev's version, #5809's last commit (f597cd8), which this branch had not merged yet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
… after the 14 to 15 upgrade The protocol 14 to 15 upgrade test reads the chunk power of the privateContactRequest store the upgrade creates. Under the 1 MiB chunk target (#5809, 84c2b96) 589-byte entries take power 10, 1024 to a chunk, not 9. #5809 merged with its workspace tests skipped (the runner image step failed), so the expectation was left at 9 on v5.1-dev. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit f7a1204)
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
Basic explanation
What this does: JavaScript apps can do three new things:
IdentitySignerthey sign transitions with;These are thin wasm-sdk bindings over the Rust SDK calls in #5809, exposed in evo-sdk as
sdk.privateDocuments.Value: web wallets can receive private contact requests and check who sent them, and can prepare one to send.
Risks: low. These are new bindings only, with no consensus change. One limitation: appending a sealed entry needs an Orchard proof, which the wasm build cannot make yet (the shielded prover is not in the wasm build). A web app can seal a request, but a wallet that can prove has to append it.
Issue being fixed or feature implemented
Part of anonymous DashPay contact requests for 5.1, on top of the SDK's send, scan and read calls.
What was done?
wasm-sdk (
src/private_contact_requests.rs)getPrivateDocumentEntries(dataContractId, documentTypeName, startPosition)returns a proved page:{ entries: [{ position, blockHeight, blockTimeMs, data }], totalCount }.sealPrivateContactRequest(options)takes:signer: IdentitySignerholding one of the sender's signing keys (an enabledAUTHENTICATIONkey atHIGHorCRITICAL);It returns the 589-byte entry.
scanPrivateContactRequests(options)takes a start position, the wallet's recipient identities with their keys, and its sender identities with their encryption keys ({ keyId, privateKey }). It returns one page:received: each checked against its sender's signing keys and the recipient's key;sent: recovered, and checked to be the identity's own;rejected: with a reason;nextPosition,totalCountandisComplete.The private keys read from the options are wiped once used.
TypeScript interfaces for the options and results sit in a custom section.
evo-sdk
PrivateDocumentsFacade(sdk.privateDocuments) hasentries,sealContactRequestandscanContactRequests.How Has This Been Tested?
cargo check -p wasm-sdk --target wasm32-unknown-unknownandcargo clippy -p wasm-sdk --target wasm32-unknown-unknown -- -D warnings: clean.node_modules. CI runs it.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
c5d08bb/skip-botsproceeds without the ones not yet reported/self-reviewedjs-wasm-sdk(packages/js-evo-sdk/src/private-documents/facade.ts,packages/js-evo-sdk/src/sdk.ts,packages/js-evo-sdk/tests/unit/facades/private-documents.spec.tsand 3 more) — shumkovrs-drive-abci— you own itWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.