refactor(sdk): share document transition preparation with embedders - #4433
Draft
PastaPastaPasta wants to merge 9 commits into
Draft
Conversation
PastaPastaPasta
requested review from
QuantumExplorer,
lklimek and
shumkov
as code owners
August 20, 2026 14:23
Contributor
|
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:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
✅ Final review complete — no blockers (commit 3f00ac2) |
thepastaclaw
approved these changes
Aug 20, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
At exact head ca2afbb, the helpers are a behavior-preserving relocation: the SDK retains property sanitization, entropy/document-ID validation, signing and broadcast ordering, and its existing Generic error classification through the query-error conversion. The moved tests cover matching and mismatched entropy and verify sanitization without mutating the caller's document; no in-scope defects were confirmed.
Source: reviewer backend model gpt-5.6-sol (general, security-auditor, rust-quality, and ffi-engineer lanes); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
…queries The DocumentQuery to DriveDocumentQuery lowering diverged from the server's DriveDocumentQuery::from_typed_clauses in three proof-sensitive ways: an omitted limit lowered to None, which SizedQuery treats as unbounded while the server applies config.default_query_limit, so a malicious node could attach an authenticated proof covering every matching document to a request that legitimately proves at most the default; explicit limits 101..=65535 passed through even though the server refuses them with InvalidLimit before answering; and limits above u16::MAX were silently truncated by an as cast (65537 became 1). Lower the sentinel to the compile-time default and refuse over-cap limits with the server's own InvalidLimit instead of truncating or passing through. Callers of the conversion see Some(100) where they previously saw None for an omitted limit, and Error::Drive(QuerySyntaxError::InvalidLimit) instead of silent acceptance for over-cap ones. No in-tree caller depends on the old shapes; the pre-existing FromProof-for-Documents verification path inherits the fix.
…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.
PastaPastaPasta
force-pushed
the
refactor/document-query-decode-builders
branch
from
August 20, 2026 15:38
5136419 to
c0ccb87
Compare
PastaPastaPasta
force-pushed
the
refactor/shared-document-transition-prep
branch
from
August 20, 2026 15:39
ca2afbb to
acb258b
Compare
…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 now binds the proof to the whole wire request, not just its SELECT projection. GroveDB and Tenderdash proofs authenticate the state and the resolved DriveDocumentQuery, and the DocumentQuery -> DriveDocumentQuery lowering drops group_by, having, offset and prove - so an untrusted transport could otherwise pair a request the real server would have refused (SELECT DOCUMENTS ... GROUP BY age) with a genuine proof for the narrower query it lowers to, and verification would accept it. Every dropped field is now rejected up front, mirroring rs-drive-abci's validate_and_route (non-empty HAVING for a non-aggregate SELECT, GROUP BY under SELECT DOCUMENTS) and reject_offset_off_the_ranked_path (OFFSET off the ranked surface); prove=false is rejected because an honest server answers such a request without a proof at all. The pre-existing aggregate- projection rejection (COUNT/SUM/AVG, which use a different proof shape) moves into the same gate. Five tests cover the rejections with a ContextProvider that panics if reached, pinning that they fire before any proof machinery runs. - TryFrom<&DocumentQuery> for DriveDocumentQuery converts the limit with a checked u16::try_from instead of an `as` cast. Drive's limit is a u16 and the server refuses anything larger with InvalidLimit, so the cast turned a request for 65537 documents into a 1-document query - and a proof for that query then verified. This matches the offset conversion right below it, which already refused rather than truncated. - try_from_request documents that it mirrors the server's wire-shape decode, not validate_and_route business rules, and points at the entry point that closes the gap. - 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>
…ions The verifier dispatched on the request oneof without checking whether that wire version is enabled by platform_version.drive_abci.query.document_query. The server performs this check before decoding or executing the request, so an untrusted transport could attach a valid proof to a request an honest server at the supplied PlatformVersion could not have answered (PlatformVersions 1-11 have bounds 0..=0 and reject every V1 request with UnsupportedQueryVersion). Derive the request feature version from the oneof arm and reject it against the supplied bounds before anything is decoded, in both verify_documents_response entry points — the same check_version gate the server's query_documents dispatch runs.
The transport-free preorder/domain builder takes the salt as an argument, so the front-running protection the preorder commitment provides now rests on the embedder: a fresh CSPRNG 32-byte salt per registration attempt, and salt/label/domain-document secrecy until the preorder create transition is confirmed. Spell out both obligations - and that the networked SDK's register_dpns_name (StdRng::from_entropy, submit-and-wait before broadcasting the domain document) is the reference behavior - on the builder's docs.
The extraction from drive-abci widened where_operator_from_proto, value_from_proto, where_clause_from_proto, order_clause_from_proto and having_clause_from_proto to pub, but the cross-crate consumers (drive-abci's v1 conversions, the SDK decode path) only use DecodeError, the plural request-level decoders and select_from_proto. Narrow the singular helpers to pub(crate) so the shared decode surface stays as small as its actual contract.
PastaPastaPasta
force-pushed
the
refactor/document-query-decode-builders
branch
from
August 20, 2026 15:48
c0ccb87 to
b3e9de1
Compare
Extracts the document create/replace preparation out of dash-sdk's PutDocument broadcast path into dash-platform-queries: property sanitization for the transition (prepare_document_for_transition) and the entropy/document-id consistency check (ensure_entropy_matches_document_id) that surfaces an id/entropy drift locally instead of after the broadcast has paid a bumped identity-contract nonce. dash-sdk delegates to the shared helpers with unchanged behavior; transport-free embedders (packages/rs-platform-cxx) assemble their own transitions through the same code instead of reimplementing it in C++. Split out of #4389 to keep that PR to its declared decode/builders/verification scope.
PastaPastaPasta
force-pushed
the
refactor/shared-document-transition-prep
branch
from
August 20, 2026 15:49
acb258b to
3f00ac2
Compare
thepastaclaw
approved these changes
Aug 20, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The helper extraction preserves the SDK's existing property sanitization, entropy/document-ID validation, call ordering, and Generic error classification while exposing the logic through the transport-free query crate. The relocated tests cover matching and mismatched entropy as well as non-mutating property normalization; no in-scope defects were confirmed.
Source: reviewer backend model gpt-5.6-sol (general and rust-quality lanes); final verifier backend model gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol was orchestration-only and is not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
PastaPastaPasta
marked this pull request as draft
August 21, 2026 17:46
QuantumExplorer
force-pushed
the
refactor/document-query-decode-builders
branch
from
August 21, 2026 18:02
b3e9de1 to
6638c91
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Transport-free embedders (the Dash Core Platform GUI via
packages/rs-platform-cxx, #4416) need the same document create/replace preparationdash-sdk'sPutDocumentbroadcast path performs — property sanitization and the entropy/document-id consistency check — or they reimplement it in C++ and drift.Split out of #4389 on scope grounds: it is neither the wire-request decode nor the DPNS/DashPay builders that PR declares. Stacked on #4389 (base
refactor/document-query-decode-builders); will be retargeted tov4.2-devwhen it merges.What was done?
dash-platform-queries::transition::put_documentgainsprepare_document_for_transition(property sanitization for the transition, e.g. integer arrays coerced back into byte arrays after a WASM boundary crossing) andensure_entropy_matches_document_id(Drive recomputes the id from the entropy duringadvanced_structurevalidation and rejects a mismatch — this surfaces the drift locally, before the broadcast has paid a bumped identity-contract nonce).dash-sdk'sPutDocumentpath delegates to the shared helpers; behavior unchanged.How Has This Been Tested?
cargo test -p dash-platform-queriesand a fulldash-sdkbuild on the stacked branch. The helpers are exercised end to end byrs-platform-cxx's transition tests in #4416.Breaking Changes
None.
Checklist: