feat(platform): getDocuments v1 — SQL-shaped select + count surface (1/3)#3633
Open
QuantumExplorer wants to merge 50 commits into
Open
feat(platform): getDocuments v1 — SQL-shaped select + count surface (1/3)#3633QuantumExplorer wants to merge 50 commits into
QuantumExplorer wants to merge 50 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>
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>
Emits `[proof-bytes]` lines for each of the seven Aggregate proof
shapes — `(empty)`, `field == X` (×3), `field IN [...]` (×2),
and `range_field > floor`. Each case prints the proof's byte
length and a hex view formatted 32 bytes per row so reviewers
can inspect the actual grovedb merk-proof bytes the dispatcher
signs without rebuilding a separate verifier harness.
Useful for understanding which primitive the drive dispatcher
routes to per shape (every case shares the same ~320-byte
preamble that descends from the GroveDB root through
`[DataContractDocuments, contract_id, 1, "widget"]`; the
per-shape divergence starts after that) and for visually
correlating proof sizes with the structural differences:
- `(empty)` (585 B): doctype-level CountTree proof; just the
primary-key fast-path descent.
- `brand == X` (1,165 B): byBrand point-lookup → trailing
`[..., "brand", "brand_050"]` + `Key([0])`.
- `color == X` (1,327 B): byColor rangeCountable point-lookup
→ trailing `[..., "color"]` + `Key("color_00000500")`
(post-optimization shape, no `[0]` descent).
- `brand == X AND color == Y` (1,907 B): byBrandColor
compound rangeCountable point-lookup.
- `brand IN[2]` (1,350 B): two parallel byBrand branches.
- `color IN[2]` (1,381 B): two parallel byColor branches
(rangeCountable).
- `color > floor` (2,072 B): AggregateCountOnRange — distinct
primitive byte layout (range boundaries + signed u64).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed-element view
Hex bytes aren't legible for understanding what a count proof
actually proves — most of the bytes are merk hashes and per-node
metadata. Swap the hex dump for a structured display that, for
each `group_by = []` case:
1. Prints the **path query** (the prover-side spec): the path
segments, the outer query items, and any subquery_path /
subquery items, with byte segments decoded as quoted UTF-8
when printable and hex otherwise.
2. Reconstructs the **same `PathQuery`** the prover used by
calling the appropriate builder on `DriveDocumentCountQuery`
(the single source of truth shared by prover + verifier).
For point-lookup and primary-key shapes this routes through
`point_lookup_count_path_query` / `primary_key_count_tree_path_query`;
for the range-aggregate primitive it routes through
`aggregate_count_path_query`.
3. Runs the matching grovedb verifier (`verify_query` for
point-lookup / primary-key; `verify_aggregate_count_query` for
the range-aggregate primitive) and prints the **verified
payload**: the merk root hash plus, depending on shape,
either the verified element list (path, key, CountTree's
`count_value_or_default`) or the single aggregated count.
The output makes the rangeCountable optimization (this PR's
`perf` commit) visible at a glance: the byColor / byBrandColor
cases show `subquery items: (none)` and the outer key holding
the serialized terminator value (or, for the compound case, a
path that stops one segment short of the legacy `[0]` descent),
while the byBrand case still shows `subquery items: [Key(0x00)]`
(the unchanged normal-countable selector).
Sample output (100k-row fixture):
[proof] [] / where=color==X (1327 bytes)
[proof] path: ["@", 0x...id, 0x01, "widget", "color"]
[proof] query items: [Key("color_00000500")]
[proof] verified:
[proof] elements (1):
[proof] path: ["@", ..., "widget", "color"]
[proof] key: "color_00000500"
[proof] element: CountTree { count_value_or_default: 100 }
Compared to the byBrand control on the same fixture:
[proof] [] / where=brand==X (1165 bytes)
[proof] path: ["@", ..., "widget", "brand", "brand_050"]
[proof] query items: [Key(0x00)] ← `[0]` descent (legacy shape)
[proof] verified:
[proof] element: CountTree { count_value_or_default: 1000 }
Helpers added: `display_segment` (UTF-8/hex auto-detection),
`display_query_items` (all `QueryItem` variants), `display_element`
(shows `count_value_or_default` for count proofs), `hex_bytes`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "8dda08930bb98e8425630351cb198b74f84d3f5d95ecf42a2082091d9a753b6c"
)Xcode manual integration:
|
Add explicit match arms for `Element::ProvableCountTree` and
`Element::ProvableCountSumTree` (previously fell through the `_`
catch-all and would have been mislabelled as "(other)"), and
append the element's full `Debug` to the per-case output so the
variant tag is unambiguous on inspection.
This addresses a reviewer question of the form "isn't this a
ProvableCountTree?" — the existing output read e.g.
`CountTree { count_value_or_default: 100000 }` for the `(empty)`
case, and the variant choice (`CountTree` vs `ProvableCountTree`)
matters because:
- The bench fixture's widget doctype has only
`documentsCountable: true` set at the document-type level
(not `rangeCountable: true`); per
`DocumentTypeRef::primary_key_tree_type`, that rule maps to
`TreeType::CountTree`. The doctype's primary-key tree at
`[..., "widget", 0]` is therefore inserted as
`Element::CountTree`, not `Element::ProvableCountTree`. Setting
`rangeCountable: true` at the doctype root level would flip
that.
- The per-index `rangeCountable: true` flag on byColor /
byBrandColor makes only those specific index trees use
`ProvableCountTree` at the property-name level (e.g.
`widget/color`) and `CountTree` at the value-tree level (e.g.
`widget/color/color_00000500`). For the seven `group_by = []`
proof cases dumped here, the verified element is always the
leaf count-bearing tree, which is `Element::CountTree` in
every case; the property-name `ProvableCountTree` is walked
over (its merk-path is part of the proof) but it isn't
*returned* as a verified element — instead `verify_query`
emits the leaves the path query asked for.
The full `Debug` output now confirms this directly: every
case shows `count_tree: [hex: …] N`, the serialized form of
`Element::CountTree`. A `ProvableCountTree` would show
`provable_count_tree: …`.
The `color > floor` case is also unchanged: it uses
`verify_aggregate_count_query` (not `verify_query`), which
returns just `(root_hash, count)` — the `ProvableCountTree` at
`widget/color` is the *source* the aggregate primitive sums
over, but the verifier doesn't expose individual elements.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rminators
Empirically surfaces the structural reason the rangeCountable
proof optimization can't be applied to non-rangeCountable
indexes: the optimization stops the descent at the property-name
level and uses `Key(serialized_value)` as the selector, which
means the *verified element* IS whatever's stored at the value
key. For that to yield the correct doc count, that element must
be a count-bearing variant (CountTree / ProvableCountTree). For
non-rangeCountable indexes it's `Element::Tree` (NormalTree),
whose `count_value_or_default()` returns `1` regardless of how
many docs sit underneath.
The new `probe_value_tree_types` directly inspects what grovedb
has stored at the two single-property index terminators:
[probe] byBrand: widget/brand/brand_050 →
Tree (NormalTree) { count_value_or_default: 1,
debug: tree: [hex: 636f6c6f72, str: color][...] }
[probe] byColor: widget/color/color_00000500 →
CountTree { count_value_or_default: 100,
debug: count_tree: [hex: 00, str: ] 100[...] }
The byBrand NormalTree's stored value is `"color"` — a pointer
to the byBrandColor continuation child living under the same
value tree. Because the parent is a NormalTree, the continuation
child contributes the default `1` to the parent's
`count_value_or_default()` rather than its own count, which is
exactly why the prove-count path has to descend one more layer
to the `[0]` CountTree to find the real doc count for byBrand.
For byColor (rangeCountable), the value tree is itself a
CountTree with count = 100; the byBrandColor continuation child
beneath is wrapped `NonCounted` so it doesn't pollute the
parent's count. That's what makes the
`path=[..., "color"], Key("color_*")` shape sound for byColor
and unsound for byBrand.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rminators
The point-lookup count proof's value-tree-direct optimization
(previous commit `d9b3d3078f`) was gated on `Index::range_countable`,
which made it activate only for indexes that ALSO opted into
`AggregateCountOnRange` support. For plain `countable: true` indexes
(the common case — e.g. `byBrand` in the bench contract), the
optimization sat on the table while proofs still descended through
a redundant `[0]` merk layer per resolved branch.
This commit generalizes the rule so every countable terminator's
value tree is a `CountTree` (with sibling continuations wrapped
`NonCounted` so they don't pollute the parent count). The
optimization now activates uniformly for any
`Countable | CountableAllowingOffset` index — `range_countable` is
preserved strictly for the orthogonal `AggregateCountOnRange`
property-name-tree upgrade (`NormalTree` → `ProvableCountTree`).
## Insertion-side changes
`add_indices_for_index_level_for_contract_operations_v0` and
`add_indices_for_top_index_level_for_contract_operations_v0`:
- Split the single `sub_level_range_countable` flag into two:
- `sub_level_is_countable_terminator` (any countability tier) —
drives the value-tree type (`CountTree` vs `NormalTree`).
- `sub_level_range_countable` (range-aggregate opt-in only) —
drives the property-name tree type (`ProvableCountTree` vs
`NormalTree`).
- Pure prefix levels (e.g. `brand` in a contract with only
`[brand, color]` and no standalone `[brand]` index) stay
`NormalTree`; there's no count surface to materialize at a
prefix level.
- Renamed the propagation flag `parent_value_tree_is_range_countable`
→ `parent_value_tree_is_count_tree` to match the new semantics —
the `NonCounted` continuation wrapping is now driven by "parent
value tree is a CountTree" (true for every countable terminator),
not by "range_countable specifically".
## Read-side changes
`DriveDocumentCountQuery::point_lookup_count_path_query` gates the
value-tree-direct shape on `self.index.countable.is_countable()`
(was: `self.index.range_countable`). The shape decision (path
stops one segment short of the legacy `[0]` descent; `Key(serialized_value)`
as selector; In-on-terminator skips the subquery) is unchanged.
`verify_point_lookup_count_proof_v0`'s docstring updated to reflect
the single uniform terminator shape; the in-value extraction logic
(`path.len() == base_path_len` ↔ grove_key carries In value, else
`path[base_path_len]`) is unchanged.
## Tests
- All 45 `query::drive_document_count_query::tests::` pass under
the new layout.
- All 41 `drive::contract::insert::insert_contract::` tests pass
(the rangeCountable e2e suite which pinned the
`ProvableCountTree` primary-key shape continues to work; that
invariant is independent of this change).
- `normal_countable_path_query_still_targets_zero_child` was an
inverse pin claiming non-rangeCountable countable indexes
*retain* the legacy `[0]` selector. That contract is now wrong —
every countable index uses the value-tree-direct shape. Replaced
with `plain_countable_path_query_targets_value_tree_directly`,
which now positively asserts the optimization activates uniformly
across countability tiers AND that `range_countable` is no longer
the gating axis.
## Measured impact (100k-row bench fixture, byBrand = plain countable)
| Shape | Before (legacy) | After (uniform) | Δ |
|--------------------------|-----------------|-----------------|------------|
| `brand == X` | 1,165 B | **1,041 B** | −124 (−11%) |
| `brand IN[2]` | 1,350 B | **1,102 B** | −248 (−18%) |
| `brand IN[100]` | 22,438 B | **10,038 B** | **−12,400 (−55%)** |
| `color IN[100]` (control)| 10,512 B | 10,512 B | 0 (already optimized) |
byBrand now matches byColor's compact shape on every point-lookup
shape — they're indistinguishable to the path-query builder beyond
their per-index merk depth.
## Bench fixture schema bump
`FIXTURE_SCHEMA_VERSION: 1 → 2` so cached `/tmp/dash-platform-document-count-bench-v1-rows-*`
directories from prior runs are rebuilt automatically. Old proofs
verified against the new code would fail (different element
variants at `widget/brand/<v>`).
## Safety: pre-v12 chain replay unaffected
Count indexes (any tier) are gated to `protocol_version >= 12` per
`packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs:356`.
Pre-v12 contracts never reach the countable-terminator branches in
the insertion code, so chain replay against the new code produces
the same merk roots as before for any block predating v12. Per
direct user confirmation, v12 has not been activated on mainnet
yet, so modifying `_v0` in place is safe rather than requiring a
versioned `_v1` migration path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iagrams New chapter under "Drive" that walks through the bench fixture's widget contract and shows what each count-query proof actually proves — both the path query the prover signs AND the verified element the verifier extracts. Every example is reproducible against the existing bench at `packages/rs-drive/benches/document_count_worst_case.rs`. Seven worked examples (the `group_by = []` proof surface): 1. `(empty)` — primary-key CountTree fast path, 585 B. 2. `brand == X` — PointLookup over byBrand (plain countable), 1,041 B post-v12 (the value-tree-direct shape now activates for any countable terminator, not just rangeCountable). 3. `color == X` — PointLookup over byColor (rangeCountable), 1,327 B. Same shape as #2 — the rangeCountable flag is only relevant for #7. 4. `brand == X AND color == Y` — PointLookup over byBrandColor; the proof descends through the byBrand value tree's `NonCounted`-wrapped `color` continuation to the byBrandColor terminator, 1,911 B. 5. `brand IN [b0, b1]` — outer Keys per In value, no subquery (the value-tree-direct shape on the In axis), 1,102 B. 6. `color IN [c0, c1]` — identical shape to #5, surfacing the v12 generalization at a glance: byBrand (plain countable) and byColor (rangeCountable) produce structurally identical proofs, 1,381 B. 7. `color > floor` — AggregateCountOnRange over byColor's ProvableCountTree; different verifier (`verify_aggregate_count_query`), single u64 result, 2,072 B. Each section has three parts: - **Path query** (decoded path + items + subquery), the prover's spec. - **Verified element / payload** (the structured output of `verify_query`/`verify_aggregate_count_query`). - **Mermaid diagram** with: - The tree element wrapper for the relevant GroveDB subtree. - Blue arrows tracing the descent. - A cyan target node for the verified element. - Faded nodes for context. A "GroveDB Layout" diagram up front shows the storage shape for the whole contract (doctype CountTree primary key, byBrand NormalTree property-name with CountTree value trees and NonCounted-wrapped byBrandColor continuations, byColor ProvableCountTree property-name with CountTree value trees). Each per-query diagram is a focused slice of this overview. An at-a-glance comparison table at the end summarizes primitive choice, verified shape, and proof size across all seven examples to make the structural symmetry between #2/#3 and #5/#6 (post-v12 generalization) and the asymmetry of #7 (AggregateCountOnRange) visually obvious. Registered under `Drive` in `SUMMARY.md` immediately after the existing `Document Count Trees` chapter, which it complements (that chapter explains the tree variants in the abstract; this one shows how queries traverse the resulting layout). Builds cleanly via `mdbook build` against the existing `mdbook-mermaid` preprocessor configuration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-05-15T03:05:38.917Z |
…xamples
Count indexes (any countability tier) are a v12 feature — they
didn't exist in earlier protocol versions, so there's no
"pre-v12 layout" to contrast with. Earlier framing in Query 2
("Pre-v12 this would have descended one more layer to Key(0x00)
under brand_050") implied byBrand had a different storage shape
in an earlier protocol version, which is wrong — that
`[0]`-child descent was a transient development iteration
during v12, never a deployed protocol behavior.
Rewrote the affected passages to describe the design statically
(every countable terminator's value tree is a CountTree;
`rangeCountable` is orthogonal, opt-in for `AggregateCountOnRange`
support only) without implying a pre/post-v12 axis:
- Contract section: dropped "from protocol v12 onward" before
the value-tree-CountTree claim; added a direct reference to
the insertion code so the rule's source is one click away.
- Query 2 ("brand == X"): replaced the pre-v12 contrast with a
static statement of why `brand_050` carries the count directly.
- GroveDB Layout section: changed "the rule generalizes to every
countability tier as of v12" to "the rule applies uniformly to
every countability tier".
- At-a-Glance: changed "The v12 generalization made the
value-tree-direct shape uniform" to "The value-tree-direct
shape is uniform across countability tiers".
No content / numbers / diagrams changed — only the prose framing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dex Example
Each Query section in Count Index Examples now contains the
literal output of the bench's `display_proofs` helper —
`path` + `query items` + optional `subquery items` on the
prover side, plus the verifier's `root_hash` + element list (or
aggregate `count: N` for Query 7) on the verified side. The
blocks are byte-for-byte what `cargo bench -p drive --bench
document_count_worst_case -- __nonexistent` prints to stderr
when run against the 100 000-row fixture, with only the
`[proof]` line prefix stripped.
Why this matters: the prior version of the chapter had hand-
trimmed "Path query" / "Verified element" summaries that
abstracted away the actual on-the-wire shape (no merk
`root_hash`, no `Element::Debug` content showing the
continuation pointer in `count_tree: [hex: 636f6c6f72, str:
color] 1000`, no `0x4ed22624…(32 bytes)` truncation marker for
the contract id). Showing the verbatim output makes:
- The `root_hash` reproducible — a reader can run the bench
and confirm they get the same hash, confirming layout +
insertion order.
- The `Element::Debug` content (`count_tree:` vs
`provable_count_tree:`, the value-slot hex/str pair, the
trailing count) visible — that's what proves the variant is
`Element::CountTree`, not `Element::ProvableCountTree`, on
every byBrand / byColor / byBrandColor terminator.
- The byBrand vs byColor continuation-pointer asymmetry
visible (`[hex: 636f6c6f72, str: color]` on byBrand, `[hex:
00, str: ]` on byColor) — which is what makes byBrand's
CountTree count correct despite the `NonCounted` wrapper
not being directly visible in `grove.get`'s output.
- The Query 7 verifier-divergence concrete: `verified:`
contains `count: 49900` (no `elements (N):` block), making
the `verify_aggregate_count_query` vs `verify_query` split
visible in the output, not just stated in prose.
Rewrote the "How To Read The Proofs" preamble to:
- Describe the new two-section structure (verbatim display +
diagram).
- Explain the `display_segment` rendering conventions: `"@"`
is the printable form of byte `0x40`
(`RootTree::DataContractDocuments`); `0x...(N bytes)`
truncation; `count_tree:` / `provable_count_tree:` variant
markers; the `[hex: …, str: …]` value-slot field.
- Reference the bench helper directly so the reproducibility
path ("re-run the bench") is one click away.
Per-section commentary tightened to call out exactly what the
displayed `debug:` field reveals — e.g. Query 2's continuation
pointer on byBrand's `brand_050` CountTree, Query 3's bare-`[0]`
value slot on byColor's leaf CountTree (no continuation since
byColor is single-property), Query 6's same-asymmetry note.
No diagrams changed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restored the chapter's previous concise structure (per-query Path query + Verified element + Diagram) and added a fourth section per query: the structured proof Display. This is the same view dash-evo-tool's Proof Log screen ([src/ui/ tools/proof_log_screen.rs in dash-evo-tool](https://github.com/ dashpay/dash-evo-tool/blob/master/src/ui/tools/proof_log_screen.rs)) shows when its display mode is set to "JSON" — bincode-decode the proof bytes into a `GroveDBProof`, then `format!("{}", proof)` runs the type's `Display` impl, which renders the layered merk-proof structure inside. To make this reproducible the bench's `display_proofs` helper gained a fifth step that decodes each query's proof bytes via `bincode::config::standard().with_big_endian().with_no_limit()` (matching dash-evo-tool's config) and prints `format!("{}", grovedb_proof)` indented under a `[proof] proof-display:` header. Grep `[proof]` from the bench's stderr after running it against the 100k-row fixture and you get the exact text inlined below in the chapter. Each per-query Proof Display block is wrapped in a `<details>` collapsible because the merk-path through 4-6 grovedb layers is long (Query 4 alone is 6 layers deep — the bench's deepest descent). Common upper layers (root → contract id → 0x01 prefix → widget doctype) are abbreviated `...` for the latter queries since they're identical to Query 1's full rendering; the bottom layers — where the actual count surface lives — are shown verbatim. The most informative blocks: - Query 2 (`brand == X`): visible at the bottom is `Push(KVValueHashFeatureTypeWithChildHash(brand_050, CountTree(636f6c6f72, 1000, flags: [0, 0, 0]), ...))` — the `636f6c6f72` value slot is the ASCII for `"color"` (the byBrandColor continuation pointer), `NonCounted`-wrapped at the storage layer so it doesn't pollute the parent count. - Query 3 (`color == X`): bottom layer carries `KVHashCount( HASH[...], N)` ops with the per-subtree counts (51 100 + 25 500 + 12 700 + 6 300 + 3 100 + 700 + 300 + ...) summing to the 100 000 doc total — `ProvableCountTree`'s count-at-every-node property visible in the proof itself. - Query 4 (`brand == X AND color == Y`): 6 layers deep, showing the descent through byBrand's value tree (which carries `CountTree(636f6c6f72, 1000, ...)` as an intermediate stop) into the byBrandColor continuation and finally to `CountTree(00, 1, ...)` — the single doc per `(brand, color)` pair the bench's deterministic schedule guarantees. - Query 7 (`color > floor`): uses entirely different bottom- layer ops than the other six — `HashWithCount(kv_hash, left, right, count)` and `KVDigestCount(key, kv_hash, count)` — showing the `AggregateCountOnRange` boundary walk over the byColor `ProvableCountTree`. The verified payload is the summed `count: 49900` rather than an element list, and no individual `CountTree(…)` appears anywhere — the running totals inside the boundary ops *are* the proof's count surface. The "How To Read The Proofs" preamble now describes the four sections and links the dash-evo-tool reference. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-by, visualizer links Chapter 29 (Count Index Examples) additions: - Query 8 section (brand == X AND color > floor) — verbatim proof, conceptual flowchart, per-layer Layer-5+ diagram, Layer-8 binary merk-tree diagram - Hash composition worked example — Blake3 formulas + step-by-step recomposition of color_00000511's node_hash from its KVDigestCount op and HashWithCount right child - Top navigation table with Filter, Complexity, Avg time, Proof size columns - Per-query GroveDB Proof Visualizer links on every Q1-Q8 details summary Chapter 30 (Count Index Group By Examples) — new chapter: - "When group_by changes the proof (and when it doesn't)" framing — group_by as both result-shaping (SDK) and proof-shaping (prover) directive - Six G* sections (G1-G6) following the Q-section template - "Group-By Shapes That Are Not Allowed" — four buckets with reasons, including the brand IN + color > floor + group_by=[brand] aggregate-style case marked as "incoming" pending grovedb's AggregateCountOnRange-as-subquery extension - Per-query GroveDB Proof Visualizer links on every G* section Bench (packages/rs-drive/benches/document_count_worst_case.rs): - query_1 through query_8 criterion bench_function calls (per-query timings) - query_g1 through query_g6 criterion bench_function calls - display_group_by_proofs helper that emits [gproof]-tagged verbatim merk proofs for G3-G6 - Matrix case for brand == X AND color > floor (Aggregate prove path, 2656 B) Visualizer links use the gzip+base64url URL fragment scheme documented at https://github.com/dashpay/grovedb-proof-visualizer-widget/blob/master/prompts/link-from-platform-book.md All 14 proofs verified to round-trip byte-identical through the encode pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…+ bench probe Pulls in dashpay/grovedb#663 ("feat(grovedb,query): allow AggregateCountOnRange as carrier subquery"), merge commit 87554188ad — bumps every pinned `grovedb*` dep across the workspace from a917d92d to 87554188. The new grovedb APIs: - `Query::validate_carrier_aggregate_count_on_range` - `Query::validate_leaf_aggregate_count_on_range` - `GroveDb::query_aggregate_count_per_key` - `GroveDb::verify_aggregate_count_query_per_key` These unblock the chapter 30 G7 shape (`brand IN[...] AND color > floor` with `group_by = [brand]`) at the grovedb layer. Drive wire-up is the next step (lift mode_detection.rs:140 rejection + new path-query builder + new DocumentCountMode variant + per-key verifier wrapper). This commit also adds `probe_carrier_acor` to document_count_worst_case bench as a feasibility smoke test against the existing 100 000-row widget fixture: [carrier-acor] no-proof entries (2): ("brand_000", 499) ("brand_001", 499) [carrier-acor] proof bytes: 4332 B [carrier-acor] verified root_hash: 0x62ee7348f4d28dd9d7cf86a6c725fa8276... [carrier-acor] verified entries (2): ("brand_000", 499) ("brand_001", 499) 4 332 B for k=2, matching the complexity estimate documented in chapter 30 (`O(k · (log B + log C'))`) and ~17 % smaller than the concatenated-leaf-ACOR alternative. Verification: - cargo check --workspace clean - cargo test -p drive --lib drive_document_count_query → 45 tests passing - cargo test -p drive --lib verify → 240 tests passing - All chapter 29 / chapter 30 proof sizes (585 / 1041 / 1327 / 1911 / 1102 / 1381 / 2072 / 2656 B for Q1..Q8) unchanged — bump is purely additive - Carrier-ACOR end-to-end: query → prove → verify all round-trip with root hash matching the chapter's known 0x62ee7348... fixture root Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r range) Lifts the `mode_detection.rs:140` rejection for the `GroupByIn + In + range + prove` case, now that grovedb PR #663 exposes `AggregateCountOnRange` as a carrier subquery under outer `Keys`. The new shape returns one `(in_key, u64)` per resolved In branch in a single proof: brand IN ["brand_000", "brand_001"] AND color > "color_00000500" group_by = [brand] → Entries([("brand_000", 499), ("brand_001", 499)]) Proof(4 332 bytes), median 255.9 µs Drive-side wire-up - New `DocumentCountMode::RangeAggregateCarrierProof` variant. - `mode_detection::detect_mode`: rejection lifted for `CountMode::GroupByIn` carrying `(In + range)`; routes to the new mode. Aggregate-mode `In + range + prove` still rejected (no group_by means caller asks for a single sum, which carrier-ACOR can't safely produce without verifier trust in the SDK's summation). - New `DriveDocumentCountQuery::carrier_aggregate_count_path_query` builder in `path_query.rs` — outer Keys per In value, subquery_path through the index's middle properties (`==` clauses) plus the terminator name, subquery is `Query::new_aggregate_count_on_range(range_item)`. Mirrors the In-fan-out pattern from `distinct_count_path_query` but with the carrier-ACOR subquery composition. - New executor `Drive::execute_document_count_range_aggregate_carrier_proof` in `executors/range_aggregate_carrier_proof.rs`, plus inner `DriveDocumentCountQuery::execute_carrier_aggregate_count_with_proof` in `execute_range_count.rs`. - New verifier `DriveDocumentCountQuery::verify_carrier_aggregate_count_proof` (`v0`) wrapping `GroveDb::verify_aggregate_count_query_per_key`. Returns `(RootHash, Vec<(Vec<u8>, u64)>)`. - Added the new method version to `DriveVerifyDocumentCountMethodVersions` (defaults to `0` in `v1.rs`). - Dispatcher arm in `drive_dispatcher.rs` matches the new mode and returns `DocumentCountResponse::Proof(...)`. Bench - Matrix entry `[brand] / where=brand IN[2] AND color > floor` flipped from rejected ("no — single-field GROUP BY with both `In` and range") to allowed ("yes (RangeAggregateCarrierProof — carrier ACOR per In branch)") with `prove: Proof(4 332 bytes)`. - New `query_g7_brand_in_color_gt_grouped_by_brand` criterion bench — 10 samples × 9 295 iters, median 255.87 µs (~4× Q8's 71 µs because it's two parallel Q8-shaped descents). - New `G7` case in `display_group_by_proofs` emits the full 186-line carrier-ACOR proof verbatim as `[gproof] G7 [brand] / where=...`. Chapter 30 (`book/src/drive/count-index-group-by-examples.md`) - G7 added to the navigation table (`O(k · (log B + log C'))`, 255.9 µs, 4 332 B, `Entries(2 groups, sum = 998)`). - New "G7 — Carrier `In` + Range, Grouped By `brand`" section with the same template as G1..G6: path query, verified payload, proof size, proof display (schematic + interactive visualizer link), narrative, conceptual flowchart, per-layer (Layer-5+) merk-tree diagram. - "Group-By Shapes That Are Not Allowed" section's bucket #4 removed (the "incoming" placeholder), replaced with a historical note pointing forward to G7. Tests + verification - `cargo test -p drive --lib drive_document_count_query` → 45 passing - `cargo test -p drive --lib verify` → 240 passing - All chapter 29 / chapter 30 documented proof sizes (Q1..Q8, G3..G6) unchanged — wire-up is purely additive on top of grovedb PR #663. - mdBook builds clean. Closes (in this PR): chapter 30 G7 placeholder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `probe_carrier_acor_range_outer` — the natural extension of
`probe_carrier_acor` from `outer In` to `outer Range`. Confirms the
grovedb feature works for our widget fixture:
[carrier-acor-range] probing: widget/brand RangeAfter(brand_050..)
subquery_path=color
subquery=AggregateCountOnRange(RangeAfter(color_00000500..))
[carrier-acor-range] no-proof entries (49):
("brand_051", 499) … ("brand_099", 499)
[carrier-acor-range] proof bytes: 84576 B
[carrier-acor-range] verified root_hash: 0x62ee7348… (matches chapter fixture)
[carrier-acor-range] verified entries (49):
("brand_051", 499) … ("brand_099", 499)
Findings worth recording:
1. The outer-Range carrier-ACOR shape WORKS at the grovedb layer
(grovedb PR #663's `validate_carrier_aggregate_count_accepts_range_outer_items`
covers this). The widget fixture's 49 brands > "brand_050" each
carry the expected per-brand count of 499 colors > "color_00000500"
(the bench's fixture has 1 doc per (brand, color) pair).
2. **`SizedQuery::limit` is rejected on any ACOR-bearing query**
(carrier or leaf) — grovedb's validator returns
`InvalidQuery("AggregateCountOnRange queries may not set SizedQuery::limit")`.
So "limit the outer Range walk to N matches" can't be expressed
today; this probe walks the full 49 brands and pays the resulting
~83 KB proof size. The natural drive-level workaround is to
compute an explicit upper bound for the outer Range from the
requested limit (e.g. rewrite `brand > X` with `limit = 20` to
`brand > X AND brand <= caller_supplied_or_precomputed_Y`) — but
that pushes the upper-bound responsibility to the caller or
requires an extra grovedb read.
3. Drive doesn't wire this through yet: `mode_detection::detect_mode`
rejects ≥2 range clauses up front ("count query supports at most
one range where-clause"), independent of the limit issue. The
shape is logged here as future work for chapter 30; lifting it
requires both the multi-range-rejection in mode_detection AND a
grovedb-level extension to support `SizedQuery::limit` on carrier
ACOR (or a drive-level upper-bound computation).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Targets grovedb PR #664 head (4e20c338) — the follow-up to PR #663 that relaxes the leaf-strict `SizedQuery::limit` rule for carrier ACOR queries. The new shape unblocks the `Q8`-with-outer-range case: brand > "brand_050" AND color > "color_00000500" group_by = [brand] limit = 20 → Entries([("brand_051", 499), …, ("brand_070", 499)]) Proof(35 122 bytes), median 1.07 ms The proof bytes scale linearly with `limit`: 20 outer matches × ~1 700 B per per-brand ACOR boundary walk ≈ 35 KB, matching the per-In slope established by Q8 vs G7. Drive-side wire-up extending the existing `RangeAggregateCarrierProof` mode: - `mode_detection`: lifts `range_count > 1` for the specific shape (CountMode::GroupByRange + prove + exactly two range clauses on distinct fields + no In). Routes to the existing `DocumentCountMode::RangeAggregateCarrierProof` arm. - `path_query::carrier_aggregate_count_path_query`: generalized to accept either an `In` clause (G7 shape) OR a range clause (G8 shape) at the carrier position. The terminator's range becomes the inner ACOR `QueryItem`. Builder now takes `limit: Option<u16>` and threads it to `SizedQuery::new`. - `execute_carrier_aggregate_count_with_proof`: accepts `limit`, passes through to the builder. - `Drive::execute_document_count_range_aggregate_carrier_proof`: accepts `limit`, plumbs through. - `drive_dispatcher`: extracts `request.limit`, validates against `max_query_limit` (same validate-don't-clamp policy as `RangeDistinctProof`), passes through. - `verify_carrier_aggregate_count_proof[_v0]`: accepts `limit`, rebuilds the same `PathQuery` byte-for-byte. - `index_picker::find_range_countable_index_for_where_clauses`: extended to accept the two-range case — finds an index whose first property carries one range (the outer carrier) and whose terminator carries the other (the inner ACOR). Single-range case is unchanged. - `drive_dispatcher::where_clauses_from_value`: catches the system- wide parser's `MultipleRangeClauses` error for count queries. Structural validation lives in `detect_mode`; the regular-query parser's "all ranges must be on same field" rule was rejecting the G8 shape upstream. Bench: - Matrix entry `[brand] / where=brand > floor AND color > floor (limit 20)` flipped from rejected to `Proof(35 122 bytes)`. - New `query_g8_brand_gt_color_gt_grouped_by_brand_limit_20` criterion bench — 10 samples × 5 060 iters, median 1.07 ms (~4× G7's 256 µs because the outer walk is 10× longer). - New `G8` case in `display_group_by_proofs`. - `probe_carrier_acor_range_outer` now uses `limit = 20` (was unbounded in the previous commit's feasibility probe). Proof shrinks from ~83 KB to 35 122 B. Chapter 30: - G8 row in the nav table (`O(L · (log B + log C'))`, 1 072 µs, 35 122 B, `Entries(20 groups, sum = 9 980)`). - New "G8 — Carrier outer Range + Range, Grouped By `brand`" section with the same template as G1..G7: path query, verified payload, schematic proof display + interactive visualizer link, conceptual flowchart, per-layer (Layer-5+) merk-tree diagram. - Complexity variables note extends `k` (|IN|) with `L` (the caller's `limit` for the Range-outer carrier shape). Tests + verification: - cargo test -p drive --lib drive_document_count_query → 45 passing - cargo test -p drive --lib verify → 240 passing - All previously documented proof sizes unchanged (Q1..Q8 / G3..G7 byte-identical to before) - G8 end-to-end via the dispatcher matches the standalone grovedb- level probe (35 122 B; root hash 0x62ee7348… matches chapter fixture) - mdBook builds clean The grovedb rev points at PR #664's open head (4e20c338); will be rebased to the merge commit once the PR lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…db to PR #664 merge Bumps grovedb to the merged PR #664 commit (7a649386) and adopts the platform-wide hardcoded outer-walk cap for G8's range+ACOR carrier shape: brand > "brand_050" AND color > "color_00000500" group_by = [brand] prove = true # caller MUST NOT pass `limit` → Entries([("brand_051", 499), …, ("brand_075", 499)]) Proof(43 638 bytes), median 1.26 ms The hardcoded constant `CARRIER_AGGREGATE_OUTER_RANGE_LIMIT = 25` lives in `query/drive_document_count_query/mod.rs` alongside `MAX_LIMIT_AS_FAILSAFE`. The dispatcher rejects any caller-supplied `limit` on this shape — the cap is part of the per-shape structural contract, not a runtime knob. Two reasons: 1. **Prover/verifier byte-for-byte agreement.** `SizedQuery::limit` feeds the merk-root reconstruction; if the caller could pick the value, the verifier would need a side channel to know which one was used. Hardcoding keeps proof bytes deterministic across callers. 2. **Proof-size bounding.** Linear in the cap (~1 700 B per outer match). 25 keeps the worst case under 50 KB (Tier-2 in the visualizer's shareable-link guidance) while covering typical "top-N brands by outer range" queries. Callers wanting fewer results narrow the where-clause range; callers wanting more results call repeatedly with disjoint outer-range windows. For the In-outer (G7) shape, the dispatcher also rejects any caller-supplied `limit` now — the In array's length already bounds the result, and accepting a sub-|In| limit would silently change which In-branches appear in the proof. Drive: - New constant `CARRIER_AGGREGATE_OUTER_RANGE_LIMIT: u16 = 25` in `drive_document_count_query/mod.rs`. - Dispatcher: split the `RangeAggregateCarrierProof` arm into the In-outer case (`limit` must be `None`) and the Range-outer case (`limit` must be `None` from the caller, hardcoded to 25 internally). Rejects caller-supplied `limit` on both with a clear error message. Bench: - `query_g8_brand_gt_color_gt_grouped_by_brand_limit_20` → `query_g8_brand_gt_color_gt_grouped_by_brand` (caller passes `None` now; the cap is platform-set). - Matrix entry label drops "(limit 20)"; verdict updated to mention the platform-cap. - `display_group_by_proofs` G8 case: caller passes `None`. - `probe_carrier_acor_range_outer` switched from `limit = 20` to `limit = 25` to match the platform's hardcoded value (the probe calls grovedb directly, so it sets the SizedQuery limit itself). Empirical numbers from the new bench run (100 000-row warmed fixture): - Q8 (brand == X AND color > floor) : 71 µs / 2 656 B - G7 (k=2) (brand IN[2] AND color > floor) : 256 µs / 4 332 B - G8 (L=25) (brand > X AND color > floor, group_by [brand]): 1.26 ms / 43 638 B Per-outer-match slope of ~1 700 B and ~50 µs holds across all three rows. Chapter 30: - G8 nav-table row updated to the new numbers (25 entries, 43 638 B, 1 260 µs, sum = 12 475). - New "Why the limit is hardcoded" subsection with the two-reason rationale. - All G8 prose, flowchart, and per-layer diagram references to "20" / "brand_070" / "9 980" / "35 122 B" replaced with "25" / "brand_075" / "12 475" / "43 638 B". - New visualizer link for the 25-entry proof (encoded payload size ≈ 56 KB — Tier-2; works in modern browsers, may truncate in some link-preview surfaces). - Complexity-variables note now describes `L` as "platform-wide outer-walk cap" rather than "caller's limit". Tests: - cargo test -p drive --lib → 3 141 passing - All previously documented proof sizes unchanged for Q1..Q8 / G1..G7 - mdBook build clean 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