feat(platform): getDocuments v1 — SQL-shaped select + count surface (1/3)#3633
Open
QuantumExplorer wants to merge 35 commits into
Open
feat(platform): getDocuments v1 — SQL-shaped select + count surface (1/3)#3633QuantumExplorer wants to merge 35 commits into
QuantumExplorer wants to merge 35 commits into
Conversation
…nt surface
PR 1 of 3 in the v1 unification track. Adds the wire format +
server dispatcher that unifies \`getDocuments\` and
\`getDocumentsCount\` under a single SQL-shaped request type with
\`select\`, \`group_by\`, and \`having\` clauses. **Pure rewiring** —
no new server-side capability ships here; every supported request
shape translates to an existing v0 (\`query_documents_v0\`) or
v0-count (\`query_documents_count_v0\`) handler invocation and
produces byte-identical proof bytes / response data. The v1
surface just makes the SQL semantics explicit on the wire.
**Wire format** (\`platform.proto\`):
- \`GetDocumentsRequestV1\` joins V0 in the existing oneof.
- \`Select { DOCUMENTS, COUNT }\` projection enum.
- \`repeated string group_by\` for explicit grouping.
- \`bytes having\` reserved for future capability.
- \`GetDocumentsResponseV1\` with \`oneof result\` carrying
\`Documents\`, \`CountResults\` (referenced from
\`GetDocumentsCountResponse\`), or \`Proof\`.
**Dispatcher rejection table** — every request shape outside the
v0 / v0-count capability surface returns
\`QuerySyntaxError::Unsupported("… is not yet implemented")\`:
- HAVING with any payload (always rejected, no exceptions).
- GROUP BY with \`SELECT DOCUMENTS\` (no aggregate to group on).
- GROUP BY on a field that's not constrained by an \`In\` or range
where clause (would need a new server primitive — walking a
property-name \`ProvableCountTree\` without a covering prefix).
- GROUP BY with more than two fields (Phase 1 cap).
- Two-field GROUP BY outside the existing \`(In, range)\` compound
shape (the server emits \`(in_key, key)\` entries in that order;
other orderings would need a new merk walk).
- \`start_after\` / \`start_at\` with \`SELECT COUNT\` (no concept
of "skip past this aggregate" — paginate by narrowing the range).
The wording "… is not yet implemented" is deliberate: it signals
future capability, not malformed requests. Clients can keep
these request shapes in code and they'll start working once the
capability lands without a wire-format change.
**Supported routing**:
- \`SELECT DOCUMENTS, group_by=[]\` → v0 documents handler.
- \`SELECT COUNT, group_by=[]\` → v0-count, aggregate; for
\`In + no range + no prove\` mode, v1 sums the PerInValue entries
server-side into a single aggregate.
- \`SELECT COUNT, group_by=[f]\` where f matches the In or range
field → v0-count's PerInValue / RangeDistinct (entries).
- \`SELECT COUNT, group_by=[in_field, range_field]\` → v0-count's
existing compound \`(In + range + distinct)\` shape.
**Platform-version**: \`document_query.max_version\` bumped to 1
(default still 0; v1 callers opt in via the request's \`version\`
oneof). v0 callers continue working unchanged.
**Tests** (12 in \`query::document_query::v1::tests\`):
- 7 rejection-table unit tests covering every \`Unsupported\` arm.
- 4 routing-decision unit tests pinning each supported shape.
- 2 end-to-end parity tests: \`SELECT DOCUMENTS\` returns the
same docs as v0, and HAVING rejection surfaces cleanly in
the response (\`Unsupported("HAVING clause is not yet
implemented")\`).
Existing v0 (27 tests) and v0-count (9 tests) suites unaffected.
**Out of scope** (follow-up PRs):
- PR 2: SDK \`DocumentQuery\` builder + \`Fetch\` wiring for v1.
- PR 3: FFI / wasm / Swift unified entry points.
- Phase 2: explicit GROUP BY on non-where-constrained fields,
multi-field GROUP BY, HAVING.
**Non-Rust client regeneration**: this commit doesn't regenerate
java / nodejs / python / objc / web clients — the docker-based
\`scripts/build.sh\` produces those. Will land as a separate
commit once the proto is reviewer-approved.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes the standalone getDocumentsCount RPC and migrates counting into GetDocuments v1. It adds GetDocumentsRequestV1/GetDocumentsResponseV1 (select/group_by/having/start/limit/prove + result oneof counts/documents/proof), updates generated clients and service descriptors, deletes legacy count modules/handlers, adds a v1 document-query handler that routes documents vs counts, and updates SDK/FFI/WASM and proof verification to the unified DocumentQuery/Select flow. ChangesUnified GetDocuments v1 (single cohort)
sequenceDiagram
autonumber
participant Client as Client (SDK/JS)
participant Server as Platform gRPC Server
participant Drive as Drive executor
participant Verifier as Proof Verifier
Client->>Server: Send GetDocumentsRequestV1 (select=DOCUMENTS|COUNT, group_by?, having?, start/limit, prove?)
alt select = DOCUMENTS
Server->>Drive: Call documents executor (v0 path) / translate start/limit
Drive-->>Server: Documents or proof
else select = COUNT
Server->>Drive: Build DocumentCountRequest / execute count path
Drive-->>Server: Counts or proof (per-key or aggregate)
end
alt proof present
Server->>Verifier: Verify GetDocumentsResponse (proof)
Verifier-->>Server: Verified result
end
Server-->>Client: Return GetDocumentsResponseV1 (documents | counts | proof + metadata)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Runs the docker-based `scripts/build.sh` codegen to produce the java / nodejs / python / objc / web client bindings for the v1 wire format introduced in the preceding commit (`feat(platform): GetDocumentsRequestV1 …`). Regenerated files: - `packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h` - `packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m` - `packages/dapi-grpc/clients/platform/v0/python/platform_pb2.py` - `packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts` - `packages/dapi-grpc/clients/platform/v0/web/platform_pb.js` - `packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js` - `packages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.js` - `packages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.js` The last one (drive's nodejs bindings) regenerates because the drive proto imports platform message types via the shared protos path; any platform proto change ripples through. The Rust client is generated by `build.rs` at compile time and needs no checked-in update. The Java client doesn't regenerate here because no fixtures or in-repo tests exercise it; downstream consumers re-run their own codegen against the proto.
Two consolidation changes on top of the prior v1 commit. **Response shape**: `GetDocumentsResponseV1` now mirrors every other `Get*Response` in the proto — a two-variant outer `oneof` with `result.data` at position 1 and `proof` at position 2. The non-proof result wraps an inner `oneof` (`ResultData.variant`) that switches between `Documents` (for `select=DOCUMENTS`) and `CountResults` (for `select=COUNT`). Keeps the canonical result-or-proof shape callers expect without flattening to a three-variant outer oneof. **`getDocumentsCount` endpoint removed entirely**: - `rpc getDocumentsCount` deleted from the proto service. - `GetDocumentsCountRequest` / `GetDocumentsCountResponse` messages deleted from the proto. - `CountResults` / `CountEntry` / `CountEntries` types moved into `GetDocumentsResponseV1` (the new and only home for the count wire shape). - `query_documents_count` + `query_documents_count_v0` handlers deleted from drive-abci; v1 dispatches to drive's `execute_document_count_request` directly without delegating through the now-gone v0-count layer. - `query/document_count_query/` directory removed from drive-abci. - `document_count_query` + `document_split_count_query` fields removed from `DriveAbciQueryVersions` (rs-platform-version). - `get_documents_count` trait method removed from rs-dapi / rs-dapi-client / rs-drive-abci's query service impl. - dapi-grpc `build.rs`: count messages dropped from `VERSIONED_REQUESTS` / `VERSIONED_RESPONSES`. - rs-sdk: `DocumentCountQuery` still presents the same surface but now builds a `GetDocumentsRequest::V1` with the right `select=COUNT` + computed `group_by` based on where-clause shape (preserves v0-count's implicit grouping behavior at the SDK seam). FromProof impls updated to consume `GetDocumentsResponse`. - rs-drive-proof-verifier: response type aliases updated. - rs-sdk mock harness: `GetDocumentsCountRequest` mock-loader arm removed; existing dumps for that request type need to be re-recorded against `GetDocumentsRequest` v1. The `getDocumentsCount` endpoint shipped briefly in #3623 and never had stable callers, so this consolidation is a clean pre-release removal rather than a deprecation cycle. v0 of the documents endpoint stays alive unchanged. Tests: - drive-abci `document_query` (v0 + v1): 38/38 (1 ignored). - SDK `fetch::document_count`: 6/6 (the count fetch path now exercises the v1 wire bytes end-to-end through the mock transport). - Workspace compiles cleanly across all touched crates. **Non-Rust client regen** lands in a follow-up commit (user runs the docker-based `scripts/build.sh`).
When `document_count_query/v0/mod.rs` was deleted in the prior
commit, its 9-test integration suite went with it — a real
coverage gap, since the v1 dispatcher inherits the entire count
execution surface those tests pinned (Total, PerInValue,
RangeNoProof summed + distinct, PointLookupProof, RangeProof,
RangeDistinctProof, plus the no-covering-index rejection paths).
Mechanical 1:1 port into a new `ported_v0_count_tests` submodule:
- Request type: `GetDocumentsCountRequestV0 { … }` →
`GetDocumentsRequestV1 { select: COUNT, group_by: <derived>, … }`
via a `count_v1_request` helper.
- `return_distinct_counts_in_range: true/false` → explicit
`group_by` field (empty for aggregate, `[in_field]` for In,
`[range_field]` for distinct range, `[in_field, range_field]`
for compound — matching the SDK's `compute_group_by` shape).
- Response pattern: `GetDocumentsCountResponseV0`'s
`Counts(CountResults { … })` envelope → v1's nested
`Data(ResultData { variant: Counts(CountResults { … }) })`.
Two helpers (`unwrap_aggregate` / `unwrap_entries`) keep the
per-test assertions focused.
- `setup_platform`, `store_data_contract`, `store_document`,
`family-contract-countable.json` fixture, and the
`store_person_document` / `serialize_where_clauses_to_cbor`
helpers all carry over verbatim.
The 9 ported tests + the existing 12 v1 unit/e2e tests now form
the complete v1 test surface — 21 tests total in the v1 module.
Combined with v0's 27 unchanged tests, `query::document_query`
has 47 passing tests (1 ignored, pre-existing).
No execution-path differences vs. the deleted v0-count tests —
the v1 handler dispatches into the same drive executor (`Drive::
execute_document_count_request`) those tests originally
exercised; only the wire shape on each end changed.
v1 is the canonical surface — it's a superset of v0 (matched documents via SELECT DOCUMENTS) plus the count surface that replaces the removed `getDocumentsCount` endpoint. Bump `default_current_version` from 0 to 1 to signal that new code should target v1; v0 remains accepted (`max_version: 1`, `min_version: 0`) so existing v0 callers continue working until they re-pin their request version. `default_current_version` is metadata only — not consumed by the dispatcher's version-check logic, which gates exclusively on `min_version` / `max_version`. Callers (handlers, SDKs) inspect it to choose which request shape to build when they have no other guidance.
PR 2 of the v1 GetDocuments migration: collapse the SDK-side DocumentCountQuery wrapper into DocumentQuery itself, exposing the count surface via builders (.with_select(Select::Count), .with_group_by(...), .with_having(...)) rather than a separate type. - DocumentQuery gains v1 fields (select, group_by, having) and builders; TryFrom switches to GetDocumentsRequest::V1. The u32-with-0-sentinel limit translates to Option<u32> at the wire boundary; V0::Start translates to V1::Start. - New document_count.rs hosts FromProof<DocumentQuery> + Fetch for DocumentCount and DocumentSplitCounts; validates select == Count at the SDK boundary so callers who forget .with_select(Count) fail loudly rather than via an opaque verifier error. - document_count_query.rs deleted (-825 LoC). - FFI (rs-sdk-ffi) and wasm-sdk count shims keep the legacy `return_distinct_counts_in_range: bool` parameter on their public C ABI / JS surface; they translate to v1 group_by internally via mirrored derive_group_by helpers, preserving binary back-compat for existing iOS and browser callers. - 9 in-tree DocumentQuery struct-literal callsites patched with the 3 new default fields (Documents, vec![], vec![]). - 6 SDK fetch tests rewritten against the unified surface; expect_fetch carries explicit turbofish since DocumentQuery is now the Request type for 3 separate Fetch impls (Document, DocumentCount, DocumentSplitCounts). All 47 drive-abci document_query tests still pass; all 6 rewritten SDK fetch tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/wasm-sdk/src/dpns.rs (1)
282-284: 💤 Low valueOptional: alias the long enum path.
The fully-qualified
dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v1::Selectpath is repeated across SDK files. Consider adding auseimport at the top of this file (or re-exporting it fromdash_sdk::platform) to improve readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/wasm-sdk/src/dpns.rs` around lines 282 - 284, The long fully-qualified enum path dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v1::Select is repeated and harms readability; add a local alias (e.g. use dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v1::Select;) at the top of packages/wasm-sdk/src/dpns.rs (or re-export it from dash_sdk::platform) and then replace occurrences like select: dash_sdk::dapi_grpc::...::Select::Documents with select: Select::Documents to simplify the code.packages/rs-sdk/src/platform/documents/document_count.rs (1)
102-291: 💤 Low valueOptional: extract shared dispatch helpers.
DocumentCount::maybe_from_proof_with_metadataandDocumentSplitCounts::maybe_from_proof_with_metadatashare substantial structure (range-countable index lookup +DriveDocumentCountQueryconstruction,documents_countablefast path, non-range countable index lookup, proof/metadata extraction). Consider extracting helpers likebuild_range_count_query(&request) -> Result<(DriveDocumentCountQuery, ...), _>andbuild_point_count_query(&request) -> Result<DriveDocumentCountQuery, _>to keep the two impls aligned as the surface evolves.Also applies to: 320-523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk/src/platform/documents/document_count.rs` around lines 102 - 291, The two big impls (DocumentCount::maybe_from_proof_with_metadata and DocumentSplitCounts::maybe_from_proof_with_metadata) duplicate logic for range-vs-point dispatch, DriveDocumentCountQuery construction, index lookup, documents_countable fast-path and proof/metadata extraction; extract shared helpers such as build_range_count_query(request: &DocumentQuery) -> Result<(DriveDocumentCountQuery, index, document_type), drive_proof_verifier::Error> and build_point_count_query(request: &DocumentQuery) -> Result<DriveDocumentCountQuery, drive_proof_verifier::Error>, plus a helper extract_proof_and_metadata(response: &GetDocumentsResponse) -> Result<(Proof, ResponseMetadata), ...>, then replace the duplicated blocks in both impls to call these helpers (preserving existing error messages and all uses of DriveDocumentCountQuery, verify_distinct_count_proof, verify_aggregate_count_proof, verify_primary_key_count_tree_proof, verify_point_lookup_count_proof) so behavior stays identical while keeping the two impls aligned as the API evolves.packages/rs-platform-wallet/src/wallet/identity/network/profile.rs (1)
156-171: 💤 Low valueOptional: extract a helper for the duplicated profile-query construction.
fetch_profile_documentandupdate_profile_with_external_signerbuild essentially the sameDocumentQuery(profiledoctype,$ownerId == identity_id,limit: 1). Extracting abuild_profile_query(contract, identity_id) -> DocumentQueryhelper would keep the V1 fields in sync as the surface evolves.Also applies to: 430-444
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/profile.rs` around lines 156 - 171, Both fetch_profile_document and update_profile_with_external_signer duplicate building the same DocumentQuery for the "profile" doctype ($ownerId == identity_id, limit: 1); extract a helper function like build_profile_query(dashpay_contract: Arc<...>, identity_id: <type>) -> dash_sdk::platform::DocumentQuery that constructs and returns the DocumentQuery (set data_contract, document_type_name="profile", where_clauses with field "$ownerId" and WhereOperator::Equal with platform_value!(identity_id), limit:1, select: Documents, and empty order_by/group_by/having), then replace the inline query constructions in fetch_profile_document and update_profile_with_external_signer with calls to build_profile_query to keep the V1 fields consistent.packages/rs-sdk/src/platform/documents/document_query.rs (2)
378-444: 💤 Low valueOptional: deduplicate the two
From<DriveDocumentQuery>impls.
From<&DriveDocumentQuery>andFrom<DriveDocumentQuery>have identical bodies (and both clone the contract). Consider having the by-value impl delegate to the by-reference impl to keep them in sync as the v1 surface evolves.♻️ Proposed refactor
impl<'a> From<DriveDocumentQuery<'a>> for DocumentQuery { fn from(value: DriveDocumentQuery<'a>) -> Self { - let data_contract = value.contract.clone(); - // ...duplicated body... + Self::from(&value) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk/src/platform/documents/document_query.rs` around lines 378 - 444, The two identical impls From<&'a DriveDocumentQuery<'a>> for DocumentQuery and From<DriveDocumentQuery<'a>> for DocumentQuery should be deduplicated: keep the existing by-reference impl and change the by-value impl for DriveDocumentQuery to delegate to it (call DocumentQuery::from(&value)) so the by-value conversion reuses the by-reference logic (preserving the current cloning behavior of contract, where_clauses, etc.) and prevents drift between the two implementations; update the impl block for From<DriveDocumentQuery<'a>> accordingly and remove the duplicated body.
330-375: 💤 Low valueOptional: drop unnecessary
.clone()calls on consumed fields.
dapi_requestis taken by value, sostart,document_type_name,group_by, andhavingcan be moved rather than cloned. This is a minor allocation win.♻️ Proposed refactor
- let start_v1 = dapi_request.start.clone().map(|s| match s { + let start_v1 = dapi_request.start.map(|s| match s { Start::StartAfter(b) => V1Start::StartAfter(b), Start::StartAt(b) => V1Start::StartAt(b), }); //todo: transform this into PlatformVersionedTryFrom Ok(GetDocumentsRequest { version: Some(V1(GetDocumentsRequestV1 { data_contract_id: dapi_request.data_contract.id().to_vec(), - document_type: dapi_request.document_type_name.clone(), + document_type: dapi_request.document_type_name, r#where: where_clauses, order_by, limit, // ...existing comment... prove: true, start: start_v1, select: dapi_request.select as i32, - group_by: dapi_request.group_by.clone(), - having: dapi_request.having.clone(), + group_by: dapi_request.group_by, + having: dapi_request.having, })), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk/src/platform/documents/document_query.rs` around lines 330 - 375, dapi_request is owned so avoid needless clones: remove .clone() on dapi_request.start, dapi_request.document_type_name, dapi_request.group_by, and dapi_request.having and move those fields by value into the GetDocumentsRequest (e.g., use dapi_request.start.map(...), dapi_request.document_type_name, dapi_request.group_by, dapi_request.having). Keep existing serialization of where_clauses/order_by unchanged if they still require cloning or borrowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dapi-grpc/protos/platform/v0/platform.proto`:
- Around line 696-700: Update the pagination cursor docblock (the comments
describing start_after/start_at) to match runtime behavior: explicitly state
that start (start_after/start_at) is rejected for all queries with select=COUNT
(regardless of group_by), and only supported for select=DOCUMENTS; remove or
change the existing sentence that allows start for select=COUNT with non-empty
group_by to avoid implying it's accepted. Reference the comment for the
pagination cursor and the symbols start_after, start_at, select=DOCUMENTS, and
select=COUNT when making the edit.
In `@packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 106-109: The current code leaks memory by using Box::leak when
mapping WhereClause::from_components errors; change the approach so the error
variant owns the formatted message instead of requiring a 'static str: update
QuerySyntaxError::InvalidFormatWhereClause to accept an owned String or Box<str>
(rather than &'static str), then construct the error with
QueryError::Query(QuerySyntaxError::InvalidFormatWhereClause(format!("invalid
where clause components: {e}"))) in the WhereClause::from_components error path;
update any other sites that construct or match on InvalidFormatWhereClause
accordingly to use the owned string type.
In `@packages/rs-sdk-ffi/src/document/queries/count.rs`:
- Around line 336-338: The code currently maps any negative `limit` to 0 via the
`limit_u32` conversion; change this to honor only -1 as the "unset" sentinel: if
`limit == -1` map to 0, if `limit < -1` return an error (e.g. Err(...) or an
InvalidArgument/Parse error consistent with this module's error handling) and
otherwise cast `limit` to `u32`; update the conversion at the `limit_u32`
binding and ensure callers of this function propagate or handle the new error
path.
In `@packages/rs-sdk/src/platform/documents/document_count.rs`:
- Around line 320-523: The function
DocumentSplitCounts::maybe_from_proof_with_metadata incorrectly falls through
for range queries with an empty group_by; add an early branch when has_range &&
request.group_by.is_empty() that mirrors the DocumentCount aggregate-path:
extract proof/metadata from response, call verify_aggregate_count_proof (or the
equivalent helper used at DocumentCount) to get the total count, then build a
single SplitCountEntry { in_key: None, key: Vec::new(), count } and return
Some(DocumentSplitCounts::from_verified(vec![...])) with the cloned metadata and
proof; if you prefer to keep unsupported semantics instead, add an explicit
early Err(drive_proof_verifier::Error::RequestError { error: "...range + empty
group_by not supported by DocumentSplitCounts..." }) to fail fast and avoid the
misleading index lookup error.
---
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/profile.rs`:
- Around line 156-171: Both fetch_profile_document and
update_profile_with_external_signer duplicate building the same DocumentQuery
for the "profile" doctype ($ownerId == identity_id, limit: 1); extract a helper
function like build_profile_query(dashpay_contract: Arc<...>, identity_id:
<type>) -> dash_sdk::platform::DocumentQuery that constructs and returns the
DocumentQuery (set data_contract, document_type_name="profile", where_clauses
with field "$ownerId" and WhereOperator::Equal with
platform_value!(identity_id), limit:1, select: Documents, and empty
order_by/group_by/having), then replace the inline query constructions in
fetch_profile_document and update_profile_with_external_signer with calls to
build_profile_query to keep the V1 fields consistent.
In `@packages/rs-sdk/src/platform/documents/document_count.rs`:
- Around line 102-291: The two big impls
(DocumentCount::maybe_from_proof_with_metadata and
DocumentSplitCounts::maybe_from_proof_with_metadata) duplicate logic for
range-vs-point dispatch, DriveDocumentCountQuery construction, index lookup,
documents_countable fast-path and proof/metadata extraction; extract shared
helpers such as build_range_count_query(request: &DocumentQuery) ->
Result<(DriveDocumentCountQuery, index, document_type),
drive_proof_verifier::Error> and build_point_count_query(request:
&DocumentQuery) -> Result<DriveDocumentCountQuery, drive_proof_verifier::Error>,
plus a helper extract_proof_and_metadata(response: &GetDocumentsResponse) ->
Result<(Proof, ResponseMetadata), ...>, then replace the duplicated blocks in
both impls to call these helpers (preserving existing error messages and all
uses of DriveDocumentCountQuery, verify_distinct_count_proof,
verify_aggregate_count_proof, verify_primary_key_count_tree_proof,
verify_point_lookup_count_proof) so behavior stays identical while keeping the
two impls aligned as the API evolves.
In `@packages/rs-sdk/src/platform/documents/document_query.rs`:
- Around line 378-444: The two identical impls From<&'a DriveDocumentQuery<'a>>
for DocumentQuery and From<DriveDocumentQuery<'a>> for DocumentQuery should be
deduplicated: keep the existing by-reference impl and change the by-value impl
for DriveDocumentQuery to delegate to it (call DocumentQuery::from(&value)) so
the by-value conversion reuses the by-reference logic (preserving the current
cloning behavior of contract, where_clauses, etc.) and prevents drift between
the two implementations; update the impl block for From<DriveDocumentQuery<'a>>
accordingly and remove the duplicated body.
- Around line 330-375: dapi_request is owned so avoid needless clones: remove
.clone() on dapi_request.start, dapi_request.document_type_name,
dapi_request.group_by, and dapi_request.having and move those fields by value
into the GetDocumentsRequest (e.g., use dapi_request.start.map(...),
dapi_request.document_type_name, dapi_request.group_by, dapi_request.having).
Keep existing serialization of where_clauses/order_by unchanged if they still
require cloning or borrowing.
In `@packages/wasm-sdk/src/dpns.rs`:
- Around line 282-284: The long fully-qualified enum path
dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v1::Select
is repeated and harms readability; add a local alias (e.g. use
dash_sdk::dapi_grpc::platform::v0::get_documents_request::get_documents_request_v1::Select;)
at the top of packages/wasm-sdk/src/dpns.rs (or re-export it from
dash_sdk::platform) and then replace occurrences like select:
dash_sdk::dapi_grpc::...::Select::Documents with select: Select::Documents to
simplify the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9765acee-3764-4979-b2a7-f167481a97ef
📒 Files selected for processing (37)
packages/dapi-grpc/build.rspackages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.mpackages/dapi-grpc/clients/platform/v0/python/platform_pb2.pypackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb.jspackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-dapi-client/src/transport/grpc.rspackages/rs-dapi/src/services/platform_service/mod.rspackages/rs-drive-abci/src/query/document_count_query/mod.rspackages/rs-drive-abci/src/query/document_count_query/v0/mod.rspackages/rs-drive-abci/src/query/document_query/mod.rspackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/mod.rspackages/rs-drive-abci/src/query/service.rspackages/rs-drive-proof-verifier/src/proof/document_count.rspackages/rs-drive-proof-verifier/src/proof/document_split_count.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rspackages/rs-platform-version/src/version/mocks/v2_test.rspackages/rs-platform-wallet/src/wallet/identity/network/profile.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-sdk-ffi/src/document/queries/count.rspackages/rs-sdk/src/mock/sdk.rspackages/rs-sdk/src/platform/dashpay/contact_request_queries.rspackages/rs-sdk/src/platform/documents/document_count.rspackages/rs-sdk/src/platform/documents/document_count_query.rspackages/rs-sdk/src/platform/documents/document_query.rspackages/rs-sdk/src/platform/documents/mod.rspackages/rs-sdk/src/platform/dpns_usernames/mod.rspackages/rs-sdk/src/platform/dpns_usernames/queries.rspackages/rs-sdk/tests/fetch/document_count.rspackages/wasm-sdk/src/dpns.rspackages/wasm-sdk/src/queries/document.rs
💤 Files with no reviewable changes (8)
- packages/rs-dapi/src/services/platform_service/mod.rs
- packages/rs-drive-abci/src/query/document_count_query/mod.rs
- packages/rs-drive-abci/src/query/mod.rs
- packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs
- packages/rs-sdk/src/platform/documents/document_count_query.rs
- packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs
- packages/rs-platform-version/src/version/mocks/v2_test.rs
- packages/rs-dapi-client/src/transport/grpc.rs
…bool The `return_distinct_counts_in_range` knob only ever lived on the v0 `GetDocumentsCountRequest` endpoint, which shipped in #3623, never had stable callers, and was fully removed (not deprecated) from the proto in PR 1 of this work. PR 2 preserved the bool on the FFI and wasm-sdk count surfaces "for back-compat" — but there was nothing to be back-compatible with, so the in-shim implicit-grouping translation is dead weight. This commit removes it. - rs-sdk-ffi `dash_sdk_document_count`: replace the `return_distinct_counts_in_range: bool` parameter with `group_by_json: *const c_char` (NUL-terminated JSON array of field names; null/empty → aggregate). Mirrors the v1 wire's `group_by: repeated string` field one-to-one. - wasm-sdk `DocumentsQuery`: replace `returnDistinctCountsInRange?: boolean` with `groupBy?: string[]`. Same one-to-one wire mirror on the JS side. - Delete `derive_group_by` helper in rs-sdk-ffi and the inline copy in wasm-sdk. No SDK-side translation table; no second source of truth for "which operators are range operators"; callsites become trivial pass-throughs. - Comment / docstring cleanup in document_count.rs and the count-fetch test to drop residual references to the legacy flag's narrative. No external callers — verified by grep against Swift / Kotlin / native / wasm-demo sources; only the auto-generated FFI header mentioned the old signature. Existing tests all still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-sdk/src/platform/documents/document_count.rs`:
- Around line 500-517: Return a single aggregate empty-key entry when the
request has no grouping: after calling verify_point_lookup_count_proof (which
yields entries: Vec<SplitCountEntry>), if request.group_by.is_empty() collapse
all per-In entries into one SplitCountEntry with in_key: None, key: Vec::new(),
and count equal to the sum of the counts from entries (replace entries with that
single entry); preserve the existing zero-case logic that pushes an empty entry
when !has_in && entries.is_empty(); keep using
DocumentSplitCounts::from_verified and the same mtd/proof return values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e6a45b2-9ea5-43a9-97b6-3a46a4300e56
📒 Files selected for processing (4)
packages/rs-sdk-ffi/src/document/queries/count.rspackages/rs-sdk/src/platform/documents/document_count.rspackages/rs-sdk/tests/fetch/document_count.rspackages/wasm-sdk/src/queries/document.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/tests/fetch/document_count.rs
`select` was tacked onto the end of the struct when it was
added, which reads awkwardly given the type is the SQL-shaped
query surface. Reorder the field declaration (and every struct
literal, the `new()` constructor, and both `From<DriveDocumentQuery>`
impls) to canonical SQL clause order:
SELECT, FROM, WHERE, GROUP BY, HAVING, ORDER BY, LIMIT, OFFSET
i.e.:
select, data_contract, document_type_name, where_clauses,
group_by, having, order_by_clauses, limit, start
The struct uses named fields throughout so this is purely
stylistic; behavior, wire format, serde shape, and the Mockable
derive are all unaffected. 6 SDK fetch tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `Select as V1Select` rename was load-bearing back when the DocumentCountQuery wrapper coexisted with a `Select` type from somewhere else; with the wrapper gone there is no other `Select` in scope inside document_query.rs or document_count.rs, so the alias is pure noise. `Start as V1Start` stays — that one is real, since `Start` is also imported from `get_documents_request_v0` for the DocumentQuery.start field. No behavior change. SDK fetch tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The non-Rust gRPC clients (nodejs / web / objective-c / python / java) still referenced the pre-reshape `GetDocumentsResponseV1` fields (`documents`, `counts`) and the removed `GetDocumentsCountResponse` types. Regenerate against the current `platform.proto` so they pick up the inner `ResultData` wrapper that the v1 response carries (`result.data.documents` for SELECT=DOCUMENTS, `result.data.counts` for SELECT=COUNT). Generated output only — no hand-edits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace comments that referenced commit-time context ("removed
in PR 2", "Pre-v1", "Phase 1", "prior regression", "v0-style",
"the v0 wrapper carried...") with comments that describe what
the code does and why, without anchoring to a transition that a
future reader has no context for.
The most egregious offender was a multi-line note in
`mock/sdk.rs` explaining why the `DocumentCountQuery` arm
"was removed in PR 2 of the v1 migration"; deleted entirely
since the match expression with one `DocumentQuery` arm
speaks for itself. Similar rewrites in `document_count.rs`,
`document_query.rs`, the FFI `count.rs`, the wasm-sdk
`document.rs`, and the SDK fetch tests.
Stable wire-version identifiers like "V1 wire" and
"GetDocumentsRequestV1" are kept — those name a proto version
that exists in the codebase, not a transition.
No behavior changes. Tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…yet"
`unimplemented!("queries without proofs are not supported yet")`
implied this was a feature waiting to land. It isn't: dash-sdk
intentionally only serves proof-verified responses, and that's
the permanent architectural contract — non-proven gRPC is a
direct-client concern (rs-dapi-client), not an SDK fetch-path
concern.
Replace the `unimplemented!` panic with a typed
`Err(Error::Config(...))` return so callers get a clean error
they can match on rather than a runtime crash, and reword the
message as a contract statement instead of a TODO.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rs-sdk-ffi/src/document/queries/count.rs (1)
321-330:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor only
-1as the limit sentinel.Line 321 still maps any negative value to
0(the "unset" sentinel), but the documented contract at line 276 defines only-1as valid. Other negatives (e.g.,-2,i64::MIN) are silently coerced to "use server default" instead of being rejected — this lets invalid input through and contradicts the doc.🛡️ Proposed fix
- let limit_u32: u32 = if limit < 0 { - 0 + let limit_u32: u32 = if limit == -1 { + 0 + } else if limit < -1 { + return Err(FFIError::InternalError(format!( + "limit {} is invalid; use -1 for server default or a non-negative value", + limit + ))); } else if limit > u32::MAX as i64 { return Err(FFIError::InternalError(format!( "limit {} exceeds u32::MAX", limit ))); } else { limit as u32 };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk-ffi/src/document/queries/count.rs` around lines 321 - 330, The current conversion of `limit` to `limit_u32` treats any negative `limit` as the sentinel for "use server default"; change this so only `-1` is accepted as that sentinel and any other negative values produce an error: check `limit` first for equality to `-1` and map that to `0u32`, then if `limit < -1` return an `FFIError::InternalError` (or appropriate error) explaining the invalid negative value, and finally handle the `limit > u32::MAX as i64` overflow case and the normal `limit as u32` conversion; update the code that sets `limit_u32` to follow this order and keep references to `FFIError::InternalError` and the `u32::MAX` check.
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/query.rs (1)
325-341: 🏗️ Heavy liftConsider applying this error-handling pattern to other Query implementations for consistency.
Currently, other
Querytrait implementations (e.g., lines 130, 144, 158, 180, 255, 282, 298, 314, etc.) still useunimplemented!()whenprove == false, which panics. For a consistent API surface and better user experience, consider refactoring them to returnError::Configas well, since the SDK is designed to only support proven queries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-sdk/src/platform/query.rs` around lines 325 - 341, Several other impl Query<...> blocks currently call unimplemented!() when prove == false, which panics; replace those with the same error-return pattern used in the DriveDocumentQuery impl. Locate each impl Query implementation that branches on prove (the ones currently invoking unimplemented!()), and change the branch to return Err(Error::Config("dash-sdk does not support non-proven queries; proof verification is mandatory on the SDK fetch path".to_string())); keep the rest of the impl (conversion to the query type, e.g., let q: DocumentQuery = (&self).into(); Ok(q)) unchanged so all Query implementations consistently return a Config error instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h`:
- Around line 235-241: The migration note about `getDocumentsCount` is
incorrectly attached to the `getIdentityByPublicKeyHash` Objective-C RPC docs;
remove that doc block from the `getIdentityByPublicKeyHash` method declarations
(`getIdentityByPublicKeyHash` / `getIdentityByPublicKeyHashV0` symbols) and
relocate it to the `getDocuments` API surface (or the proto/codegen source that
defines `GetDocumentsRequestV1` / `getDocuments`), updating the comment there to
describe that `getDocumentsCount` was removed in v1 and callers should use
`GetDocumentsRequestV1` with `version.v1.select = COUNT` (and optional
`group_by`); ensure the internal issue reference (`#3623`) is omitted from shipped
client docs per guidance.
---
Duplicate comments:
In `@packages/rs-sdk-ffi/src/document/queries/count.rs`:
- Around line 321-330: The current conversion of `limit` to `limit_u32` treats
any negative `limit` as the sentinel for "use server default"; change this so
only `-1` is accepted as that sentinel and any other negative values produce an
error: check `limit` first for equality to `-1` and map that to `0u32`, then if
`limit < -1` return an `FFIError::InternalError` (or appropriate error)
explaining the invalid negative value, and finally handle the `limit > u32::MAX
as i64` overflow case and the normal `limit as u32` conversion; update the code
that sets `limit_u32` to follow this order and keep references to
`FFIError::InternalError` and the `u32::MAX` check.
---
Nitpick comments:
In `@packages/rs-sdk/src/platform/query.rs`:
- Around line 325-341: Several other impl Query<...> blocks currently call
unimplemented!() when prove == false, which panics; replace those with the same
error-return pattern used in the DriveDocumentQuery impl. Locate each impl Query
implementation that branches on prove (the ones currently invoking
unimplemented!()), and change the branch to return Err(Error::Config("dash-sdk
does not support non-proven queries; proof verification is mandatory on the SDK
fetch path".to_string())); keep the rest of the impl (conversion to the query
type, e.g., let q: DocumentQuery = (&self).into(); Ok(q)) unchanged so all Query
implementations consistently return a Config error instead of panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e030b6f-6ccd-4808-a520-c87519997e8f
📒 Files selected for processing (21)
packages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.jspackages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.javapackages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.mpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.hpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.mpackages/dapi-grpc/clients/platform/v0/python/platform_pb2.pypackages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.pypackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb.jspackages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb_service.jspackages/rs-sdk-ffi/src/document/queries/count.rspackages/rs-sdk/src/mock/sdk.rspackages/rs-sdk/src/platform/documents/document_count.rspackages/rs-sdk/src/platform/documents/document_query.rspackages/rs-sdk/src/platform/query.rspackages/rs-sdk/tests/fetch/document_count.rspackages/wasm-sdk/src/queries/document.rs
💤 Files with no reviewable changes (3)
- packages/rs-sdk/src/mock/sdk.rs
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts
✅ Files skipped from review due to trivial changes (6)
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m
- packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m
- packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-sdk/src/platform/documents/document_count.rs
- packages/wasm-sdk/src/queries/document.rs
- packages/rs-sdk/src/platform/documents/document_query.rs
- packages/rs-sdk/tests/fetch/document_count.rs
Comment on lines
+235
to
+241
| /** | ||
| * `getDocumentsCount` removed in v1: callers express counts via | ||
| * `getDocuments` with `version.v1.select = COUNT` (optionally | ||
| * with `group_by`). See `GetDocumentsRequestV1` for the unified | ||
| * SQL-shaped surface. The v0-count endpoint shipped briefly in | ||
| * #3623 and never had stable callers; v1 supersedes it entirely. | ||
| */ |
Contributor
There was a problem hiding this comment.
Move this migration note off the getIdentityByPublicKeyHash methods.
These doc blocks are now attached to the getIdentityByPublicKeyHash APIs, so generated Objective-C help text for that RPC becomes unrelated and misleading. It also bakes internal rollout history (#3623) into shipped client docs. Please move the note to the getDocuments surface or the proto/codegen source that owns the migration guidance instead of annotating an unrelated method.
Also applies to: 571-579, 582-590
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h` around
lines 235 - 241, The migration note about `getDocumentsCount` is incorrectly
attached to the `getIdentityByPublicKeyHash` Objective-C RPC docs; remove that
doc block from the `getIdentityByPublicKeyHash` method declarations
(`getIdentityByPublicKeyHash` / `getIdentityByPublicKeyHashV0` symbols) and
relocate it to the `getDocuments` API surface (or the proto/codegen source that
defines `GetDocumentsRequestV1` / `getDocuments`), updating the comment there to
describe that `getDocumentsCount` was removed in v1 and callers should use
`GetDocumentsRequestV1` with `version.v1.select = COUNT` (and optional
`group_by`); ensure the internal issue reference (`#3623`) is omitted from shipped
client docs per guidance.
QuantumExplorer
changed the title
Four independent fixes raised in review of 4e6d040: [P1] Reject `SELECT COUNT, group_by=[]` with non-None `limit`. The handler previously forwarded `request_v1.limit` to Drive even on the aggregate path, so Drive's PerInValue fan-out would honor the limit and return a partial sum that looked like a total. Now validated up front in `validate_and_route` and rejected with `QuerySyntaxError::InvalidLimit`, matching the proto contract that aggregate count is structurally a single row. [P1] Reject single-field `group_by` when both `In` and range clauses are constrained. `group_by=[in_field]` (or `[range_field]`) routes through `dispatch_count_v1` with `return_distinct_counts_in_range` toggled, but Drive's compound walk emits unmerged `(in_key, key)` rows that don't match a single-field grouping. Now both single-field branches require the other clause to be absent; callers wanting the compound shape must spell it out with `[in_field, range_field]`. [P2] Drop cursor-on-grouped-count claim from the proto docs. The handler at `dispatch_count_v1` rejects every count cursor permanently, but `start.proto`'s docstring still claimed they were valid for `select=COUNT` with non-empty `group_by`. Update the comment to reflect reality: cursors are documents-only, and count pagination happens by narrowing the where-clause range. [P2] Drop `Box::leak` from the malformed-where-clause path. `QuerySyntaxError::InvalidFormatWhereClause(&'static str)` was forcing a permanent leak on every malformed external request — a slow unbounded DoS vector. Variant now takes `String`; 12 callsites updated to `.to_string()` (or `format!()` at the formerly-leaky site). Three new validation tests cover the new rejections; all 50 v1 document_query tests, 38 rs-drive count query tests, and 6 SDK fetch tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ool with CountMode enum
The boolean flag on `DocumentCountRequest` conflated two distinct
caller intents — `(In + no-distinct + aggregate-sum)` and
`(In + no-distinct + per-In-entries)` shared the same `false`
value despite producing very different responses. That overlap
was the root cause of Codex review Finding 1 (aggregate
silently honoring `limit` via PerInValue fan-out).
Introduce `enum CountMode { Aggregate, GroupByIn, GroupByRange,
GroupByCompound }` as the SQL-shape contract the caller asserts
on the wire via `(select, group_by)`. The dispatcher uses it
directly instead of re-deriving from booleans; `detect_mode`
takes `mode: CountMode` instead of the bool; the dispatcher's
range-no-proof arm asks `mode.is_aggregate()` instead of
inverting a flag; the v1 handler's `RoutingDecision` collapses
to `enum { Documents, Count(CountMode) }` so the count branch
of dispatch carries the mode literally.
Existing `DocumentCountMode` (executor-strategy enum: Total /
PerInValue / RangeNoProof / RangeProof / RangeDistinctProof /
PointLookupProof) stays — it's a different abstraction layer
(which proof primitive / which walk), derived from
`(CountMode, where_clauses, prove)`. The two coexist with a
clarifying docstring at `CountMode`'s definition.
Test fixtures in rs-drive/tests.rs and the lone v0
contract-insert test updated from `return_distinct_counts_in_range:
bool` to `mode: CountMode::*`. detect_mode_tests bulk-updated
via regex (false→Aggregate, true→GroupByRange — both
behaviorally equivalent since detect_mode only branches on
`mode.requires_distinct_walk()`).
All test suites still pass: 50 drive-abci v1 tests, 38 rs-drive
count tests, 6 SDK fetch tests, 0 regressions.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review caught that `point_lookup_count_path_query` builds its outer `SizedQuery::new(_, None, None)` and the PointLookupProof dispatch never threaded `request.limit` into it. Effect: proof-backed `select=COUNT, group_by=[in_field], limit=N` queries silently returned all In branches in the proof, ignoring N. The right fix isn't to thread limit through to the path query — the In array is already capped at 100 entries by `WhereClause::in_values()`, so result size is bounded by construction and a separate limit is either redundant (≤ 100) or would require partial-In SizedQuery semantics the verifier can't reconstruct symmetrically. Reject limit upstream instead. Add `CountMode::accepts_limit()` so the contract lives on the mode itself (`GroupByRange` / `GroupByCompound` accept; the other two reject), restructure `validate_and_route` to compute the mode first and check `limit` once at the bottom, and add a breadcrumb comment on the `SizedQuery::new(_, None, None)` site so a future reader knows the `None` limit is the deliberate contract, not an oversight. New test `reject_count_group_by_in_with_limit` companion to the existing `reject_count_aggregate_with_limit`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ating Some(0) and None Codex review flagged that proof-backed `select=COUNT, group_by=[in_field]` queries omit zero-count entries for In values with no matching documents, despite proto docs promising them. The underlying issue is structural: the PointLookupProof shape only materializes existing CountTree elements (zero-count branches aren't stored in the merk tree), so "absent from proof" collapses with "verified zero" if both are encoded as `count: 0`. Three-value contract instead: - `Some(n)` with `n > 0` — verified count for an existing CountTree element. - `Some(0)` — caller queried this branch and the executor / verifier confirmed zero (no-proof path, or documents_countable fast path where the empty tree IS cryptographically committed). - `None` — caller asked but the verifier was silent. Distinct from `Some(0)` so callers don't conflate "verified zero" with "proof didn't cover this." Implementation: - `SplitCountEntry.count` type changes from `u64` to `Option<u64>`. ~20 production callsites updated. - Drive executors emit `Some(n)` (the no-proof path knows what it queried). - Drive verifiers emit `Some(n)` for proof-materialized entries. - SDK's `FromProof<DocumentQuery>` for `DocumentSplitCounts` appends `count: None` entries for In values in the request that the proof was silent on (new helper `synthesize_missing_in_entries`, keyed off `document_type.serialize_value_for_key` so the synthesized keys byte-match the verified ones). - `DocumentCount` summing uses `filter_map(|e| e.count)` so `None` entries don't contaminate the aggregate. - `into_flat_map` treats `None` as 0 for the sum (caller can iterate `self.0` directly for the full three-valued view). - Wire format unchanged — wire `CountEntry.count` stays `uint64`; server-side never emits `None` so the conversion is lossless. `None` synthesis lives SDK-only because the In array context lives on the request, not the wire response. Proto docstring rewritten: the "Zero-count entries on `In`-grouped queries" section now describes the actual behavior (wire emits only existing CountTree entries; SDK synthesizes `None` for absent ones). New test `test_mock_fetch_document_split_counts_preserves_none_for_absent_in_values` pins the wire/mock shape round-trip for mixed `Some(_)` / `None` fixtures. Counts: 38/38 rs-drive count + 51/51 drive-abci v1 + 225/225 drive-proof-verifier + 7/7 SDK fetch (up from 6) — all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The proto docstring on `GetDocumentsRequestV1.limit` still said aggregate count "ignores" `limit`, but the v1 handler now rejects every `select=COUNT, group_by=[]` request that sets one (landed earlier in this PR after Codex flagged it). Generated client docs (nodejs / web / objective-c / python / java) pointed integrators at behavior the server no longer honors. Update the comment to spell out the per-shape contract: - `Aggregate`: rejected with `InvalidLimit` when set. - `GroupByIn`: rejected with `InvalidLimit` when set (In array already capped at 100 by `WhereClause::in_values()`; partial- In SizedQuery selection isn't representable). - `GroupByRange` / `GroupByCompound`: accepted, validate-don't- clamp on the prove path. Generated comments only — no code or wire format change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3633 +/- ##
============================================
- Coverage 88.25% 88.24% -0.02%
============================================
Files 2494 2504 +10
Lines 304580 307236 +2656
============================================
+ Hits 268812 271106 +2294
- Misses 35768 36130 +362
🚀 New features to boost your workflow:
|
…s grovedb's None
The SDK's `synthesize_missing_in_entries` was re-deriving
information the verifier already had and was throwing away.
`GroveDb::verify_query` returns `(path, key, Option<Element>)`
triples for every key the path query enumerates — `Some(element)`
for present branches, `None` for absent ones — but
`verify_point_lookup_count_proof_v0` discarded the `None` triples
via `let Some(e) = elem else { continue };`, then the SDK
reconstructed them client-side by comparing the request's In
array against the surviving entries.
Pass through what grovedb already gives us:
- `verify_point_lookup_count_proof_v0` now maps `elem.map(|e|
e.count_value_or_default())` directly onto
`SplitCountEntry::count`, so absent branches surface as
`count: None` from the verifier itself.
- `synthesize_missing_in_entries` deleted from
`dash-sdk/.../document_count.rs` along with its call site and
the unused `has_in` derivation.
- The Equal-only fully-covered re-emit-zero hack is also gone —
the verifier now emits the entry with `count: None` instead of
silently dropping it, so the caller always sees one entry per
queried key regardless of presence.
Wire format unchanged; the change is purely in how
verifier-internal data is shaped. Proto doc + SDK doc updated to
describe the simpler model.
All 38 rs-drive count + 51 drive-abci v1 + 7 SDK fetch tests
still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The core rewiring is sound: the new v1 document-query surface routes into the existing executors and I did not confirm any consensus or panic-safety blocker in the reviewed paths. The remaining issues are targeted quality gaps: one real request-validation mismatch, one misleading error classification, two compatibility/UX issues at language boundaries, and missing tests around newly introduced behavior.
Reviewed commit: 55e3526
🟡 4 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🟡 suggestion: The verifier's new `None` propagation is untested on the real proof path
packages/rs-drive/src/verify/document_count/verify_point_lookup_count_proof/v0/mod.rs (lines 89-111)
This verifier now preserves None from GroveDb::verify_query instead of skipping missing branches. That is a behavioral change in the proof decoder itself, but the test coverage only exercises a mock DocumentSplitCounts round-trip in rs-sdk/tests/fetch/document_count.rs; it does not invoke verify_point_lookup_count_proof_v0 against an actual proof containing an absent In branch. A regression back to continue on None would still pass the current tests. Add a proof-backed test that verifies an In request with a missing branch yields a SplitCountEntry { count: None }.
🟡 suggestion: The WASM query input silently ignores the removed `returnDistinctCountsInRange` option
packages/wasm-sdk/src/queries/document.rs (lines 129-155)
DocumentsQueryInput accepts unknown fields, and the legacy returnDistinctCountsInRange property is no longer part of the struct. A plain JavaScript caller that still sends returnDistinctCountsInRange: true will not get an error; serde drops the field and the request falls back to empty groupBy, which changes the response shape from distinct entries to an aggregate count. For a boundary-layer deprecation, silent reinterpretation is the wrong failure mode. Reject unknown fields or keep a shim field that throws a migration error when the deprecated option is supplied.
🤖 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-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] lines 470-500: The new aggregate `In` no-proof collapse has no dedicated regression test
`dispatch_count_v1` has a special-case branch for `select=COUNT`, `group_by=[]`, `prove=false` when Drive returns `DocumentCountResponse::Entries`: it folds the per-`In` entries back into a single aggregate result. None of the ported count tests exercise that exact wire shape. `ported_documents_count_with_in_operator` covers `group_by=["age"]`, which expects entries on the wire, not the aggregate collapse path. Because this is the only response-shape transformation introduced in the v1 handler, it needs a direct test to lock the contract and catch future refactors that accidentally return entry rows instead of an aggregate.
- [SUGGESTION] lines 260-280: `limit: Some(0)` has inconsistent semantics between document and count requests
The v1 handler treats `limit: Some(0)` as an explicit, invalid limit for count modes that do not accept `limit`, because it rejects any `request_v1.limit.is_some()`. The documents path treats the same wire value as unset by forwarding `request_v1.limit.unwrap_or(0)` into the v0 sentinel semantics. That means the same encoded request value is accepted as "use server default" for `SELECT DOCUMENTS` but rejected for aggregate and `GROUP BY In` count requests. Raw gRPC, WASM, or FFI callers can legitimately send `Some(0)`, so this is observable behavior, not just an internal SDK detail.
In `packages/rs-drive/src/verify/document_count/verify_point_lookup_count_proof/v0/mod.rs`:
- [SUGGESTION] lines 89-111: The verifier's new `None` propagation is untested on the real proof path
This verifier now preserves `None` from `GroveDb::verify_query` instead of skipping missing branches. That is a behavioral change in the proof decoder itself, but the test coverage only exercises a mock `DocumentSplitCounts` round-trip in `rs-sdk/tests/fetch/document_count.rs`; it does not invoke `verify_point_lookup_count_proof_v0` against an actual proof containing an absent `In` branch. A regression back to `continue` on `None` would still pass the current tests. Add a proof-backed test that verifies an `In` request with a missing branch yields a `SplitCountEntry { count: None }`.
In `packages/wasm-sdk/src/queries/document.rs`:
- [SUGGESTION] lines 129-155: The WASM query input silently ignores the removed `returnDistinctCountsInRange` option
`DocumentsQueryInput` accepts unknown fields, and the legacy `returnDistinctCountsInRange` property is no longer part of the struct. A plain JavaScript caller that still sends `returnDistinctCountsInRange: true` will not get an error; serde drops the field and the request falls back to empty `groupBy`, which changes the response shape from distinct entries to an aggregate count. For a boundary-layer deprecation, silent reinterpretation is the wrong failure mode. Reject unknown fields or keep a shim field that throws a migration error when the deprecated option is supplied.
Comment on lines
+470
to
+500
| DocumentCountResponse::Entries(entries) => { | ||
| if mode.is_aggregate() { | ||
| // `select=COUNT, group_by=[]` against a request | ||
| // that drove a PerInValue execution (In + no | ||
| // range + no prove). Sum entries into a single | ||
| // aggregate before emission. `saturating_add` | ||
| // on the off-chance an operator-misconfigured | ||
| // count tree exceeds u64; realistic ceiling is | ||
| // `|In| × max_per-branch-count`, well under u64. | ||
| let total: u64 = entries | ||
| .iter() | ||
| // `count.unwrap_or(0)` here is safe: this | ||
| // arm is server-side, summing entries the | ||
| // executor emitted. Executor never emits | ||
| // `None` (that's an SDK-side | ||
| // synthesis-for-missing concept). The | ||
| // `unwrap_or(0)` is a belt-and-suspenders | ||
| // guard against any future executor that | ||
| // forgets the contract. | ||
| .map(|e| e.count.unwrap_or(0)) | ||
| .fold(0u64, |a, b| a.saturating_add(b)); | ||
| GetDocumentsResponseV1 { | ||
| result: Some(get_documents_response_v1::Result::Data(ResultData { | ||
| variant: Some(result_data::Variant::Counts(CountResults { | ||
| variant: Some(count_results::Variant::AggregateCount(total)), | ||
| })), | ||
| })), | ||
| metadata: Some( | ||
| self.response_metadata_v0(platform_state, CheckpointUsed::Current), | ||
| ), | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: The new aggregate In no-proof collapse has no dedicated regression test
dispatch_count_v1 has a special-case branch for select=COUNT, group_by=[], prove=false when Drive returns DocumentCountResponse::Entries: it folds the per-In entries back into a single aggregate result. None of the ported count tests exercise that exact wire shape. ported_documents_count_with_in_operator covers group_by=["age"], which expects entries on the wire, not the aggregate collapse path. Because this is the only response-shape transformation introduced in the v1 handler, it needs a direct test to lock the contract and catch future refactors that accidentally return entry rows instead of an aggregate.
source: ['claude']
🤖 Fix this 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-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] lines 470-500: The new aggregate `In` no-proof collapse has no dedicated regression test
`dispatch_count_v1` has a special-case branch for `select=COUNT`, `group_by=[]`, `prove=false` when Drive returns `DocumentCountResponse::Entries`: it folds the per-`In` entries back into a single aggregate result. None of the ported count tests exercise that exact wire shape. `ported_documents_count_with_in_operator` covers `group_by=["age"]`, which expects entries on the wire, not the aggregate collapse path. Because this is the only response-shape transformation introduced in the v1 handler, it needs a direct test to lock the contract and catch future refactors that accidentally return entry rows instead of an aggregate.
Comment on lines
+260
to
+280
| if request_v1.limit.is_some() && !mode.accepts_limit() { | ||
| let reason = match mode { | ||
| CountMode::Aggregate => { | ||
| "`limit` is not valid for SELECT COUNT with empty GROUP BY \ | ||
| (aggregate count is a single row; omit `limit` to fix)" | ||
| } | ||
| CountMode::GroupByIn => { | ||
| "`limit` is not valid for SELECT COUNT with GROUP BY on an \ | ||
| `In` field (result is bounded by the In array — capped at \ | ||
| 100 entries; narrow the In array directly to reduce the \ | ||
| result set)" | ||
| } | ||
| CountMode::GroupByRange | CountMode::GroupByCompound => unreachable!( | ||
| "`accepts_limit()` returns true for these variants; \ | ||
| outer guard already filtered them out" | ||
| ), | ||
| }; | ||
| return Err(QueryError::Query(QuerySyntaxError::InvalidLimit( | ||
| reason.to_string(), | ||
| ))); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: limit: Some(0) has inconsistent semantics between document and count requests
The v1 handler treats limit: Some(0) as an explicit, invalid limit for count modes that do not accept limit, because it rejects any request_v1.limit.is_some(). The documents path treats the same wire value as unset by forwarding request_v1.limit.unwrap_or(0) into the v0 sentinel semantics. That means the same encoded request value is accepted as "use server default" for SELECT DOCUMENTS but rejected for aggregate and GROUP BY In count requests. Raw gRPC, WASM, or FFI callers can legitimately send Some(0), so this is observable behavior, not just an internal SDK detail.
source: ['claude']
🤖 Fix this 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-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] lines 260-280: `limit: Some(0)` has inconsistent semantics between document and count requests
The v1 handler treats `limit: Some(0)` as an explicit, invalid limit for count modes that do not accept `limit`, because it rejects any `request_v1.limit.is_some()`. The documents path treats the same wire value as unset by forwarding `request_v1.limit.unwrap_or(0)` into the v0 sentinel semantics. That means the same encoded request value is accepted as "use server default" for `SELECT DOCUMENTS` but rejected for aggregate and `GROUP BY In` count requests. Raw gRPC, WASM, or FFI callers can legitimately send `Some(0)`, so this is observable behavior, not just an internal SDK detail.
Comment on lines
+145
to
+150
| let select = Select::try_from(request_v1.select).map_err(|_| { | ||
| not_yet_implemented(&format!( | ||
| "select value {} (not in the Select enum)", | ||
| request_v1.select | ||
| )) | ||
| })?; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: An unknown Select enum value is reported as "not yet implemented" instead of invalid input
Select::try_from(request_v1.select) maps an unknown integer to QuerySyntaxError::Unsupported("... not yet implemented"). That message is appropriate for reserved-but-planned query shapes like HAVING, but not for a malformed enum value such as 42. Returning the same "future capability" error for garbage input makes client-side error handling less precise and misclassifies a bad request as a feature gap.
source: ['claude']
Comment on lines
316
to
330
| // Sentinel decoding for the C ABI. `-1` means "unset; use | ||
| // server-side default". The Rust-side request field is | ||
| // `Option<u32>` so `None` here is the same as the request | ||
| // omitting the field on the wire. | ||
| let limit_opt = if limit < 0 { | ||
| None | ||
| // server-side default". The DocumentQuery `limit` field is | ||
| // a `u32` with `0` as its "unset" sentinel (translated to | ||
| // `None` on the V1 wire's `optional uint32`), so the FFI | ||
| // `-1` maps to `0`. | ||
| let limit_u32: u32 = if limit < 0 { | ||
| 0 | ||
| } else if limit > u32::MAX as i64 { | ||
| return Err(FFIError::InternalError(format!( | ||
| "limit {} exceeds u32::MAX", | ||
| limit | ||
| ))); | ||
| } else { | ||
| Some(limit as u32) | ||
| limit as u32 | ||
| }; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: The FFI limit documentation does not match the sentinel handling
The C ABI docs say -1 means unset and any value >= 0 is an explicit cap. The implementation maps every negative value to 0, and 0 is then translated into the Rust-side unset sentinel as well. In practice, -2, -100, and 0 all mean "use server default", which is not what the function contract documents. Either tighten the decoding to match the documented contract or update the docs to state that all values <= 0 are treated as unset.
source: ['claude']
…MAX_LIMIT_AS_FAILSAFE) The PerInValue dispatcher arm and the Aggregate sub-case of RangeNoProof both unwrapped `request.limit` to `drive_config.default_query_limit`. `CountMode::Aggregate` and `CountMode::GroupByIn` reject explicit `limit` upstream in `validate_and_route`, so `request.limit` was always None at those sites — and `default_query_limit` is a documents-fetch knob that doesn't belong on count fan-out. Result: under operator-tuned `default_query_limit < |In|`, the per-In fan-out was silently truncated and aggregate sums were wrong. Introduce `MAX_LIMIT_AS_FAILSAFE: u32 = 1024` in rs-drive as the executor-level cap for fan-out arms. The `In` array is structurally capped at 100 by `WhereClause::in_values()`, so 1024 sits well above the real bound — it's not load-bearing, just keeps the dispatcher's correctness independent of operator config. If it ever fires that's a signal to revisit the bound before raising the constant. - PerInValue: cap at `MAX_LIMIT_AS_FAILSAFE` (was `default_query_limit`). - RangeNoProof: split on `request.mode.is_aggregate()`. Aggregate uses `MAX_LIMIT_AS_FAILSAFE`; distinct-walk modes (GroupByRange / GroupByCompound) keep the existing `default_query_limit` / `max_query_limit` clamping since range-distinct is genuinely unbounded. Regression test inserts 8 docs across 8 distinct ages, sets `default_query_limit = 3`, asks for an Aggregate over the full 8-element In array, and asserts both the entry count (8) and the summed total (8). Pre-fix this returned 3 / 3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ompound limit + verifier docs Three Codex findings from the head-9ab96ef6e9 review, all in one commit because they're each small and the third one's doc fix references the wire-shape clarification from the second. [P1] `FromProof<DocumentQuery>` for `DocumentSplitCounts` only entered the distinct-range branch when `has_range && !group_by.is_empty()`. For aggregate-mode requests (`group_by = []`) the existing dispatch did the wrong thing: - `Aggregate + In + prove` → fell to point-lookup, returned per-In entries instead of the documented single empty-key total. - `Aggregate + range + prove` → tried to find a `countable: true` index for a range clause that needs `rangeCountable: true`, errored out instead of routing to `AggregateCountOnRange`. FFI / wasm-sdk both call `DocumentSplitCounts::fetch` for every count mode (the count surface's `Map<string, bigint>` return is shape-uniform across modes), so any external aggregate caller hit one of these cases. Fix by delegating to `DocumentCount`'s `FromProof` when `group_by.is_empty()` — it already owns the per-shape dispatch — and wrapping the verified `u64` as a single empty-key `SplitCountEntry`. Per-group paths (GroupByIn / GroupByRange / GroupByCompound) keep their existing dispatch. [P2] Compound limit docs (proto + `CountMode::GroupByCompound`) claimed `limit` was per-In-branch, but the executor's path-query builder pushes a single `SizedQuery::limit` over the lex `(in_key, key)` tuple stream — i.e., the cap is global, not per-branch. Align docs to the implementation: a request with `|In| = 3` and `limit = 5` returns at most 5 entries total. The implementation was already consistent; only the contract docs were lying. [P3] Stale verifier docs: - `document_split_count.rs` rejection message still pointed callers at the deleted `DocumentCountQuery` type and the removed `document_count_query.rs` file. Updated to point at the current `DocumentQuery + .with_select(Count)` entry point. - `document_count.rs` proof-verifier facade said zero-doc In branches "are omitted from the result" — that was true before the `Option<u64>` count refactor; now grovedb's `None` triples are propagated as `count: None` entries. Updated the entry shape documentation to describe the actual behavior. All 39 rs-drive count + 51 drive-abci v1 + 7 SDK fetch tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ts into own file `DocumentSplitCounts::FromProof` used to delegate its `group_by = []` branch to `DocumentCount::FromProof` via cross-impl call. That worked but read as architectural awkwardness — readers don't expect one trait impl to invoke another's `maybe_from_proof_with_metadata` to do its job, and it left the per-shape proof verification body buried inside `DocumentCount`. Pull the per-shape dispatch into a free function `verify_aggregate_count(request, response, version, provider)` in a new `count_proof_helpers.rs` sibling module. Both impls become thin wrappers around it: - `DocumentCount::FromProof` calls it once, maps `Option<u64>` to `Option<DocumentCount>`. - `DocumentSplitCounts::FromProof` (aggregate branch) calls it, maps `Option<u64>` to a single empty-key entry; per-group branches keep their existing direct-verifier dispatch since they emit shaped entries from the verifier, not a sum. Same opportunity to fix the file sizes: `document_count.rs` was ~600 LoC carrying two `FromProof` impls. Split into `document_count.rs` (~55 LoC) and `document_split_counts.rs` (~225 LoC). Shared helpers (`assert_select_is_count`, `limit_to_u16_or_default`, `verify_aggregate_count`) live in `count_proof_helpers.rs` and are `pub(super)`. No behavior change. All 7 SDK fetch tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r.rs `drive_dispatcher.rs` had two distinct responsibilities glued together at ~930 LoC: the per-mode `impl Drive` executors (`execute_document_count_total_no_proof`, `execute_document_count_per_in_value_no_proof`, etc.) AND the top-level dispatcher (`execute_document_count_request`) that routes among them. Physical split: - `drive_dispatcher.rs` (~470 LoC) — top-level dispatcher, `DocumentCountRequest` / `DocumentCountResponse` types, `where_clauses_from_value` / `order_clauses_from_value` CBOR decoders. - `executors.rs` (~480 LoC, new) — all six `execute_document_count_*` methods on `impl Drive` plus the `read_primary_key_count_tree` helper. Both files now read top-to-bottom in one mental model. The dispatcher comment block at the top points at the executors module so the relationship is discoverable. No behavior change; all 39 rs-drive count + 51 drive-abci v1 + 7 SDK fetch tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…thod docs Two readability touchups: CountMode helper methods (`is_aggregate`, `requires_distinct_walk`, `accepts_limit`) had docstrings that repeated the per-variant rules already documented on each variant. Replaced with one-line summaries that cross-ref the variants, cutting ~30 LoC of duplication and giving readers one canonical source for the per-shape semantics. `drive_dispatcher.rs` PerInValue and RangeNoProof arms had 10-15 line comment blocks explaining the limit logic. The reasoning now lives on [`MAX_LIMIT_AS_FAILSAFE`] (with an added "Pattern" section establishing the failsafe-cap pattern as a project convention for structurally-bounded executor ops); inline comments became one-line summaries pointing at the constant's docstring. Net: ~40 LoC removed across the two files. No behavior change. 39 + 51 + 7 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified all six convergent findings against SHA 75e220e. No blocking issues. The codex 'security' blockers conflate request-shape validation with proof soundness: the verifier deterministically reconstructs a path-query from where_clauses + index, and merk verification binds the proof to that path-query — a malicious server cannot substitute a proof for a different path-query. The real gap is that the SDK doesn't pre-reject unsupported (select, group_by, having, start_after) shapes the way the server does, so a malicious node could return a cryptographically valid count for a request the server would have rejected; the count is still numerically correct for the reconstructed path-query. Reframing as defense-in-depth (suggestion) rather than blocking. Remaining valid findings: two test-coverage gaps (aggregate-collapse path, verifier None propagation against real proof), one limit:Some(0) cross-mode asymmetry, one error classification miss for unknown Select enums, one WASM silent-drop of removed legacy fields, and one FFI limit-sentinel doc/impl mismatch.
Reviewed commit: 75e220e
🟡 4 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🟡 suggestion: Verifier's `None` propagation on absent In branch is not tested against a real proof
packages/rs-drive/src/verify/document_count/verify_point_lookup_count_proof/v0/mod.rs (lines 89-111)
This verifier preserves Option<Element> from GroveDb::verify_query and maps it onto SplitCountEntry::count so SDK callers can distinguish Some(0) (verified zero) from None (proof was silent on this branch). The contract is load-bearing — it's documented in the proto comment and is the stated reason the SDK no longer synthesizes missing entries — but coverage exists only via the mock round-trip test_mock_fetch_document_split_counts_preserves_none_for_absent_in_values in rs-sdk/tests/fetch/document_count.rs, which hand-builds the DocumentSplitCounts and never invokes verify_point_lookup_count_proof_v0 on real proof bytes. The two integration tests in rs-drive that do call this verifier (around lines 728 and 852) collapse the result through entries.iter().map(|e| e.count.unwrap_or(0)).sum() and only assert the sum, so a regression to continue on None or to Some(0) would still pass. Add a proof-backed test where an In array contains a value with no matching CountTree element, run prove+verify, and assert the resulting entry has count: None (not Some(0)).
🟡 suggestion: WASM `DocumentsQueryInput` silently ignores removed legacy fields
packages/wasm-sdk/src/queries/document.rs (lines 129-155)
DocumentsQueryInput derives Deserialize without #[serde(deny_unknown_fields)], so unknown JS properties are dropped silently. The legacy returnDistinctCountsInRange knob (which toggled distinct-per-value vs. aggregate response shape under the v0 count path) is no longer present and no shim catches it. A JS caller migrating from the previous binding who still passes returnDistinctCountsInRange: true gets no error: serde drops the field, groupBy defaults to [], and the request degrades from per-distinct entries to a single aggregate count without any signal. At a language boundary, this kind of lossy decode is worse than an explicit error — it turns a migration failure into a silent semantic change. Either add #[serde(deny_unknown_fields)], or temporarily reintroduce the field as Option<bool> and reject with a typed migration error when set.
💡 Suggested change
#[derive(Deserialize)]
#[serde(rename_all = "camelCase", deny_unknown_fields)]
struct DocumentsQueryInput {
🤖 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-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] lines 470-515: Aggregate-collapse-of-Entries branch in `dispatch_count_v1` has no dedicated test
`dispatch_count_v1` rewrites `DocumentCountResponse::Entries` into a single `AggregateCount` server-side (lines 470–500) when `mode.is_aggregate()` but Drive routed through the per-In executor — reachable only via `select=COUNT, group_by=[], where=[In(...)], prove=false, no range` (Drive's `detect_mode` maps that to `DocumentCountMode::PerInValue` → emits `Entries`). This is the only response-shape transformation introduced by the v1 handler. None of the ported tests hit this exact wire shape: `ported_documents_count_with_in_operator` uses `group_by=["age"]` (routes to `GroupByIn`, emits Entries unchanged), `ported_documents_count_no_prove` has no In clause (Drive returns Aggregate directly via the `documents_countable` fast path), and `ported_documents_count_range_query_no_prove` exercises the drive-side `RangeNoProof → Aggregate` collapse in `drive_dispatcher.rs`, not this handler-level fold. A regression that leaks per-In rows on the wire (or produces the wrong `saturating_add` total) would still pass. Add an end-to-end test that seeds documents, issues `select=COUNT, group_by=[], where=[(field, in, [...])], prove=false`, and asserts the response is `AggregateCount(N)`.
- [SUGGESTION] lines 260-370: `limit: Some(0)` has inconsistent semantics across v1 SELECT modes
`limit` is `optional uint32` on the wire, so `Some(0)` is a distinct value any raw-gRPC, WASM, or FFI caller can encode. The same wire bytes fan out into three behaviors: (1) `select=DOCUMENTS` (line 365) normalizes `Some(0)` via `unwrap_or(0)` and forwards it to v0, which reads `0` as 'use server default'; (2) `select=COUNT` with `mode ∈ {Aggregate, GroupByIn}` rejects `Some(0)` with `QuerySyntaxError::InvalidLimit` (line 260, `limit.is_some()` check); (3) `select=COUNT` with `mode ∈ {GroupByRange, GroupByCompound}` forwards `Some(0)` to drive's dispatcher, where it produces an empty/0-limit proof. Pick one rule consistently — either normalize `Some(0)` to `None` at handler entry, or reject `Some(0)` explicitly on the documents path — so identical encoded requests don't get three different interpretations across modes.
In `packages/rs-drive/src/verify/document_count/verify_point_lookup_count_proof/v0/mod.rs`:
- [SUGGESTION] lines 89-111: Verifier's `None` propagation on absent In branch is not tested against a real proof
This verifier preserves `Option<Element>` from `GroveDb::verify_query` and maps it onto `SplitCountEntry::count` so SDK callers can distinguish `Some(0)` (verified zero) from `None` (proof was silent on this branch). The contract is load-bearing — it's documented in the proto comment and is the stated reason the SDK no longer synthesizes missing entries — but coverage exists only via the mock round-trip `test_mock_fetch_document_split_counts_preserves_none_for_absent_in_values` in `rs-sdk/tests/fetch/document_count.rs`, which hand-builds the `DocumentSplitCounts` and never invokes `verify_point_lookup_count_proof_v0` on real proof bytes. The two integration tests in `rs-drive` that do call this verifier (around lines 728 and 852) collapse the result through `entries.iter().map(|e| e.count.unwrap_or(0)).sum()` and only assert the sum, so a regression to `continue` on `None` or to `Some(0)` would still pass. Add a proof-backed test where an In array contains a value with no matching CountTree element, run prove+verify, and assert the resulting entry has `count: None` (not `Some(0)`).
In `packages/wasm-sdk/src/queries/document.rs`:
- [SUGGESTION] lines 129-155: WASM `DocumentsQueryInput` silently ignores removed legacy fields
`DocumentsQueryInput` derives `Deserialize` without `#[serde(deny_unknown_fields)]`, so unknown JS properties are dropped silently. The legacy `returnDistinctCountsInRange` knob (which toggled distinct-per-value vs. aggregate response shape under the v0 count path) is no longer present and no shim catches it. A JS caller migrating from the previous binding who still passes `returnDistinctCountsInRange: true` gets no error: serde drops the field, `groupBy` defaults to `[]`, and the request degrades from per-distinct entries to a single aggregate count without any signal. At a language boundary, this kind of lossy decode is worse than an explicit error — it turns a migration failure into a silent semantic change. Either add `#[serde(deny_unknown_fields)]`, or temporarily reintroduce the field as `Option<bool>` and reject with a typed migration error when set.
Comment on lines
+470
to
+515
| DocumentCountResponse::Entries(entries) => { | ||
| if mode.is_aggregate() { | ||
| // `select=COUNT, group_by=[]` against a request | ||
| // that drove a PerInValue execution (In + no | ||
| // range + no prove). Sum entries into a single | ||
| // aggregate before emission. `saturating_add` | ||
| // on the off-chance an operator-misconfigured | ||
| // count tree exceeds u64; realistic ceiling is | ||
| // `|In| × max_per-branch-count`, well under u64. | ||
| let total: u64 = entries | ||
| .iter() | ||
| // `count.unwrap_or(0)` here is safe: this | ||
| // arm is server-side, summing entries the | ||
| // executor emitted. Executor never emits | ||
| // `None` (that's an SDK-side | ||
| // synthesis-for-missing concept). The | ||
| // `unwrap_or(0)` is a belt-and-suspenders | ||
| // guard against any future executor that | ||
| // forgets the contract. | ||
| .map(|e| e.count.unwrap_or(0)) | ||
| .fold(0u64, |a, b| a.saturating_add(b)); | ||
| GetDocumentsResponseV1 { | ||
| result: Some(get_documents_response_v1::Result::Data(ResultData { | ||
| variant: Some(result_data::Variant::Counts(CountResults { | ||
| variant: Some(count_results::Variant::AggregateCount(total)), | ||
| })), | ||
| })), | ||
| metadata: Some( | ||
| self.response_metadata_v0(platform_state, CheckpointUsed::Current), | ||
| ), | ||
| } | ||
| } else { | ||
| GetDocumentsResponseV1 { | ||
| result: Some(get_documents_response_v1::Result::Data(ResultData { | ||
| variant: Some(result_data::Variant::Counts(CountResults { | ||
| variant: Some(count_results::Variant::Entries(CountEntries { | ||
| entries: entries.into_iter().map(into_v1_entry).collect(), | ||
| })), | ||
| })), | ||
| })), | ||
| metadata: Some( | ||
| self.response_metadata_v0(platform_state, CheckpointUsed::Current), | ||
| ), | ||
| } | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Aggregate-collapse-of-Entries branch in dispatch_count_v1 has no dedicated test
dispatch_count_v1 rewrites DocumentCountResponse::Entries into a single AggregateCount server-side (lines 470–500) when mode.is_aggregate() but Drive routed through the per-In executor — reachable only via select=COUNT, group_by=[], where=[In(...)], prove=false, no range (Drive's detect_mode maps that to DocumentCountMode::PerInValue → emits Entries). This is the only response-shape transformation introduced by the v1 handler. None of the ported tests hit this exact wire shape: ported_documents_count_with_in_operator uses group_by=["age"] (routes to GroupByIn, emits Entries unchanged), ported_documents_count_no_prove has no In clause (Drive returns Aggregate directly via the documents_countable fast path), and ported_documents_count_range_query_no_prove exercises the drive-side RangeNoProof → Aggregate collapse in drive_dispatcher.rs, not this handler-level fold. A regression that leaks per-In rows on the wire (or produces the wrong saturating_add total) would still pass. Add an end-to-end test that seeds documents, issues select=COUNT, group_by=[], where=[(field, in, [...])], prove=false, and asserts the response is AggregateCount(N).
source: ['claude', 'codex']
🤖 Fix this 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-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] lines 470-515: Aggregate-collapse-of-Entries branch in `dispatch_count_v1` has no dedicated test
`dispatch_count_v1` rewrites `DocumentCountResponse::Entries` into a single `AggregateCount` server-side (lines 470–500) when `mode.is_aggregate()` but Drive routed through the per-In executor — reachable only via `select=COUNT, group_by=[], where=[In(...)], prove=false, no range` (Drive's `detect_mode` maps that to `DocumentCountMode::PerInValue` → emits `Entries`). This is the only response-shape transformation introduced by the v1 handler. None of the ported tests hit this exact wire shape: `ported_documents_count_with_in_operator` uses `group_by=["age"]` (routes to `GroupByIn`, emits Entries unchanged), `ported_documents_count_no_prove` has no In clause (Drive returns Aggregate directly via the `documents_countable` fast path), and `ported_documents_count_range_query_no_prove` exercises the drive-side `RangeNoProof → Aggregate` collapse in `drive_dispatcher.rs`, not this handler-level fold. A regression that leaks per-In rows on the wire (or produces the wrong `saturating_add` total) would still pass. Add an end-to-end test that seeds documents, issues `select=COUNT, group_by=[], where=[(field, in, [...])], prove=false`, and asserts the response is `AggregateCount(N)`.
Comment on lines
+260
to
+370
| if request_v1.limit.is_some() && !mode.accepts_limit() { | ||
| let reason = match mode { | ||
| CountMode::Aggregate => { | ||
| "`limit` is not valid for SELECT COUNT with empty GROUP BY \ | ||
| (aggregate count is a single row; omit `limit` to fix)" | ||
| } | ||
| CountMode::GroupByIn => { | ||
| "`limit` is not valid for SELECT COUNT with GROUP BY on an \ | ||
| `In` field (result is bounded by the In array — capped at \ | ||
| 100 entries; narrow the In array directly to reduce the \ | ||
| result set)" | ||
| } | ||
| CountMode::GroupByRange | CountMode::GroupByCompound => unreachable!( | ||
| "`accepts_limit()` returns true for these variants; \ | ||
| outer guard already filtered them out" | ||
| ), | ||
| }; | ||
| return Err(QueryError::Query(QuerySyntaxError::InvalidLimit( | ||
| reason.to_string(), | ||
| ))); | ||
| } | ||
|
|
||
| Ok(RoutingDecision::Count(mode)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Outcome of `validate_and_route` — names the path the v1 request | ||
| /// will dispatch to. | ||
| /// | ||
| /// `Count(CountMode)` carries the SQL-shape contract (`Aggregate` / | ||
| /// `GroupByIn` / `GroupByRange` / `GroupByCompound`) directly; the | ||
| /// dispatcher passes it through to [`DocumentCountRequest::mode`] | ||
| /// without further translation. | ||
| enum RoutingDecision { | ||
| Documents, | ||
| Count(CountMode), | ||
| } | ||
|
|
||
| /// Test-only: expose the routing decision for unit tests without | ||
| /// needing a full `Platform` setup. | ||
| #[cfg(test)] | ||
| pub(super) fn validate_and_route_for_tests( | ||
| request_v1: &GetDocumentsRequestV1, | ||
| where_clauses: &[WhereClause], | ||
| ) -> Result<&'static str, QueryError> { | ||
| validate_and_route(request_v1, where_clauses).map(|d| match d { | ||
| RoutingDecision::Documents => "documents", | ||
| RoutingDecision::Count(CountMode::Aggregate) => "count_aggregate", | ||
| RoutingDecision::Count(CountMode::GroupByIn) => "count_entries_via_in_field", | ||
| RoutingDecision::Count(CountMode::GroupByRange) => "count_entries_via_range_field", | ||
| RoutingDecision::Count(CountMode::GroupByCompound) => "count_entries_via_compound", | ||
| }) | ||
| } | ||
|
|
||
| impl<C> Platform<C> { | ||
| pub(super) fn query_documents_v1( | ||
| &self, | ||
| request_v1: GetDocumentsRequestV1, | ||
| platform_state: &PlatformState, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<QueryValidationResult<GetDocumentsResponseV1>, Error> { | ||
| let where_clauses = match decode_where_clauses(&request_v1.r#where) { | ||
| Ok(c) => c, | ||
| Err(e) => return Ok(QueryValidationResult::new_with_error(e)), | ||
| }; | ||
|
|
||
| let routing = match validate_and_route(&request_v1, &where_clauses) { | ||
| Ok(r) => r, | ||
| Err(e) => return Ok(QueryValidationResult::new_with_error(e)), | ||
| }; | ||
|
|
||
| match routing { | ||
| RoutingDecision::Documents => { | ||
| self.dispatch_documents_v1(request_v1, platform_state, platform_version) | ||
| } | ||
| RoutingDecision::Count(mode) => { | ||
| self.dispatch_count_v1(request_v1, mode, platform_state, platform_version) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Forward a `select = DOCUMENTS` request through the v0 | ||
| /// handler. v1 doesn't add any documents-side capability — the | ||
| /// SQL-shaped fields (`select`, `group_by`, `having`) are all | ||
| /// validated as documents-compatible above (empty `group_by`, | ||
| /// empty `having`, etc.) before reaching here. | ||
| fn dispatch_documents_v1( | ||
| &self, | ||
| request_v1: GetDocumentsRequestV1, | ||
| platform_state: &PlatformState, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<QueryValidationResult<GetDocumentsResponseV1>, Error> { | ||
| let start = request_v1.start.map(|s| match s { | ||
| RequestV1Start::StartAfter(b) => RequestV0Start::StartAfter(b), | ||
| RequestV1Start::StartAt(b) => RequestV0Start::StartAt(b), | ||
| }); | ||
| // `limit` is `optional uint32` on v1 vs unwrapped `uint32` | ||
| // (default 0) on v0. Unset on v1 → 0 on v0 (v0 reads `0` | ||
| // as "use the server's `default_query_limit`"). | ||
| let request_v0 = GetDocumentsRequestV0 { | ||
| data_contract_id: request_v1.data_contract_id, | ||
| document_type: request_v1.document_type, | ||
| r#where: request_v1.r#where, | ||
| order_by: request_v1.order_by, | ||
| limit: request_v1.limit.unwrap_or(0), | ||
| prove: request_v1.prove, | ||
| start, | ||
| }; | ||
| let result = self.query_documents_v0(request_v0, platform_state, platform_version)?; | ||
| Ok(result.map(translate_documents_v0_to_v1)) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: limit: Some(0) has inconsistent semantics across v1 SELECT modes
limit is optional uint32 on the wire, so Some(0) is a distinct value any raw-gRPC, WASM, or FFI caller can encode. The same wire bytes fan out into three behaviors: (1) select=DOCUMENTS (line 365) normalizes Some(0) via unwrap_or(0) and forwards it to v0, which reads 0 as 'use server default'; (2) select=COUNT with mode ∈ {Aggregate, GroupByIn} rejects Some(0) with QuerySyntaxError::InvalidLimit (line 260, limit.is_some() check); (3) select=COUNT with mode ∈ {GroupByRange, GroupByCompound} forwards Some(0) to drive's dispatcher, where it produces an empty/0-limit proof. Pick one rule consistently — either normalize Some(0) to None at handler entry, or reject Some(0) explicitly on the documents path — so identical encoded requests don't get three different interpretations across modes.
source: ['claude', 'codex']
🤖 Fix this 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-abci/src/query/document_query/v1/mod.rs`:
- [SUGGESTION] lines 260-370: `limit: Some(0)` has inconsistent semantics across v1 SELECT modes
`limit` is `optional uint32` on the wire, so `Some(0)` is a distinct value any raw-gRPC, WASM, or FFI caller can encode. The same wire bytes fan out into three behaviors: (1) `select=DOCUMENTS` (line 365) normalizes `Some(0)` via `unwrap_or(0)` and forwards it to v0, which reads `0` as 'use server default'; (2) `select=COUNT` with `mode ∈ {Aggregate, GroupByIn}` rejects `Some(0)` with `QuerySyntaxError::InvalidLimit` (line 260, `limit.is_some()` check); (3) `select=COUNT` with `mode ∈ {GroupByRange, GroupByCompound}` forwards `Some(0)` to drive's dispatcher, where it produces an empty/0-limit proof. Pick one rule consistently — either normalize `Some(0)` to `None` at handler entry, or reject `Some(0)` explicitly on the documents path — so identical encoded requests don't get three different interpretations across modes.
Comment on lines
+145
to
+150
| let select = Select::try_from(request_v1.select).map_err(|_| { | ||
| not_yet_implemented(&format!( | ||
| "select value {} (not in the Select enum)", | ||
| request_v1.select | ||
| )) | ||
| })?; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Unknown Select enum integer is classified as 'not yet implemented' instead of malformed input
Select::try_from(request_v1.select).map_err(|_| not_yet_implemented("select value N (not in the Select enum)")) reuses the future-capability error class for what is structurally a malformed wire value (e.g. select = 42). not_yet_implemented carries the documented contract (line 63) that 'the request structure is fine and callers can keep it unchanged when the capability lands' — which is wrong for a garbage enum discriminant that has no future-protocol meaning. QuerySyntaxError::InvalidParameter exists and is a better fit; client error handling can then distinguish 'my request shape is wrong' from 'this server doesn't do this yet.'
💡 Suggested change
Suggested change
| let select = Select::try_from(request_v1.select).map_err(|_| { | |
| not_yet_implemented(&format!( | |
| "select value {} (not in the Select enum)", | |
| request_v1.select | |
| )) | |
| })?; | |
| let select = Select::try_from(request_v1.select).map_err(|_| { | |
| QueryError::Query(QuerySyntaxError::InvalidParameter(format!( | |
| "select value {} is not in the Select enum", | |
| request_v1.select | |
| ))) | |
| })?; |
source: ['claude', 'codex']
Comment on lines
316
to
330
| // Sentinel decoding for the C ABI. `-1` means "unset; use | ||
| // server-side default". The Rust-side request field is | ||
| // `Option<u32>` so `None` here is the same as the request | ||
| // omitting the field on the wire. | ||
| let limit_opt = if limit < 0 { | ||
| None | ||
| // server-side default". The DocumentQuery `limit` field is | ||
| // a `u32` with `0` as its "unset" sentinel (translated to | ||
| // `None` on the V1 wire's `optional uint32`), so the FFI | ||
| // `-1` maps to `0`. | ||
| let limit_u32: u32 = if limit < 0 { | ||
| 0 | ||
| } else if limit > u32::MAX as i64 { | ||
| return Err(FFIError::InternalError(format!( | ||
| "limit {} exceeds u32::MAX", | ||
| limit | ||
| ))); | ||
| } else { | ||
| Some(limit as u32) | ||
| limit as u32 | ||
| }; |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: FFI dash_sdk_document_count limit decoding doesn't match its documented sentinel contract
The doc comment (lines 276–284) states -1 = use server default, ≥ 0 = explicit cap. The implementation (lines 321–330) maps every negative value to 0, then passes 0 to DocumentQuery::with_limit(0) — where 0 is itself the SDK's 'unset' sentinel (translated to None on the V1 wire). Net effect: -1, -2, -100, and 0 all mean 'use server default,' and 0 cannot express 'explicit cap of zero' even though the doc says it can. Either tighten the decode (-1 → unset, < -1 → InvalidParameter error, 0 → explicit zero cap that gets sent as Some(0)) or update the doc to state that values ≤ 0 are all treated as unset.
source: ['claude']
…de variant
`executors.rs` had one `impl Drive { ... }` block holding all
six per-mode executor methods (plus a private helper) at ~480
LoC. Each method is independent of the others — they share no
state, only the `impl Drive` block. Splitting along mode
boundaries makes "find the X-mode executor" a directory lookup
rather than a grep through 480 lines.
New layout:
drive_document_count_query/
└── executors/
├── mod.rs (32 LoC, module decls + overview)
├── total.rs (114 LoC) — DocumentCountMode::Total + read_primary_key_count_tree helper
├── per_in_value.rs (158 LoC) — DocumentCountMode::PerInValue
├── range_no_proof.rs (54 LoC) — DocumentCountMode::RangeNoProof
├── range_proof.rs (50 LoC) — DocumentCountMode::RangeProof
├── range_distinct_proof.rs (70 LoC) — DocumentCountMode::RangeDistinctProof
└── point_lookup_proof.rs (91 LoC) — DocumentCountMode::PointLookupProof
The `read_primary_key_count_tree` private helper stays in
`total.rs` (its only caller); it's `pub(super)` so a future
PointLookupProof variant that needs it can pull it in without
re-declaring.
`executors.rs` file deleted, replaced by the directory module.
No re-exports needed — each file adds methods directly to
`impl Drive`, so the dispatcher sees them on the `Drive` type
unchanged.
39 + 51 + 7 tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-and-count fallback Codex spotted Drive-side comments still describing the pre-refactor SDK and proof architecture. None of these are behavioral bugs — they just contradict the architecture/readability work this PR landed. Stale `FromProof<DocumentCountQuery>` references (the SDK `DocumentCountQuery` type was deleted; the impl is now `FromProof<DocumentQuery>`): - `path_query.rs`: `aggregate_count_path_query` docstring. - `mode_detection.rs`: the `(false, true, true, _) → PointLookupProof` match-arm comment. - `drive_dispatcher.rs`: the RangeDistinctProof arm's left_to_right comment. Stale "materialize-and-count" framing on PointLookupProof (point-lookup is a CountTree-element proof primitive, not a materialize-and-count fallback): - `mode_detection.rs:detect_mode` docstring: replaced the catch-all "Equal/In point lookup, range walk, range proof, materialize-and-count proof, etc." list with the actual six-variant enumeration. - `tests.rs:in_with_prove_routes_to_point_lookup_proof` docstring: rewrote to describe what PointLookupProof actually does (one `Element::CountTree` per In branch, verifier reads `count_value_or_default()` directly; O(|In| × log n), no doc materialization). The old "materialize path is the only correct route until grovedb gains a per-key count proof" was doubly wrong — grovedb already has CountTree-element proofs AND that's what this path uses. Time-locked "pre-this-PR" framing in `execute_point_lookup.rs`: rephrased as a timeless comparison against the regular doc-query's materialize-and-count path. No code change; 39 rs-drive count tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…impls become wrappers The previous extraction (b8974a8) pulled the per-shape proof verification into a `verify_aggregate_count` helper that returned `Option<u64>`. `DocumentCount`'s `FromProof` shrank to a 5-line wrapper, but `DocumentSplitCounts`'s non-aggregate branches still carried ~150 LoC of their own dispatch: range path looked up document_type / picked range_countable_index / built count_query / extracted proof+metadata / called `verify_distinct_count_proof`; no-range path did the same with point-lookup. That logic overlapped with `verify_aggregate_count`'s internals but used different verifier calls and a different return shape. Collapse all four count-proof dispatch paths into a single `verify_count_query` helper returning `Result<(Option<Vec<SplitCountEntry>>, ResponseMetadata, Proof), _>`: - range + non-empty group_by → `RangeDistinctProof` → per-distinct-value entries. - range + empty group_by → `AggregateCountOnRange` → single u64 wrapped as one empty-key entry. - no range + empty where + documents_countable → primary-key CountTree → single u64 wrapped as one empty-key entry. - no range + covering countable index → `PointLookupProof` → per-branch entries. Wrapping the two u64-returning primitives as single empty-key entries is the only shape massage; the verifier calls are unchanged. Both consumers reduce to one-call wrappers: - DocumentCount: sums entries via `filter_map(|e| e.count)`, wraps in `DocumentCount(u64)`. - DocumentSplitCounts: passes entries through `DocumentSplitCounts::from_verified`. File sizes: - count_proof_helpers.rs: 240 LoC (was ~225; +15 from one extra helper `single_empty_key_entry` and unified case structure) - document_count.rs: 53 LoC (was 55, basically unchanged) - document_split_counts.rs: 65 LoC (was 225 — 71% reduction) Net: -107 LoC across the three files. The dispatch logic now lives in exactly one place; adding a fifth proof shape touches `verify_count_query` and nothing else. All 7 SDK fetch + 39 rs-drive count + 51 drive-abci v1 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tted, |In|=100 boundary accepted Two real-proof tests catch behaviors the existing mocks/sum-only tests miss, and one of them surfaced a misleading docstring contract that's fixed here. 1. `test_point_lookup_proof_omits_absent_in_branches_from_entries` — queries `age IN [30, 40, 99, 50]` against `byAge` (99 absent), gets real proof bytes, and asserts the verifier emits 3 entries, not 4. The `point_lookup_count_path_query` doesn't set `absence_proofs_for_non_existing_searched_keys: true`, so grovedb's `verify_query` silently omits absent-Key branches from the elements stream rather than surfacing them as `(path, key, None)`. This makes the test the semantic anchor for any future flip of that flag — the failure message tells the next maintainer exactly what to do. 2. `test_count_query_in_operator_accepts_max_sized_array` — pins the `|In| = 100` boundary on `WhereClause::in_values()`'s cap. Existing tests cover < 100 (happy) and 101 (rejection); off-by-one in the cap would silently reject all max-sized queries while passing every smaller test. Asserts per-entry `Some(1)` not just the sum, so a verifier bug that splits counts unevenly across branches fails loudly. The first test exposed that the verifier's docstring claim "grovedb already enumerates [missing keys]" and `SplitCountEntry::count`'s `None`-emission claim were both aspirational — `None` is reserved for a future absence-proof variant and isn't produced today. Updated docstrings across rs-drive (verifier + struct), rs-drive-proof-verifier (public API), and rs-sdk (count_proof_helpers + document_split_counts) to describe the actual contract: present branches surface as `Some(n)` entries; absent branches are omitted; callers diff the In array against returned `key`s to detect "queried but absent." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stContext destructure, extract v1 tests file
Three review-driven cleanups, no behavior change:
**Drop stale `removed in v1` comments from platform.proto.** The two
comments documenting that `getDocumentsCount` (rpc) and
`GetDocumentsCountRequest`/`GetDocumentsCountResponse` (messages) were
removed in v1 served the in-flight reviewer; once this PR lands they're
just commit-archeology noise that any future reader can recover from
git log. The removal is structurally evident from the absence of the
RPC and message names.
**Replace `.unwrap()` on `CostContext` with explicit destructure (5
sites).** `CostContext::unwrap()` is infallible (it discards the cost
field, not panic-on-None), so there was no DoS risk, but the visual
pattern collides with `Option/Result::unwrap` and reviewers (rightly)
flag every one. Replaced with the canonical
`let CostContext { value, cost: _ } = …` pattern the codebase already
uses in `grove_get_proved_path_query_v0`. Sites:
- `execute_range_count.rs`: In fan-out, flat summed,
`execute_aggregate_count_with_proof`, `execute_distinct_count_with_proof`
- `execute_point_lookup.rs`: `execute_point_lookup_count_with_proof`
The destructure documents intent (we're deliberately discarding cost
because the per-mode dispatcher in `drive_dispatcher` wraps these
executors with its own fee accounting) and doesn't read as a panic-
prone unwrap to a future reviewer.
**Extract v1 `getDocuments` tests into `v1/tests.rs`.** The inline test
modules in `query/document_query/v1/mod.rs` had grown to ~1250 lines
across two `#[cfg(test)]` blocks (validate-and-route routing tests +
the full ported v0-count integration suite), pushing the file to 1827
lines and burying the 569-line production handler. Moved both blocks
into `v1/tests.rs` as a single `mod tests` with `ported_v0_count_tests`
preserved as a nested submodule. The nested module's `use super::*;`
becomes `use super::super::*;` so it still resolves to `v1/mod.rs`
items (since `super` now refers to `tests.rs`'s top level, not v1).
`mod.rs` shrinks from 1827 → 572 LoC (production-only); tests.rs is
1255 LoC.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…item lint errors Pre-existing CI failure: every macOS Tests run on this branch since the first PR commit has been failing the `dapi-grpc` build with 9 `error: doc list item without indentation` errors at lines 1508-1525 of the generated `org.dash.platform.dapi.v0.rs` (server-side codegen). Root cause: prost-build converts proto `//` line comments to `///` doc comments and strips common leading whitespace. The multi-line markdown bullets in `GetDocumentsRequestV1`'s "Phase 1 supported shapes" section relied on a 7-space continuation indent to make each second line visually attached to its parent `-` bullet — but the prost-stripped output landed both lines at the same column. rustdoc then flagged each continuation as a top-level `///` line that broke out of the list context, which fails the build under the CI's all-features compile path (likely via `cargo llvm-cov nextest` doctest extraction; doesn't reproduce on the local `cargo build`-only path). Fix: every bullet in the supported/rejected shape tables now fits on a single source line. Comment lines wrap freely at the paragraph level (one bullet per `//` line, no continuation lines under bullets). Generated `dapi_grpc/platform/server/org.dash.platform.dapi.v0.rs` lines 1508-1525 now have no continuation `///` lines under any list item — rustdoc accepts the doc comments cleanly. Also corrected the absent-In-branch contract paragraph at the same spot to match the actual verifier behavior (which my last commit's new test, `test_point_lookup_proof_omits_absent_in_branches_from_entries`, pins). The proto comment previously claimed grovedb's `verify_query` enumerates every queried key as `(path, key, Option<Element>)` triples including `None` for absent ones — but the current `point_lookup_count_path_query` doesn't set `absence_proofs_for_non_existing_searched_keys: true`, so grovedb silently omits absent keys from the stream. Callers detect "queried but absent" by diffing their request's In array against the returned entries' `key` field. Same correction I applied to rs-drive / rs-drive-proof-verifier / rs-sdk docstrings in 396d9f3, now mirrored onto the wire-format source of truth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…0) uniformity, FFI limit decode Three independent review findings from thepastaclaw / coderabbitai addressed together because the proto contract change in #2 is the single source of truth for both server (#2) and client (#4). **#1: Unknown `Select` enum discriminant** classified as `QueryError::InvalidArgument` instead of `not_yet_implemented`. `Select::try_from(42)` is structurally malformed wire input — there's no future protocol value that would make `42` a valid `Select` behavior, so the future-capability error class (with its "valid request structure, callers can keep it unchanged when capability lands" contract from `not_yet_implemented`'s docstring) is the wrong class. New test `reject_unknown_select_enum_value_as_invalid_argument` pins the discriminator so a future refactor that re-collapses the two error classes for "consistency" fails loudly. **#2: `limit: Some(0)` uniformly invalid across SELECT modes.** Pre-fix, three legacy behaviors collided on the same wire value: - `SELECT DOCUMENTS` `unwrap_or(0)` and forwarded to v0, where `limit=0` is "use server default" (accept-as-default). - `SELECT COUNT, {Aggregate, GroupByIn}` rejected via `is_some()` with mode-specific message (reject-as-invalid). - `SELECT COUNT, {GroupByRange, GroupByCompound}` passed `Some(0)` to drive (accept-as-zero). Three semantics for the same wire bytes is bad contract. The v1 wire's whole point of switching to `optional uint32` was to make "unset" explicit (`None`), so `Some(0)` only makes sense as an *explicit* zero — structurally meaningless regardless of mode. Centralized `limit == Some(0)` rejection at the top of `validate_and_route`; the existing per-mode `is_some()` checks still catch `Some(N>0)` correctly. Updated the documents-path `unwrap_or(0)` comment to note `Some(0)` can't reach it. Proto docstring on `optional uint32 limit` calls out the cross-mode contract explicitly. New test `reject_limit_some_zero_uniformly_across_select_modes` exercises all 5 mode combinations and asserts the centralized message fires. **#4: FFI `dash_sdk_document_count` limit decode matches docs.** Pre-fix doc said `-1` = unset, `≥0` = explicit cap. Implementation mapped every negative value to `0` (SDK's unset sentinel), and `0` was also the SDK's unset sentinel — so `-1`, `-2`, `-100`, AND `0` all silently meant "use server default", masking caller bugs from uninitialized memory / arithmetic underflow / etc. Extracted the decode into `decode_ffi_limit(i64) -> Result<u32, _>` so it's unit-testable in isolation. New contract is single-valued per input: `-1` → unset sentinel; `> 0` → explicit cap; `0` and `< -1` → rejected at the FFI boundary with messages directing callers to valid alternatives. Server-side #2 rejection happens anyway, but surfacing the rejection at the FFI is faster and mode-independent. 5 new tests cover each sentinel category (minus_one_is_unset, zero_is_rejected, negative_other_than_minus_one_is_rejected, positive_decodes_verbatim, over_u32_max_is_rejected). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's macOS Tests job runs \`cargo clippy --workspace --all-features --locked -- --no-deps -D warnings\` before the test step. Every Tests run on this branch has been failing on clippy errors — partly from this PR's own code, partly from pre-existing issues in unrelated crates whose fix had landed on \`v3.1-dev\` after our branch's base (unrelated-histories git state prevents a clean merge). **This PR's code** (two findings introduced by the count surface): - \`rs-sdk/src/mock/requests.rs\`: extracted the \`(Option<Vec<u8>>, Vec<u8>, Option<u64>)\` triple type into a module-level \`DocumentSplitCountTriples\` alias used by both \`mock_serialize\`/\`mock_deserialize\` (clippy::type_complexity). - \`rs-sdk/src/platform/documents/count_proof_helpers.rs\`: dropped the explicit \`'a\` lifetime on \`verify_count_query\` since elision handles it (clippy::needless_lifetimes). **Drive-by fixes** for pre-existing clippy errors that blocked CI on every recent run (matching the fixes landed on \`v3.1-dev\` via #3638 — applied directly because cherry-pick / merge fails on unrelated histories): - \`rs-platform-wallet-ffi/src/tokens/group_info.rs\`: replaced \`matches!(result, Err(_))\` with \`result.is_err()\` (×2 sites; clippy::redundant_pattern_matching). - \`rs-platform-wallet-ffi/tests/integration_tests.rs\`: dropped the stale \`std::ptr::null()\` arg from \`platform_wallet_info_create_from_mnemonic\` test calls (×2 sites; the function takes 3 args but tests were passing 4 — E0061). - \`rs-dpp/src/withdrawal/mod.rs\`: dropped the unnecessary \`&\` on \`Wrap(variant)\` argument to \`bincode::serde::encode_to_vec\` (clippy::needless_borrows_for_generic_args). \`Pooling\` is \`Copy\`, so \`Wrap(variant)\` is movable inline. Workspace clippy now clean under \`-D warnings\`: \`RUSTFLAGS="-D warnings" cargo clippy --workspace --all-features --locked --tests\` → \`Finished\`. All 41 drive count-query tests + 27 drive-abci v1 tests + 5 rs-sdk-ffi limit-decode tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ntable terminators
For rangeCountable indexes the terminator's value tree IS a CountTree
— its `count_value_or_default()` already equals the per-branch doc
count, because continuation property-name subtrees beneath are wrapped
`Element::NonCounted` and don't pollute the value tree's count
(see `add_indices_for_index_level_for_contract_operations_v0`'s
docstring). The point-lookup count proof's legacy
`base_path + Key([0])` shape was correct for normal `countable: true`
indexes (NormalTree value trees with `[0]`-child CountTrees) but ran
one merk layer too deep on rangeCountable, inflating proof bytes
linearly with the number of resolved branches.
This change adapts `point_lookup_count_path_query` (the single source
of truth shared by the prover and the SDK verifier) to target the
terminator's value tree directly when `self.index.range_countable`:
- **Equal-only**: pop the trailing `last_value` off `base_path` and
use it as the query's `Key(last_value)`. Path now ends at
`[..., last_field]`; resolved element is the value-tree CountTree.
- **In on terminator**: outer `Key(in_value)` items resolve directly
to value-tree CountTrees; no subquery is set. grovedb returns one
element per matched outer Key without descending another layer.
- **In + trailing Equals (terminator is a trailing Equal)**: hoist
the trailing `termval` from `subquery_path_extension` into the
subquery's `Key(termval)`. `subquery_path` ends at the terminator's
property-name segment only.
Normal `countable: true` (NOT rangeCountable) keeps `Key([0])`
unchanged — load-bearing because `Element::count_value_or_default()`
returns 1 for `NormalTree`, so applying this optimization there would
silently break counts.
Verifier (`verify_point_lookup_count_proof_v0`) updates only its
per-branch key extraction: when `path.len() == base_path_len`
(reachable only via the rangeCountable In-on-terminator shape), the
In value lives in `grove_key` instead of `path[base_path_len]`. The
shared builder guarantees byte-identical `PathQuery` reconstruction
across prover and verifier — no merk-root mismatch risk.
Tests in `range_countable_point_lookup_tests` (new submodule):
- `equal_only_rangecountable_path_query_targets_value_tree_directly`
— asserts path ends at `[..., "brand"]`, query item is
`Key(serialize("acme"))`, no subquery; end-to-end count agreement
between no-proof and prove.
- `in_on_rangecountable_terminator_path_query_has_no_subquery` —
asserts no subquery set, outer Keys are the serialized In values;
verifier demuxes back via `grove_key`. Absent In branches still
silently omitted (semantic preserved).
- `compound_in_prefix_plus_trailing_equal_on_rangecountable_terminator`
— compound `byBrandColor`: asserts `subquery_path = ["color"]`
(name only, no value) and subquery's `Key` is the serialized
terminator value. End-to-end count check.
- `normal_countable_path_query_still_targets_zero_child` —
regression for a non-rangeCountable `byCategory`: asserts the
legacy `Key([0])` selector is preserved (the inverse pin — a
regression that accidentally generalized the optimization would
fail loudly here, not silently mis-count via NormalTree's
`count_value_or_default() = 1`).
All 45 tests in `query::drive_document_count_query::tests::` pass
(41 pre-existing + 4 new). Drive-abci v1 (27 tests) and rs-sdk-ffi
count (5 tests) suites unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…g, default 100k rows Three complementary additions to the document-count worst-case bench: 1. **`group_by_color_in_proof_100_rangecountable_branches`** — new bench shape that targets the existing `byColor` index (1-property, `rangeCountable: true`) with `color IN(100)` and `prove=true`. Pairs with the existing `byBrand` variant (`group_by_in_proof_100_count_tree_branches`) which uses the non-rangeCountable `byBrand` index, so the two together surface the byte-savings delta of the rangeCountable point-lookup optimization (this PR's `perf` commit). Reuses the fixture's 100-color prefix from `color_label(0..100)` so all branches are *present* in the merk tree — absent branches are silently omitted from the verified-elements stream and would trivially shrink the proof without exercising the optimization. 2. **One-shot proof-size logging** — `report_proof_sizes` runs each proof-emitting shape once at bench setup and prints `[proof-size] rows=N <shape>: <bytes>` lines to stderr. Criterion measures wall-clock time; for count-proof work the load-bearing number is bytes-per-proof (smaller proofs save network/disk linearly with the number of resolved branches, while per-request wall-clock is dominated by setup and merk- traversal CPU). Surfacing the byte size directly removes the need to wire `proof.len()` into criterion's HTML output or re-run with ad-hoc instrumentation when reviewing a perf change. 3. **`DEFAULT_ROW_COUNT: 2_000_000 → 100_000`** — the 100k fixture takes ~1 minute to build vs. the 2M fixture's ~20+ minutes, while still being large enough that the per-branch merk paths exercise multiple tree layers. Callers who want the heavier worst-case run can set `DASH_PLATFORM_COUNT_BENCH_ROWS=2000000` on the command line; the default now matches the routine- development case rather than the worst-case-baseline case. Measured impact of the rangeCountable optimization on the new shape (`group_by_color_in_proof_100_rangecountable_branches`, 100k fixture): - Pre-optimization (HEAD~1, normal `[0]`-child descent): **20,212 bytes**. - Post-optimization (HEAD, value-tree-direct target): **10,512 bytes** — −9,700 B, **−48%**. The non-rangeCountable control (`group_by_in_proof_100_count_tree_branches`, byBrand) is unchanged at 22,438 bytes before and after the optimization, confirming the optimization is correctly gated on `Index::range_countable` and doesn't leak to indexes whose value trees are `NormalTree`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prints `[matrix]` lines reporting drive-level no-proof + prove outcomes for every meaningful combination of `group_by` (over the contract's `[brand, color]` properties) and where-clause shape, with each case annotated with the platform-level `validate_and_route` verdict. Useful for understanding which combinations the v1 dispatcher accepts, which the drive dispatcher accepts but the platform layer rejects (drive's CountMode mapping is more permissive than the platform's `group_by-field ↔ In/range-field` alignment check), and what proof bytes each accepted shape emits on the current fixture. Runs once at bench setup. Output is grep-able via `[matrix]` — each case spans four lines (label + no-proof + prove + platform). 19 cases cover: - `group_by = []` (8 where shapes — total, single-prop ==, In, range, compound) - `group_by = [color]` (4 shapes — In, range, `==`, In+range) - `group_by = [brand]` (4 shapes — In, In+`==`, In+range, `==`) - `group_by = [brand, color]` (2 shapes — `(In, range)` and `(In, ==)`) - `group_by = [color, brand]` (1 shape — illustrates picker rejection when no rangeCountable index has `brand` as terminator) Notable findings the matrix surfaces: - The rangeCountable optimization activates for `[color] / color IN[2]` and `[color] / color IN[100]`; with the 2-value test set the per-branch savings are visible but small, but scale linearly with In array length (the 100-value variant measured at 10,512 B post-opt vs 20,212 B pre-opt). - Some combinations the drive layer accepts (e.g. `[brand] / brand==X`, `[color] / color==X`) are rejected at the platform layer because the `group_by` field isn't constrained by `In` or range — surfaced as the "Aggregate(_) but platform: no" rows. - `[brand] / brand IN AND color > floor` is the most subtle case: drive returns `Entries(len=1)` (semantically wrong — caller expected per-brand entries), while the platform layer correctly rejects it with `single-field GROUP BY when both In and range clauses are present`. The matrix shows both verdicts so the rejection's value (vs silent shape mismatch) is concrete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
PR 1 of 3 in the v1 unification track. Wire-format change adding a SQL-shaped `GetDocumentsRequestV1` that unifies `getDocuments` and `getDocumentsCount` under a single request type with typed `select`, `group_by`, and `having` clauses. After all three PRs land and a deprecation cycle, callers can drop their dual-endpoint plumbing in favor of one unified surface.
This PR is pure rewiring — no new server-side execution capability. Every supported request shape translates to an existing v0 (`query_documents_v0`) or v0-count (`query_documents_count_v0`) handler invocation and produces byte-identical proof bytes / response data. The SDK-side wiring (PR 2) and FFI/wasm/Swift surface (PR 3) ship separately.
What was done?
Wire format (`platform.proto`):
Dispatcher (`drive-abci/src/query/document_query/v1/mod.rs`):
Rejection table (the only "new" logic in v1):
Platform-version: `document_query.max_version` bumped to 1; default stays at 0. v0 callers unchanged.
Supported routing:
How Has This Been Tested?
12 new tests in `query::document_query::v1::tests`:
Existing v0 (27 tests) and v0-count (9 tests) suites unaffected — both continue to pass unchanged.
Breaking Changes
None. v0 callers continue to work; v1 callers opt in via the request's `version` oneof. The dual-endpoint shape stays alive during the deprecation cycle.
Out of scope (tracked as follow-ups)
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Breaking Changes