refactor(drive-abci): share the v1 document-query wire decoders - #4618
refactor(drive-abci): share the v1 document-query wire decoders#4618PastaPastaPasta wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds the ChangesPlatform query wire integration
Priority: ➖ Normal — Schedule the shared query-decoder change because it alters the Drive ABCI validation boundary and adds bounded deserialization across the workspace. Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change centralizes v1 query wire decoding, preserves query error behavior, and bounds deserialization allocations. No concrete current-head merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant QueryWire as platform_query_wire
participant DriveABCI as rs-drive-abci
participant Validation as QueryValidationResult
QueryWire->>DriveABCI: Decode query clauses
QueryWire-->>DriveABCI: Return DecodeError
DriveABCI->>Validation: Convert and wrap error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PastaPastaPasta
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4618 +/- ##
============================================
- Coverage 87.72% 87.30% -0.42%
============================================
Files 2762 2795 +33
Lines 360146 364721 +4575
============================================
+ Hits 315922 318433 +2511
- Misses 44224 46288 +2064
🚀 New features to boost your workflow:
|
PastaPastaPasta
added
the
ready for final review
…atform-query-wire
Pure move: the wire-proto -> drive-type decoders for the v1 getDocuments surface now live in a new packages/rs-platform-query-wire micro-crate (crate name platform-query-wire) behind a neutral DecodeError { InvalidArgument, Unsupported }, and drive-abci's v1/conversions.rs becomes a thin adapter mapping DecodeError onto its QueryError surface with the exact same message strings (including the aggregate ORDER BY arm, which maps Unsupported onto QuerySyntaxError::Unsupported). No behavior change to server request decoding.
The decode of a wire request into a rich query is an equivalence contract at a trust boundary: a client-side proof verifier must interpret a request exactly as the server does, or a proof could verify against a different query than the server answered. Hosting the shared decoders in a neutral crate lets both rs-drive-abci and (in a follow-up) the client-side SDK decoders run the same functions, without making the consensus server depend on SDK-branded code: the new crate's dependencies (dapi-grpc platform+client without transport, dpp, drive/verify, thiserror) are a strict subset of what drive-abci already carries.
|
✅ Final review complete — no blockers (commit 3da51c3) · triage: critical |
ffb791d to
3da51c3
Compare
PastaPastaPasta
changed the title
PastaPastaPasta
removed
the
ready for final review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 3da51c3. The decoder extraction preserves behavior and server error mapping; no blocking defects were confirmed. One documentation nit remains: references in neighboring crates still point to the deleted decoder module.
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (agent: phase1-reviewer, role: rust-quality); reviewer 3: glm-5.3-flash (agent: phase1-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — The extraction changes the dependency and error-conversion boundaries of remotely supplied document-query decoding in drive-abci, where a regression could alter server query semantics, proof-verification consistency, or consensus-binary feature composition. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort max); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort max); agentphase1-reviewer,glm-5.3-flash— security-auditor (completed, effort max); agentphase1-reviewer - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (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
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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-drive/src/query/having.rs`:
- [NITPICK] packages/rs-drive/src/query/having.rs:31: Update cross-references to the relocated wire decoders
This refactor deletes `rs-drive-abci/src/query/document_query/v1/conversions.rs`, but this comment and `packages/rs-drive/src/query/projection.rs:16` still identify it as the wire-decoding layer. The encoding documentation in `packages/dash-platform-queries/src/documents/document_query.rs:1317,1331` likewise references the deleted file's value decoders. Update these four references to `rs-platform-query-wire/src/proto_conversions.rs` so readers can locate the shared decoding implementation and its nesting-limit contract. These are documentation-only changes and require no new dependency.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- New crate's default-feature clippy (-D warnings) fails on pre-existing unused import in rs-drive — Out of scope and tooling-detectable. The parent commit already contains the unconditional DocumentPropertyType import, its use inside a server-gated function, and a verify-only dependency from drive-proof-verifier. This PR neither introduces nor worsens that feature-cut warning. Its stated clippy command explicitly uses --all-features; it does not claim standalone default-feature clippy passes.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Disposition (not closing yet): QuantumExplorer's suggestion to use the SDK was prototyped and passes Core's requirements, so the Core GUI now consumes 🤖 Posted autonomously by Claude on behalf of pasta. |
2 participants
Issue being fixed or feature implemented
First of three PRs replacing the transport-free-embedder series (#4464, #4478, #4389, #4433, #4416), rebuilt after a review of that stack found it duplicating code the workspace already has and carrying two remotely triggerable aborts in the C++ bridge. This PR is the server-side half; the SDK verification/builders and the C++ crate follow in stacked PRs. The dpp decode-bounds fix that was originally stacked here is now #4629, so it can land independently.
The v1
getDocumentswire decode becomes a shared crate. A client that verifies a proof against the request it actually sent must reconstruct the query exactly as the server did. Today that decode lives only insiders-drive-abci, so the earlier client-side PR carried a byte-for-byte copy with a "keep in lockstep" comment. Supersedes #4464, re-derived from the current server file (which had grown the time-range decoders since #4464 was cut).What was done?
packages/rs-platform-query-wire(platform-query-wire): the wire-proto → drive-type decoders forgetDocumentsv1 (where / order-by / having / select / time-range clauses), moved verbatim fromrs-drive-abci/src/query/document_query/v1/conversions.rs, with a neutralDecodeError { InvalidArgument, Unsupported }.drivefeatures of its own beyond averifydefault and aserverpassthrough.rs-drive-abciopts out of defaults and takesserver, so the consensus binary gains no proof-verification code from this crate (cargo tree -p drive-abci -e normal,features -i driveshows noverifyedge, same as before). Client verifiers take the default.rs-drive-abcimapsDecodeErrorontoQueryErrorthrough aFromimpl inerror/query.rs, matching the six#[from]conversions already there. The v1 handler, routing and dispatch code call the shared decoders directly; the former adapter module is deleted. A unit test pins the variant mapping and message preservation, so a later reclassification in the shared crate cannot silently change what clients see.COPY --parents, package filters (triggering ondapi-grpcchanges as well asdrive), the transport-free CI cut (cargo check -p platform-query-wire --lockedand the banned-crate loop), and the nextest package allowlist gain the new crate.How Has This Been Tested?
cargo test -p drive-abci --lib -- query::document_query::v1 error::query: 106 passed (the server's own decode tests now exercise the shared crate directly).cargo check -p platform-query-wire --locked(defaultverify) and--no-default-features --features server: both clean.cargo clippy -p drive-abci -p platform-query-wire --all-targets --all-features -- -D warnings,cargo fmt --check,cargo machete: clean.Breaking Changes
None. Server error strings are unchanged.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
getDocumentsquery clauses, including WHERE, ORDER BY, HAVING, and SELECT.Bug Fixes
Documentation