refactor(sdk): shared wire-request decode and pure DPNS/DashPay document builders - #4389
Conversation
…d client code The proto-to-domain decoding for document queries existed only server-side (rs-drive-abci's v1 conversions), so a client verifying a documents proof had to reconstruct the query shape by hand and could silently drift from what the server actually proves. Move the decode logic into dash-platform-queries::documents::proto_conversions with a neutral error type; drive-abci's conversions module becomes a thin mapping onto its QueryError surface with identical error message strings. On top of the shared decoder, DocumentQuery::try_from_request(request, contract) reconstructs the rich query from the wire request (both request versions), and verify_documents_response(...) / verify_documents_response_with_provider_contract(...) give embedders a request-driven verification entry point that delegates to the existing FromProof machinery, resolving the contract explicitly or via ContextProvider::get_data_contract. Round-trip tests cover encode-decode equality for representative queries in both wire versions plus malformed-clause rejection; drive-abci's document_query unit tests pass unchanged (76 cases).
Move the document *content* assembly of rs-sdk's networked DPNS and DashPay flows into transport-free functions in dash-platform-queries, so offline/embedder consumers and rs-sdk share one implementation: - build_dpns_preorder_and_domain_documents assembles the preorder and domain documents exactly as register_dpns_name did: both ids from the same entropy via generate_document_id_v0, saltedDomainHash = sha256d(salt || normalized_label + ".dash"), and the full domain property map (parentDomainName/normalizedParentDomainName, label, normalizedLabel, preorderSalt, records.identity, subdomainRules.allowSubdomains=false). It additionally rejects labels failing is_valid_username up front - previously only enforced by rs-sdk-ffi and platform consensus - so register_dpns_name now fails locally on an invalid label instead of after a network round-trip. - build_contact_request_document assembles the DIP-15 contactRequest id and property map from already-derived crypto material (encrypted xpub/label bytes, key indices, entropy). ECDH, encryption, the 69-byte compact-xpub check, key purpose checks, and recipient fetching stay in rs-sdk; the ciphertext size validations (96-byte xpub, 48-80-byte label, 38-102-byte autoAcceptProof) moved into the builder, with validate_auto_accept_proof also called early in create_contact_request to keep the pre-fetch fail-fast. - ensure_entropy_matches_document_id and prepare_document_for_transition moved from put_document.rs into dash_platform_queries::transition::put_document; rs-sdk re-exports and keeps calling them. Entropy/salt generation and all networking remain in rs-sdk. Builder validation errors surface through the new dash_platform_queries::Error::InvalidInput variant, which rs-sdk maps back to Error::Generic with the exact pre-move messages. New unit tests in dash-platform-queries pin a DPNS known vector (document ids and property maps for fixed label/entropy/salt), mirror the entropy-derives-id relation for contact requests, and cover the negative validation paths.
…er seams Review follow-ups on the transport-free series: - The DPNS document builder validated labels with is_valid_username, whose consecutive-hyphen rejection is stricter than the DPNS contract's schema pattern - consensus accepts names like ab--cd. Split the check: new is_consensus_valid_label matches the contract pattern exactly and gates the builder (so dash-sdk's register_dpns_name no longer refuses consensus-valid labels), while is_valid_username keeps the stricter policy for its existing FFI/wasm gates and now documents the difference. - verify_documents_response rejects aggregate projections (COUNT/SUM/AVG) up front with a pointer to the aggregate proof helpers, instead of surfacing an opaque low-level proof error; try_from_request documents that it mirrors the server's wire-shape decode, not validate_and_route business rules. - The CI transport-leak guard also asserts wasm-sdk's wasm32 tree stays free of the native transport stack. - The proof-vector corpus gains a README with an explicit coverage matrix: the four documents-family cases pin query shape and clean decode failure but stop before the BLS check (placeholder payloads in the fixture state); identity, contested, and quorum-sig families run the full pipeline. This corrects the corpus commit's broader claim.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
⛔ Blockers found — Opus deferred (commit 2a6dbe3) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The shared decoders and extracted builders largely preserve the existing behavior, but the new request-driven document verifier does not bind verification to every semantically relevant request field. A malicious transport can therefore substitute a valid proof for a different query, so this trust-boundary issue must be fixed before merging.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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/dash-platform-queries/src/documents/document_query.rs`:
- [BLOCKING] packages/dash-platform-queries/src/documents/document_query.rs:665-678: Reject request fields that are discarded before proof verification
Validating only the `select` projection does not ensure that the proof corresponds to the wire request. The subsequent `TryFrom<&DocumentQuery> for DriveDocumentQuery` conversion discards `group_by` and `having`, even though the server rejects both for `SELECT DOCUMENTS`; consequently, an untrusted transport can pair a request such as `SELECT DOCUMENTS GROUP BY age` with a valid proof for the corresponding plain document query and this function will accept it. The conversion also narrows `request.limit` with `as u16` at lines 1127-1129, so a wire limit of 65537 becomes 1 even though the server rejects limits above `u16::MAX`. Plain-document `offset` and a false `prove` flag are additional request shapes that cannot produce this proved response from the real server but are not rejected here. GroveDB and Tenderdash proofs authenticate the state and resolved Drive query, not the discarded request envelope. Before delegating, reject every field incompatible with a proved plain-document request (`group_by`, `having`, `offset`, and `prove == false`) and use a checked `u16::try_from` conversion for the limit so no request information is silently changed.
| // This entry point verifies plain document fetches only. An aggregate | ||
| // projection (COUNT/SUM/AVG) is proved with a different proof shape; | ||
| // handing it to the Documents verifier would surface as an opaque | ||
| // low-level proof error, so reject it up front instead. | ||
| if query.select != drive::query::SelectProjection::documents() { | ||
| return Err(drive_proof_verifier::Error::RequestError { | ||
| error: format!( | ||
| "verify_documents_response only verifies plain document fetches; the request \ | ||
| carries a {:?} projection — use the aggregate proof helpers instead", | ||
| query.select.function | ||
| ), | ||
| }); | ||
| } | ||
| <Documents as FromProof<DocumentQuery>>::maybe_from_proof_with_metadata( |
There was a problem hiding this comment.
🔴 Blocking: Reject request fields that are discarded before proof verification
Validating only the select projection does not ensure that the proof corresponds to the wire request. The subsequent TryFrom<&DocumentQuery> for DriveDocumentQuery conversion discards group_by and having, even though the server rejects both for SELECT DOCUMENTS; consequently, an untrusted transport can pair a request such as SELECT DOCUMENTS GROUP BY age with a valid proof for the corresponding plain document query and this function will accept it. The conversion also narrows request.limit with as u16 at lines 1127-1129, so a wire limit of 65537 becomes 1 even though the server rejects limits above u16::MAX. Plain-document offset and a false prove flag are additional request shapes that cannot produce this proved response from the real server but are not rejected here. GroveDB and Tenderdash proofs authenticate the state and resolved Drive query, not the discarded request envelope. Before delegating, reject every field incompatible with a proved plain-document request (group_by, having, offset, and prove == false) and use a checked u16::try_from conversion for the limit so no request information is silently changed.
source: ['codex']
…-builders base Move all seven dashpay/platform git dependencies from the old feat/transport-free-embedder-core pin (e8e1961fe54f) to rev 2a6dbe39065104981b7f9bb4fbee598aab869fe4, the head of refactor/document-query-decode-builders (PR dashpay/platform#4389) whose content is the rebased equivalent on the current v4.2-dev base. No FFI-visible API drift: the crate builds unchanged and all 31 rust/platform tests pass against the new revision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
Third slice of the
feat/transport-free-embedder-coreseries (#4335; after #4344, #4345, and #4388): gives transport-free embedders the remaining pieces they need to construct and verify Platform interactions without reimplementing SDK logic — the drift-prone code Dash Core's Platform GUI (PastaPastaPasta/dash#67, dashpay/dash#7512) currently hand-builds in C++.Stacked on #4388 (base branch
refactor/dash-platform-queries); will be retargeted tov4.2-devwhen that merges. Only the last four commits are new.What was done?
DocumentQuery::try_from_requestdecodes a wire-formatGetDocumentsRequestback into a richDocumentQuery— the inverse of request encoding — by liftingdrive-abci's server-side proto conversions into shared client code, so the bytes the server decodes and the client verifies go through the same code.drive-abcinow consumes the shared conversions (deduplicated, −382 lines in itsconversions.rs). Round-trip coverage intests/document_query_wire_roundtrip.rs.documents::verify_documents_responsedelegating todrive-proof-verifier'sFromProof, keyed on the exact request bytes sent.build_dpns_preorder_and_domain_documents(salted-domain-hash preorder/domain pair) anddashpay::build_contact_request_document, extracted from rs-sdk's networked flows. Crypto material is supplied by the caller — ECDH/key custody stays out of this crate.is_consensus_valid_label(matches the DPNS contract regex; gates the builders) fromis_valid_username(stricter client-side policy, e.g. consecutive-hyphen rejection) so builders cannot reject labels the contract accepts.rs-sdkre-exports everything at its old paths; no consumer changes imports.How Has This Been Tested?
cargo test -p dash-platform-queries(36 unit + 9 integration tests, including wire round-trip and builder/validation coverage);cargo checkfordash-sdkanddrive-abci;cargo fmt --check; clippy clean.Breaking Changes
None. Moved items remain importable at their previous
dash_sdkpaths;drive-abci's request decoding behavior is unchanged (same conversions, now shared).