feat(platform): getDocuments v1 — SQL-shaped select + count surface (1/3)#3633
Open
QuantumExplorer wants to merge 12 commits into
Open
feat(platform): getDocuments v1 — SQL-shaped select + count surface (1/3)#3633QuantumExplorer wants to merge 12 commits into
QuantumExplorer wants to merge 12 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
📝 WalkthroughWalkthroughThis PR removes the standalone getDocumentsCount RPC and folds counting into GetDocuments v1. It adds GetDocumentsRequestV1/GetDocumentsResponseV1 (select/group_by/having/start/limit/prove + result oneof counts/documents/proof), updates generated clients across languages, removes legacy count modules/handlers, adds a v1 document query handler that routes documents vs counts, and migrates 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
Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 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
Comment on lines
+696
to
+700
| // Pagination cursor. Valid for `select=DOCUMENTS` and for | ||
| // `select=COUNT` with non-empty `group_by` (paginate entries by | ||
| // the grouping field's serialized key). Rejected on | ||
| // `select=COUNT, group_by=[]` — no concept of "start" for a | ||
| // single aggregate. |
Contributor
There was a problem hiding this comment.
start_after/start_at docs are currently inconsistent with runtime behavior.
Line 696-Line 700 says start is valid for select=COUNT with non-empty group_by, but the v1 server handler currently rejects start for all SELECT COUNT shapes. Please align this doc block with the current rejection behavior to avoid client-side misimplementation.
🤖 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/protos/platform/v0/platform.proto` around lines 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.
Comment on lines
+106
to
+109
| let clause = WhereClause::from_components(&components).map_err(|e| { | ||
| QueryError::Query(QuerySyntaxError::InvalidFormatWhereClause(Box::leak( | ||
| format!("invalid where clause components: {e}").into_boxed_str(), | ||
| ))) |
Contributor
There was a problem hiding this comment.
Avoid leaking memory on malformed where clauses.
Line 107-Line 109 uses Box::leak(...) to satisfy a static string type, which permanently allocates on each parse failure. On a public endpoint, repeated bad requests can cause unbounded memory growth.
Proposed fix
let clause = WhereClause::from_components(&components).map_err(|e| {
- QueryError::Query(QuerySyntaxError::InvalidFormatWhereClause(Box::leak(
- format!("invalid where clause components: {e}").into_boxed_str(),
- )))
+ let _ = e; // keep parse detail out of leaked/static error path
+ QueryError::Query(QuerySyntaxError::InvalidFormatWhereClause(
+ "invalid where clause components",
+ ))
})?;🤖 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-drive-abci/src/query/document_query/v1/mod.rs` around lines 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.
Comment on lines
+336
to
338
| let limit_u32: u32 = if limit < 0 { | ||
| 0 | ||
| } else if limit > u32::MAX as i64 { |
Contributor
There was a problem hiding this comment.
Honor only -1 as the limit sentinel.
Line 336 currently maps any negative value to “unset”, but the API contract/documentation defines only -1 as valid sentinel. Other negatives should be rejected to avoid silently accepting invalid input.
Suggested 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
)));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let limit_u32: u32 = if limit < 0 { | |
| 0 | |
| } else if limit > u32::MAX as i64 { | |
| 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 | |
| ))); |
🤖 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 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.
Comment on lines
+320
to
+523
| impl FromProof<DocumentQuery> for DocumentSplitCounts { | ||
| type Request = DocumentQuery; | ||
| type Response = GetDocumentsResponse; | ||
|
|
||
| fn maybe_from_proof_with_metadata<'a, I: Into<Self::Request>, O: Into<Self::Response>>( | ||
| request: I, | ||
| response: O, | ||
| _network: Network, | ||
| platform_version: &PlatformVersion, | ||
| provider: &'a dyn ContextProvider, | ||
| ) -> Result<(Option<Self>, ResponseMetadata, Proof), drive_proof_verifier::Error> | ||
| where | ||
| Self: 'a, | ||
| { | ||
| let request: Self::Request = request.into(); | ||
| assert_select_is_count(&request)?; | ||
|
|
||
| // `has_in` controls the single-empty-key-entry guarantee on | ||
| // the no-range prove path: Equal-only fully-covered queries | ||
| // promise one entry with empty key (the verified count, even | ||
| // if zero); In-on-last queries promise one entry per emitted | ||
| // In value (zero-count branches are simply absent — | ||
| // intentional v1 divergence from SQL; see proto docs). | ||
| let has_in = request | ||
| .where_clauses | ||
| .iter() | ||
| .any(|wc| wc.operator == drive::query::WhereOperator::In); | ||
|
|
||
| let has_range = request | ||
| .where_clauses | ||
| .iter() | ||
| .any(|wc| DriveDocumentCountQuery::is_range_operator(wc.operator)); | ||
|
|
||
| // Range + non-empty group_by (with or without In on prefix): | ||
| // per-distinct-value counts via a regular merk range proof | ||
| // (no `AggregateCountOnRange` wrapper). The proof's | ||
| // `KVCount` ops carry per-`(in_key, key)` counts that the | ||
| // merk root commits to via `node_hash_with_count`, so | ||
| // `verify_distinct_count_proof` runs the standard hash | ||
| // chain check and reads the counts back as a verified | ||
| // `Vec<SplitCountEntry>`. Only reachable when the SDK | ||
| // builder set `.with_group_by(...)` — the v1 successor of | ||
| // the v0 wrapper's `return_distinct_counts_in_range = true`. | ||
| if has_range && !request.group_by.is_empty() { | ||
| let response: Self::Response = response.into(); | ||
|
|
||
| let document_type = request | ||
| .data_contract | ||
| .document_type_for_name(&request.document_type_name) | ||
| .map_err(|e| drive_proof_verifier::Error::RequestError { | ||
| error: format!( | ||
| "document type {} not found in contract: {}", | ||
| request.document_type_name, e | ||
| ), | ||
| })?; | ||
| let index = DriveDocumentCountQuery::find_range_countable_index_for_where_clauses( | ||
| document_type.indexes(), | ||
| &request.where_clauses, | ||
| ) | ||
| .ok_or_else(|| drive_proof_verifier::Error::RequestError { | ||
| error: "distinct range count requires a `range_countable: true` index whose \ | ||
| last property matches the range field" | ||
| .to_string(), | ||
| })?; | ||
|
|
||
| let count_query = DriveDocumentCountQuery { | ||
| document_type, | ||
| contract_id: request.data_contract.id().to_buffer(), | ||
| document_type_name: request.document_type_name.clone(), | ||
| index, | ||
| where_clauses: request.where_clauses.clone(), | ||
| }; | ||
| // Match the prover's defaults for limit and order so | ||
| // the verifier helper can rebuild the same path query | ||
| // internally. Both sides anchor limit to | ||
| // `drive::config::DEFAULT_QUERY_LIMIT` (the compile-time | ||
| // constant) rather than the operator-tunable | ||
| // `drive_config.default_query_limit`, so proof bytes | ||
| // are deterministic across operators. Direction comes | ||
| // from the first `order_by` clause; empty `order_by` | ||
| // defaults to ascending — symmetric with the server's | ||
| // `RangeDistinctProof` arm. | ||
| let limit_u16 = limit_to_u16_or_default(request.limit)?; | ||
| let left_to_right = request | ||
| .order_by_clauses | ||
| .first() | ||
| .map(|c| c.ascending) | ||
| .unwrap_or(true); | ||
|
|
||
| let proof = response | ||
| .proof() | ||
| .or(Err(drive_proof_verifier::Error::NoProofInResult))?; | ||
| let mtd = response | ||
| .metadata() | ||
| .or(Err(drive_proof_verifier::Error::EmptyResponseMetadata))?; | ||
|
|
||
| let entries = verify_distinct_count_proof( | ||
| &count_query, | ||
| proof, | ||
| mtd, | ||
| limit_u16, | ||
| left_to_right, | ||
| platform_version, | ||
| provider, | ||
| )?; | ||
| return Ok(( | ||
| Some(DocumentSplitCounts::from_verified(entries)), | ||
| mtd.clone(), | ||
| proof.clone(), | ||
| )); | ||
| } | ||
|
|
||
| // No range clause + `prove = true`: route through the count- | ||
| // tree proof primitives, mirroring `DocumentCount`'s dispatch. | ||
| // Two sub-cases: | ||
| // | ||
| // 1. **documents_countable + empty where**: prove the | ||
| // doctype's primary-key CountTree directly. Result is a | ||
| // single empty-key entry with the verified count. | ||
| // 2. **Else**: require a covering countable index. Server | ||
| // proves the per-branch CountTree elements; SDK returns | ||
| // them as `Vec<SplitCountEntry>`. For Equal-only fully- | ||
| // covered the verifier returns one empty-key entry | ||
| // (re-emitted as zero-count if absent); for Equal-prefix | ||
| // + In-on-last it returns one entry per In value (zero- | ||
| // count In branches are simply absent). | ||
| let response: Self::Response = response.into(); | ||
| let document_type = request | ||
| .data_contract | ||
| .document_type_for_name(&request.document_type_name) | ||
| .map_err(|e| drive_proof_verifier::Error::RequestError { | ||
| error: format!( | ||
| "document type {} not found in contract: {}", | ||
| request.document_type_name, e | ||
| ), | ||
| })?; | ||
| let proof = response | ||
| .proof() | ||
| .or(Err(drive_proof_verifier::Error::NoProofInResult))?; | ||
| let mtd = response | ||
| .metadata() | ||
| .or(Err(drive_proof_verifier::Error::EmptyResponseMetadata))?; | ||
|
|
||
| // documents_countable fast path → single empty-key entry. | ||
| if request.where_clauses.is_empty() && document_type.documents_countable() { | ||
| let contract_id = request.data_contract.id().to_buffer(); | ||
| let count = verify_primary_key_count_tree_proof( | ||
| contract_id, | ||
| &request.document_type_name, | ||
| proof, | ||
| mtd, | ||
| platform_version, | ||
| provider, | ||
| )?; | ||
| let entries = vec![SplitCountEntry { | ||
| in_key: None, | ||
| key: Vec::new(), | ||
| count, | ||
| }]; | ||
| return Ok(( | ||
| Some(DocumentSplitCounts::from_verified(entries)), | ||
| mtd.clone(), | ||
| proof.clone(), | ||
| )); | ||
| } | ||
|
|
||
| let index = DriveDocumentCountQuery::find_countable_index_for_where_clauses( | ||
| document_type.indexes(), | ||
| &request.where_clauses, | ||
| ) | ||
| .ok_or_else(|| drive_proof_verifier::Error::RequestError { | ||
| error: "prove count requires a `countable: true` index whose properties \ | ||
| exactly match the where clause fields, or `documentsCountable: \ | ||
| true` on the document type for unfiltered total counts" | ||
| .to_string(), | ||
| })?; | ||
| let count_query = DriveDocumentCountQuery { | ||
| document_type, | ||
| contract_id: request.data_contract.id().to_buffer(), | ||
| document_type_name: request.document_type_name.clone(), | ||
| index, | ||
| where_clauses: request.where_clauses.clone(), | ||
| }; | ||
|
|
||
| let mut entries = | ||
| verify_point_lookup_count_proof(&count_query, proof, mtd, platform_version, provider)?; | ||
| // Total-count case (Equal-only fully-covered) MUST surface as | ||
| // a single empty-key entry — callers distinguish "verified | ||
| // zero" from "no proof returned" purely by structure. If the | ||
| // verifier dropped the entry because count was 0, re-emit it. | ||
| if !has_in && entries.is_empty() { | ||
| entries.push(SplitCountEntry { | ||
| in_key: None, | ||
| key: Vec::new(), | ||
| count: 0, | ||
| }); | ||
| } | ||
| Ok(( | ||
| Some(DocumentSplitCounts::from_verified(entries)), | ||
| mtd.clone(), | ||
| proof.clone(), | ||
| )) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Verify range + empty group_by handling for DocumentSplitCounts.
The dispatch on Line 363 (has_range && !request.group_by.is_empty()) only takes the distinct-range path when group_by is non-empty. For a range query with empty group_by (which DocumentCount handles via verify_aggregate_count_proof at Line 215-216), the code falls through to the non-range branch at Line 446+, where:
- The
documents_countablefast path requireswhere_clauses.is_empty(), so it won't match (range queries have non-empty where clauses). find_countable_index_for_where_clausesat Line 486 looks for a non-range countable index and will likely returnNone, producing the misleading error: "prove count requires acountable: trueindex..."
If a caller invokes DocumentSplitCounts::fetch for a range query without grouping (e.g., to get the SQL-shape-uniform single-empty-key-entry view of a range total), the server returns an AggregateCountOnRange proof and the SDK rejects it with the wrong error. Two questions:
- Is
DocumentSplitCountsintentionally unsupported forrange + empty group_by? If so, consider rejecting it explicitly with a clear message at the start of the function rather than failing late on index lookup. - If supported, the aggregate proof needs to be decoded and re-wrapped as a single empty-key
SplitCountEntry(mirroring thedocuments_countablefast path at Line 464-484).
🤖 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 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.
…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
Comment on lines
+500
to
+517
| let mut entries = | ||
| verify_point_lookup_count_proof(&count_query, proof, mtd, platform_version, provider)?; | ||
| // Total-count case (Equal-only fully-covered) MUST surface as | ||
| // a single empty-key entry — callers distinguish "verified | ||
| // zero" from "no proof returned" purely by structure. If the | ||
| // verifier dropped the entry because count was 0, re-emit it. | ||
| if !has_in && entries.is_empty() { | ||
| entries.push(SplitCountEntry { | ||
| in_key: None, | ||
| key: Vec::new(), | ||
| count: 0, | ||
| }); | ||
| } | ||
| Ok(( | ||
| Some(DocumentSplitCounts::from_verified(entries)), | ||
| mtd.clone(), | ||
| proof.clone(), | ||
| )) |
Contributor
There was a problem hiding this comment.
Collapse ungrouped In proofs into the aggregate empty-key shape.
When request.group_by.is_empty(), the count surface is supposed to return one total entry. This branch currently returns the raw per-In entries from verify_point_lookup_count_proof, so the proof path diverges from the aggregate v1 contract and from the plain count wrappers.
Proposed fix
- let mut entries =
- verify_point_lookup_count_proof(&count_query, proof, mtd, platform_version, provider)?;
- // Total-count case (Equal-only fully-covered) MUST surface as
- // a single empty-key entry — callers distinguish "verified
- // zero" from "no proof returned" purely by structure. If the
- // verifier dropped the entry because count was 0, re-emit it.
- if !has_in && entries.is_empty() {
+ let mut entries =
+ verify_point_lookup_count_proof(&count_query, proof, mtd, platform_version, provider)?;
+ if request.group_by.is_empty() {
+ let total = entries.iter().map(|entry| entry.count).sum();
entries.push(SplitCountEntry {
in_key: None,
key: Vec::new(),
- count: 0,
+ count: total,
});
+ } else if !has_in && entries.is_empty() {
+ // Total-count case (Equal-only fully-covered) MUST surface as
+ // a single empty-key entry — callers distinguish "verified
+ // zero" from "no proof returned" purely by structure. If the
+ // verifier dropped the entry because count was 0, re-emit it.
+ entries.push(SplitCountEntry {
+ in_key: None,
+ key: Vec::new(),
+ count: 0,
+ });
}🤖 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 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.
`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
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