feat(platform)!: unify documents count + split-count into one endpoint#3623
Open
QuantumExplorer wants to merge 34 commits intov3.1-devfrom
Open
feat(platform)!: unify documents count + split-count into one endpoint#3623QuantumExplorer wants to merge 34 commits intov3.1-devfrom
QuantumExplorer wants to merge 34 commits intov3.1-devfrom
Conversation
Collapse the two count gRPC endpoints (`getDocumentsCount` and
`getDocumentsSplitCount`) into a single unified `getDocumentsCount`
that handles both modes. Wire format and consumer cleanup; logic
parity for total + In-split modes; new fields stubbed for follow-up.
**Wire format** (`packages/dapi-grpc/protos/platform/v0/platform.proto`):
- `GetDocumentsCountRequestV0` gains `return_distinct_counts_in_range`,
`order_by_ascending`, `limit`, `start_after_split_key` fields.
- `GetDocumentsCountResponseV0.result` becomes `oneof { CountResults
counts; Proof proof; }`. `CountResults` carries `repeated CountEntry
{ bytes key; uint64 count; }`. Total count is one entry with empty
key; per-In-value counts are one entry per In value.
- `GetDocumentsSplitCount{Request,Response}` deleted.
**Mode dispatch from where clauses**:
- No `In` clause → total count, single CountEntry with empty key.
- Exactly one `In` clause → per-In-value entries. The In's field is
the split property; the In's array determines which values appear.
- Multiple `In` clauses → InvalidArgument (only one split dimension
per request).
- `return_distinct_counts_in_range = true` → InvalidArgument for now;
this needs `range_countable` indexes (parallel rs-dpp work) and the
`NonCounted<*>` element variants from grovedb.
**Per-layer changes**:
- `dapi-grpc` build.rs: remove `GetDocumentsSplitCount{Request,Response}`
from the versioned-message arrays (counts go from 58/56 to 57/55).
- `rs-dapi-client` transport: remove `getDocumentsSplitCount` impl.
- `rs-dapi` server: remove `get_documents_split_count` drive_method
passthrough.
- `rs-drive-abci`: delete `query/document_split_count_query/` module
and the trait method on `PlatformService`. Rewrite
`query_documents_count_v0` to dispatch on In-presence and emit
`CountResults` instead of bare `count`. Per-In-value entries are
produced by replacing the In with an Equal on each value and
point-looking-up the count (each entry uses
`serialize_value_for_key` for its `key` so the bytes round-trip
consistently with the proof-path verifier's bucket keys).
- `rs-drive-proof-verifier`: `DocumentSplitCounts` now targets
`GetDocumentsCountResponse` (just a type-name change in the
`Response` associated type; the proof-aggregation logic is
unchanged).
- `rs-sdk`: delete `DocumentSplitCountQuery` type. `DocumentCount`
and `DocumentSplitCounts` both `impl Fetch with Request =
DocumentCountQuery`. New `FromProof<DocumentCountQuery> for
DocumentSplitCounts` derives the split property from the request's
In clause field name and routes through
`maybe_from_proof_with_split_property`. Mock-loader entries for
the deleted types removed.
- `wasm-sdk` / `rs-sdk-ffi`: `getDocumentsSplitCount` /
`dash_sdk_document_split_count` keep their names but drop the
`splitProperty` parameter — splitting is now signalled by including
an `in` where-clause.
**Tests**:
- All 14 rs-drive `drive_document_count_query` lib tests pass (no
changes — the rs-drive primitives are the same; the wire-level
unification happens in drive-abci).
- All 5 rs-drive-abci handler tests pass: total / empty / proof /
range-rejection / In. Existing assertions updated from `Result::
Count(count)` patterns to summing `CountResults.entries`.
- The existing `test_documents_split_count_*` handler tests are
removed alongside the deleted handler module.
**Not yet in this PR** (follow-ups):
- `limit` / `start_after_split_key` / `order_by_ascending` are
accepted in the request but currently unused by the handler; the
underlying `DriveDocumentCountQuery` doesn't yet plumb them through.
- `return_distinct_counts_in_range = true` and range operators on
the no-prove path remain rejected; both depend on the parallel
`range_countable` index property + grovedb `NonCounted<*>`
variants. Design is documented in `book/src/drive/indexes.md`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplaces GetDocumentsSplitCount with a unified GetDocumentsCount, adds ChangesUnified count API and range-countable indexing
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…ange) Brings in dashpay/grovedb#654 (Element::NonCounted wrapper) and #656 (QueryItem::AggregateCountOnRange + Node::HashWithCount). Both are prerequisites for the `range_countable` index property that the parallel design work in `book/src/drive/indexes.md` depends on: - `Element::NonCounted(Box<Element>)` — wrapper variant whose count contributes 0 to a parent count tree's aggregate. Lets a count tree hold housekeeping rows / sibling sub-property continuations without polluting the count. Only insertable into count-bearing trees; nested wrappers rejected at construction / serialize / deserialize. - `QueryItem::AggregateCountOnRange(Box<QueryItem>)` — count-only proof shape returning `(CryptoHash, u64)` in O(log n) bytes. Backed by a new self-verifying `Node::HashWithCount(kv_hash, l, r, count)` proof node so the count is bound by the proof, not trusted on faith. Restricted to `ProvableCountTree` / `ProvableCountSumTree` (and their `NonCounted*` wrappers) at proof time. Verified via `GroveDb::verify_aggregate_count_query`. Together these unblock implementing `range_countable` indexes (per- node counts on the property-name tree, NonCounted wrappers for sibling continuations) and `return_distinct_counts_in_range` / range count queries on the no-prove and prove paths — both currently gated as "not yet supported" in the unified count handler. Workspace fixups required by the bump: - `wasm-drive-verify` JS shim: add a `QueryItem::AggregateCountOnRange` arm in `serialize_query_item` (descriptive type, no recursion into the inner range — the wasm verify path doesn't drive these queries today, but the variant must be matched for the workspace to compile). - `rs-sdk-ffi` path-elements display: add `Element::NonCounted(_)` arm reporting `"non_counted"` (placeholder display; we'll inflate it to describe the inner element when the wrapper is actually used in contracts). - `rs-drive-abci` shielded common: orchard's transitive bump made `Action::from_parts` return `Option<Action>`. Wrap with `.ok_or_else` surfacing `InvalidShieldedProofError("invalid action parts")` rather than panicking; otherwise behaviorally unchanged. Tests: 14 rs-drive count-query lib tests, 5 drive-abci handler tests, 3079 rs-drive lib tests, and 3435 dpp lib tests all still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ing) Per-index `rangeCountable: bool` flag, additive on top of `countable`. When true, the index is laid out so that range-count queries on the indexed property can be answered in O(log n): - Property-name level: `ProvableCountTree` (per-node counts let a range query walk just the boundary path). - Each value tree under it: `CountTree` (count-bearing so the property-name aggregate sums per-value counts cleanly). - Sibling continuations inside a value tree: wrapped with `Element::NonCounted` so their counts don't pollute the value tree's count. Depends on the grovedb features bumped in the previous commit (`Element::NonCounted` + `QueryItem::AggregateCountOnRange` from dashpay/grovedb#654 + #656). This commit lands the schema-level plumbing only: - `Index.range_countable: bool` field + serde derive. - Index parser reads `"rangeCountable"` (boolean only — no enum form needed). - Cross-field validation in `Index::try_from`: `range_countable: true` requires `countable.is_countable()`. Without that, it would change layout of a non-count-bearing tree, which is meaningless. - v1 meta-schema schema entry under each index in `documentSchemas`. - Protocol-version gate in `try_from_schema/v1`: `range_countable: true` on protocol_version < 12 raises `UnsupportedFeatureError`. Pre-v12 nodes therefore reject the contract at validation time, before any state mutation. Mirrors the existing v12 gate on countable indexes. - `IndexLevelTypeInfo.range_countable` populated from the source index so the insert/delete walkers can reach it (used in a follow-up). - `random_index` default + ~16 IndexLevel test-init sites updated. Storage layout change (the actual `NonCounted` wrapping + `ProvableCountTree`/`CountTree` selection in the insert / delete walkers) is **deferred to a follow-up commit**. Until that lands, `IndexLevelTypeInfo.range_countable` is read but not yet acted on — the on-disk layout is unchanged, so the schema gate is the only gate in effect right now. Combined with the v12 protocol gate, no v11 node ever sees a `range_countable` contract, and no v12 node yet emits NonCounted-wrapped writes. Tests: 79 dpp index tests, 14 rs-drive count-query lib tests, 5 drive-abci handler tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the foundational helper for the upcoming `range_countable` storage layout, plus a runtime guard that fails loudly if a v12+ contract with `range_countable: true` reaches the insert walker before the rest of the storage-layout work lands. - `LowLevelDriveOperation::for_known_path_key_empty_non_counted_normal_tree`: builds a `GroveOperation` that inserts `Element::NonCounted(empty_tree())` at the given path and key. The wrapper makes the inserted subtree contribute 0 to a parent count tree's aggregate (per dashpay/grovedb#654), which is what the index walker needs for sibling continuations under a `range_countable` value tree (e.g., the `'shape'` continuation under a `byColor` value tree, when `byColor` is range_countable but `byColorShape` shares its prefix). Construction is infallible by `new_non_counted`'s contract — the `expect` documents the invariant. - `add_indices_for_top_index_level_for_contract_operations_v0` and `add_indices_for_index_level_for_contract_operations_v0`: both now inspect `sub_level.has_index_with_type().range_countable` and return `DriveError::NotSupported` if true, with TODO comments pointing to the exact lines that need to switch tree types and the helper to use for NonCounted wrapping. Belt-and-suspenders alongside the rs-dpp v12 validation gate added in the previous commit — pre-v12 nodes already reject the contract; on v12+ the contract reaches here and we refuse rather than corrupt the count aggregation by writing a NormalTree where a CountTree / ProvableCountTree / NonCounted is required. Tests: 79 dpp + 14 rs-drive + 5 drive-abci tests still pass. Next chunks (still TODO on this PR — best as separate focused commits): - Insert walker: switch property-name tree to ProvableCountTree, value tree to CountTree, and wrap sibling continuations with NonCounted when `IndexLevelTypeInfo.range_countable` is true. Threads a `parent_value_tree_is_count_bearing` flag through recursion. - Same in cost-estimation paths (`EstimatedLayerInformation.tree_type`). - Mirror in delete (`remove_*_for_index_level_*`). - Count picker: accept `range_countable` indexes for range operators. - `DriveDocumentCountQuery::execute_no_proof` range mode via grovedb's `AggregateCountOnRange` query item. - Drive-abci handler: route `return_distinct_counts_in_range = true` to the new range-mode logic instead of erroring. - Drop the `u16::MAX` materialization cap on prove path for range counts via `verify_aggregate_count_query`. - Tests covering count-aggregation correctness with NonCounted siblings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ists Adds the public Drive helper the index walker will call when inserting sibling continuations under a `range_countable` value tree (a `CountTree`). Without `NonCounted` wrapping, the empty `NormalTree` would contribute 1 to the parent's count via grovedb's default `count_value_or_default` (returns 1 for non-CountTree children); the wrapper makes it contribute 0 so the value tree's count cleanly reflects "docs at this value" rather than "docs + sibling-continuation-trees". Implementation: - Internal `batch_insert_empty_tree_if_not_exists_v0` now takes a `wrap_in_non_counted: bool` parameter. The body's existing per-PathKeyInfo-variant branches all funnel through a small `build_op` closure that picks between the regular `tree_type.empty_tree_operation_for_known_path_key` and the new `LowLevelDriveOperation::for_known_path_key_empty_non_counted_normal_tree` helper. Wrap is only valid with `TreeType::NormalTree` for now (the only shape the walker needs); other combinations return `DriveError::NotSupported` so callers don't accidentally request ill-defined wrapping. - Public `batch_insert_empty_tree_if_not_exists` wrapper passes `false` — behavior unchanged for existing callers. - New public `batch_insert_empty_non_counted_normal_tree_if_not_exists` passes `true` and fixes `tree_type` to `NormalTree`. Same not-exists-check / pending-batch-deduplication semantics as the regular helper. Test fixtures updated to thread the new parameter through direct `*_v0` calls (5 sites in this file's existing test module). Tests: full count-query test suite still passes (14 rs-drive lib + 5 drive-abci handler). Note: this is a foundational helper for the `range_countable` walker work that's still pending (see TODO markers in `add_indices_for_*_index_level_*_for_contract_operations_v0`). The walker's actual integration — switching property-name tree to `ProvableCountTree`, value tree to `CountTree`, wrapping siblings via this new helper — and the matching delete-path mirror are deferred to a follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switches the index walker over from a runtime guard to the actual storage layout for `range_countable` indexes: - Top-level property-name tree (`[contract_doc, doctype, prop]`) is now a `ProvableCountTree` at contract setup when any range_countable index terminates at that property. - Value tree (`[..., prop, <value>]`) becomes a `CountTree` when the IndexLevel sub_level it lives under is a range_countable terminator. - Recursive walker emits `ProvableCountTree` / `CountTree` at deeper levels following the same rule, and threads a `parent_value_tree_is_range_countable` flag so sibling continuations inside a `CountTree` are wrapped with `Element::NonCounted` (so compound continuations contribute 0 to the parent count instead of polluting it via grovedb's `count_value_or_default`). Generalizes the NonCounted helpers (`for_known_path_key_empty_non_counted_tree`, `batch_insert_empty_non_counted_tree_if_not_exists`) to work for NormalTree / CountTree / ProvableCountTree, so nested-range_countable layouts (e.g. `[color]` and `[color, size]` both range_countable) wrap the inner ProvableCountTree continuation correctly. 10 existing countable_e2e_tests still pass; full drive::document::insert suite (23 tests) green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The delete walker only removes references (the count tree decrement is handled inside grovedb), so the substantive change here is propagating the same `parent_value_tree_is_range_countable` flag through the recursion so cost estimation reports the correct tree variant for each layer (CountTree at value-level, ProvableCountTree at property-name level under a range_countable terminator). Without this, storage-cost math for delete operations on range_countable contracts would diverge from the actual stored shape. All existing drive::document::delete tests (16) still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five new tests in `range_countable_index_e2e_tests` exercise the index walker storage layout end-to-end against a real Drive (grovedb), using a v12 contract whose `widget` document type carries an actual `rangeCountable: true` index over the `color` property: 1. `property_name_tree_for_range_countable_index_is_provable_count_tree` — verifies contract setup creates `[contract_doc, doctype, "color"]` as a `ProvableCountTree`. 2. `value_tree_for_range_countable_index_is_count_tree_after_insert` — on document insert the value tree at `[..., "color", "red"]` is a `CountTree`, and the parent `ProvableCountTree`'s aggregate moves from 0 → 1. 3. `count_tree_value_count_excludes_compound_continuation_via_non_counted` — with a sibling `[color, size]` compound index, the `CountTree` count stays at 1 (not 2) and the continuation tree at `[..., "color", "red", "size"]` is `Element::NonCounted<Tree>`. This is the load-bearing correctness check for NonCounted-wrapping. 4. `aggregate_count_grows_across_distinct_values` — 6 documents at 3 distinct color values produce the right per-value `CountTree` counts AND the right aggregate at the property-name `ProvableCountTree`. 5. `delete_decrements_count_tree_and_provable_count_aggregate` — the delete walker correctly decrements both counts (CountTree and parent ProvableCountTree aggregate). These pin the observable storage shape so any regression in the walker's tree-type selection or NonCounted-wrapping would fail loudly rather than silently producing wrong counts at query time. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `DriveDocumentCountQuery::find_range_countable_index_for_where_clauses` and the supporting `is_range_operator` helper. The picker matches a range count query (e.g. `color > 'a'` or `brand = 'acme' AND color BETWEEN 'a' AND 'z'`) to a `range_countable` index whose: - Equal/In where-clause fields form a prefix of the index properties - Range operator targets the LAST property of the index (the IndexLevel terminator — where the walker emits the `ProvableCountTree`) - `range_countable: true` and `countable.is_countable()` are both set Six unit tests cover the picker rules: 1. picks single-property range_countable 2. picks compound range_countable with Equal prefix 3. rejects range on non-terminator property (no ProvableCountTree exists at that level) 4. rejects non-range_countable index 5. rejects multiple range operators 6. rejects pure point-lookup queries (those go to find_countable_index_for_where_clauses) The executor side (range walk on the property-name ProvableCountTree to read per-value CountTree counts) and the drive-abci handler routing are deferred to a follow-up — this commit only lands the detection logic so a query can be classified correctly. The runtime handler still rejects `return_distinct_counts_in_range=true`; the next step is wiring the executor and removing that gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `DriveDocumentCountQuery::execute_range_count_no_proof` plus the `RangeCountOptions` knob struct (distinct / limit / start_after_split_key / order_by_ascending). Walks children of the property-name `ProvableCountTree` at `[contract_doc, doctype, prefix..., range_prop_name]` whose keys lie within the range expressed by the where clause, reads `count_value_or_default()` from each child `CountTree`, and either sums them (single entry) or returns one entry per distinct property value. Range operator → `QueryItem` mapping covers `>`, `>=`, `<`, `<=`, `Between`, `BetweenExcludeBounds`, `BetweenExcludeLeft`, `BetweenExcludeRight`. `StartsWith` is rejected with a clear message since its grovedb encoding requires a byte-incremented upper bound that's not generic. `In` on prefix properties forks the walk into one path per deduped value and merges per-key entries across forks. Distinct-mode pagination matches the protobuf doc: - ordering: `order_by_ascending = true` is BTreeMap natural order; false reverses - cursor: `start_after_split_key` skips up to AND INCLUDING that key (drops it from the result set in either direction) - limit: applied last, after order + cursor Two e2e tests exercise the full path against a real Drive: 1. `range_count_executor_sums_and_splits_correctly` — six docs at three colors, `color > "blue"` → sum mode returns 5, distinct mode returns [(green, 3), (red, 2)], plus limit + cursor + descending variants 2. `range_count_executor_between_is_inclusive_on_both_bounds` — `Between [bbb, ccc]` returns both bounds (inclusive) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Updates `query_documents_count_v0` to: 1. Detect range operators in the where clauses and, when present, route through the new `find_range_countable_index_for_where_clauses` picker + `execute_range_count_no_proof` executor. 2. Plumb `order_by_ascending`, `limit`, `start_after_split_key`, and `return_distinct_counts_in_range` from the proto request into the `RangeCountOptions` knob struct. Limit is clamped to `max_query_limit` server-side. 3. On the prove path, generate a grovedb `AggregateCountOnRange` proof via the new `execute_aggregate_count_with_proof` helper. Replaces the materialize-and-count proof path (which capped at u16::MAX) for range queries — clients verify with `verify_aggregate_count_query` to recover `(root_hash, count)` without materializing any docs. 4. Reject `return_distinct_counts_in_range = true` on the prove path (the merk-level `AggregateCountOnRange` returns a single aggregate; per-distinct-value entries can't be expressed as one proof shape). 5. Reject mixing `In` with range, and reject multiple range operators in one query, with clear messages directing the caller to use `between*` or split client-side. The previous "range operators not yet supported" hard error is gone: range queries with a covering `range_countable: true` index now succeed end-to-end. The point-lookup proof path (no range) still uses the materialize-and-count flow with the u16::MAX cap, since per- CountTree count proofs aren't wired through a single aggregate primitive yet. Existing test renamed/updated to assert the new behavior — a range query against a contract WITHOUT a range_countable index returns a clear "range count requires `range_countable: true` index" error rather than a generic "range operators not supported" error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Runs the proto-generator pipeline (web, nodejs, java, objective-c, python) against the current `platform.proto`, picking up the new fields on `GetDocumentsCountRequestV0`: - `return_distinct_counts_in_range = 4` - `order_by_ascending = 5` - `limit = 6` - `start_after_split_key = 7` - `prove = 8` (renumbered from 4) The previous committed clients were generated against an older proto revision (only `prove` at field 4) and were missing the pagination / distinct knobs entirely. The Rust handler in this branch already plumbs all five fields end-to-end; this commit aligns the wire format on the JS / Java / ObjC / Python sides. Generated via `yarn build` in packages/dapi-grpc. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Updates `book/src/drive/document-count-trees.md` to reflect the now- released range count behavior: - Replaces "Range operators return InvalidArgument" with the actual range path (find_range_countable_index_for_where_clauses + execute_range_count_no_proof). - Documents the four request modes derivable from the unified `GetDocumentsCount` endpoint (total / per-In-value / per-distinct- range-value / total-range) and the `return_distinct_counts_in_range` toggle. - Documents the new pagination knobs (`order_by_ascending`, `limit`, `start_after_split_key`) and clarifies they only apply in distinct- range mode. - Documents the `AggregateCountOnRange` prove path: range proofs are no longer bounded by the materialize-and-count `u16::MAX` cap; point-lookup count proofs still use the materialize-and-count flow pending a CountTree-direct proof primitive. - Removes references to the legacy `GetDocumentsSplitCount` endpoint (split is now an `In` clause / `return_distinct_counts_in_range` variant of the unified `GetDocumentsCount`). - Updates the cheat-sheet table with concrete schema → query-mode mappings, including the difference between `countable` and `range_countable` per-index flags. 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-10T14:15:24.739Z |
Collaborator
Review GateCommit:
|
…Info The "Mutually compatible with the `countable` flag" sentence on the `range_countable` field's docstring was glued onto the bullet list above it, which clippy 1.92's `doc-lazy-continuation` lint now treats as a hard error (under `-D warnings`). Adding a blank line above it makes it a separate paragraph, which is what the docstring meant. Caught by the macOS `Tests` workflow on PR #3623. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three bullet continuation lines on `range_clause_to_query_item`'s docstring were indented with 4 spaces instead of 3 (2-space continuation after `/// `). Clippy 1.92's `doc-overindented-list-items` lint catches this under `-D warnings` and now treats it as a hard error. Caught by the macOS Tests workflow on PR #3623 (after the prior doc-lazy-continuation fix). 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 #3623 +/- ##
============================================
- Coverage 88.27% 88.25% -0.03%
============================================
Files 2493 2491 -2
Lines 304434 305819 +1385
============================================
+ Hits 268751 269898 +1147
- Misses 35683 35921 +238
🚀 New features to boost your workflow:
|
Codecov flagged 71% patch coverage on PR #3623, with the largest gaps in the new range-count executor and abci handler routing. This commit adds five tests covering the load-bearing paths: drive (rs-drive/.../insert_contract/v0/mod.rs): - aggregate_count_proof_verifies_and_returns_correct_count — generates an `AggregateCountOnRange` proof via execute_aggregate_count_with_proof and verifies it via GroveDb::verify_aggregate_count_query, asserting the recovered count matches the no-proof sum (5 docs). - range_count_with_in_on_prefix_forks_and_merges — exercises the cartesian-fork path through a compound `[brand, color]` range_countable index with `brand IN (acme, contoso)` plus `color > "blue"`. Verifies per-key entries are merged across the In fork (red: 3 acme + 2 contoso = 5). - range_count_executor_rejects_starts_with — confirms the executor's StartsWith branch returns InvalidWhereClauseComponents rather than silently using a wrong range. drive-abci (rs-drive-abci/.../document_count_query/v0/mod.rs): - test_documents_count_range_query_no_prove — full handler integration with a v12 range_countable contract: 6 docs across 3 colors, asserts sum mode, distinct ascending, distinct + limit, and distinct descending all behave correctly. - test_documents_count_range_with_prove_rejects_distinct — confirms the prove path rejects `return_distinct_counts_in_range = true` because grovedb's AggregateCountOnRange proof returns one aggregate. 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: "819e1335063d38ee8f92de4c72715fbea508942aaa451b24cfc7c9711e0db5df"
)Xcode manual integration:
|
The `try_from_schema` dispatch table routes protocol_version ≥ 12 to the v2 module via `CONTRACT_VERSIONS_V4.try_from_schema = 2`. Inside the v1 body we are therefore guaranteed to be at protocol v9/v10/v11 — `platform_version.protocol_version < 12` is always true. Removes the redundant version comparison from both the existing `countable.is_countable()` gate (PR #3457) and the new `range_countable` gate (this PR), keeping the rejections themselves as belt-and-suspenders defense against any future dispatch changes. Updated the comment to explain why the gate is here at all. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous docstring said `rangeCountable` makes the property-name tree a `ProvableCountTree`, but for compound indexes that's only the *last* property (the IndexLevel terminator) — prefix properties keep their default tree shape. The wording could mislead readers into thinking the whole index path becomes a count tree. Also drops the trailing "gated on protocol version 12+ ..." sentence; that's a deployment detail belonging in the v12 protocol notes, not on a per-field docstring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous refactor (80e668a) was wrong: I claimed the `platform_version.protocol_version < 12` guards were dead code on the assumption that the dispatch table routes v12+ to v2. That's true at the OUTER dispatch level, but `try_from_schema_v2` delegates to `DocumentTypeV1::try_from_schema` internally for shared core parsing — so v1's body IS reached at protocol v12+, and the version guard is load-bearing. Without the guards, every v12 contract with a `countable` or `rangeCountable` index gets rejected at v1's validation gate, which broke all 10 range_countable_index_e2e_tests on macOS CI. Update the comment to flag this so future readers (including future-me) don't make the same mistake. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Codex review findings on PR #3623. I rechecked these against the latest head ( Finding 1: [P1] Range count proofs fail SDK verification
The server now returns an How I would fix it: detect the same single-range + covering Finding 2: [P1] Unset distinct range limit is unbounded
This maps an omitted How I would fix it: apply a default limit for distinct mode, for example Finding 3: [P2] SDK cannot reach new no-proof range modes
The unified request adds How I would fix it: add request options/builders plus a |
First step of the document_count_query handler refactor: lift the where-clause-shape validation out of the drive-abci handler into rs-drive. Pure validation now lives in `DriveDocumentCountQuery::detect_mode` which: - Returns a `DocumentCountMode` enum (Total / PerInValue / RangeNoProof / RangeProof / PointLookupProof) classifying the query shape. - Surfaces every where-clause/flag mismatch (multiple range, range + In, distinct without range, distinct on prove path, more than one In, unrecognized operator) as `QuerySyntaxError::InvalidWhereClauseComponents` instead of inline `QueryError::InvalidArgument` strings spread across the handler. The drive-abci handler now calls `detect_mode` once and `match`es on the returned mode tag, with each per-mode body kept in place. Index coverage validation (no covering countable / range_countable index) stays at the call site since it depends on the contract's index map. 14 new unit tests in `rs-drive` cover the truth table without requiring a `Drive` instance, a contract, or a `PlatformVersion`. Existing 7 drive-abci handler tests still pass; one assertion updated to allow either the old `InvalidArgument` shape or the new `Query(InvalidWhereClauseComponents)` shape since the rejection moved between error variants. Sets up step 2 (extract per-mode executors behind `Drive::execute_document_count_request_<mode>`) and step 3 (collapse into a single `Drive::execute_document_count_request`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…drive Step 2 of the document_count_query handler refactor. Adds five methods on `Drive` that own the index-pick + executor-call cycle for each `DocumentCountMode`: - `Drive::execute_document_count_total_no_proof` - `Drive::execute_document_count_per_in_value_no_proof` (cartesian fork over the In values, dedup-by-serialized-key) - `Drive::execute_document_count_range_no_proof` - `Drive::execute_document_count_range_proof` (AggregateCountOnRange) - `Drive::execute_document_count_point_lookup_proof` (materialize-and- count fallback, capped at u16::MAX) Each method: - Picks the right covering index, returning `Error::Query(QuerySyntaxError::WhereClauseOnNonIndexedProperty)` when no index covers the where clauses (so the abci handler can map it to `QueryError::Query(qe)` uniformly). - Builds the appropriate `DriveDocumentCountQuery` (or `DriveDocumentQuery` for the materialize fallback). - Returns `Vec<SplitCountEntry>` (no-proof modes) or `Vec<u8>` proof bytes (proof modes). The drive-abci handler `query_documents_count_v0` now: - Calls `detect_mode` once (step 1). - Each per-mode arm is a single `self.drive.execute_*` call wrapped in a `handle_drive_result!` macro that maps `Error::Query` → `QueryError::Query`. Result wrapping is consolidated into the new `count_response_with_entries` free helper. - Net handler size: 1128 → 924 lines (-18%); business logic per arm dropped from ~30-40 lines to ~10-15 lines including response wrapping. One existing handler test had its assertion updated to accept either the old `InvalidArgument` rejection shape OR the new `Query(WhereClauseOnNonIndexedProperty)` shape (both are valid now that the rejection moved between error variants). All tests green: 7 abci handler tests, 3109 drive lib tests, 14 detect_mode unit tests, 10 range_countable e2e tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 3 (final) of the document_count_query handler refactor. Adds: - `DocumentCountRequest<'a>` — bundles every input the unified count pipeline needs: contract, document_type, parsed where_clauses, raw where Value (for the materialize fallback), all four request flags (`return_distinct_counts_in_range`, `order_by_ascending`, `limit` (pre-clamped), `start_after_split_key`, `prove`), and `drive_config`. - `DocumentCountResponse` — `Counts(Vec<SplitCountEntry>)` or `Proof(Vec<u8>)`, mapped 1:1 onto the protobuf `oneof` result. - `Drive::execute_document_count_request` — single entry point that owns: detect_mode → per-mode index pick → executor → wrap in `DocumentCountResponse`. Maps mode rejection / no-covering-index failures to `Error::Query(QuerySyntaxError::*)`. The drive-abci handler `query_documents_count_v0` is now ~30 lines of business logic (parse contract_id, decode where bytes, build `DocumentCountRequest`, call rs-drive, wrap response in protobuf). Net change: - Step 0 (PR start): 1128 lines, all dispatch + biz logic in handler. - Step 1: detect_mode extracted (~75 lines moved). - Step 2: per-mode executors extracted (~200 lines moved). - Step 3 (this commit): 824 lines, single `execute_document_count_request` call. Domain logic owners are now properly aligned: rs-drive owns query semantics, drive-abci owns gRPC ↔ domain-types translation. Total handler shrinkage 1128 → 824 lines (-27%) and the per-mode match arms are now pure protobuf glue. All 7 abci handler tests + 3109 drive lib tests + 14 detect_mode unit tests + 10 range_countable e2e tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ings Two errors caught by macOS clippy 1.92 + `-D warnings`: - `execute_document_count_range_no_proof` has 8 args, just past clippy's `too_many_arguments` threshold of 7. The args are all load-bearing (contract_id + document_type + name + where_clauses + options + transaction + platform_version + self), so an `#[allow(clippy::too_many_arguments)]` on the method matches the pattern used elsewhere in this file (the other count executors already have the allow). - Two bullet continuation lines on the `DocumentCountResponse::Counts` doc comment were padded to 20-space alignment for visual parallelism; clippy 1.92's `doc-overindented-list-items` lint requires the conventional 2-space continuation. Caught by macOS Tests workflow on PR #3623. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three follow-ups to the unified count endpoint:
1. **rs-sdk DocumentCountQuery builder** — adds public fields and
`with_*` setters for `return_distinct_counts_in_range`,
`order_by_ascending`, `limit`, `start_after_split_key`. The
underlying `TryFrom<DocumentCountQuery> for GetDocumentsCountRequest`
threads them onto the gRPC request. The Fetch trait still always
sets `prove = true` (a no-proof distinct-mode entry point can be a
follow-up). The `QuerySyntaxError` import in
`drive_document_count_query/mod.rs` was widened to
`cfg(any(server, verify))` because `detect_mode` is callable from
the SDK proof-verifier path that compiles under `verify` only.
2. **wasm-sdk count query sites** — fixes the four
`DocumentCountQuery { document_query: base_query }` struct
literals in `wasm-sdk/src/queries/document.rs` to populate the new
fields with their gRPC defaults. JS-level surfacing of the new
flags is intentionally deferred — wasm-sdk's existing four count
methods are all proof-path, and distinct mode is rejected
server-side on the prove path; that needs a separate JS API entry
point.
3. **book/src/drive/indexes.md** — replaces the stale "Compound
indexes (open question)" paragraph that said compound
`range_countable` was "left for later design". The walker actually
does emit `ProvableCountTree` at the terminator and NonCounted-
wraps prefix siblings, with the
`count_tree_value_count_excludes_compound_continuation_via_non_counted`
e2e test pinning the storage layout. Updates the section to
describe the actual implementation.
Verified with `cargo check -p dash-sdk`, `cargo check -p wasm-sdk
--target wasm32-unknown-unknown`, and 117 dash-sdk lib tests.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
book/mermaid-init.js (1)
5-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix indentation to use 2 spaces instead of 4.
The file uses 4-space indentation throughout, but the coding guidelines require 2-space indentation for JS/TS files. As per coding guidelines, "Use 2-space indent for JS/TS files".
♻️ Proposed fix to standardize indentation
(() => { - const darkThemes = ['ayu', 'navy', 'coal']; - const lightThemes = ['light', 'rust']; + const darkThemes = ['ayu', 'navy', 'coal']; + const lightThemes = ['light', 'rust']; - const classList = document.getElementsByTagName('html')[0].classList; + const classList = document.getElementsByTagName('html')[0].classList; - let lastThemeWasLight = true; - for (const cssClass of classList) { - if (darkThemes.includes(cssClass)) { - lastThemeWasLight = false; - break; - } - } + let lastThemeWasLight = true; + for (const cssClass of classList) { + if (darkThemes.includes(cssClass)) { + lastThemeWasLight = false; + break; + } + } - const theme = lastThemeWasLight ? 'default' : 'dark'; - mermaid.initialize({ startOnLoad: true, theme }); + const theme = lastThemeWasLight ? 'default' : 'dark'; + mermaid.initialize({ startOnLoad: true, theme }); - // Simplest way to make mermaid re-render the diagrams in the new theme is via refreshing the page + // Simplest way to make mermaid re-render the diagrams in the new theme is via refreshing the page - for (const darkTheme of darkThemes) { - document.getElementById(darkTheme).addEventListener('click', () => { - if (lastThemeWasLight) { - window.location.reload(); - } - }); - } + for (const darkTheme of darkThemes) { + document.getElementById(darkTheme).addEventListener('click', () => { + if (lastThemeWasLight) { + window.location.reload(); + } + }); + } - for (const lightTheme of lightThemes) { - document.getElementById(lightTheme).addEventListener('click', () => { - if (!lastThemeWasLight) { - window.location.reload(); - } - }); - } + for (const lightTheme of lightThemes) { + document.getElementById(lightTheme).addEventListener('click', () => { + if (!lastThemeWasLight) { + window.location.reload(); + } + }); + } })();🤖 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 `@book/mermaid-init.js` around lines 5 - 39, The file uses 4-space indentation; convert all indentation in this IIFE to 2-space indentation to follow the JS/TS style guide: reformat every block (the arrays darkThemes and lightThemes, the loop over classList that sets lastThemeWasLight, the mermaid.initialize call, and both event-listener loops that reference darkThemes/lightThemes and lastThemeWasLight) so each nested level uses 2 spaces; ensure alignment for the const declarations, for/of loops, arrow functions in getElementById(...).addEventListener callbacks, and the final IIFE closure remain consistent after re-indenting.packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
2587-2588:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd
jstype = JS_STRINGtoCountEntry.countin proto and regenerate.
CountEntry.countis defined asuint64inplatform.proto(line 665) but lacks thejstype = JS_STRINGoption. The generated TypeScript code treats it asnumber, which silently loses precision for values above 2^53 − 1. Otheruint64fields in the same proto file that need to preserve precision usejstype = JS_STRING(e.g.,snapshot_chunks_count,remaining_time). Apply the same pattern here by adding the option to the proto definition and regenerating.🤖 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/web/platform_pb.d.ts` around lines 2587 - 2588, The CountEntry.count field is defined as uint64 but the generated accessors (getCount / setCount) are using number and risk precision loss; update the platform.proto definition for CountEntry.count to include the option `jstype = JS_STRING` (matching other uint64 fields like snapshot_chunks_count and remaining_time), then regenerate the TypeScript protobufs so the generated platform_pb.d.ts and related methods (getCount/setCount) use string types to preserve full uint64 precision.packages/rs-sdk/src/platform/documents/document_count_query.rs (2)
164-188:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
prove: truehardcoded — new no-proof distinct/pagination modes are unreachable viaFetch.The new setters (
with_distinct_counts_in_range,with_order_by_ascending,with_limit,with_start_after_split_key) advertise pagination / per-key distinct ranges, buttry_fromalways setsprove: true(line 183) andFetch for DocumentCount/DocumentSplitCountsis the only consumer. Since the server rejectsreturn_distinct_counts_in_range=truetogether withprove=true(per the field comment on lines 50-55), a caller that setswith_distinct_counts_in_range(true)on the SDK will get a server-side error rather than the new behavior. The same is true forstart_after_split_key/limitpaginated distinct results, which are only meaningful in the no-proof mode.Concretely, this needs a separate transport / fetch entry point (e.g. a
FetchUnproved-style API or a dedicated split-count/range-histogram method) that builds the request withprove: falseand decodesCountResultsdirectly, rather than going through the proof verifier. Otherwise the new request fields exposed onDocumentCountQueryare effectively dead weight from the SDK side.Sketch of the gap
// Today, this compiles and sends a request that the server will reject: let q = DocumentCountQuery::new(contract, "doc")? .with_where(range_clause) .with_distinct_counts_in_range(true) // forces no-proof mode on server .with_limit(Some(50)); let _ = DocumentSplitCounts::fetch(&sdk, q).await?; // Fetch sets prove: true → server rejects🤖 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_query.rs` around lines 164 - 188, The conversion impl TryFrom<DocumentCountQuery> for GetDocumentsCountRequest currently hardcodes GetDocumentsCountRequestV0.prove = true which makes no-proof pagination/distinct modes unreachable; add a separate transport path that builds the same GetDocumentsCountRequest with prove = false (e.g., a new constructor or helper like build_unproved_get_documents_count_request(query: DocumentCountQuery) -> GetDocumentsCountRequest) and expose a new fetch entry point (e.g., DocumentSplitCounts::fetch_unproved or FetchUnproved API) that uses this builder and decodes CountResults directly without running the proof verifier; keep the existing TryFrom/DocumentSplitCounts::fetch for proof-mode but ensure the new method references GetDocumentsCountRequestV0, the prove field, DocumentCountQuery, and DocumentSplitCounts so callers can request no-proof distinct/pagination behavior.
216-249:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRange-count proofs fail SDK verification due to missing AggregateCountOnRange adapter.
The backend generates
AggregateCountOnRangeproofs whenprove=trueand the request contains a range clause (documented in book/src/drive/document-count-trees.md:147,202). BothFromProof<DocumentCountQuery> for DocumentCount(lines 216–249) and its fallback inFromProof<DocumentCountQuery> for DocumentSplitCounts(lines 313–319) unconditionally delegate to<DocumentCount as FromProof<DriveDocumentQuery>>, which expects document-materialization proofs and will reject anAggregateCountOnRangeproof.To verify range-count proofs, add an adapter that detects range clauses in the request, verifies the proof via
GroveDb::verify_aggregate_count_query, and returns the recovered count. A test exercisingprove=truewith a rangewhereclause would catch this regression.🤖 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_query.rs` around lines 216 - 249, The current FromProof implementation for DocumentCountQuery unconditionally forwards to <DocumentCount as FromProof<DriveDocumentQuery>>::maybe_from_proof_with_metadata which fails for backend AggregateCountOnRange proofs; update maybe_from_proof_with_metadata in the impl for DocumentCountQuery (and the similar fallback for DocumentSplitCounts) to detect when the incoming request contains a range clause, and in that case verify the proof via GroveDb::verify_aggregate_count_query (passing the proof/response, network, platform_version and provider), extract and return the recovered count wrapped in DocumentCount along with ResponseMetadata and Proof; otherwise fall back to converting the request to DriveDocumentQuery and delegating to FromProof<DriveDocumentQuery>::maybe_from_proof_with_metadata as before. Ensure you reference DocumentCountQuery, AggregateCountOnRange, GroveDb::verify_aggregate_count_query, DocumentCount, DocumentSplitCounts, DriveDocumentQuery and maybe_from_proof_with_metadata while implementing the adapter.
🧹 Nitpick comments (7)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs (1)
130-147: ⚡ Quick winConsider adding test coverage for the "invalid action parts" error path.
The new error path where
Action::from_partsreturnsNoneis not explicitly tested. While existing tests verify earlier validation failures, a dedicated test exercising this specific error would improve coverage and document when malformed action parts can occur.🧪 Suggested test structure
#[test] fn test_invalid_action_parts_returns_error() { // Craft a SerializedAction with valid field sizes but invalid content // that passes early checks but fails Action::from_parts construction. // The exact values depend on what makes action parts "invalid" in // grovedb-commitment-tree::Action::from_parts. let action = create_serialized_action_with_invalid_parts(); let result = reconstruct_and_verify_bundle( &[action], FLAGS_SPENDS_AND_OUTPUTS, 0, &[42u8; 32], &[0u8; 100], &[0u8; 64], &[], ); assert!(result.is_err()); let err = result.unwrap_err(); assert!( err.message().contains("invalid action parts"), "expected invalid action parts error, got: {}", err.message() ); }🤖 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/execution/validation/state_transition/state_transitions/shielded_common/mod.rs` around lines 130 - 147, Add a unit test that triggers the new None-return path of Action::from_parts and asserts it maps to InvalidShieldedProofError; specifically craft a SerializedAction (or use an existing test helper) with valid-sized fields but malformed content so Action::from_parts(None) is returned, call the surrounding function that invokes Action::from_parts (e.g., the bundle reconstruction/verification function used in this module such as reconstruct_and_verify_bundle or the function that wraps Action::from_parts in mod.rs), and assert the Result is Err and the error is an InvalidShieldedProofError whose message contains "invalid action parts".packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs (1)
336-340: ⚡ Quick winPlease add at least one test covering the new
wrap_in_non_counted = truepath.This PR adds a new execution branch, but current test updates only exercise
false. A targeted test for the true path (and ideally an unsupportedTreeTypeerror case) would harden this change.🤖 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/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs` around lines 336 - 340, Add a unit/integration test that invokes batch_insert_empty_tree_if_not_exists_v0 with wrap_in_non_counted = true (the new branch) to exercise the new logic path: call batch_insert_empty_tree_if_not_exists_v0(info, TreeType::NormalTree, true, None) and assert the resulting state (e.g., the tree was created and marked non-counted or the expected DB entry/flag is present) and any side effects expected by the function; also add a separate test that passes an unsupported TreeType to batch_insert_empty_tree_if_not_exists_v0 and asserts it returns the appropriate error. Use the same test harness/fixtures as existing tests for this module so setup/teardown mirror current coverage.packages/rs-dpp/src/data_contract/document_type/index/mod.rs (1)
700-753: ⚡ Quick winAdd focused unit tests for
rangeCountableparsing + invariants.The new parser/guard logic is central to contract compatibility, but there are no direct tests for: invalid type,
rangeCountable=true+NotCountablerejection, and acceptance withCountable/CountableAllowingOffset.🤖 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-dpp/src/data_contract/document_type/index/mod.rs` around lines 700 - 753, Add focused unit tests that exercise the rangeCountable parsing and invariants: write tests that feed the index-parsing path (the code constructing range_countable and calling IndexProperty::from_platform_value / the document type index parser in mod.rs) with (1) rangeCountable of the wrong type to assert it returns DataContractError::ValueWrongType, (2) rangeCountable=true combined with a countable value that is NotCountable to assert it returns DataContractError::InvalidContractStructure (message about rangeCountable requires countable), and (3) rangeCountable=true combined with Countable and with CountableAllowingOffset to assert successful parsing (no error) and that range_countable is set; use the symbols range_countable, countable.is_countable(), Countable, CountableAllowingOffset, NotCountable, and the DataContractError variants to locate and validate behavior.packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs (1)
328-339: ⚡ Quick winAdd a compound
rangeCountableprefix test here.This branch now relies on
has_index_with_type()to mean “the range-countable index terminates at this level”, but the new e2e coverage only proves the single-property case. Please add a[brand, color]rangeCountablecontract test asserting thebrandnode stays a normal tree and only the terminal range-countable layer gets the provable/count-tree treatment.Based on learnings: In
packages/rs-drive(Rust),CountTreeshould only be used at the reference/leaf level of a countable index. It must not be used for intermediate/top-level index path nodes.🤖 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/src/drive/contract/insert/insert_contract/v0/mod.rs` around lines 328 - 339, The test coverage is missing a compound-index case: add an insert_contract v0 test that exercises a rangeCountable compound prefix like [brand, color] and asserts that the intermediate `brand` node remains a NormalTree while only the terminal `color` layer becomes a Provable/CountTree; modify or add a test that inserts data and then inspects the index_structure via `index_structure.sub_levels()` (the logic around `property_name_is_range_countable_terminator`, `has_index_with_type()` and `range_countable`) to verify CountTree is only created at the leaf/terminal level and not for intermediate path nodes.book/src/drive/document-count-trees.md (2)
147-148: ⚡ Quick winConsider noting the current SDK verification limitation for range count proofs.
The documentation describes the intended verification flow using
GroveDb::verify_aggregate_count_query, but per the PR objectives (finding#1), the SDK proof-verifier (rs-drive-proof-verifier,rs-sdk,wasm-sdk,rs-sdk-ffi) does not yet implement verification forAggregateCountOnRangeproofs—it still uses the old document-materializing verifier, which will fail when it receives the new proof shape.This means
prove=truecombined with a range clause will currently fail at SDK verification time. Consider adding a note or callout (e.g., "Note: SDK verification for range count proofs will be available in a future release") to prevent users from encountering unexpected verification failures when following this documentation.🤖 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 `@book/src/drive/document-count-trees.md` around lines 147 - 148, Add a short note to the document explaining the current SDK limitation: mention that although drive-abci produces AggregateCountOnRange proofs via get_proved_path_query and GroveDb::verify_aggregate_count_query, the SDK proof-verifier implementations (rs-drive-proof-verifier, rs-sdk, wasm-sdk, rs-sdk-ffi) do not yet support verifying AggregateCountOnRange proofs and still expect the older document-materializing proof shape, so using prove=true with a range clause will currently fail verification; suggest using prove=false for range+distinct needs or wait for a future SDK release that adds AggregateCountOnRange verification support.
398-399: ⚡ Quick winConsider noting SDK coverage gaps for new features.
The documentation describes several new fields (
return_distinct_counts_in_range,order_by_ascending,limit,start_after_split_key,prove=false) in the Range Modes section, but per the PR objectives (finding#3), the currentrs-sdk/wasm-sdk/rs-sdk-ffibuilders don't yet expose these fields—they auto-derive mode from where clauses and default toprove=true.Users reading this doc might expect to control these options via the SDK APIs shown here. Consider adding a brief note (e.g., a callout box or inline remark) clarifying which features are available in the current SDK release vs. planned for future versions, to set correct expectations.
🤖 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 `@book/src/drive/document-count-trees.md` around lines 398 - 399, Add a brief note to the "Range Modes" section clarifying SDK coverage gaps: state that the current builders (DocumentCountQuery, DocumentSplitCountQuery and their underlying DocumentQuery -> GetDocumentsCountRequest) in rs-sdk / wasm-sdk / rs-sdk-ffi do not yet expose the new fields return_distinct_counts_in_range, order_by_ascending, limit, start_after_split_key and that mode is auto-derived from where clauses and prove currently defaults to true; place this as a short callout or inline remark near the Range Modes paragraph so readers know these options are planned for future SDK releases.packages/rs-drive/src/query/drive_document_count_query/mod.rs (1)
1116-1117: 💤 Low valueDoc claims
In-on-prefix is supported, but the dispatch rejects it.
execute_range_count_no_proof's doc (lines 1116–1117) and the implementation (lines 1203–1226) explicitly handleInon the prefix with cartesian-fork + dedupe. However,detect_moderejectsrange + Inoutright (lines 206–212), so via the unifiedDrive::execute_document_count_requestentry point this branch is unreachable.Since the function is
puband self-validates, the In-handling code isn't dead per se, but the current state is confusing: the pickerfind_range_countable_index_for_where_clausesaccepts anInprefix, the executor handles it, the dispatch rejects it. Either tightendetect_modeto allow range + (In on prefix), or trim the unreachable In branch and update the doc to say "Equal-only prefix, In + range is rejected upstream". Whichever way it goes, the three layers should agree.Also applies to: 1203-1226
🤖 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/src/query/drive_document_count_query/mod.rs` around lines 1116 - 1117, The docs and execute_range_count_no_proof (and its In-handling code at 1203–1226) claim support for "In on prefix" but detect_mode currently rejects range + In; fix detect_mode so Drive::execute_document_count_request does not reject a range query when the only In appears on the prefix: update detect_mode's logic (the branch that rejects "range + In") to allow cases where find_range_countable_index_for_where_clauses identifies an index with an In-only prefix, keep execute_range_count_no_proof's cartesian-fork + dedupe behavior, and add/update tests to cover range + In-on-prefix dispatch so the three layers (picker, dispatcher, executor) agree.
🤖 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 `@book/mermaid-init.js`:
- Around line 24-38: The loops over darkThemes and lightThemes call
document.getElementById(...) and immediately addEventListener, which can throw
if the element is null; update both loops in mermaid-init.js to first assign the
result to a variable (e.g., btn), check for null/undefined, and only call
addEventListener when btn exists, preserving the existing callback logic that
uses lastThemeWasLight; this prevents TypeError crashes when some theme IDs are
missing.
In `@book/src/drive/document-count-trees.md`:
- Line 196: Add a concise note under the `limit` row clarifying the unset
semantics: state that when `limit` is omitted the query is treated as unbounded
(no client-side truncation), that this currently can bypass `max_query_limit`
unless server-side enforcement is applied, and recommend that clients explicitly
set `limit` to avoid accidental DoS; reference the `limit` and `max_query_limit`
symbols so readers know exactly which fields/controls the note applies to.
In `@book/src/drive/indexes.md`:
- Around line 405-416: Update the section status text for range-countable
indexes to reflect that the feature is implemented and shipped instead of
“design / not implemented”: change the status label and any surrounding wording
that claims it’s unimplemented to state it’s implemented and covered by tests,
and reference the existing implementation details (the walker
add_indices_for_index_level_for_contract_operations, the range_countable
behavior in compound indexes, and the end-to-end test suite
range_countable_index_e2e_tests including the
count_tree_value_count_excludes_compound_continuation_via_non_counted test) so
the prose and status are consistent.
In `@packages/dapi-grpc/protos/platform/v0/platform.proto`:
- Around line 663-665: The CountEntry.count field (message CountEntry, field
name "count") is missing the JS-safe annotation; update the proto by adding the
option `[jstype = JS_STRING]` to the `uint64 count = 2;` declaration so
generated JS/Web clients represent the 64-bit value as a string and avoid
precision loss for large counts, then re-run your proto generation to regenerate
the JS clients.
In `@packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs`:
- Around line 45-57: Index validation currently only rejects changes to the
`countable` flag but `range_countable` (now stored on `IndexLevelTypeInfo`) also
changes index-tree layout and must be treated as immutable; update the index
update validation logic that compares old and new `IndexLevelTypeInfo` to also
check `old.range_countable != new.range_countable` and reject the update with
the same error path/behavior used for `countable` mismatches so toggling
`range_countable` is disallowed just like `countable`.
In `@packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs`:
- Around line 142-145: The current mapping uses limit.map(...) so None remains
None and bypasses the server default; change the logic to first normalize None
to the configured default and then clamp to max_query_limit—i.e. replace the
limit mapping so it uses limit.unwrap_or(self.config.drive.default_query_limit)
(or the actual configured default field in config) and then apply
.min(self.config.drive.max_query_limit as u32) before assigning to limit (keep
start_after_split_key unchanged).
In `@packages/rs-drive/src/query/drive_document_count_query/mod.rs`:
- Around line 1696-1699: The DocumentCountRequest::limit field can be None and
currently gets forwarded into RangeCountOptions unchecked, allowing
distinct-range counts to return unbounded results; update the dispatch that
builds RangeCountOptions to clamp/replace request.limit with the system max
(ensuring RangeCountOptions.limit is always Some and ≤ system cap) before
calling execute_range_count_no_proof, and adjust the DocumentCountRequest::limit
docstring to reflect that Drive enforces a server-side cap; reference
DocumentCountRequest::limit, RangeCountOptions, and execute_range_count_no_proof
when making the change and add/update a handler test for distinct=true with
limit=None to validate the behavior.
- Around line 226-235: detect_mode currently maps (has_range=false, has_in=true,
prove=_) to DocumentCountMode::PerInValue and thus silently drops prove=true;
change detect_mode to return an Err (InvalidWhereClauseComponents) when has_in
is true and prove is true (i.e. reject In-only queries requesting proofs)
instead of mapping to PerInValue, and add/adjust a unit test asserting
detect_mode(&[in_clause], false, true) returns Err; also ensure
execute_document_count_request (the PerInValue arm) remains unchanged for
non-proof requests so behavior is consistent.
In `@packages/rs-sdk-ffi/src/document/queries/count.rs`:
- Around line 219-223: The DocumentCountQuery struct literal is missing required
fields causing compilation failure; update the creation of count_query (used
with DocumentSplitCounts::fetch and built from base_query) to initialize all
five fields by adding return_distinct_counts_in_range: false,
order_by_ascending: None, limit: None, and start_after_split_key: None alongside
document_query: base_query so it matches DocumentCountQuery::new() and the
pattern in packages/wasm-sdk/src/queries/document.rs.
In `@packages/wasm-sdk/src/queries/document.rs`:
- Around line 464-476: The wrappers construct DocumentCountQuery with hard-coded
proof-only fields (DocumentCountQuery usage sets
return_distinct_counts_in_range=false and clears
order_by_ascending/limit/start_after_split_key), preventing the new no-proof
distinct-range and pagination options from being reachable; update the
WASM-facing API to accept optional fields and propagate them into the
DocumentCountQuery: expose return_distinct_counts_in_range, order_by_ascending,
limit, and start_after_split_key from the JS query object (or add dedicated
range-count entrypoints) and use those values instead of the current literals
when building DocumentCountQuery (see where DocumentCountQuery is constructed
around base_query).
---
Outside diff comments:
In `@book/mermaid-init.js`:
- Around line 5-39: The file uses 4-space indentation; convert all indentation
in this IIFE to 2-space indentation to follow the JS/TS style guide: reformat
every block (the arrays darkThemes and lightThemes, the loop over classList that
sets lastThemeWasLight, the mermaid.initialize call, and both event-listener
loops that reference darkThemes/lightThemes and lastThemeWasLight) so each
nested level uses 2 spaces; ensure alignment for the const declarations, for/of
loops, arrow functions in getElementById(...).addEventListener callbacks, and
the final IIFE closure remain consistent after re-indenting.
In `@packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts`:
- Around line 2587-2588: The CountEntry.count field is defined as uint64 but the
generated accessors (getCount / setCount) are using number and risk precision
loss; update the platform.proto definition for CountEntry.count to include the
option `jstype = JS_STRING` (matching other uint64 fields like
snapshot_chunks_count and remaining_time), then regenerate the TypeScript
protobufs so the generated platform_pb.d.ts and related methods
(getCount/setCount) use string types to preserve full uint64 precision.
In `@packages/rs-sdk/src/platform/documents/document_count_query.rs`:
- Around line 164-188: The conversion impl TryFrom<DocumentCountQuery> for
GetDocumentsCountRequest currently hardcodes GetDocumentsCountRequestV0.prove =
true which makes no-proof pagination/distinct modes unreachable; add a separate
transport path that builds the same GetDocumentsCountRequest with prove = false
(e.g., a new constructor or helper like
build_unproved_get_documents_count_request(query: DocumentCountQuery) ->
GetDocumentsCountRequest) and expose a new fetch entry point (e.g.,
DocumentSplitCounts::fetch_unproved or FetchUnproved API) that uses this builder
and decodes CountResults directly without running the proof verifier; keep the
existing TryFrom/DocumentSplitCounts::fetch for proof-mode but ensure the new
method references GetDocumentsCountRequestV0, the prove field,
DocumentCountQuery, and DocumentSplitCounts so callers can request no-proof
distinct/pagination behavior.
- Around line 216-249: The current FromProof implementation for
DocumentCountQuery unconditionally forwards to <DocumentCount as
FromProof<DriveDocumentQuery>>::maybe_from_proof_with_metadata which fails for
backend AggregateCountOnRange proofs; update maybe_from_proof_with_metadata in
the impl for DocumentCountQuery (and the similar fallback for
DocumentSplitCounts) to detect when the incoming request contains a range
clause, and in that case verify the proof via
GroveDb::verify_aggregate_count_query (passing the proof/response, network,
platform_version and provider), extract and return the recovered count wrapped
in DocumentCount along with ResponseMetadata and Proof; otherwise fall back to
converting the request to DriveDocumentQuery and delegating to
FromProof<DriveDocumentQuery>::maybe_from_proof_with_metadata as before. Ensure
you reference DocumentCountQuery, AggregateCountOnRange,
GroveDb::verify_aggregate_count_query, DocumentCount, DocumentSplitCounts,
DriveDocumentQuery and maybe_from_proof_with_metadata while implementing the
adapter.
---
Nitpick comments:
In `@book/src/drive/document-count-trees.md`:
- Around line 147-148: Add a short note to the document explaining the current
SDK limitation: mention that although drive-abci produces AggregateCountOnRange
proofs via get_proved_path_query and GroveDb::verify_aggregate_count_query, the
SDK proof-verifier implementations (rs-drive-proof-verifier, rs-sdk, wasm-sdk,
rs-sdk-ffi) do not yet support verifying AggregateCountOnRange proofs and still
expect the older document-materializing proof shape, so using prove=true with a
range clause will currently fail verification; suggest using prove=false for
range+distinct needs or wait for a future SDK release that adds
AggregateCountOnRange verification support.
- Around line 398-399: Add a brief note to the "Range Modes" section clarifying
SDK coverage gaps: state that the current builders (DocumentCountQuery,
DocumentSplitCountQuery and their underlying DocumentQuery ->
GetDocumentsCountRequest) in rs-sdk / wasm-sdk / rs-sdk-ffi do not yet expose
the new fields return_distinct_counts_in_range, order_by_ascending, limit,
start_after_split_key and that mode is auto-derived from where clauses and prove
currently defaults to true; place this as a short callout or inline remark near
the Range Modes paragraph so readers know these options are planned for future
SDK releases.
In `@packages/rs-dpp/src/data_contract/document_type/index/mod.rs`:
- Around line 700-753: Add focused unit tests that exercise the rangeCountable
parsing and invariants: write tests that feed the index-parsing path (the code
constructing range_countable and calling IndexProperty::from_platform_value /
the document type index parser in mod.rs) with (1) rangeCountable of the wrong
type to assert it returns DataContractError::ValueWrongType, (2)
rangeCountable=true combined with a countable value that is NotCountable to
assert it returns DataContractError::InvalidContractStructure (message about
rangeCountable requires countable), and (3) rangeCountable=true combined with
Countable and with CountableAllowingOffset to assert successful parsing (no
error) and that range_countable is set; use the symbols range_countable,
countable.is_countable(), Countable, CountableAllowingOffset, NotCountable, and
the DataContractError variants to locate and validate behavior.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs`:
- Around line 130-147: Add a unit test that triggers the new None-return path of
Action::from_parts and asserts it maps to InvalidShieldedProofError;
specifically craft a SerializedAction (or use an existing test helper) with
valid-sized fields but malformed content so Action::from_parts(None) is
returned, call the surrounding function that invokes Action::from_parts (e.g.,
the bundle reconstruction/verification function used in this module such as
reconstruct_and_verify_bundle or the function that wraps Action::from_parts in
mod.rs), and assert the Result is Err and the error is an
InvalidShieldedProofError whose message contains "invalid action parts".
In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- Around line 328-339: The test coverage is missing a compound-index case: add
an insert_contract v0 test that exercises a rangeCountable compound prefix like
[brand, color] and asserts that the intermediate `brand` node remains a
NormalTree while only the terminal `color` layer becomes a Provable/CountTree;
modify or add a test that inserts data and then inspects the index_structure via
`index_structure.sub_levels()` (the logic around
`property_name_is_range_countable_terminator`, `has_index_with_type()` and
`range_countable`) to verify CountTree is only created at the leaf/terminal
level and not for intermediate path nodes.
In `@packages/rs-drive/src/query/drive_document_count_query/mod.rs`:
- Around line 1116-1117: The docs and execute_range_count_no_proof (and its
In-handling code at 1203–1226) claim support for "In on prefix" but detect_mode
currently rejects range + In; fix detect_mode so
Drive::execute_document_count_request does not reject a range query when the
only In appears on the prefix: update detect_mode's logic (the branch that
rejects "range + In") to allow cases where
find_range_countable_index_for_where_clauses identifies an index with an In-only
prefix, keep execute_range_count_no_proof's cartesian-fork + dedupe behavior,
and add/update tests to cover range + In-on-prefix dispatch so the three layers
(picker, dispatcher, executor) agree.
In
`@packages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rs`:
- Around line 336-340: Add a unit/integration test that invokes
batch_insert_empty_tree_if_not_exists_v0 with wrap_in_non_counted = true (the
new branch) to exercise the new logic path: call
batch_insert_empty_tree_if_not_exists_v0(info, TreeType::NormalTree, true, None)
and assert the resulting state (e.g., the tree was created and marked
non-counted or the expected DB entry/flag is present) and any side effects
expected by the function; also add a separate test that passes an unsupported
TreeType to batch_insert_empty_tree_if_not_exists_v0 and asserts it returns the
appropriate error. Use the same test harness/fixtures as existing tests for this
module so setup/teardown mirror current coverage.
🪄 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: 8d7977d1-7bd3-42fb-bd0e-8282591bc6cb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
book/book.tomlbook/mermaid-init.jsbook/src/drive/document-count-trees.mdbook/src/drive/indexes.mdpackages/dapi-grpc/build.rspackages/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/dapi-grpc/protos/platform/v0/platform.protopackages/rs-dapi-client/src/transport/grpc.rspackages/rs-dapi/src/services/platform_service/mod.rspackages/rs-dpp/Cargo.tomlpackages/rs-dpp/schema/meta_schemas/document/v1/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/index/mod.rspackages/rs-dpp/src/data_contract/document_type/index/random_index.rspackages/rs-dpp/src/data_contract/document_type/index_level/mod.rspackages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rspackages/rs-drive-abci/src/query/document_count_query/v0/mod.rspackages/rs-drive-abci/src/query/document_split_count_query/mod.rspackages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rspackages/rs-drive-abci/src/query/mod.rspackages/rs-drive-abci/src/query/service.rspackages/rs-drive-proof-verifier/src/proof/document_split_count.rspackages/rs-drive/Cargo.tomlpackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-drive/src/query/drive_document_count_query/mod.rspackages/rs-drive/src/query/drive_document_count_query/tests.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rspackages/rs-platform-version/Cargo.tomlpackages/rs-platform-wallet/Cargo.tomlpackages/rs-sdk-ffi/src/document/queries/count.rspackages/rs-sdk-ffi/src/system/queries/path_elements.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/mock/sdk.rspackages/rs-sdk/src/platform/documents/document_count_query.rspackages/rs-sdk/src/platform/documents/document_split_count_query.rspackages/rs-sdk/src/platform/documents/mod.rspackages/rs-sdk/tests/fetch/document_split_count.rspackages/rs-sdk/tests/fetch/mod.rspackages/wasm-drive-verify/src/state_transition/state_transition_execution_path_queries/token_transition.rspackages/wasm-sdk/src/queries/document.rs
💤 Files with no reviewable changes (15)
- packages/rs-sdk/src/platform/documents/mod.rs
- packages/rs-drive-abci/src/query/mod.rs
- packages/rs-dapi/src/services/platform_service/mod.rs
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js
- packages/rs-drive-abci/src/query/document_split_count_query/mod.rs
- packages/rs-drive-abci/src/query/document_split_count_query/v0/mod.rs
- packages/rs-sdk/tests/fetch/mod.rs
- 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.pbrpc.h
- packages/rs-dapi-client/src/transport/grpc.rs
- packages/rs-sdk/tests/fetch/document_split_count.rs
- packages/rs-sdk/src/mock/sdk.rs
- packages/rs-sdk/src/platform/documents/document_split_count_query.rs
Comment on lines
24
to
38
| for (const darkTheme of darkThemes) { | ||
| const el = document.getElementById(darkTheme); | ||
| if (el) el.addEventListener('click', () => { | ||
| if (lastThemeWasLight) window.location.reload(); | ||
| document.getElementById(darkTheme).addEventListener('click', () => { | ||
| if (lastThemeWasLight) { | ||
| window.location.reload(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| for (const lightTheme of lightThemes) { | ||
| const el = document.getElementById(lightTheme); | ||
| if (el) el.addEventListener('click', () => { | ||
| if (!lastThemeWasLight) window.location.reload(); | ||
| document.getElementById(lightTheme).addEventListener('click', () => { | ||
| if (!lastThemeWasLight) { | ||
| window.location.reload(); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Add null checks before calling addEventListener.
document.getElementById() returns null if the element doesn't exist. Calling addEventListener on null will throw a TypeError at runtime. The current code assumes all theme button IDs ('ayu', 'navy', 'coal', 'light', 'rust') exist in the DOM, but if any are missing (due to configuration changes or incomplete HTML), the script will crash.
🛡️ Proposed fix to add null-safety guards
for (const darkTheme of darkThemes) {
- document.getElementById(darkTheme).addEventListener('click', () => {
- if (lastThemeWasLight) {
- window.location.reload();
- }
- });
+ const darkButton = document.getElementById(darkTheme);
+ if (darkButton) {
+ darkButton.addEventListener('click', () => {
+ if (lastThemeWasLight) {
+ window.location.reload();
+ }
+ });
+ }
}
for (const lightTheme of lightThemes) {
- document.getElementById(lightTheme).addEventListener('click', () => {
- if (!lastThemeWasLight) {
- window.location.reload();
- }
- });
+ const lightButton = document.getElementById(lightTheme);
+ if (lightButton) {
+ lightButton.addEventListener('click', () => {
+ if (!lastThemeWasLight) {
+ window.location.reload();
+ }
+ });
+ }
}🤖 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 `@book/mermaid-init.js` around lines 24 - 38, The loops over darkThemes and
lightThemes call document.getElementById(...) and immediately addEventListener,
which can throw if the element is null; update both loops in mermaid-init.js to
first assign the result to a variable (e.g., btn), check for null/undefined, and
only call addEventListener when btn exists, preserving the existing callback
logic that uses lastThemeWasLight; this prevents TypeError crashes when some
theme IDs are missing.
Comment on lines
+405
to
416
| #### Compound indexes | ||
|
|
||
| What `range_countable` means on a compound index — e.g., `byColorShape = [color, shape]` with `range_countable: true` — is left for later design. The natural reading is "the parent of the *terminating* level of this index", i.e., the `'shape'` tree under each color value, which would itself become a `ProvableCountTree` (and `'circle'` / `'square'` would become `CountTree`s). When that compound's leading prefix is itself another index (`byColor`), the layering of `NonCounted` and counted variants needs to be worked out so neither index's counts pollute the other. We'll cross that bridge when we actually need range queries on a compound index. | ||
| `range_countable: true` on a compound index applies at the index's *terminating* level (its last property). For `byColorShape = [color, shape]` with `range_countable: true`: | ||
|
|
||
| - `'shape'` (the property-name tree under each color value) becomes a `ProvableCountTree`. | ||
| - Each `'circle'` / `'square'` value tree becomes a `CountTree`. | ||
| - Documents are referenced as `Element::Reference` leaves under those `CountTree`s, contributing 1 each to the count aggregate. | ||
|
|
||
| When the compound's leading prefix is also indexed by another `range_countable` index (e.g. `byColor` is also `range_countable`), sibling continuations under each color `CountTree` are wrapped with `Element::NonCounted` so a doc routed via `byColorShape` doesn't double-count under `byColor`'s color aggregate. The walker (`add_indices_for_index_level_for_contract_operations`) threads a `parent_value_tree_is_range_countable` flag down the recursion to decide when to wrap, regardless of whether the inner tree is itself a `ProvableCountTree`, `CountTree`, or plain `NormalTree`. | ||
|
|
||
| End-to-end coverage in `range_countable_index_e2e_tests` (in `packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`) pins the storage layout against a real grovedb — including the `count_tree_value_count_excludes_compound_continuation_via_non_counted` test that proves NonCounted-wrapping is load-bearing for compound-index correctness. | ||
|
|
Contributor
There was a problem hiding this comment.
Clarify implementation status wording in this section.
This new block describes shipped behavior and cites e2e coverage, but the chapter still labels range-countable as “design / not implemented.” Please align the status text so readers don’t get contradictory guidance.
🤖 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 `@book/src/drive/indexes.md` around lines 405 - 416, Update the section status
text for range-countable indexes to reflect that the feature is implemented and
shipped instead of “design / not implemented”: change the status label and any
surrounding wording that claims it’s unimplemented to state it’s implemented and
covered by tests, and reference the existing implementation details (the walker
add_indices_for_index_level_for_contract_operations, the range_countable
behavior in compound indexes, and the end-to-end test suite
range_countable_index_e2e_tests including the
count_tree_value_count_excludes_compound_continuation_via_non_counted test) so
the prose and status are consistent.
Comment on lines
+464
to
476
| // Wasm-sdk's count entry points are all proof-path Fetch calls. | ||
| // Range no-proof distinct mode (`return_distinct_counts_in_range`, | ||
| // pagination knobs) needs a separate JS-facing API entry point | ||
| // since proof + distinct is rejected server-side; tracked as a | ||
| // follow-up. Defaults match the gRPC defaults for the | ||
| // proof-path total/split modes that wasm-sdk currently exposes. | ||
| let count_query = DocumentCountQuery { | ||
| document_query: base_query, | ||
| return_distinct_counts_in_range: false, | ||
| order_by_ascending: None, | ||
| limit: None, | ||
| start_after_split_key: None, | ||
| }; |
Contributor
There was a problem hiding this comment.
These wrappers still hard-code the old proof-only count mode.
All four entry points force return_distinct_counts_in_range = false and drop order_by_ascending / limit / start_after_split_key, so the new no-proof distinct-range and pagination flow added in this PR is still unreachable from WASM. Please surface these as optional JS query fields (or add dedicated range-count APIs) before merging.
Also applies to: 495-507, 540-546, 566-572
🤖 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/queries/document.rs` around lines 464 - 476, The
wrappers construct DocumentCountQuery with hard-coded proof-only fields
(DocumentCountQuery usage sets return_distinct_counts_in_range=false and clears
order_by_ascending/limit/start_after_split_key), preventing the new no-proof
distinct-range and pagination options from being reachable; update the
WASM-facing API to accept optional fields and propagate them into the
DocumentCountQuery: expose return_distinct_counts_in_range, order_by_ascending,
limit, and start_after_split_key from the JS query object (or add dedicated
range-count entrypoints) and use those values instead of the current literals
when building DocumentCountQuery (see where DocumentCountQuery is constructed
around base_query).
Same fix as the wasm-sdk one in commit 8c1f872 — two struct literals at packages/rs-sdk-ffi/src/document/queries/count.rs (lines 157 and 219) were missing the four new fields added to `DocumentCountQuery` (`return_distinct_counts_in_range`, `order_by_ascending`, `limit`, `start_after_split_key`). I missed this package when sweeping the wasm-sdk sites. Like wasm-sdk, FFI count entry points are proof-path Fetch calls and distinct mode is server-rejected on the prove path, so the new flags default to their gRPC zero values (no behavior change for FFI callers). A dedicated FFI entry point for no-proof distinct mode can be a follow-up. Caught by macOS Tests workflow on PR #3623. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…K error
Two pieces of progress toward client-side range-count proof
verification:
1. **`DriveDocumentCountQuery::aggregate_count_path_query`** —
extracted from `execute_aggregate_count_with_proof` and re-gated
`cfg(any(server, verify))`. The server prove path now calls it;
client-side verifiers can call it too, given access to the same
inputs (contract, document_type, picked range_countable index,
where_clauses), to build the byte-identical `PathQuery` the prover
used. Both sides must produce the same path for
`GroveDb::verify_aggregate_count_query` to recompute the same
merk root, so keeping the construction in one helper is
load-bearing.
The supporting helpers (`range_clause_to_query_item`) and several
imports (`PathQuery`, `QueryItem`, `RootTree`, `PlatformVersion`,
`DocumentTypeV0Getters/Methods`, `Error`) were widened from
`cfg(server)` to `cfg(any(server, verify))` accordingly.
2. **SDK `FromProof<DocumentCountQuery>` for `DocumentCount`** —
detects range queries up front and surfaces a clear error
pointing callers at:
- `prove = false` for the no-proof range count path, or
- `DriveDocumentCountQuery::aggregate_count_path_query` +
`GroveDb::verify_aggregate_count_query` directly (with grovedb
pulled in under `feature = "minimal"`).
Wiring `verify_aggregate_count_query` into the standard SDK path
is blocked on an upstream grovedb gate widening — the function
currently lives behind `feature = "minimal"`, not `"verify"`, so
it isn't reachable from rs-drive-proof-verifier's lean profile.
That's a separate grovedb PR; this commit lands the rs-drive
primitives and the clear client-side error so users aren't left
debugging silent proof-shape mismatches.
10 range_countable_index_e2e_tests still green (including the
`aggregate_count_proof_verifies_and_returns_correct_count` test
that exercises the path-builder via the rs-drive direct call).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single bullet continuation line on `aggregate_count_path_query`'s docstring (the second bullet under "Inputs come from the struct fields") was indented to 4 spaces; clippy 1.92's `doc-overindented-list-items` lint requires 3 (the conventional 2-space continuation after `/// `). Re-indented. Caught by macOS Tests workflow on PR #3623. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
Five fixes from the CodeRabbit review on PR #3623: 1. **`range_countable` immutable in `validate_update`** `IndexLevel::find_first_countable_change` only flagged `countable` diffs; toggling `range_countable` between contract versions also changes index-tree storage layout (ProvableCountTree at property-name level + NonCounted-wrapped continuations) and must be rejected the same way. Renamed to `find_first_countability_change` and added the `range_countable` comparison. 2. **Reject `prove = true` + `In` in `detect_mode`** Was silently mapping `(in_clause, prove=true)` to `PerInValue`, which dispatches to N no-proof point-count lookups — downgrading the caller's explicit proof request to an unproven count without any error or log. Added an early-rejection guard with a clear message ("per-In-value proofs are not yet implemented") plus a `detect_mode_tests::in_with_prove_is_rejected` unit test pinning the new behavior. 3. **Default unset `limit` to `default_query_limit` in abci handler** `limit.map(|req| req.min(...))` left `None` untouched, which the distinct-mode walk treats as "no limit" — letting a caller bypass `max_query_limit` and walk arbitrarily large per-distinct-value result sets. Now `None` → `default_query_limit`, then `.min(max_query_limit)`. After this point the handler always passes `Some(_)` ≤ system cap to rs-drive. 4. **Defense-in-depth `limit` clamp in rs-drive's RangeNoProof dispatch** Even if a future caller forgets the handler-side clamp, drive itself now folds `None` → `default_query_limit` and clamps `Some(_)` to `max_query_limit` before forwarding to `execute_range_count_no_proof`. After this point `RangeCountOptions::limit` is always `Some(_)` ≤ system cap, regardless of caller hygiene. Updated the `DocumentCountRequest::limit` docstring to reflect the new contract. 5. **Doc note on unset `limit` semantics** `book/src/drive/document-count-trees.md` now documents that an omitted `limit` is normalized to `default_query_limit` server-side (not unbounded), so reading the table doesn't leave callers thinking they need to set it explicitly to avoid DoS. Skipped: - `[jstype = JS_STRING]` on `CountEntry.count` was added to the proto in b5cee1d but the local JS regen pipeline isn't producing `platform_pb.d.ts` in this environment (likely a docker image / plugin issue — d0cdcce produced it correctly). The proto change remains in HEAD; the next CI/maintainer regen cycle will reconcile the JS clients. - `mermaid-init.js` null-safety / 2-space-indent suggestions: that file is the canonical asset shipped by `mdbook-mermaid install`, not authored here. Forking it would create a maintenance burden for what amounts to defense against missing theme buttons in a default mdbook template. - Several test-coverage nitpicks (shielded action_from_parts, batch_insert_empty_tree NonCounted path, compound rangeCountable test) — out of scope for this PR's review feedback round. 15/15 detect_mode tests + 35/35 drive_document_count_query tests + 7/7 abci tests still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
May 10, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The unified getDocumentsCount refactor and new range_countable index plumbing are coherent at the drive layer, but the SDK seam has multiple verified regressions: SDK Fetch hard-codes prove=true while the server silently downgrades In-clause requests to a no-proof Counts response (NoProofInResult), the SDK Fetch range path is intentionally rejected client-side with no working alternative through Fetch, and the PerInValue executor ignores the documented limit/order/cursor knobs. Generated JS web/node bindings also drop the proto's [jstype = JS_STRING] annotation on the new CountEntry.count field, undoing the precision protection the proto comment is trying to guarantee.
Reviewed commit: b5cee1d
🔴 4 blocking | 🟡 5 suggestion(s) | 💬 1 nitpick(s)
4 additional findings
🔴 blocking: Generated JS web/node bindings ignore `[jstype = JS_STRING]` on new CountEntry.count — large counts will be rounded
packages/dapi-grpc/clients/platform/v0/web/platform_pb.js (lines 26408-26429)
platform.proto:669 annotates uint64 count = 2 [jstype = JS_STRING] precisely so JS consumers don't lose precision above Number.MAX_SAFE_INTEGER, and the doc-comment immediately above explicitly cites the convention. But the generated bindings checked into this PR don't honor it: platform_pb.js:26420 calls reader.readUint64() (not readUint64String), :26378 uses getFieldWithDefault(msg, 2, 0) (not "0"), and platform_pb.d.ts:2587-2588 types getCount()/setCount() as number. For comparison, an existing count = 2 [jstype = JS_STRING] field at platform.proto:425 properly generates readUint64String() and getCount(): string (platform_pb.js:18496,18538; platform_pb.d.ts:1447-1448), so the in-tree artifact is genuinely inconsistent — not a generator-doesn't-support-jstype problem. The same loss-of-precision bug is duplicated in packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js. Re-run the JS protoc against the current proto so the new field's type matches its annotation.
🟡 suggestion: `execute_split_count` / `expand_split_prefix_paths` / `collect_split_at_prefix` / `find_countable_index_for_split` are dead code in production
packages/rs-drive/src/query/drive_document_count_query/mod.rs (lines 638-877)
After the refactor, Drive::execute_document_count_request always builds DriveDocumentCountQuery with split_by_property: None (mod.rs:1500, 1582, 1628, 1663), and the In mode dispatches to execute_document_count_per_in_value_no_proof (which builds Equal-on-each-value subqueries) rather than execute_no_proof with a split property. Grep confirms execute_split_count, expand_split_prefix_paths, collect_split_at_prefix, and find_countable_index_for_split are only referenced from tests.rs and within the mod itself — no production caller. That's ~250 lines of working-but-orphaned logic plus tests pinning behavior nothing dispatches to anymore. Worth deleting (and trimming the corresponding tests) so the file's surface matches what the dispatcher actually exercises.
🟡 suggestion: `.expect()` inside `execute_transport` can panic on CBOR encode failure
packages/rs-sdk/src/platform/documents/document_count_query.rs (lines 204-213)
self.try_into().expect("DocumentCountQuery should always be valid") runs inside the BoxFuture returned by the transport layer. The TryFrom<DocumentCountQuery> for GetDocumentsCountRequest impl above goes through serialize_where_clauses_to_cbor, which surfaces Error::Protocol(ProtocolError::EncodingError(...)) for ciborium failures. A panic inside the future is worse than a returned TransportError — async runtimes typically convert this into a hung/aborted task instead of a recoverable error. Even if today's WhereClause shapes can't trigger the encode failure, the pattern is fragile under future Value -> CborValue semantic changes. Map the error into a TransportError so the failure is recoverable.
💬 nitpick: Stale doc reference points to test files deleted in this PR
packages/rs-drive-proof-verifier/src/proof/document_split_count.rs (lines 154-157)
The trailing comment points readers to packages/rs-sdk/tests/fetch/document_split_count.rs and to drive-abci's src/query/document_split_count_query/v0/mod.rs tests, but both are removed in this PR (document_split_count.rs | 176 --, document_split_count_query/{mod.rs,v0/mod.rs} | 725 --). Future contributors hit a dead link. Repoint to the surviving fixtures (e.g. packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs tests) or drop the stale bullets.
🤖 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-sdk/src/platform/documents/document_count_query.rs`:
- [BLOCKING] lines 179-183: SDK count Fetch with `In` clause fails end-to-end with NoProofInResult
`DocumentCountQuery -> GetDocumentsCountRequest` hard-codes `prove: true` here, but in `packages/rs-drive/src/query/drive_document_count_query/mod.rs:235-244` `detect_mode` maps `(has_range=false, has_in=true, _)` to `DocumentCountMode::PerInValue` regardless of `prove`, and `execute_document_count_request` (mod.rs:1842-1851) dispatches `PerInValue` to `execute_document_count_per_in_value_no_proof`, which always returns `DocumentCountResponse::Counts(...)` — never a proof. The SDK proof verifier `DocumentSplitCounts::maybe_from_proof_with_split_property` (`packages/rs-drive-proof-verifier/src/proof/document_split_count.rs:97`) then bails with `Error::NoProofInResult`. Net effect: every SDK / wasm-sdk / rs-sdk-ffi caller using an `In` where-clause — i.e. the entire split-count entry-point this PR exposes via `DocumentSplitCounts::fetch` (rs-sdk doc_count_query.rs:301, wasm-sdk queries/document.rs:529-548, rs-sdk-ffi document/queries/count.rs:209-265) — gets a runtime error. The regression is uncaught because the only SDK split-count integration test (`packages/rs-sdk/tests/fetch/document_split_count.rs`) was deleted in this PR with no replacement. Either generate a real proof for `In`-bearing requests when `prove=true`, or reject `prove=true && in` server-side and add a no-proof transport entry point on the SDK so the public API actually works.
- [BLOCKING] lines 232-264: Range count via DocumentCount::fetch always errors — newly unified count API is unusable for ranges from Rust clients
`FromProof<DocumentCountQuery> for DocumentCount` rejects any range operator before parsing the response and tells callers to use `prove = false`, but the same SDK request builder above hard-codes `prove: true` and there is no alternative `Fetch` path that passes `prove = false`. Before this refactor, `prove=true` count queries fell through the materialize-and-count document-proof path; after the new server dispatch, range + prove is routed to `DocumentCountMode::RangeProof` (returns `AggregateCountOnRange` proof bytes) and the SDK can no longer decode it. Net: every wasm-sdk / rs-sdk-ffi count call with `>`, `<`, `between*` (or `startsWith` once enabled) is a hard runtime failure with no usable workaround through the SDK Fetch surface. Either wire up `GroveDb::verify_aggregate_count_query` (the doc-comment notes grovedb's `feature = "minimal"` gate) or expose a no-proof Fetch path before merging.
- [SUGGESTION] lines 355-366: DocumentSplitCounts total-count case yields empty map for count=0 instead of the documented `[(empty key, 0)]` entry
Both the proto wire-format comment (`platform.proto:621-622`: "single CountEntry with empty key") and the wasm/FFI docstrings (`packages/wasm-sdk/src/queries/document.rs:520-524`, `packages/rs-sdk-ffi/src/document/queries/count.rs:194-201`) say the no-`In` total-count case returns one entry with empty key. But here the closure does `if count > 0 { m.insert(Vec::new(), count); }`, so when 0 docs match, JS receives `new Map()` and iOS receives `{"counts": {}}` — indistinguishable from "no entries at all" and contradicting the spec. Drop the `count > 0` guard so the empty-key entry is always present.
- [SUGGESTION] lines 204-213: `.expect()` inside `execute_transport` can panic on CBOR encode failure
`self.try_into().expect("DocumentCountQuery should always be valid")` runs inside the BoxFuture returned by the transport layer. The `TryFrom<DocumentCountQuery> for GetDocumentsCountRequest` impl above goes through `serialize_where_clauses_to_cbor`, which surfaces `Error::Protocol(ProtocolError::EncodingError(...))` for ciborium failures. A panic inside the future is worse than a returned `TransportError` — async runtimes typically convert this into a hung/aborted task instead of a recoverable error. Even if today's `WhereClause` shapes can't trigger the encode failure, the pattern is fragile under future `Value -> CborValue` semantic changes. Map the error into a `TransportError` so the failure is recoverable.
In `packages/rs-drive/src/query/drive_document_count_query/mod.rs`:
- [BLOCKING] lines 1512-1593: PerInValue executor silently ignores `limit`, `order_by_ascending`, and `start_after_split_key`
`packages/dapi-grpc/protos/platform/v0/platform.proto:642-653` documents `order_by_ascending`, `limit`, and `start_after_split_key` as applying to *split-mode entries (per-`In`-value or per-range-distinct-value)* — i.e. PerInValue is in scope. The unified dispatcher at `mod.rs:1842-1851` calls `execute_document_count_per_in_value_no_proof` with no options, and that executor iterates the `In` array in input order, dedupes, and returns every entry with no clamp/skip/sort. Only the `RangeNoProof` arm (1853-1858) threads these into `RangeCountOptions`. Callers will get silently wrong (full, input-ordered, unbounded) results instead of the documented page/sort/cursor semantics. Either honor the options for `PerInValue` or carve them out of the proto contract.
- [SUGGESTION] lines 1348-1353: `startsWith` is advertised as a supported range count operator but the executor rejects it
`is_range_operator` (mod.rs:144) lists `WhereOperator::StartsWith`, `find_range_countable_index_for_where_clauses` will happily pick a covering index for it, and `platform.proto:627` explicitly enumerates `startsWith` alongside `>`/`<`/`between*` as a supported range-clause form. But `range_clause_to_query_item` then errors with `InvalidWhereClauseComponents("startsWith is not yet supported on the range_countable count fast path")`. Two layers disagree on what the count fast path supports, and clients trying to use the documented operator hit a runtime failure. Either reject `StartsWith` in `is_range_operator` / the proto comment until grovedb encoding is wired, or implement the encoding before merging the doc claim.
- [SUGGESTION] lines 638-877: `execute_split_count` / `expand_split_prefix_paths` / `collect_split_at_prefix` / `find_countable_index_for_split` are dead code in production
After the refactor, `Drive::execute_document_count_request` always builds `DriveDocumentCountQuery` with `split_by_property: None` (mod.rs:1500, 1582, 1628, 1663), and the `In` mode dispatches to `execute_document_count_per_in_value_no_proof` (which builds Equal-on-each-value subqueries) rather than `execute_no_proof` with a split property. Grep confirms `execute_split_count`, `expand_split_prefix_paths`, `collect_split_at_prefix`, and `find_countable_index_for_split` are only referenced from `tests.rs` and within the mod itself — no production caller. That's ~250 lines of working-but-orphaned logic plus tests pinning behavior nothing dispatches to anymore. Worth deleting (and trimming the corresponding tests) so the file's surface matches what the dispatcher actually exercises.
In `packages/dapi-grpc/clients/platform/v0/web/platform_pb.js`:
- [BLOCKING] lines 26408-26429: Generated JS web/node bindings ignore `[jstype = JS_STRING]` on new CountEntry.count — large counts will be rounded
`platform.proto:669` annotates `uint64 count = 2 [jstype = JS_STRING]` precisely so JS consumers don't lose precision above `Number.MAX_SAFE_INTEGER`, and the doc-comment immediately above explicitly cites the convention. But the generated bindings checked into this PR don't honor it: `platform_pb.js:26420` calls `reader.readUint64()` (not `readUint64String`), `:26378` uses `getFieldWithDefault(msg, 2, 0)` (not `"0"`), and `platform_pb.d.ts:2587-2588` types `getCount()/setCount()` as `number`. For comparison, an existing `count = 2 [jstype = JS_STRING]` field at `platform.proto:425` properly generates `readUint64String()` and `getCount(): string` (`platform_pb.js:18496,18538`; `platform_pb.d.ts:1447-1448`), so the in-tree artifact is genuinely inconsistent — not a generator-doesn't-support-jstype problem. The same loss-of-precision bug is duplicated in `packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js`. Re-run the JS protoc against the current proto so the new field's type matches its annotation.
In `packages/rs-sdk-ffi/src/document/queries/count.rs`:
- [SUGGESTION] lines 209-215: `dash_sdk_document_split_count` C ABI silently changed without a symbol/version bump
The exported `extern "C"` symbol kept the same name `dash_sdk_document_split_count` but its parameter list went from `(sdk, contract, doc_type, split_property, where_json)` (5 args) to `(sdk, contract, doc_type, where_json)` (4 args). cbindgen at `packages/rs-sdk-ffi/build.rs:21-31` regenerates the header, but any iOS/Swift caller already linked against the previous header will still pass five arguments and on arm64/x86_64 the callee will read the old `split_property` pointer in the slot now occupied by `where_json`, so `serde_json::from_str` on the property name fails and the call returns an `InternalError` JSON without any compile-time signal. Pre-release scope is acknowledged, but consider renaming (`_v2` suffix) or otherwise breaking the symbol so stale wrappers fail loudly at link time.
Comment on lines
+179
to
183
| // SDK Fetch path always requests a proof; users | ||
| // wanting no-proof distinct-mode would need a | ||
| // separate transport entry point that doesn't | ||
| // try to verify the response as a proof. | ||
| prove: true, |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: SDK count Fetch with In clause fails end-to-end with NoProofInResult
DocumentCountQuery -> GetDocumentsCountRequest hard-codes prove: true here, but in packages/rs-drive/src/query/drive_document_count_query/mod.rs:235-244 detect_mode maps (has_range=false, has_in=true, _) to DocumentCountMode::PerInValue regardless of prove, and execute_document_count_request (mod.rs:1842-1851) dispatches PerInValue to execute_document_count_per_in_value_no_proof, which always returns DocumentCountResponse::Counts(...) — never a proof. The SDK proof verifier DocumentSplitCounts::maybe_from_proof_with_split_property (packages/rs-drive-proof-verifier/src/proof/document_split_count.rs:97) then bails with Error::NoProofInResult. Net effect: every SDK / wasm-sdk / rs-sdk-ffi caller using an In where-clause — i.e. the entire split-count entry-point this PR exposes via DocumentSplitCounts::fetch (rs-sdk doc_count_query.rs:301, wasm-sdk queries/document.rs:529-548, rs-sdk-ffi document/queries/count.rs:209-265) — gets a runtime error. The regression is uncaught because the only SDK split-count integration test (packages/rs-sdk/tests/fetch/document_split_count.rs) was deleted in this PR with no replacement. Either generate a real proof for In-bearing requests when prove=true, or reject prove=true && in server-side and add a no-proof transport entry point on the SDK so the public API actually works.
source: ['claude-general', 'codex-general', 'codex-rust-quality', 'codex-security-auditor']
🤖 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-sdk/src/platform/documents/document_count_query.rs`:
- [BLOCKING] lines 179-183: SDK count Fetch with `In` clause fails end-to-end with NoProofInResult
`DocumentCountQuery -> GetDocumentsCountRequest` hard-codes `prove: true` here, but in `packages/rs-drive/src/query/drive_document_count_query/mod.rs:235-244` `detect_mode` maps `(has_range=false, has_in=true, _)` to `DocumentCountMode::PerInValue` regardless of `prove`, and `execute_document_count_request` (mod.rs:1842-1851) dispatches `PerInValue` to `execute_document_count_per_in_value_no_proof`, which always returns `DocumentCountResponse::Counts(...)` — never a proof. The SDK proof verifier `DocumentSplitCounts::maybe_from_proof_with_split_property` (`packages/rs-drive-proof-verifier/src/proof/document_split_count.rs:97`) then bails with `Error::NoProofInResult`. Net effect: every SDK / wasm-sdk / rs-sdk-ffi caller using an `In` where-clause — i.e. the entire split-count entry-point this PR exposes via `DocumentSplitCounts::fetch` (rs-sdk doc_count_query.rs:301, wasm-sdk queries/document.rs:529-548, rs-sdk-ffi document/queries/count.rs:209-265) — gets a runtime error. The regression is uncaught because the only SDK split-count integration test (`packages/rs-sdk/tests/fetch/document_split_count.rs`) was deleted in this PR with no replacement. Either generate a real proof for `In`-bearing requests when `prove=true`, or reject `prove=true && in` server-side and add a no-proof transport entry point on the SDK so the public API actually works.
Comment on lines
+232
to
+264
| // Range queries arrive with a grovedb `AggregateCountOnRange` | ||
| // proof (produced by `Drive::execute_document_count_range_proof`), | ||
| // which the materialize-and-count verifier below cannot decode. | ||
| // The merk-level verifier `GroveDb::verify_aggregate_count_query` | ||
| // is gated to grovedb's `feature = "minimal"`, not `"verify"`, | ||
| // so it isn't reachable from rs-drive-proof-verifier today. | ||
| // Wiring this up requires an upstream grovedb feature-gate | ||
| // change; until then, surface a clear error directing callers | ||
| // to either: | ||
| // - Use `prove = false` for range counts (no SDK gap), or | ||
| // - Build the path-query via | ||
| // `DriveDocumentCountQuery::aggregate_count_path_query` and | ||
| // call `GroveDb::verify_aggregate_count_query` directly with | ||
| // `grovedb` pulled in under `feature = "minimal"`. | ||
| // | ||
| // The path-builder is intentionally kept in rs-drive under | ||
| // `cfg(any(server, verify))` so direct callers don't have to | ||
| // duplicate it. | ||
| if request | ||
| .document_query | ||
| .where_clauses | ||
| .iter() | ||
| .any(|wc| DriveDocumentCountQuery::is_range_operator(wc.operator)) | ||
| { | ||
| return Err(drive_proof_verifier::Error::RequestError { | ||
| error: "AggregateCountOnRange proof verification is not yet wired in the SDK \ | ||
| (grovedb's verify_aggregate_count_query is gated to feature = \"minimal\", \ | ||
| not \"verify\"). Use prove = false for range counts, or call \ | ||
| GroveDb::verify_aggregate_count_query directly with the path query \ | ||
| from DriveDocumentCountQuery::aggregate_count_path_query." | ||
| .to_string(), | ||
| }); | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Range count via DocumentCount::fetch always errors — newly unified count API is unusable for ranges from Rust clients
FromProof<DocumentCountQuery> for DocumentCount rejects any range operator before parsing the response and tells callers to use prove = false, but the same SDK request builder above hard-codes prove: true and there is no alternative Fetch path that passes prove = false. Before this refactor, prove=true count queries fell through the materialize-and-count document-proof path; after the new server dispatch, range + prove is routed to DocumentCountMode::RangeProof (returns AggregateCountOnRange proof bytes) and the SDK can no longer decode it. Net: every wasm-sdk / rs-sdk-ffi count call with >, <, between* (or startsWith once enabled) is a hard runtime failure with no usable workaround through the SDK Fetch surface. Either wire up GroveDb::verify_aggregate_count_query (the doc-comment notes grovedb's feature = "minimal" gate) or expose a no-proof Fetch path before merging.
source: ['codex-general', 'codex-rust-quality']
🤖 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-sdk/src/platform/documents/document_count_query.rs`:
- [BLOCKING] lines 232-264: Range count via DocumentCount::fetch always errors — newly unified count API is unusable for ranges from Rust clients
`FromProof<DocumentCountQuery> for DocumentCount` rejects any range operator before parsing the response and tells callers to use `prove = false`, but the same SDK request builder above hard-codes `prove: true` and there is no alternative `Fetch` path that passes `prove = false`. Before this refactor, `prove=true` count queries fell through the materialize-and-count document-proof path; after the new server dispatch, range + prove is routed to `DocumentCountMode::RangeProof` (returns `AggregateCountOnRange` proof bytes) and the SDK can no longer decode it. Net: every wasm-sdk / rs-sdk-ffi count call with `>`, `<`, `between*` (or `startsWith` once enabled) is a hard runtime failure with no usable workaround through the SDK Fetch surface. Either wire up `GroveDb::verify_aggregate_count_query` (the doc-comment notes grovedb's `feature = "minimal"` gate) or expose a no-proof Fetch path before merging.
Comment on lines
+1512
to
+1593
| pub fn execute_document_count_per_in_value_no_proof( | ||
| &self, | ||
| contract_id: [u8; 32], | ||
| document_type: DocumentTypeRef, | ||
| document_type_name: String, | ||
| where_clauses: Vec<WhereClause>, | ||
| transaction: TransactionArg, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<Vec<SplitCountEntry>, Error> { | ||
| let in_clause = where_clauses | ||
| .iter() | ||
| .find(|wc| wc.operator == WhereOperator::In) | ||
| .ok_or_else(|| { | ||
| Error::Query(QuerySyntaxError::InvalidWhereClauseComponents( | ||
| "execute_document_count_per_in_value_no_proof requires exactly one `in` clause", | ||
| )) | ||
| })? | ||
| .clone(); | ||
| let in_values = in_clause.value.as_array().ok_or_else(|| { | ||
| Error::Query(QuerySyntaxError::InvalidWhereClauseComponents( | ||
| "In where-clause value must be an array", | ||
| )) | ||
| })?; | ||
|
|
||
| let other_clauses: Vec<WhereClause> = where_clauses | ||
| .iter() | ||
| .filter(|wc| wc.operator != WhereOperator::In) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| let mut entries = Vec::with_capacity(in_values.len()); | ||
| let mut seen_keys: BTreeSet<Vec<u8>> = BTreeSet::new(); | ||
| for value in in_values { | ||
| // Pre-serialize so wire keys round-trip consistently with | ||
| // the no-In total-count path AND so we dedupe when an `In` | ||
| // value list contains duplicates. | ||
| let key_bytes = document_type.serialize_value_for_key( | ||
| in_clause.field.as_str(), | ||
| value, | ||
| platform_version, | ||
| )?; | ||
| if !seen_keys.insert(key_bytes.clone()) { | ||
| continue; | ||
| } | ||
|
|
||
| let mut clauses_for_value = other_clauses.clone(); | ||
| clauses_for_value.push(WhereClause { | ||
| field: in_clause.field.clone(), | ||
| operator: WhereOperator::Equal, | ||
| value: value.clone(), | ||
| }); | ||
|
|
||
| let index = DriveDocumentCountQuery::find_countable_index_for_where_clauses( | ||
| document_type.indexes(), | ||
| &clauses_for_value, | ||
| ) | ||
| .ok_or_else(|| { | ||
| Error::Query(QuerySyntaxError::WhereClauseOnNonIndexedProperty( | ||
| "count query requires a countable index on the document type that \ | ||
| matches the where clause properties" | ||
| .to_string(), | ||
| )) | ||
| })?; | ||
|
|
||
| let count_query = DriveDocumentCountQuery { | ||
| document_type, | ||
| contract_id, | ||
| document_type_name: document_type_name.clone(), | ||
| index, | ||
| where_clauses: clauses_for_value, | ||
| split_by_property: None, | ||
| }; | ||
| let results = count_query.execute_no_proof(self, transaction, platform_version)?; | ||
| let count = results.first().map_or(0, |entry| entry.count); | ||
|
|
||
| entries.push(SplitCountEntry { | ||
| key: key_bytes, | ||
| count, | ||
| }); | ||
| } | ||
| Ok(entries) | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: PerInValue executor silently ignores limit, order_by_ascending, and start_after_split_key
packages/dapi-grpc/protos/platform/v0/platform.proto:642-653 documents order_by_ascending, limit, and start_after_split_key as applying to split-mode entries (per-In-value or per-range-distinct-value) — i.e. PerInValue is in scope. The unified dispatcher at mod.rs:1842-1851 calls execute_document_count_per_in_value_no_proof with no options, and that executor iterates the In array in input order, dedupes, and returns every entry with no clamp/skip/sort. Only the RangeNoProof arm (1853-1858) threads these into RangeCountOptions. Callers will get silently wrong (full, input-ordered, unbounded) results instead of the documented page/sort/cursor semantics. Either honor the options for PerInValue or carve them out of the proto contract.
source: ['codex-general']
🤖 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/src/query/drive_document_count_query/mod.rs`:
- [BLOCKING] lines 1512-1593: PerInValue executor silently ignores `limit`, `order_by_ascending`, and `start_after_split_key`
`packages/dapi-grpc/protos/platform/v0/platform.proto:642-653` documents `order_by_ascending`, `limit`, and `start_after_split_key` as applying to *split-mode entries (per-`In`-value or per-range-distinct-value)* — i.e. PerInValue is in scope. The unified dispatcher at `mod.rs:1842-1851` calls `execute_document_count_per_in_value_no_proof` with no options, and that executor iterates the `In` array in input order, dedupes, and returns every entry with no clamp/skip/sort. Only the `RangeNoProof` arm (1853-1858) threads these into `RangeCountOptions`. Callers will get silently wrong (full, input-ordered, unbounded) results instead of the documented page/sort/cursor semantics. Either honor the options for `PerInValue` or carve them out of the proto contract.
Comment on lines
+1348
to
+1353
| WhereOperator::StartsWith => { | ||
| return Err(Error::Query( | ||
| QuerySyntaxError::InvalidWhereClauseComponents( | ||
| "startsWith is not yet supported on the range_countable count fast path", | ||
| ), | ||
| )); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: startsWith is advertised as a supported range count operator but the executor rejects it
is_range_operator (mod.rs:144) lists WhereOperator::StartsWith, find_range_countable_index_for_where_clauses will happily pick a covering index for it, and platform.proto:627 explicitly enumerates startsWith alongside >/</between* as a supported range-clause form. But range_clause_to_query_item then errors with InvalidWhereClauseComponents("startsWith is not yet supported on the range_countable count fast path"). Two layers disagree on what the count fast path supports, and clients trying to use the documented operator hit a runtime failure. Either reject StartsWith in is_range_operator / the proto comment until grovedb encoding is wired, or implement the encoding before merging the doc claim.
source: ['claude-general', 'codex-general']
🤖 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/src/query/drive_document_count_query/mod.rs`:
- [SUGGESTION] lines 1348-1353: `startsWith` is advertised as a supported range count operator but the executor rejects it
`is_range_operator` (mod.rs:144) lists `WhereOperator::StartsWith`, `find_range_countable_index_for_where_clauses` will happily pick a covering index for it, and `platform.proto:627` explicitly enumerates `startsWith` alongside `>`/`<`/`between*` as a supported range-clause form. But `range_clause_to_query_item` then errors with `InvalidWhereClauseComponents("startsWith is not yet supported on the range_countable count fast path")`. Two layers disagree on what the count fast path supports, and clients trying to use the documented operator hit a runtime failure. Either reject `StartsWith` in `is_range_operator` / the proto comment until grovedb encoding is wired, or implement the encoding before merging the doc claim.
Comment on lines
+355
to
+366
| .map(|(opt, mtd, proof)| { | ||
| let map = opt | ||
| .map(|DocumentCount(count)| { | ||
| let mut m = std::collections::BTreeMap::new(); | ||
| if count > 0 { | ||
| m.insert(Vec::new(), count); | ||
| } | ||
| m | ||
| }) | ||
| .unwrap_or_default(); | ||
| (Some(DocumentSplitCounts(map)), mtd, proof) | ||
| }) |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: DocumentSplitCounts total-count case yields empty map for count=0 instead of the documented [(empty key, 0)] entry
Both the proto wire-format comment (platform.proto:621-622: "single CountEntry with empty key") and the wasm/FFI docstrings (packages/wasm-sdk/src/queries/document.rs:520-524, packages/rs-sdk-ffi/src/document/queries/count.rs:194-201) say the no-In total-count case returns one entry with empty key. But here the closure does if count > 0 { m.insert(Vec::new(), count); }, so when 0 docs match, JS receives new Map() and iOS receives {"counts": {}} — indistinguishable from "no entries at all" and contradicting the spec. Drop the count > 0 guard so the empty-key entry is always present.
💡 Suggested change
Suggested change
| .map(|(opt, mtd, proof)| { | |
| let map = opt | |
| .map(|DocumentCount(count)| { | |
| let mut m = std::collections::BTreeMap::new(); | |
| if count > 0 { | |
| m.insert(Vec::new(), count); | |
| } | |
| m | |
| }) | |
| .unwrap_or_default(); | |
| (Some(DocumentSplitCounts(map)), mtd, proof) | |
| }) | |
| .map(|(opt, mtd, proof)| { | |
| let map = opt | |
| .map(|DocumentCount(count)| { | |
| let mut m = std::collections::BTreeMap::new(); | |
| m.insert(Vec::new(), count); | |
| m | |
| }) | |
| .unwrap_or_default(); | |
| (Some(DocumentSplitCounts(map)), mtd, proof) | |
| }) |
source: ['claude-ffi-engineer']
🤖 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-sdk/src/platform/documents/document_count_query.rs`:
- [SUGGESTION] lines 355-366: DocumentSplitCounts total-count case yields empty map for count=0 instead of the documented `[(empty key, 0)]` entry
Both the proto wire-format comment (`platform.proto:621-622`: "single CountEntry with empty key") and the wasm/FFI docstrings (`packages/wasm-sdk/src/queries/document.rs:520-524`, `packages/rs-sdk-ffi/src/document/queries/count.rs:194-201`) say the no-`In` total-count case returns one entry with empty key. But here the closure does `if count > 0 { m.insert(Vec::new(), count); }`, so when 0 docs match, JS receives `new Map()` and iOS receives `{"counts": {}}` — indistinguishable from "no entries at all" and contradicting the spec. Drop the `count > 0` guard so the empty-key entry is always present.
Comment on lines
209
to
215
| #[no_mangle] | ||
| pub unsafe extern "C" fn dash_sdk_document_split_count( | ||
| sdk_handle: *const SDKHandle, | ||
| data_contract_handle: *const DataContractHandle, | ||
| document_type: *const c_char, | ||
| split_property: *const c_char, | ||
| where_json: *const c_char, | ||
| ) -> DashSDKResult { |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: dash_sdk_document_split_count C ABI silently changed without a symbol/version bump
The exported extern "C" symbol kept the same name dash_sdk_document_split_count but its parameter list went from (sdk, contract, doc_type, split_property, where_json) (5 args) to (sdk, contract, doc_type, where_json) (4 args). cbindgen at packages/rs-sdk-ffi/build.rs:21-31 regenerates the header, but any iOS/Swift caller already linked against the previous header will still pass five arguments and on arm64/x86_64 the callee will read the old split_property pointer in the slot now occupied by where_json, so serde_json::from_str on the property name fails and the call returns an InternalError JSON without any compile-time signal. Pre-release scope is acknowledged, but consider renaming (_v2 suffix) or otherwise breaking the symbol so stale wrappers fail loudly at link time.
source: ['claude-ffi-engineer', 'codex-ffi-engineer']
🤖 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-sdk-ffi/src/document/queries/count.rs`:
- [SUGGESTION] lines 209-215: `dash_sdk_document_split_count` C ABI silently changed without a symbol/version bump
The exported `extern "C"` symbol kept the same name `dash_sdk_document_split_count` but its parameter list went from `(sdk, contract, doc_type, split_property, where_json)` (5 args) to `(sdk, contract, doc_type, where_json)` (4 args). cbindgen at `packages/rs-sdk-ffi/build.rs:21-31` regenerates the header, but any iOS/Swift caller already linked against the previous header will still pass five arguments and on arm64/x86_64 the callee will read the old `split_property` pointer in the slot now occupied by `where_json`, so `serde_json::from_str` on the property name fails and the call returns an `InternalError` JSON without any compile-time signal. Pre-release scope is acknowledged, but consider renaming (`_v2` suffix) or otherwise breaking the symbol so stale wrappers fail loudly at link time.
…s to PerInValue Three review-feedback fixes from thepastaclaw on PR #3623, all confined to the count-query dispatcher: 1. `prove + In` no longer rejects up front. Pre-refactor, this combination ran through `DriveDocumentQuery::execute_with_proof` (capped at u16::MAX docs) and the SDK grouped verified docs by the `In` field's serialized value. The hard reject silently broke `DocumentSplitCounts::fetch` end-to-end. Route `(false, true, true)` to `PointLookupProof` instead so the SDK materialize path keeps working until an aggregate-proof primitive for `In` lands. 2. `startsWith` is in `is_range_operator` but `range_clause_to_query_item` can't yet encode the byte-incremented upper bound for arbitrary key types. Reject up front in `detect_mode` so the picker doesn't accept queries that the dispatcher would later fail at execution. 3. Thread `limit` / `order_by_ascending` / `start_after_split_key` through `execute_document_count_per_in_value_no_proof`. The proto contract on `GetDocumentsCountRequestV0` says these apply to PerInValue split entries too, so the executor honors them after aggregating into a key-ordered `BTreeMap` (which also dedupes duplicate `In` values via the canonical serialized-key rule). Updated `in_with_prove_is_rejected` test to `in_with_prove_routes_to_point_lookup_proof`.
…erification grovedb#658 widened the gates on the merk-level aggregate-count verifier from feature = "minimal" to any(feature = "minimal", feature = "verify"), which makes `GroveDb::verify_aggregate_count_query` reachable from downstream lean-verifier crates that depend on grovedb with default-features = false, features = ["verify"]. With that landed, the SDK can finally verify range-count proofs end-to-end instead of returning the stub error introduced in 22594d7. Bump grovedb across the workspace from 347bd9b5 to 1206049b58 (the merge commit on develop). In drive-proof-verifier, expose a `verify_aggregate_count_proof` free function that wraps `GroveDb::verify_aggregate_count_query` plus the internal `verify_tenderdash_proof` and returns the verified u64 count. Callers build the `PathQuery` via the shared `DriveDocumentCountQuery::aggregate_count_path_query` builder so the prover and verifier produce the *exact same* path query. In rs-sdk's `FromProof<DocumentCountQuery> for DocumentCount`, replace the range-rejection stub with: look up the document type, run the `range_countable` index picker, build a `DriveDocumentCountQuery`, ask it for the path query, and call the new helper. The materialize path remains the fallback for non-range proof modes (point lookups + In). Also fix two thepastaclaw review findings on the SDK side: - `DocumentSplitCounts` total-count branch: drop the `count > 0` guard so a verified count of zero still emits the empty-key entry. A zero count is a valid result, not absence — callers should distinguish "no docs matched" from "no proof returned" purely by structure. - `execute_transport`: replace the panic-on-conversion-failure `.expect()` with a `TransportError::Grpc(Status::internal(...))` mapping. CBOR-serializing where clauses can fail on values that aren't representable; the failure should be recoverable, not abort. Drive-side cleanup: narrow the `DocumentTypeV0Getters` import in drive_document_count_query/mod.rs to `feature = "server"` only (it's unused under verify-only because the picker takes the BTreeMap directly).
thepastaclaw
suggested changes
May 10, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest commit closes some prior issues (server now rejects prove=true + In rather than silently downgrading; both abci and rs-drive RangeNoProof paths clamp limit). However, the public count API surface remains broken end-to-end: the SDK's TryFrom hard-codes prove=true, so every wasm-sdk/FFI split-count call with In now hits a hard server reject, and every range count fails verifier-side. PerInValue still ignores limit/order/cursor and walks the In array unbounded (DoS amplification). The regenerated JS protobuf bindings still ignore [jstype = JS_STRING] on CountEntry.count. Several smaller follow-ups (dead split-count code, count=0 empty-map, FFI symbol silently re-shaped, .expect() in transport future, stale doc refs) carry forward.
Reviewed commit: 10e34a7
🔴 5 blocking | 🟡 5 suggestion(s)
10 additional findings
🔴 blocking: SDK split-count Fetch with `In` is end-to-end broken — `prove: true` is hard-coded but the server now rejects prove+In
packages/rs-sdk/src/platform/documents/document_count_query.rs (lines 167-188)
TryFrom<DocumentCountQuery> for GetDocumentsCountRequest always sets prove: true (line 183). On this head, DriveDocumentCountQuery::detect_mode (packages/rs-drive/src/query/drive_document_count_query/mod.rs:241-246) explicitly rejects prove=true && has_in with InvalidWhereClauseComponents("prove = true is not supported with an in where-clause"). That means every public caller of DocumentSplitCounts::fetch with an In clause — wasm-sdk's getDocumentsSplitCount / getDocumentsSplitCountWithProofInfo (packages/wasm-sdk/src/queries/document.rs:529-581) and FFI's dash_sdk_document_split_count (packages/rs-sdk-ffi/src/document/queries/count.rs:209-265) — is now a guaranteed InvalidArgument runtime failure. The split-count surface this PR ships is unreachable through every supported client. Either implement a per-In-value aggregate proof primitive on the server so prove=true + In works, or expose a no-proof Fetch entry point so the SDK can flip prove=false for In-bearing count requests. The deleted packages/rs-sdk/tests/fetch/document_split_count.rs integration test would have caught this; reinstate equivalent coverage before merge.
🔴 blocking: Range count via `DocumentCount::fetch` is unusable — no SDK Fetch path issues `prove=false`
packages/rs-sdk/src/platform/documents/document_count_query.rs (lines 232-264)
The new FromProof<DocumentCountQuery> for DocumentCount arm explicitly rejects any range operator with a clear error pointing callers to either prove=false or GroveDb::verify_aggregate_count_query plus DriveDocumentCountQuery::aggregate_count_path_query. But the request builder above hard-codes prove: true, the path-builder is gated behind rs-drive's cfg(any(server, verify)), and verify_aggregate_count_query lives behind grovedb's feature = "minimal" (which rs-drive-proof-verifier does not enable). Net result: every wasm-sdk / rs-sdk-ffi / rs-sdk count call with >, <, between* (or startsWith, when its other rejection lifts) returns Error::RequestError from this verifier check before any productive round-trip, and there is no public Rust API that constructs a prove=false count fetch. Either widen grovedb's feature gate so the verify path is reachable from rs-drive-proof-verifier, or add a prove=false Fetch entry point on DocumentCountQuery returning the parsed Counts directly.
🔴 blocking: PerInValue dispatch ignores `limit`, `order_by_ascending`, and `start_after_split_key`
packages/rs-drive/src/query/drive_document_count_query/mod.rs (lines 1861-1870)
platform.proto:642-653 documents order_by_ascending, limit, and start_after_split_key as split-mode controls applying to both per-In-value and per-range-distinct-value entries. The dispatcher's PerInValue arm calls execute_document_count_per_in_value_no_proof(contract_id, document_type, document_type_name, where_clauses, transaction, platform_version) with no options arg, and the executor at lines 1526-1607 iterates the In array in input order, dedupes via BTreeSet, and emits every entry with no limit, no skip, and no sort. Only the RangeNoProof arm threads these into RangeCountOptions. The new abci-side limit clamp (packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs:143-145) is a no-op for the PerInValue path: callers passing limit=1 still get all N In entries back, and start_after_split_key cursors are ignored. Either honor the options for PerInValue (sort + skip + clamp) or carve them out of the proto contract for In mode.
🔴 blocking: Generated JS bindings ignore `[jstype = JS_STRING]` on new `CountEntry.count` — counts > 2^53−1 silently round in JS
packages/dapi-grpc/clients/platform/v0/web/platform_pb.js (lines 26408-26429)
The proto field is uint64 count = 2 [jstype = JS_STRING] at platform.proto:669, with the doc-comment immediately above explicitly citing the convention. The checked-in JS bindings don't honor it: platform_pb.js:26420 calls reader.readUint64() (returns Number, not String); :26378 uses getFieldWithDefault(msg, 2, 0) (numeric default); platform_pb.d.ts:2587-2604 types count: number; and :26461 calls writer.writeUint64(). For comparison the existing count = 2 [jstype = JS_STRING] field at platform.proto:425 correctly emits readUint64String() and getCount(): string (platform_pb.js:18496-18539), so the generator does support the annotation — the in-tree artifact is genuinely stale. The same loss-of-precision is duplicated in packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js. Re-run protoc against the current proto so the new field's wire decoding matches its declared type-precision contract.
🔴 blocking: PerInValue no-proof count path is unbounded — request-amplification DoS
packages/rs-drive/src/query/drive_document_count_query/mod.rs (lines 1526-1606)
execute_document_count_per_in_value_no_proof reads in_clause.value.as_array() directly (line 1544) and loops over every element, performing one find_countable_index_for_where_clauses + count_query.execute_no_proof GroveDB walk per value. The 100-element cap in WhereClause::in_values() (packages/rs-drive/src/query/conditions.rs:354-365) is not on this path: the abci handler builds WhereClauses via WhereClause::from_components (packages/rs-drive/src/query/conditions.rs:477-525), which performs no length validation, and the dispatcher's PerInValue arm doesn't apply max_query_limit either — only RangeNoProof does. Since DAPI accepts gRPC messages up to 64 MiB and CBOR-encoded small integers cost ~1-2 bytes each, an unauthenticated client can issue a single getDocumentsCount carrying hundreds of thousands of In values, each triggering an independent GroveDB scan. This bypasses the max_query_limit budget the handler is explicitly trying to enforce on the parallel range path. Either invoke WhereClause::in_values() to inherit the existing 100-cap + dedup + non-empty checks, clamp in_values.len() against drive_config.max_query_limit analogously to the RangeNoProof arm, or push an effective_limit into the PerInValue executor.
🟡 suggestion: DocumentSplitCounts total-count case drops the documented empty-key entry when count=0
packages/rs-sdk/src/platform/documents/document_count_query.rs (lines 355-366)
The proto wire-format comment (platform.proto:620-622: "single CountEntry with empty key") and the wasm/FFI docstrings (packages/wasm-sdk/src/queries/document.rs:520-524, packages/rs-sdk-ffi/src/document/queries/count.rs:194-201) all specify that the no-In total-count case returns one entry with empty key. The drive-abci handler also unconditionally produces a [(empty, 0)] entry on Total mode (packages/rs-drive/src/query/drive_document_count_query/mod.rs:1845-1849). But the closure here does if count > 0 { m.insert(Vec::new(), count); }, so when 0 docs match the proven path, JS receives new Map() and iOS receives {"counts":{}} — indistinguishable from "no entries at all" and contradicting both the no-proof path and the published wire shape. Drop the count > 0 guard so the empty-key entry is always present.
💡 Suggested change
.map(|(opt, mtd, proof)| {
let map = opt
.map(|DocumentCount(count)| {
let mut m = std::collections::BTreeMap::new();
m.insert(Vec::new(), count);
m
})
.unwrap_or_default();
(Some(DocumentSplitCounts(map)), mtd, proof)
})
🟡 suggestion: `dash_sdk_document_split_count` C ABI silently changed without symbol/version bump
packages/rs-sdk-ffi/src/document/queries/count.rs (lines 209-215)
The exported extern "C" fn dash_sdk_document_split_count keeps the same name but its parameter list shrank from 5 args (sdk, contract, doc_type, split_property, where_json) to 4 args (sdk, contract, doc_type, where_json). cbindgen at packages/rs-sdk-ffi/build.rs regenerates the header, but any iOS/Swift wrapper still linked against the previous header will continue to push five arguments; on arm64/x86_64 the callee will read what was the old split_property slot as where_json, fail serde_json::from_str, and return an InternalError JSON with no compile- or link-time signal. Pre-release scope acknowledged, but consider renaming (e.g. _v2 suffix) so stale wrappers fail loudly at link time rather than at runtime.
🟡 suggestion: `startsWith` is advertised as a supported range-count operator but rejected at runtime
packages/rs-drive/src/query/drive_document_count_query/mod.rs (lines 1362-1368)
is_range_operator (line 144) lists WhereOperator::StartsWith as range-capable, find_range_countable_index_for_where_clauses will pick a covering index for it, and platform.proto:627 enumerates startsWith alongside >/</between* as a supported range form. But range_clause_to_query_item returns InvalidWhereClauseComponents("startsWith is not yet supported on the range_countable count fast path"). Two layers disagree on what the count fast path supports, and clients trying the documented operator pass mode detection only to hit a late runtime failure when the path query is materialized. Either drop StartsWith from is_range_operator and the proto comment until the byte-incremented upper-bound encoding is wired, or implement it before publishing the doc claim.
🟡 suggestion: `execute_split_count` / `expand_split_prefix_paths` / `collect_split_at_prefix` / `find_countable_index_for_split` are dead in production
packages/rs-drive/src/query/drive_document_count_query/mod.rs (lines 638-877)
After the unified-handler refactor, every production caller of DriveDocumentCountQuery builds the struct with split_by_property: None (mod.rs:1514, 1596, 1642, 1677), and the PerInValue arm dispatches to execute_document_count_per_in_value_no_proof (Equal-on-each-value subqueries) rather than execute_no_proof with a split property. Grep confirms split_by_property: Some(_) is set only in tests.rs:261 and :557. The execute_split_count / expand_split_prefix_paths / collect_split_at_prefix / find_countable_index_for_split helpers and the Some(_) arm of execute_no_proof are no longer reachable from any production path — ~250 lines of working-but-orphaned logic plus tests pinning behavior nothing dispatches to. Either delete them (and trim the dependent tests) so the file's surface matches what the dispatcher exercises, or wire PerInValue to use execute_split_count if its prefix-walk encoding is preferable to the per-value Equal-on-each loop.
🟡 suggestion: `.expect()` inside `execute_transport` future can panic on CBOR encode failure
packages/rs-sdk/src/platform/documents/document_count_query.rs (lines 204-213)
self.try_into().expect("DocumentCountQuery should always be valid") runs inside the BoxFuture returned by the transport layer. The TryFrom<DocumentCountQuery> for GetDocumentsCountRequest impl above goes through serialize_where_clauses_to_cbor (lines 375-390), which surfaces Error::Protocol(ProtocolError::EncodingError(...)) for ciborium failures. A panic inside an async transport future is worse than a returned TransportError — async runtimes typically convert it into a hung/aborted task rather than a recoverable error. Even if today's WhereClause shapes can't trigger the encode failure, the pattern is fragile under future Value -> CborValue semantic changes. Map the conversion error into a TransportError so the failure stays recoverable.
🤖 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-sdk/src/platform/documents/document_count_query.rs`:
- [BLOCKING] lines 167-188: SDK split-count Fetch with `In` is end-to-end broken — `prove: true` is hard-coded but the server now rejects prove+In
`TryFrom<DocumentCountQuery> for GetDocumentsCountRequest` always sets `prove: true` (line 183). On this head, `DriveDocumentCountQuery::detect_mode` (`packages/rs-drive/src/query/drive_document_count_query/mod.rs:241-246`) explicitly rejects `prove=true && has_in` with `InvalidWhereClauseComponents("prove = true is not supported with an `in` where-clause")`. That means every public caller of `DocumentSplitCounts::fetch` with an `In` clause — wasm-sdk's `getDocumentsSplitCount` / `getDocumentsSplitCountWithProofInfo` (`packages/wasm-sdk/src/queries/document.rs:529-581`) and FFI's `dash_sdk_document_split_count` (`packages/rs-sdk-ffi/src/document/queries/count.rs:209-265`) — is now a guaranteed `InvalidArgument` runtime failure. The split-count surface this PR ships is unreachable through every supported client. Either implement a per-`In`-value aggregate proof primitive on the server so `prove=true + In` works, or expose a no-proof Fetch entry point so the SDK can flip `prove=false` for In-bearing count requests. The deleted `packages/rs-sdk/tests/fetch/document_split_count.rs` integration test would have caught this; reinstate equivalent coverage before merge.
- [BLOCKING] lines 232-264: Range count via `DocumentCount::fetch` is unusable — no SDK Fetch path issues `prove=false`
The new `FromProof<DocumentCountQuery> for DocumentCount` arm explicitly rejects any range operator with a clear error pointing callers to either `prove=false` or `GroveDb::verify_aggregate_count_query` plus `DriveDocumentCountQuery::aggregate_count_path_query`. But the request builder above hard-codes `prove: true`, the path-builder is gated behind rs-drive's `cfg(any(server, verify))`, and `verify_aggregate_count_query` lives behind grovedb's `feature = "minimal"` (which `rs-drive-proof-verifier` does not enable). Net result: every wasm-sdk / rs-sdk-ffi / rs-sdk count call with `>`, `<`, `between*` (or `startsWith`, when its other rejection lifts) returns `Error::RequestError` from this verifier check before any productive round-trip, and there is no public Rust API that constructs a `prove=false` count fetch. Either widen grovedb's feature gate so the verify path is reachable from `rs-drive-proof-verifier`, or add a `prove=false` Fetch entry point on `DocumentCountQuery` returning the parsed `Counts` directly.
- [SUGGESTION] lines 355-366: DocumentSplitCounts total-count case drops the documented empty-key entry when count=0
The proto wire-format comment (`platform.proto:620-622`: "single CountEntry with empty key") and the wasm/FFI docstrings (`packages/wasm-sdk/src/queries/document.rs:520-524`, `packages/rs-sdk-ffi/src/document/queries/count.rs:194-201`) all specify that the no-`In` total-count case returns one entry with empty key. The drive-abci handler also unconditionally produces a `[(empty, 0)]` entry on Total mode (`packages/rs-drive/src/query/drive_document_count_query/mod.rs:1845-1849`). But the closure here does `if count > 0 { m.insert(Vec::new(), count); }`, so when 0 docs match the proven path, JS receives `new Map()` and iOS receives `{"counts":{}}` — indistinguishable from "no entries at all" and contradicting both the no-proof path and the published wire shape. Drop the `count > 0` guard so the empty-key entry is always present.
- [SUGGESTION] lines 204-213: `.expect()` inside `execute_transport` future can panic on CBOR encode failure
`self.try_into().expect("DocumentCountQuery should always be valid")` runs inside the `BoxFuture` returned by the transport layer. The `TryFrom<DocumentCountQuery> for GetDocumentsCountRequest` impl above goes through `serialize_where_clauses_to_cbor` (lines 375-390), which surfaces `Error::Protocol(ProtocolError::EncodingError(...))` for ciborium failures. A panic inside an async transport future is worse than a returned `TransportError` — async runtimes typically convert it into a hung/aborted task rather than a recoverable error. Even if today's `WhereClause` shapes can't trigger the encode failure, the pattern is fragile under future `Value -> CborValue` semantic changes. Map the conversion error into a `TransportError` so the failure stays recoverable.
In `packages/rs-drive/src/query/drive_document_count_query/mod.rs`:
- [BLOCKING] lines 1861-1870: PerInValue dispatch ignores `limit`, `order_by_ascending`, and `start_after_split_key`
`platform.proto:642-653` documents `order_by_ascending`, `limit`, and `start_after_split_key` as split-mode controls applying to *both* per-`In`-value and per-range-distinct-value entries. The dispatcher's `PerInValue` arm calls `execute_document_count_per_in_value_no_proof(contract_id, document_type, document_type_name, where_clauses, transaction, platform_version)` with no options arg, and the executor at lines 1526-1607 iterates the In array in input order, dedupes via `BTreeSet`, and emits every entry with no limit, no skip, and no sort. Only the `RangeNoProof` arm threads these into `RangeCountOptions`. The new abci-side limit clamp (`packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs:143-145`) is a no-op for the PerInValue path: callers passing `limit=1` still get all N In entries back, and `start_after_split_key` cursors are ignored. Either honor the options for `PerInValue` (sort + skip + clamp) or carve them out of the proto contract for In mode.
- [BLOCKING] lines 1526-1606: PerInValue no-proof count path is unbounded — request-amplification DoS
`execute_document_count_per_in_value_no_proof` reads `in_clause.value.as_array()` directly (line 1544) and loops over every element, performing one `find_countable_index_for_where_clauses` + `count_query.execute_no_proof` GroveDB walk per value. The 100-element cap in `WhereClause::in_values()` (`packages/rs-drive/src/query/conditions.rs:354-365`) is **not** on this path: the abci handler builds `WhereClause`s via `WhereClause::from_components` (`packages/rs-drive/src/query/conditions.rs:477-525`), which performs no length validation, and the dispatcher's `PerInValue` arm doesn't apply `max_query_limit` either — only `RangeNoProof` does. Since DAPI accepts gRPC messages up to 64 MiB and CBOR-encoded small integers cost ~1-2 bytes each, an unauthenticated client can issue a single `getDocumentsCount` carrying hundreds of thousands of In values, each triggering an independent GroveDB scan. This bypasses the `max_query_limit` budget the handler is explicitly trying to enforce on the parallel range path. Either invoke `WhereClause::in_values()` to inherit the existing 100-cap + dedup + non-empty checks, clamp `in_values.len()` against `drive_config.max_query_limit` analogously to the RangeNoProof arm, or push an effective_limit into the PerInValue executor.
- [SUGGESTION] lines 1362-1368: `startsWith` is advertised as a supported range-count operator but rejected at runtime
`is_range_operator` (line 144) lists `WhereOperator::StartsWith` as range-capable, `find_range_countable_index_for_where_clauses` will pick a covering index for it, and `platform.proto:627` enumerates `startsWith` alongside `>`/`<`/`between*` as a supported range form. But `range_clause_to_query_item` returns `InvalidWhereClauseComponents("startsWith is not yet supported on the range_countable count fast path")`. Two layers disagree on what the count fast path supports, and clients trying the documented operator pass mode detection only to hit a late runtime failure when the path query is materialized. Either drop `StartsWith` from `is_range_operator` and the proto comment until the byte-incremented upper-bound encoding is wired, or implement it before publishing the doc claim.
- [SUGGESTION] lines 638-877: `execute_split_count` / `expand_split_prefix_paths` / `collect_split_at_prefix` / `find_countable_index_for_split` are dead in production
After the unified-handler refactor, every production caller of `DriveDocumentCountQuery` builds the struct with `split_by_property: None` (mod.rs:1514, 1596, 1642, 1677), and the `PerInValue` arm dispatches to `execute_document_count_per_in_value_no_proof` (Equal-on-each-value subqueries) rather than `execute_no_proof` with a split property. Grep confirms `split_by_property: Some(_)` is set only in `tests.rs:261` and `:557`. The `execute_split_count` / `expand_split_prefix_paths` / `collect_split_at_prefix` / `find_countable_index_for_split` helpers and the `Some(_)` arm of `execute_no_proof` are no longer reachable from any production path — ~250 lines of working-but-orphaned logic plus tests pinning behavior nothing dispatches to. Either delete them (and trim the dependent tests) so the file's surface matches what the dispatcher exercises, or wire `PerInValue` to use `execute_split_count` if its prefix-walk encoding is preferable to the per-value Equal-on-each loop.
In `packages/dapi-grpc/clients/platform/v0/web/platform_pb.js`:
- [BLOCKING] lines 26408-26429: Generated JS bindings ignore `[jstype = JS_STRING]` on new `CountEntry.count` — counts > 2^53−1 silently round in JS
The proto field is `uint64 count = 2 [jstype = JS_STRING]` at `platform.proto:669`, with the doc-comment immediately above explicitly citing the convention. The checked-in JS bindings don't honor it: `platform_pb.js:26420` calls `reader.readUint64()` (returns Number, not String); `:26378` uses `getFieldWithDefault(msg, 2, 0)` (numeric default); `platform_pb.d.ts:2587-2604` types `count: number`; and `:26461` calls `writer.writeUint64()`. For comparison the existing `count = 2 [jstype = JS_STRING]` field at `platform.proto:425` correctly emits `readUint64String()` and `getCount(): string` (`platform_pb.js:18496-18539`), so the generator does support the annotation — the in-tree artifact is genuinely stale. The same loss-of-precision is duplicated in `packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js`. Re-run protoc against the current proto so the new field's wire decoding matches its declared type-precision contract.
In `packages/rs-sdk-ffi/src/document/queries/count.rs`:
- [SUGGESTION] lines 209-215: `dash_sdk_document_split_count` C ABI silently changed without symbol/version bump
The exported `extern "C" fn dash_sdk_document_split_count` keeps the same name but its parameter list shrank from 5 args `(sdk, contract, doc_type, split_property, where_json)` to 4 args `(sdk, contract, doc_type, where_json)`. cbindgen at `packages/rs-sdk-ffi/build.rs` regenerates the header, but any iOS/Swift wrapper still linked against the previous header will continue to push five arguments; on arm64/x86_64 the callee will read what was the old `split_property` slot as `where_json`, fail `serde_json::from_str`, and return an `InternalError` JSON with no compile- or link-time signal. Pre-release scope acknowledged, but consider renaming (e.g. `_v2` suffix) so stale wrappers fail loudly at link time rather than at runtime.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings in the top-level review body for the dispatcher-assigned commit.
…helpers Two related thepastaclaw findings on PR #3623, both confined to the count-query module: 1. PerInValue executor was unbounded — request-amplification DoS. `execute_document_count_per_in_value_no_proof` walks every In value with one independent `count_query.execute_no_proof` GroveDB scan, so its iteration cost is proportional to the input array length rather than `max_query_limit`. The output `limit` truncation that 3ef2ca3 added is cosmetic at that point — the work has already run. With DAPI accepting 64 MiB messages and small CBOR-encoded uint64s costing 1-2 bytes each, an unauthenticated client could schedule arbitrarily many backend reads in one request. Switch the executor's value extraction from `value.as_array()` to `in_clause.in_values().into_data_with_error()??`, which inherits the existing 100-element cap + non-empty + no-duplicates validator that `mod.rs:1246` and `conditions.rs:852` already use for In consumers. Same defensive bound regular query path applies via `WhereClause::from_clause`. Pin the cap with a unit test that drives the executor directly with a 101-element array and asserts the rejection message. 2. Delete ~250 lines of dead split-count helpers. After the unified-handler refactor, every production caller of `DriveDocumentCountQuery` builds the struct with `split_by_property: None`. PerInValue dispatches to `execute_document_count_per_in_value_no_proof` (Equal-on-each-value subqueries), not to `execute_no_proof` with `split_by_property: Some(_)`. So the `Some(_)` arm of `execute_no_proof` plus the four helpers it transitively calls (`find_countable_index_for_split`, `execute_split_count`, `expand_split_prefix_paths`, `collect_split_at_prefix`) are unreachable from any production path, along with the three tests pinning their behavior. Delete just the helpers + their dependent tests, and narrow `execute_no_proof` to the unconditional `execute_total_count` body. Leave the `split_by_property: Option<String>` field on the struct so the existing `split_by_property: None` call sites (server dispatcher, SDK, tests) keep compiling without churn — the field is now a no-op but removing it would touch dozens of unrelated files. Net: -392 lines.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs (1)
607-793: ⚡ Quick winAdd test coverage for
range_countableimmutability.The validation logic correctly rejects
range_countablechanges, but no tests verify this behavior. Consider adding parallel tests to the existingcountableimmutability suite:
should_return_invalid_result_if_range_countable_changed_from_false_to_trueshould_return_invalid_result_if_range_countable_changed_from_true_to_false- (optional) compound index test for
range_countablechangesThese tests would document the immutability constraint and prevent regressions if the validation logic is later modified.
📋 Example test structure
#[test] fn should_return_invalid_result_if_range_countable_changed_from_false_to_true() { let platform_version = PlatformVersion::latest(); let document_type_name = "test"; let old_indices = vec![Index { name: "test".to_string(), properties: vec![IndexProperty { name: "test".to_string(), ascending: false, }], unique: false, null_searchable: true, contested_index: None, countable: IndexCountability::NotCountable, range_countable: false, }]; let new_indices = vec![Index { name: "test".to_string(), properties: vec![IndexProperty { name: "test".to_string(), ascending: false, }], unique: false, null_searchable: true, contested_index: None, countable: IndexCountability::NotCountable, range_countable: true, }]; let old_index_structure = IndexLevel::try_from_indices(&old_indices, document_type_name, platform_version) .expect("failed to create old index level"); let new_index_structure = IndexLevel::try_from_indices(&new_indices, document_type_name, platform_version) .expect("failed to create new index level"); let result = old_index_structure.validate_update(document_type_name, &new_index_structure); assert_matches!( result.errors.as_slice(), [ConsensusError::BasicError( BasicError::DataContractInvalidIndexDefinitionUpdateError(e) )] if e.index_path() == "test -> (range_countable changed)" ); }🤖 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-dpp/src/data_contract/document_type/index_level/mod.rs` around lines 607 - 793, Add tests mirroring the existing countable immutability suite to cover range_countable changes: implement tests named like should_return_invalid_result_if_range_countable_changed_from_false_to_true, should_return_invalid_result_if_range_countable_changed_from_true_to_false (and optionally a compound index test). Each test should construct old and new Index vectors (using Index, IndexProperty, IndexCountability as in existing tests) that only toggle range_countable, build IndexLevel instances via IndexLevel::try_from_indices, call IndexLevel::validate_update, and assert the result contains a DataContractInvalidIndexDefinitionUpdateError with e.index_path() matching "test -> (range_countable changed)" (or the compound path "first -> second -> (range_countable changed)").
🤖 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 `@book/src/drive/document-count-trees.md`:
- Line 369: The docs text for the migration check uses snake_case
`range_countable` which is inconsistent with the public schema/contract JSON
casing used elsewhere; update the example/verbiage to use `rangeCountable`
instead of `range_countable` so references to the per-index flags (`countable`,
`rangeCountable`) and the `GetDocumentsCount` behavior match the rest of the
chapter and SDK/API examples.
- Around line 179-180: Clarify the contradictory guidance by splitting the rules
into path/mode-specific statements: explicitly state that for the query handler
(reference symbols: Equal, In, range, InvalidArgument, between*), in "no-prove"
mode Equal/In may be used to cover an index prefix with a single range
terminator, but in "prove" mode mixing In and a range is rejected and the
handler returns InvalidArgument (use between* for two-sided ranges); update the
paragraph to clearly label these two cases (no-prove vs prove) and describe the
allowed/forbidden combinations and the exact error behavior.
In `@packages/rs-sdk/src/platform/documents/document_count_query.rs`:
- Around line 179-187: The request builder in DocumentCountQuery is hard-coding
prove: true which prevents no-proof distinct/pagination options
(return_distinct_counts_in_range, order_by_ascending, limit,
start_after_split_key) from reaching the no-proof executors; update the fetch
path used by the DocumentCountQuery/Fetch flow to conditionally set prove based
on whether those distinct/pagination knobs are set (or alternatively validate
and reject those setters early), e.g., propagate a new boolean or enum from the
DocumentCountQuery builder into the code that constructs the transport request
instead of always assigning prove: true, add a code path that performs a
no-proof fetch when appropriate, and add an SDK integration test that exercises
with_distinct_counts_in_range(true) plus order/limit/start_after_split_key to
verify the no-proof executor is used and the server accepts the request.
- Around line 390-398: The total-count branch in DocumentSplitCounts is calling
DocumentCount::maybe_from_proof_with_metadata via FromProof<DriveDocumentQuery>,
bypassing the new range-proof verifier; change the call to use
FromProof<DocumentCountQuery> for DocumentCount (i.e. invoke
DocumentCount::maybe_from_proof_with_metadata via the DocumentCountQuery path so
AggregateCountOnRange is used), then take the verified DocumentCount result and
insert it into the returned map under the empty key ("" or the same empty-key
used elsewhere) so the function returns a single-entry map with the verified
count; update the call sites that currently reference DriveDocumentQuery and
ensure the provider/network/platform_version args are forwarded unchanged.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/index_level/mod.rs`:
- Around line 607-793: Add tests mirroring the existing countable immutability
suite to cover range_countable changes: implement tests named like
should_return_invalid_result_if_range_countable_changed_from_false_to_true,
should_return_invalid_result_if_range_countable_changed_from_true_to_false (and
optionally a compound index test). Each test should construct old and new Index
vectors (using Index, IndexProperty, IndexCountability as in existing tests)
that only toggle range_countable, build IndexLevel instances via
IndexLevel::try_from_indices, call IndexLevel::validate_update, and assert the
result contains a DataContractInvalidIndexDefinitionUpdateError with
e.index_path() matching "test -> (range_countable changed)" (or the compound
path "first -> second -> (range_countable changed)").
🪄 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: d1301141-2f98-483f-8494-13b1f2832178
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
book/src/drive/document-count-trees.mdpackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/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-dpp/Cargo.tomlpackages/rs-dpp/src/data_contract/document_type/index_level/mod.rspackages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/src/query/document_count_query/v0/mod.rspackages/rs-drive-proof-verifier/src/lib.rspackages/rs-drive-proof-verifier/src/proof/document_count.rspackages/rs-drive/Cargo.tomlpackages/rs-drive/src/query/drive_document_count_query/mod.rspackages/rs-drive/src/query/drive_document_count_query/tests.rspackages/rs-platform-version/Cargo.tomlpackages/rs-platform-wallet/Cargo.tomlpackages/rs-sdk-ffi/src/document/queries/count.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/platform/documents/document_count_query.rs
✅ Files skipped from review due to trivial changes (4)
- packages/rs-sdk/Cargo.toml
- packages/rs-dpp/Cargo.toml
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h
- packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-platform-version/Cargo.toml
- packages/rs-platform-wallet/Cargo.toml
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts
- packages/rs-drive-abci/src/query/document_count_query/v0/mod.rs
- packages/rs-sdk-ffi/src/document/queries/count.rs
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.js
- packages/dapi-grpc/protos/platform/v0/platform.proto
…Query verifier CodeRabbit finding on PR #3623: the total-count branch in `FromProof<DocumentCountQuery> for DocumentSplitCounts` (no `In` clause) was calling `<DocumentCount as FromProof<DriveDocumentQuery>>::...` directly. That path runs the materialize-and-count verifier, which can't decode `AggregateCountOnRange` proofs — so range-only requests through `DocumentSplitCounts::fetch` would fail verifier-side even though `DocumentCount::fetch` started supporting range proofs in 8fb7a47. Route through `<DocumentCount as FromProof<DocumentCountQuery>>::...` instead so the dispatch in 8fb7a47's SDK-level impl picks the right verifier (merk-level aggregate for ranges, materialize-and-count for point lookups). Also add a new e2e test covering the dual-`range_countable` index layout that the book documents but no test directly exercises: `byColor [color]` and `byColorSize [color, size]` both with `rangeCountable: true`. The test asserts: 1. The NonCounted-wrapping invariant still holds when the wrapped sub-tree is itself a `ProvableCountTree` (the existing single-doc test only reaches `NonCounted<NormalTree>`). With 4 distinct sizes under "red", a missing wrapper would push red's count to ≥5; the test pins it at exactly 4. 2. The "find the most common color" client pattern works end-to-end: distinct-range count over a wide-open range (`color > ""`) returns per-color counts, client sorts by count desc, takes the first.
…e_countable immutability Three CodeRabbit findings on PR #3623, all small: 1. book: `In + range` guidance was contradictory. Line 138 says `Equal/In` clauses can cover the index prefix on the no-prove range path; line 179 said the handler rejects `In + range`. Both are true at different layers: the rs-drive `execute_range_count_no_proof` executor *does* accept `In`-on-prefix + range-on-terminator (covered by `range_count_with_in_on_prefix _forks_and_merges`), but the unified `GetDocumentsCount` request handler rejects the combination because the proto contract makes `In` doubly meaningful (cartesian-fork covering AND per-value split signal). Pairing it with a range would conflict with `return_distinct_counts_in_range`. Reword line 179 to state explicitly that the request-handler constraint is at a different layer than the executor's capability. 2. book: snake_case `range_countable` in a contract-creation context was inconsistent with the JSON-schema `rangeCountable` casing used elsewhere in the chapter. One-character fix. 3. dpp: `range_countable` is just as load-bearing for state-sync determinism as `countable` itself (same tree-shape consequences), but the immutability validation only had test coverage for `countable`. Add three parallel tests (`should_return_invalid_result_if_range_countable_changed_*`) mirroring the existing `countable` suite — single-prop false→true, single-prop true→false, and compound. Each pins the `(range_countable changed)` error path emitted by the validator.
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.
Summary
Collapse the two count gRPC endpoints (`getDocumentsCount` + `getDocumentsSplitCount`) into a single unified `getDocumentsCount` that handles both modes via the where clauses. Split semantics are signalled by an `In` clause: the In's field becomes the split property and the In's array enumerates the values to count. No In clause → single total count.
This is a wire-format breaking change. Pre-release, so OK.
Wire format (this PR)
`GetDocumentsCountRequestV0` gets new fields:
`GetDocumentsCountResponseV0.result` becomes `oneof { CountResults counts; Proof proof; }`. `CountResults` carries `repeated CountEntry { bytes key; uint64 count; }`. Total count is one entry with empty `key`; per-In-value counts are one entry per In value.
`GetDocumentsSplitCount{Request,Response}` deleted.
Mode dispatch (handler)
Per-layer changes
Status
Not yet in this PR (follow-ups, in priority order)
Depends on (separate work)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
API Changes
Documentation