Repository navigation
feat(sdk): query the elements of typed arrays over the wire (PV14) - #5372
Conversation
An index may name a top-level typed array, at any position and one per index: the document is entered once under each distinct element, keyed as a property of the element's type is. An empty or absent list is entered nowhere (the array is always in the index's skip set). A replace diffs the old and new entries of an index naming an array or bucketing its first property, inserting first and deleting last; delete removes every element's entry. Queries read the elements with `contains` (one element, cursors allowed) and `containsAny` (several, repeats dropped on the server and in verification, no cursor); counts, sums and averages read several elements only grouped per element, and a ranking of the array ranks elements by the documents holding them. Limits (SYSTEM_LIMITS_V4): an indexed array holds at most 32 elements, or 8 under a timeRange or integerRange window, and a document of such a type writes at most 256 index entries. Unique, contested, indexOnly, nullSearchable false and rankings above the array are refused. Refs #3696 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…es are A typed array's `contains` and `containsAny` values were checked against the element type's in-memory value kinds, which refused an identifier element given as base58 text (or a byte array as base64), the way clients send them and scalar index values are accepted. A value is now judged by whether it serializes as an element key, through one check shared by documents queries and the aggregate keying (`WhereClause::validate_array_element_clause`). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 41 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…inct A count, sum or average keys `contains ""` as an equality, whose skip binding counts an empty string as possibly missing, so the array's index was refused. An array is now bound by any constraint on it (`SkipIfAbsentBinding::constrains`); an ordering alone still binds nothing. The worst-case document stamped string elements from 26 letters, so a one-character element repeated past the 26th and the estimate priced fewer entries than a document of 32 distinct elements writes. It now uses the 94 printable ASCII characters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3b93235 to
2050c6d
Compare
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit 5327f7b. Normal review starts when eligible; priority review starts as soon as a slot is available. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The aggregate proof helpers have a blocking verification flaw: a valid per-element proof can be accepted as an unsupported, duplicate-counted total. Four additional suggestions address binary binding inputs, nested version gating, historical-generation documentation, and SDK proof round-trip coverage. Verification was static against the exact head; no builds or tests were run, and the supplied CI snapshot reports successful completed checks.
🔴 1 blocking | 🟡 4 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Preserve binary operands in element-query bindings
packages/wasm-sdk/src/queries/document.rs:533-545
The newly documented identifier query does not work when identityId is a Uint8Array. DocumentsFacade.query forwards it unchanged, but DocumentsQueryInput.where_clauses deserializes into serde_json::Value. deserialize_required_query uses wasm_dpp2::from_object, which preserves the typed array as platform bytes; the platform-value deserializer then calls visit_bytes, which serde_json's value visitor does not support. Both a binary contains operand and containsAny candidates supplied as typed arrays fail before this parser runs.
Using plain numeric arrays instead does not preserve binary semantics either: this conversion produces Value::Array containing numeric values, and a containsAny candidate list becomes nested arrays that value_to_proto_at_depth rejects. The JSON-backed FFI search/count parsers have that same nested-list problem. These numeric arrays are not already valid byte operands—the converters produce I64 values, while Drive's array-to-bytes conversion expects U8.
Preserve platform byte values through the WASM input boundary and normalize supported JSON binary representations using the element schema before wire encoding. Keep the wire's flat-list restriction and add binding coverage for binary contains and containsAny operands.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This cross-cutting change modifies peer-facing wire deserialization in rs-drive-abci's where_operator_from_proto and proof-query reconstruction in dash-platform-queries' aggregate proof helpers, combining substantial integration complexity with critical network and proof-verification surfaces. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/dash-platform-queries/src/documents/count_proof_helpers.rs`:
- [BLOCKING] packages/dash-platform-queries/src/documents/count_proof_helpers.rs:181-189: Enforce per-element aggregate restrictions before verifying proofs
Passing `true` unconditionally bypasses the restriction that Drive derives from the original grouping and range shape. Drive permits `containsAny` aggregates only for `GroupByIn` without a range, but this helper also lowers an ungrouped request to ordinary `In`. Both ungrouped `Aggregate` and grouped `GroupByIn` then select `PointLookupProof`, and `point_lookup_count_path_query` does not bind the grouping mode.
A malicious responder can therefore return a genuine proof for `COUNT(*) WHERE hashtags containsAny ['dash', 'news'] GROUP BY hashtags` to an otherwise identical ungrouped request. The reconstructed lookup query and signed state root match, so verification succeeds. `DocumentCount` sums the authenticated branch counters and returns 2 for one document holding both tags, although Drive deliberately refuses that total. The signature authenticates the counters, not that their groups are disjoint. The same unconditional allowance in the sum and average helpers permits duplicate-counted sums and multiplicity-weighted averages.
Derive the allowance from the original request shape before lowering erases the element operator, matching Drive's grouping/range restriction in all three helpers. Add a regression that supplies a valid grouped proof to an ungrouped request and requires rejection.
In `packages/wasm-sdk/src/queries/document.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document.rs:533-545: Preserve binary operands in element-query bindings
The newly documented identifier query does not work when `identityId` is a `Uint8Array`. `DocumentsFacade.query` forwards it unchanged, but `DocumentsQueryInput.where_clauses` deserializes into `serde_json::Value`. `deserialize_required_query` uses `wasm_dpp2::from_object`, which preserves the typed array as platform bytes; the platform-value deserializer then calls `visit_bytes`, which serde_json's value visitor does not support. Both a binary `contains` operand and `containsAny` candidates supplied as typed arrays fail before this parser runs.
Using plain numeric arrays instead does not preserve binary semantics either: this conversion produces `Value::Array` containing numeric values, and a `containsAny` candidate list becomes nested arrays that `value_to_proto_at_depth` rejects. The JSON-backed FFI search/count parsers have that same nested-list problem. These numeric arrays are not already valid byte operands—the converters produce `I64` values, while Drive's array-to-bytes conversion expects `U8`.
Preserve platform byte values through the WASM input boundary and normalize supported JSON binary representations using the element schema before wire encoding. Keep the wire's flat-list restriction and add binding coverage for binary `contains` and `containsAny` operands.
In `packages/dash-platform-queries/src/documents/document_query.rs`:
- [SUGGESTION] packages/dash-platform-queries/src/documents/document_query.rs:819-821: Apply the element-operator version gate to composite sub-queries
The activation check scans only the page's `where_clauses`. `check_composite_shape` does not reject element operators in sub-queries, and `sub_queries_to_proto` subsequently encodes their clauses through the mapper that this PR changes to accept `Contains` and `ContainsAny`. An ordinary page with either operator only in a sub-query therefore emits discriminant 13 or 14 for PV12/PV13, which already use the V1 wire, instead of returning the SDK's PV14 configuration error.
Older servers remain fail-closed, so this is not a consensus or proof-soundness blocker. Extend the existing grammar-generation check to every composite sub-query's clauses and add pre-PV14 encoding tests for both nested operators.
In `packages/rs-drive-abci/src/query/document_query/v1/compute_aggregate_mode_and_check_limit/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/document_query/v1/compute_aggregate_mode_and_check_limit/v0/mod.rs:53-59: Record the historical safety argument for the in-place routing edit
Historical query tables still select this v0 implementation, while PV14 reaches it through v2 → v1 delegation. The edit does not establish a historical consensus break: pre-PV14 contract tables set `parse_typed_array` to `None`, so the typed-array state needed for a successful element query cannot exist, and element validation rejects these operators on other property types.
The repository's coding conventions explicitly allow such consensus-inert in-place edits, but require the safety argument at the edited lines and an 'In-place changes to shipped generations' section in the PR description. The current comment explains only the new semantics, and the supplied description lacks that section. Document the historical parser/validation invariant and the versions selecting this generation; a regression for historical rejection would strengthen the justification. A new generation is not required solely because this file is named v0.
In `packages/rs-drive-abci/src/query/document_query/v1/tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/query/document_query/v1/tests.rs:7455-7457: Round-trip element-query proofs through the SDK adapters
The new proof assertion checks only that the server produced nonempty bytes. The `ContainsAny` document and grouped-count cases use `prove: false`, so these tests do not exercise the SDK normalization calls added by this PR. They would pass even if an aggregate proof helper reconstructed a different query from the prover.
Add offline signed-response round trips through the relevant `FromProof<DocumentQuery>` entry points: overlapping `ContainsAny` document results and grouped counts, plus representative SUM/AVG and pinned `Contains` ranked/having cases. The neighboring `time_range_proof_verification` module already provides the prover/verifier setup. Keep the grouped-proof substitution rejection regression requested above as the negative counterpart.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Replace existing WASM cursor-length panics with returned errors — Out of scope: the fallible cursor conversions followed by unwrap are present unchanged in the base revision. This PR adds operator mappings without changing cursor handling or introducing a new cursor precondition, so the adjacent error-handling cleanup is not carried into this review.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
Review fixes for typed array element indexes (PV14). - Grouping v1 keeps `contains`/`containsAny` on `$id` grouped, so the element-clause check refuses it. Before, the primary key slots took only `Equal`/`In` and the clause was dropped: the query ran unfiltered and its proof verified. - A composite sub-query refuses `containsAny`: its documents are read out of the merged proof without the repeats dropped. A count sub-query keys its element clauses as the count surface does. - `answers_per_element` is the one rule for when a count, sum or average reads several elements; the dispatchers use it and the SDK verifiers can too. The refusal names both conditions (grouped by the array alone, no range clause). - `skipIfAbsent` whose only skip property is the indexed array is refused with a reason naming the array, not as a keyword on required properties. - The repeat drop borrows the stored bytes instead of copying them, and reads `ItemWithSumItem` documents as items. - `typed_array_property` and `Index::typed_arrays` replace seven copies of the typed-array lookup. `validate_in_clause_shape` is renamed `validate_where_clause_shape`, the preflight it has become. - Tests: `$id` element clauses refused, `containsAny` in a sub-query, count sub-query keying, the reworded refusals, and the estimated (`apply: false`) create, replace and delete of an array-indexed post. - Stale "a typed array cannot be an index property" docs corrected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2050c6d to
7d19cae
Compare
Review fixes for typed array element indexes (PV14), round two. - `Index::indexed_array` names the typed array an index keys by (stamped by the parser on every parse). The skip gate binds that member of the skip set by any element constraint, as it does without a declared `skipIfAbsent`, while the declared members keep the missing-value rule. Before, on an index such as `[hashtags, language]` with `skipIfAbsent: ["language"]`, a count or sum with `contains ""` was refused: keyed as `== ""`, the element read as a possibly missing value. - `execute_document_count_and_sum_request` is crate-internal: it takes the average dispatcher's already keyed clauses and cannot tell a keyed `contains` from a raw equality on the array. - The e2e storage helpers treat only a missing path or key as absent; any other read error fails the test. - Tests: per-element sums and averages, proved and unproved, through an amount-only replace, an element change and a delete, with a total over several elements refused; the empty-string element beside a declared skip property; the key codec at protocol version 13 and latest (scalar keys frozen and alike, generation 0 refusing an array key, element keys only from 14); the parser's `indexed_array` stamp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7d19cae to
f133921
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The exact-head diff addresses four prior findings, including the aggregate proof-substitution blocker, and preserves existing wire discriminants and dependency boundaries. Three non-blocking binding and validation issues remain: advertised raw-byte operands, ranked/having TypeScript pins, and historical element-operator acceptance in standalone WASM verifiers. Verification was static only; the supplied head-specific CI snapshot still has build/test-related checks queued or pending.
🟡 3 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Preserve binary operands in element-query bindings
packages/wasm-sdk/src/queries/document.rs:544-545
The new operator mappings do not make the README's advertised byte-valued identity operand usable. DocumentsFacade.query forwards the input unchanged, and deserialize_required_query uses from_object: a Uint8Array becomes platform Value::Bytes, but DocumentsQueryInput.where_clauses is Option<Vec<serde_json::Value>>. The platform-value deserializer invokes visit_bytes, which serde_json's value visitor does not implement, so the query fails before this operand conversion.
Replacing Uint8Array with a plain numeric array does not solve it: json_to_platform_value creates Array, while to_identifier_bytes and to_binary_bytes accept array members only as U8. A containsAny list of numeric byte arrays also hits the wire encoder's nested-list rejection. The FFI JSON converters share that numeric-array limitation; encoded identifier and binary strings remain usable.
The underlying marshalling limitation predates this PR, but the newly added 'base58 or bytes' example exposes it as a supported element-query API. Either preserve binary platform values through the WASM operand path and cover contains/containsAny, or narrow the new README example to base58 strings. A broader FFI/parser redesign is not required merely to correct the newly advertised contract.
source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)
🟡 Suggestion: Expose element pins in the ranked/having TypeScript contract
packages/wasm-sdk/src/queries/document_ranked.rs:105-107
The new ranked and having pre-checks correctly normalize a typed-array contains pin through Drive's shared helper, but DocumentsIndexPin still permits only equality and IN. Both DocumentsRankedQuery.where and DocumentsHavingQuery.where use this type, and the evo-sdk facade exposes those interfaces directly. TypeScript callers therefore cannot express the newly supported pinned-element query without a cast; substituting == is not equivalent because Drive rejects equality on a typed array.
Add contains/Contains to the single-value pin alternative and update the associated pin documentation. Keep containsAny excluded, since these surfaces deliberately permit only one pinned array element, and add a TypeScript API fixture for the newly supported spelling.
export type DocumentsIndexPin =
| [string, '==' | '=' | 'contains' | 'Contains', unknown]
| [string, 'in' | 'In', unknown[]];
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The cross-cutting change is intricate and modifies cryptographic proof acceptance in array_element_clauses.rs and the count, sum, and average proof helpers, where incorrect multi-element verification could accept double-counted aggregate totals. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/wasm-sdk/src/queries/document.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document.rs:544-545: Preserve binary operands in element-query bindings
The new operator mappings do not make the README's advertised byte-valued identity operand usable. DocumentsFacade.query forwards the input unchanged, and deserialize_required_query uses from_object: a Uint8Array becomes platform Value::Bytes, but DocumentsQueryInput.where_clauses is Option<Vec<serde_json::Value>>. The platform-value deserializer invokes visit_bytes, which serde_json's value visitor does not implement, so the query fails before this operand conversion.
Replacing Uint8Array with a plain numeric array does not solve it: json_to_platform_value creates Array<I64>, while to_identifier_bytes and to_binary_bytes accept array members only as U8. A containsAny list of numeric byte arrays also hits the wire encoder's nested-list rejection. The FFI JSON converters share that numeric-array limitation; encoded identifier and binary strings remain usable.
The underlying marshalling limitation predates this PR, but the newly added 'base58 or bytes' example exposes it as a supported element-query API. Either preserve binary platform values through the WASM operand path and cover contains/containsAny, or narrow the new README example to base58 strings. A broader FFI/parser redesign is not required merely to correct the newly advertised contract.
In `packages/wasm-sdk/src/queries/document_ranked.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document_ranked.rs:105-107: Expose element pins in the ranked/having TypeScript contract
The new ranked and having pre-checks correctly normalize a typed-array contains pin through Drive's shared helper, but DocumentsIndexPin still permits only equality and IN. Both DocumentsRankedQuery.where and DocumentsHavingQuery.where use this type, and the evo-sdk facade exposes those interfaces directly. TypeScript callers therefore cannot express the newly supported pinned-element query without a cast; substituting == is not equivalent because Drive rejects equality on a typed array.
Add contains/Contains to the single-value pin alternative and update the associated pin documentation. Keep containsAny excluded, since these surfaces deliberately permit only one pinned array element, and add a TypeScript API fixture for the newly supported spelling.
In `packages/wasm-drive-verify/src/document/verify_proof.rs`:
- [SUGGESTION] packages/wasm-drive-verify/src/document/verify_proof.rs:280-281: Gate standalone WASM element operators on PV14
These new parser arms accept element operators for historical platform versions, but this wrapper builds InternalClauses directly instead of calling its versioned extract_from_clauses path. At PV13, validate_where_clause_shape checks only the number of IN clauses; the element-type validation runs only in the PV14 generation. A Contains clause supplied in equal_clauses on an indexed scalar consequently reaches to_path_query, whose as_key_operator maps it to equality. A genuine scalar-equality proof can therefore verify for an element query that the historical server rejects. The absence of historical typed-array state does not protect this manually constructed path.
The same parser bypass exists in verify_proof_keep_serialized.rs and verify_start_at_document_in_proof.rs. Pass PlatformVersion into parsing and reject element operators before the PV14 grammar, or reconstruct the clauses through versioned extraction. Add historical rejection coverage and PV14 standalone-wrapper proof round trips without changing shipped Drive generations.
This is a standalone client query-validation defect, not a consensus change or a bypass of Merkle authentication, so it is retained as a suggestion rather than the proposed blocker.
| "Contains" => WhereOperator::Contains, | ||
| "ContainsAny" => WhereOperator::ContainsAny, |
There was a problem hiding this comment.
🟡 Suggestion: Gate standalone WASM element operators on PV14
These new parser arms accept element operators for historical platform versions, but this wrapper builds InternalClauses directly instead of calling its versioned extract_from_clauses path. At PV13, validate_where_clause_shape checks only the number of IN clauses; the element-type validation runs only in the PV14 generation. A Contains clause supplied in equal_clauses on an indexed scalar consequently reaches to_path_query, whose as_key_operator maps it to equality. A genuine scalar-equality proof can therefore verify for an element query that the historical server rejects. The absence of historical typed-array state does not protect this manually constructed path.
The same parser bypass exists in verify_proof_keep_serialized.rs and verify_start_at_document_in_proof.rs. Pass PlatformVersion into parsing and reject element operators before the PV14 grammar, or reconstruct the clauses through versioned extraction. Add historical rejection coverage and PV14 standalone-wrapper proof round trips without changing shipped Drive generations.
This is a standalone client query-validation defect, not a consensus change or a bypass of Merkle authentication, so it is retained as a suggestion rather than the proposed blocker.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
…a window An indexed typed array's `maxItems` is held to 16 (was 32), or 4 when the index also has a `timeRange` or `integerRange` window (was 8), for the first release: `SYSTEM_LIMITS_V4.max_indexed_array_items` and `max_windowed_indexed_array_items`, edited in place since protocol version 14 is unreleased. A later limits table can raise them without stranding a contract; lowering them after release could. The 256-entry budget per document is unchanged. With 16 elements and 10 indexes to a type, plain array indexes stay under it (160 entries); only windows reach it, so its test now spends it on two windowed indexes of a 4-element array (96 entries each) and plain ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`getDocuments` v1 gains `CONTAINS = 13` and `CONTAINS_ANY = 14`; the regenerated clients carry them. drive-abci decodes them into Drive's element operators and routes `containsAny` grouped by the array as a per-element count. dash-platform-queries encodes them (refused below protocol version 14) and keys element clauses before verifying a count, sum, average, ranked or having proof, as the server does. wasm-sdk, the FFI search and count parsers and wasm-drive-verify accept `contains` and `containsAny`, and the evo-sdk README documents them. Refs #3696 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review fixes for the typed array element wire and SDKs (PV14). - The count, sum and average proof verifiers keyed element clauses with several elements always allowed, trusting the server to have refused a total over several. A dishonest node could prove the per-element counts of such a total and the SDK would sum them, counting a post holding two of the elements twice. They now pass the server's own rule (`aggregate_answers_per_element`, over drive's `answers_per_element`), so they refuse what the server refuses. - One helper, `documents::array_element_clauses::key_element_clauses`, keys element clauses for the five verifiers and the wasm-sdk ranked and having pre-checks instead of seven copies. - Test names start with "should", and the abci wire tests import their types instead of spelling `crate::` paths inline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review fixes for the typed array element wire and SDKs (PV14), round two. - The encoder's protocol version 14 gate for `contains`/`containsAny` also reads composite sub-queries, which ride the same wire mapping; an element operator only in a sub-query was encoded for protocol version 13 instead of refused with the version floor. - The in-place edit of the protocol version 13 routing table states its safety argument where it is made, and a test pins it: at protocol version 13 a documents read and a grouped count with an element operator are both refused. - Proofs from the v1 handler verified through the SDK's `FromProof<DocumentQuery>` entry points: documents under several elements (each post once), per-element counts and sums, a pinned element's average, ranking and having bound within one element, and a grouped proof offered for an ungrouped total, which the SDK refuses (with the earlier blanket allowance it verified a count of 5 for 4 posts). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f133921 to
307c822
Compare
Review fixes for typed array element indexes (PV14), round three. - A document of a type with a `ttl` prepays its cleanup by the index entries its indexed arrays' `maxItems` allow, and a replace pays only for its body's growth (one long element can make way for many short ones of the same size). So an update may no longer raise an indexed array's `maxItems` on such a type (`DocumentTypeUpdateError`, 40212, in update validation v1, the protocol version 14 generation). - The cost model sizes a string element at least one character when its type allows one: a `maxLength: 1` element with no positive `minLength` had a middle length of 0, so every sample element was empty and the quote priced one entry instead of `maxItems`. - The nested insert and delete walkers move the path into the last value instead of cloning it for every value. - Tests: elements across overlapping windows (every window and element entered, an overlapping update, a delete pruning per window while another post keeps a shared element); sustained writes draining expired windows of an indexed array with a property below it, and a replace and a delete over drained windows (without the drain budget's `maxItems` term it falls 19 to 46 seconds behind); the time-to-live bound rule; the short-string sizing (fails without the fix). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The aggregate-proof restriction, composite version gate, historical-safety documentation, and signed SDK round-trip coverage are present and address four prior findings. Three non-blocking binding/API issues remain. Validation was static only: the supplied exact-head CI snapshot reports successful NPM release validation and Kotlin build/tests, but contains no dedicated Rust-suite result.
🟡 3 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Expose element pins in the ranked/having TypeScript contract
packages/wasm-sdk/src/queries/document_ranked.rs:105-107
The parser and ranked/having prechecks now accept a typed-array prefix pinned with contains, but the exported DocumentsIndexPin union still permits only equality and IN. Both DocumentsRankedQuery.where and DocumentsHavingQuery.where use this union, and evo-sdk exposes those types directly. A valid pin such as where: [['hashtags', 'contains', 'dash']] therefore fails TypeScript checking unless the caller casts away the public contract. Equality is not a workaround because Drive rejects ordinary equality on a typed array. Add the accepted contains/Contains spellings to the scalar-pin alternative and document their element semantics. Keep containsAny excluded, matching the runtime's deliberate single-element restriction.
export type DocumentsIndexPin =
| [string, '==' | '=' | 'contains' | 'Contains', unknown]
| [string, 'in' | 'In', unknown[]];
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)
🟡 Suggestion: Preserve binary operands in element-query bindings
packages/wasm-sdk/src/queries/document.rs:544-545
The new operators still pass through JSON-only where-clause inputs. DocumentsQueryInput.where_clauses and the ranked/having equivalents store serde_json::Value, while deserialize_required_query uses from_object, which preserves a JavaScript Uint8Array as platform Value::Bytes. Deserializing that value into JSON invokes visit_bytes, which JSON's value visitor does not support, so the README's mentions contains identityBytes form fails before this parser runs. Nested Uint8Array candidates in containsAny fail the same way. Plain number arrays are not a substitute: this converter produces I64 elements, whereas the identifier/binary encoders accept arrays of U8. The FFI search/count JSON converters have the same numeric-array mismatch. Base58 identifiers and base64 binary text remain usable; the defect is the byte-input spelling. Preserve platform values through the WASM input/parser boundary and normalize JSON byte operands according to the declared element type where supported. Keep the outer containsAny candidate list distinct from each binary element, and add binding-level binary coverage.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate cross-layer change modifies proof verification in dash-platform-queries/src/documents/array_element_clauses.rs and the aggregate proof helpers, where element keying and multi-element eligibility determine whether potentially double-counted aggregate proofs are accepted. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/wasm-sdk/src/queries/document_ranked.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document_ranked.rs:105-107: Expose element pins in the ranked/having TypeScript contract
The parser and ranked/having prechecks now accept a typed-array prefix pinned with `contains`, but the exported `DocumentsIndexPin` union still permits only equality and IN. Both `DocumentsRankedQuery.where` and `DocumentsHavingQuery.where` use this union, and evo-sdk exposes those types directly. A valid pin such as `where: [['hashtags', 'contains', 'dash']]` therefore fails TypeScript checking unless the caller casts away the public contract. Equality is not a workaround because Drive rejects ordinary equality on a typed array. Add the accepted `contains`/`Contains` spellings to the scalar-pin alternative and document their element semantics. Keep `containsAny` excluded, matching the runtime's deliberate single-element restriction.
In `packages/wasm-sdk/src/queries/document.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document.rs:544-545: Preserve binary operands in element-query bindings
The new operators still pass through JSON-only where-clause inputs. `DocumentsQueryInput.where_clauses` and the ranked/having equivalents store `serde_json::Value`, while `deserialize_required_query` uses `from_object`, which preserves a JavaScript `Uint8Array` as platform `Value::Bytes`. Deserializing that value into JSON invokes `visit_bytes`, which JSON's value visitor does not support, so the README's `mentions contains identityBytes` form fails before this parser runs. Nested `Uint8Array` candidates in `containsAny` fail the same way. Plain number arrays are not a substitute: this converter produces I64 elements, whereas the identifier/binary encoders accept arrays of U8. The FFI search/count JSON converters have the same numeric-array mismatch. Base58 identifiers and base64 binary text remain usable; the defect is the byte-input spelling. Preserve platform values through the WASM input/parser boundary and normalize JSON byte operands according to the declared element type where supported. Keep the outer `containsAny` candidate list distinct from each binary element, and add binding-level binary coverage.
In `packages/wasm-drive-verify/src/document/verify_proof.rs`:
- [SUGGESTION] packages/wasm-drive-verify/src/document/verify_proof.rs:280-281: Gate standalone WASM element operators on PV14
(existing thread: https://github.com/dashpay/platform/pull/5372#discussion_r4242600610)
All three standalone document verifiers accept the new operators without checking the supplied platform version, then construct `InternalClauses` directly instead of using the versioned where-clause grouper. At PV13, `validate_where_clause_shape` checks only the number of IN clauses; it does not validate element operators. A `Contains` clause supplied in `equal_clauses` on an ordinary indexed string field therefore reaches the historical secondary-index lowering, where `to_path_query` maps it to equality. An ordinary equality proof can consequently verify under that inactive element request, although the regular PV13 query path rejects it. This is an activation/input-validation mismatch, not a cryptographic bypass. Apply the activation check before verification in these exports, or enforce it through shared Drive preflight/grouping that also covers directly constructed clauses. Add a regression offering a valid historical equality proof under a `Contains` request.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Return an input error for malformed standalone-verifier cursors — Duplicate of the pre-existing malformed-cursor observation. The unchanged cursor handling is outside this PR's scope.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
…s queries do A filter validated an element clause by the scalar rules of the element type, so a boolean array's containsAny was refused, and its containsAny matched only a list, not the bytes a u8 element list may be given as. Both validators now share one element-value rule, and containsAny matches each element as an in. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s out The cleanup weight multiplied every level of an index naming an array by maxItems, though a delete walks the levels above the array once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves serialize_value_for_key generation 1 between #5350 (unsigned integers keyed in value order) and this branch (a typed array keyed by its elements): one generation 1 resolves a typed array to its element type first, so an unsigned element takes the value-order key, and every other element keeps the element type's encoding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e array from the stamp - A ranked element key over the limit is reported as `<array>.items.maxLength` (or `.maxItems`), as the unranked check reports it, not under the array. - The skip set, level stamps, entry cap, skip rules and the ttl update rule read `Index::indexed_array` (`indexed_array_fan_out`, `windows_per_document`) instead of re-deriving it; the ranking-above rule reads `ranked_at_levels`. - Tests for the nested-array and contested refusals and for an unsigned element keyed in value order; stale comments corrected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… an empty byte list - Update v1's unique-index sibling scan read an indexed array's level as one value and failed every replace of such a document; an element level carries no missing value and is passed by. - An empty list counts as absent only for an indexed typed array, so a byte array sent as an empty list is carried, as the indexOnly probes and row commitments read it. - The fanned-out update reads each standing tree it shares once. - The walkers read a level's keys through one `level_keys` rule; estimated element keys reuse the scalar estimate and `distinct_estimated_keys`. - Tests: a replace beside a unique index, an empty-list byte trigger, and the drain test checks the drained post's own windows; shared test helpers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oof repeats by id - `key_array_element_clauses` takes `grouped_by_in` and applies the per-element rule to its own clauses; `answers_per_element` is private. - `verify_proof_keep_serialized` drops a containsAny page's repeats by the proved document ids. - The per-mode aggregate executors are crate-internal; the index pickers say their clauses must be keyed first. - Grouping v0's element arms note why they are unreachable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # packages/rs-platform-version/src/version/mocks/v2_test.rs # packages/rs-platform-version/src/version/system_limits/mod.rs # packages/rs-platform-version/src/version/system_limits/v1.rs # packages/rs-platform-version/src/version/system_limits/v2.rs # packages/rs-platform-version/src/version/system_limits/v3.rs # packages/rs-platform-version/src/version/system_limits/v4.rs
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ent-indexes-sdk Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…views DocumentCount, DocumentSum and DocumentAverage add the verified entries up, so a grouped containsAny proof would count a document once per element it holds; they now refuse several elements even grouped, and the split views keep one entry per element. One helper, aggregate_where_clauses, keys and canonicalizes for the three verifiers on the type each resolved, and the keying takes grouped_by_in (grouped_by_in_field) as Drive's now does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… table Routing v2, which only protocol version 14 selects, routes a containsAny as the In it keys like, on a copy of the clauses. Routing v0 is back to its shipped text, so protocol version 13 answers a grouped containsAny as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tals and pages - At protocol version 13 a grouped containsAny gets routing v0's refusal, with a limit and without. - The single-total views refuse the grouped per-element proofs. - The documents round trip checks Drive's verified page, which the SDK's map would hide a repeat in. - The wire request reuses empty_v1_request, and the SDK round trips share one prove-and-verify helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuantumExplorer
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # packages/rs-drive/src/drive/document/insert/add_document_for_contract/mod.rs # packages/rs-platform-version/src/version/v14.rs
…ent-indexes-sdk Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two lines of the note began with `1)`, which rustdoc reads as list items, so clippy's doc_lazy_continuation failed the platform-version build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete diff preserves existing wire discriminants and historical routing, and the aggregate-proof restrictions and signed round-trip coverage address the earlier blocker. Three binding/API issues and one breaking-change metadata issue remain as non-blocking suggestions. This was static verification only: the supplied exact-head CI snapshot reports successful checks, but does not independently establish execution of the targeted Rust proof regressions.
🟡 4 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Preserve binary operands in element-query bindings
packages/wasm-sdk/src/queries/document.rs:544-545
The newly supported operators still receive operands through DocumentsQueryInput.where_clauses: Option<Vec<JsonValue>>. deserialize_required_query calls from_object, which preserves a JavaScript Uint8Array as a platform byte value and then deserializes it into the JSON-only input model. Platform-value's non-human-readable deserializer invokes visit_bytes, which serde_json::Value does not support, so the README's documented byte-form identifier operand fails before this parser or the wire encoder runs. Binary candidates nested inside containsAny fail the same way. Plain numeric byte arrays are not a workaround: this converter produces arrays of I64, while identifier and binary key conversion require U8 entries. Base58 identifier strings and base64 binary strings remain usable; the problem is the advertised binary representation, not the absence of any usable spelling. Preserve platform-value operands through input deserialization, or parse them directly from JavaScript, and add boundary coverage for both operators with binary elements. Any normalization of plain arrays must use the element schema rather than converting ordinary numeric lists indiscriminately.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)
🟡 Suggestion: Expose element pins in the ranked/having TypeScript contract
packages/wasm-sdk/src/queries/document_ranked.rs:105-107
The ranked and having runtime pre-checks now key a valid typed-array contains prefix pin as equality, but their shared exported DocumentsIndexPin type still permits only equality and in. Both query interfaces use this union, and the evo-sdk facades accept those interfaces directly, so TypeScript callers cannot express the newly supported pin without a cast. Substituting == does not work because Drive rejects ordinary equality on a typed array. Add the accepted contains and Contains spellings to the single-element alternative and update the accompanying pin documentation. Keep containsAny excluded because these surfaces deliberately reject it.
export type DocumentsIndexPin =
| [string, '==' | '=' | 'contains' | 'Contains', unknown]
| [string, 'in' | 'In', unknown[]];
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate cross-layer change directly alters cryptographic proof acceptance and reconstructed query semantics in packages/dash-platform-queries/src/documents/array_element_clauses.rs and count_proof_helpers.rs, including safeguards against accepting incorrect multi-element totals. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/wasm-sdk/src/queries/document.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document.rs:544-545: Preserve binary operands in element-query bindings
The newly supported operators still receive operands through `DocumentsQueryInput.where_clauses: Option<Vec<JsonValue>>`. `deserialize_required_query` calls `from_object`, which preserves a JavaScript `Uint8Array` as a platform byte value and then deserializes it into the JSON-only input model. Platform-value's non-human-readable deserializer invokes `visit_bytes`, which `serde_json::Value` does not support, so the README's documented byte-form identifier operand fails before this parser or the wire encoder runs. Binary candidates nested inside `containsAny` fail the same way. Plain numeric byte arrays are not a workaround: this converter produces arrays of `I64`, while identifier and binary key conversion require `U8` entries. Base58 identifier strings and base64 binary strings remain usable; the problem is the advertised binary representation, not the absence of any usable spelling. Preserve platform-value operands through input deserialization, or parse them directly from JavaScript, and add boundary coverage for both operators with binary elements. Any normalization of plain arrays must use the element schema rather than converting ordinary numeric lists indiscriminately.
In `packages/wasm-sdk/src/queries/document_ranked.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/document_ranked.rs:105-107: Expose element pins in the ranked/having TypeScript contract
The ranked and having runtime pre-checks now key a valid typed-array `contains` prefix pin as equality, but their shared exported `DocumentsIndexPin` type still permits only equality and `in`. Both query interfaces use this union, and the evo-sdk facades accept those interfaces directly, so TypeScript callers cannot express the newly supported pin without a cast. Substituting `==` does not work because Drive rejects ordinary equality on a typed array. Add the accepted `contains` and `Contains` spellings to the single-element alternative and update the accompanying pin documentation. Keep `containsAny` excluded because these surfaces deliberately reject it.
In `packages/dapi-grpc/protos/platform/v0/platform.proto`:
- [SUGGESTION] packages/dapi-grpc/protos/platform/v0/platform.proto:1286-1294: Mark the public Rust enum extension as source-breaking
Appending these values preserves protobuf compatibility, but adds variants to the public generated Rust `WhereOperator` enum, which is not marked `#[non_exhaustive]`. Downstream exhaustive matches must therefore change, as the PR's Breaking Changes section already acknowledges. Repository guidance and the PR template require `!` for breaking changes, not only consensus changes, so the explanation that no consensus change occurred does not justify omitting it. Change the title to `feat(sdk)!: ...` and complete the corresponding checklist item. This metadata correction does not require another protocol generation or an independent Cargo version bump.
In `packages/wasm-drive-verify/src/document/verify_proof.rs`:
- [SUGGESTION] packages/wasm-drive-verify/src/document/verify_proof.rs:280-281: Gate standalone WASM element operators on PV14
(existing thread: https://github.com/dashpay/platform/pull/5372#discussion_r4242600610)
All three standalone document verifiers now parse these operators without checking the supplied platform version, and they construct `InternalClauses` directly instead of passing through versioned raw-clause grouping. The pre-PV14 rejection in `group_where_clauses` therefore never runs. Historical `validate_where_clause_shape` checks only the number of `in` clauses, and historical index selection uses the caller-supplied clause buckets without validating element/property compatibility. A `Contains` clause on an ordinary scalar indexed string in `equal_clauses` consequently constructs the same path query as `Equal`, because `WhereClause::to_path_query` lowers the operator unconditionally. Thus a genuine scalar-equality proof can be accepted under an element-query label at PV13; historical typed-array state is not required, and deeper type errors do not reliably close this path. Enforce the activation boundary consistently in `verify_proof`, `verify_proof_keep_serialized`, and `verify_start_at_document_in_proof`, or route their inputs through the appropriate shared versioned preflight. Add historical refusal coverage using a valid scalar proof. This is a query-API activation inconsistency, not a forged-proof or chain-consensus blocker.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Return errors instead of trapping on malformed standalone WASM cursors — Outside this PR's scope. The malformed-length start_at conversions in verify_proof.rs and verify_start_at_document_in_proof.rs are unchanged from the exact base, and the new operator mappings neither worsen nor rely on their panic behavior. The equivalent cursor-trap observations from the other lanes are consolidated here rather than retained as an exceptional follow-up.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| CONTAINS = 13; | ||
| // An element of a typed array is one of the operands (v1 only; | ||
| // protocol version 14): an `IN` over the elements. A document | ||
| // holding several of them is under each, and a documents page | ||
| // returns it once, where it first appears; the limit counts every | ||
| // entry the walk passes. Takes an `ORDER BY` on the array (the | ||
| // order the elements are walked in) and no cursor. Counts, sums and | ||
| // averages read several elements only grouped by them. | ||
| CONTAINS_ANY = 14; |
There was a problem hiding this comment.
🟡 Suggestion: Mark the public Rust enum extension as source-breaking
Appending these values preserves protobuf compatibility, but adds variants to the public generated Rust WhereOperator enum, which is not marked #[non_exhaustive]. Downstream exhaustive matches must therefore change, as the PR's Breaking Changes section already acknowledges. Repository guidance and the PR template require ! for breaking changes, not only consensus changes, so the explanation that no consensus change occurred does not justify omitting it. Change the title to feat(sdk)!: ... and complete the corresponding checklist item. This metadata correction does not require another protocol generation or an independent Cargo version bump.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
…a change estimate A change was estimated as an insert of the new version alone, so a replace emptying or shrinking an indexed array, which removes an entry per dropped element in every window, could cost more processing than its estimate: 24 one-day windows of 4 elements emptied cost 15,450,740 against 12,346,320. For a type with an index naming a typed array, the estimate now also prices removing the stored version's index entries at their worst case, as a delete prices it. Other types estimate as before. The estimate tests shrink to one element and to none, under 24 windows and on a type with a ttl, and require processing to be covered too; the ranked proof test checks the live root hash. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`LevelKeys::nested_keys` yields the elements of an array level or the single value of any other, without a Vec for a single key. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A subscription filter compared elements by value variant, so a u8 element missed a u64 clause value and an identifier missed its bytes or base58 text, though validation and indexed queries take them. Element clauses now match by the element type's key, as indexed queries do; scalar clauses unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # packages/rs-platform-version/src/version/v14.rs
…ent-indexes-sdk Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # packages/dapi-grpc/clients/platform/v0/python/platform_pb2.py
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> # Conflicts: # packages/dash-platform-queries/src/documents/document_query.rs
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
Basic explanation
#5370 lets Platform index the items of a list, such as a post's hashtags, and query them with two new operators:
contains(one item) andcontainsAny(any of several). This PR carries those operators over the network and through every SDK, so an app can ask for "posts tagged #dash" from JavaScript, Rust, Swift or Kotlin and check the proof it gets back.#5370 is merged; this PR now targets
v5.0-devdirectly.Issue being fixed or feature implemented
The wire and SDK half of #3696. Until this lands, the SDK refuses to encode
containsandcontainsAny(the placeholder in #5370), so they reach Drive only through the CBOR v0 request.Before and after
where: [['hashtags', 'contains', 'dash']]in evo-sdkUnknown operator: containsdash, provedwhere: [['hashtags', 'containsAny', ['dash', 'news']]], orderBy: [['hashtags', 'asc']]Unknown operator: containsAnyhashtagswithcontainsAny"contains"in the FFI (Swift, Kotlin)Operator 'contains' is not supportedWhat was done?
GetDocumentsRequest.WhereOperatorgainsCONTAINS = 13andCONTAINS_ANY = 14, documented. The JS, web, Objective-C and Python clients are regenerated (yarn workspace @dashevo/dapi-grpc build). The only differences are the two values, apart from the Python descriptor bytes.where_operator_from_protomaps the two values. Aggregate routing v2, which only PV14 selects, routes acontainsAnyas theinit keys like, so a count, sum or average grouped by the array gives one entry per element. It routes a copy of the clauses; Drive keys the ones sent. The PV13 table (v0) is untouched.where_operator_to_protomaps the two operators (no longer fallible).inclauses, as the server does. Otherwise mode detection and the rebuilt path query would differ from the prover's. Documents proofs already go through Drive's query, which drops repeats.grouped_by_in_field, the SDK's copy of drive-abci's routing check, handed to Drive'skey_array_element_clauses, which holds the rule); ranked and having read one. So the SDK refuses to verify a proof of a total over several elements, which a dishonest node could build from per-element counts and which would count a post holding two of them twice.DocumentCount,DocumentSum,DocumentAverage) add the verified entries up, so they refuse several elements even grouped by the array; the split views keep one entry per element.aggregate_where_clauses, keys and canonicalizes for the count, sum and average verifiers, on the document type each already resolved.key_element_clausesdoes it for the ranked and having verifiers and the wasm-sdk pre-checks.parse_where_clauseacceptscontains/ContainsandcontainsAny/ContainsAny, and theDocumentWhereOperatorTypeScript type lists them. The ranked and having pre-checks key acontainspin as the server does, through the shared helper.search.rsandcount.rs):containsandcontainsAnymap to the operators. Swift and Kotlin pass operator strings through, so they need nothing else.ContainsandContainsAny.How Has This Been Tested?
array_element_wire_tests:CONTAINSdocuments query, with and without proof;CONTAINS_ANYreturning each post once;CONTAINS_ANYgrouped count per element;EQUALon the array refused on the wire;CONTAINSdocuments read refused, and aCONTAINS_ANYgrouped count refused by routing v0 as before, with a limit and without.array_element_proof_round_trips: proofs from the v1 handler verified through the SDK'sFromProof<DocumentQuery>entry points:CONTAINS_ANYdocuments, each post once though one holds two of the elements (also checked on Drive's verified page, a list, since the SDK'sDocumentsmap would hide a repeat);DocumentSplitCounts) and sums (DocumentSplitSums);DocumentAverage);DocumentRankedEntries,DocumentHavingEntries);DocumentCount,DocumentSumandDocumentAverage, all refused.containsAnytotal, and keyscontainsAnyasinandcontainsas an equality.v5.0-dev(with feat(platform)!: index the elements of typed arrays (PV14) #5370):cargo test -p drive-abci --lib -- query::document_query::v1(135 passed),-p dash-platform-queries(63),-p rs-sdk-ffi --lib -- document::queries(22).cargo clippy --testsis clean on drive-abci and dash-platform-queries.cargo checkpasses for wasm-sdk (wasm32), wasm-drive-verify, rs-sdk-ffi, dash-sdk, drive-abci and dash-platform-queries;cargo clippyis clean on dash-platform-queries (with tests) and wasm-sdk (wasm32).In-place changes to shipped generations
Each of these is reached at protocol versions 13 and earlier, and none changes what those versions do:
where_operator_from_protomapsCONTAINSandCONTAINS_ANYat every protocol version. Before PV14 both are refused, where the conversion refused them before: by grouping v0 for documents, and for aggregates by routing v0 (acontainsAnyis nointhere) or Drive's keying step (no type has a typed array). Queries only; no state.should_refuse_element_reads_at_protocol_version_13pins it.Breaking Changes
GetDocumentsRequest.WhereOperatorwire enum gains two values. A node before this change refuses them as an unknown discriminant, and the SDK doesn't send them below protocol version 14.WhereOperatormust cover them.dash-platform-queries:key_element_clausestakesgrouped_by_in(whether the request is grouped by itsInfield) instead ofseveral_elements_allowed, andaggregate_answers_per_elementis replaced bygrouped_by_in_field. None of this changes consensus, so the title carries no!.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
5327f7b/skip-botsproceeds without the ones not yet reported/self-reviewedjs-wasm-sdk(packages/js-evo-sdk/README.md,packages/wasm-sdk/src/queries/document.rs,packages/wasm-sdk/src/queries/document_ranked.rs) — shumkovrs-drive-abci— you own itrust-sdk-ffi(packages/rs-sdk-ffi/src/document/queries/count.rs,packages/rs-sdk-ffi/src/document/queries/search.rs) — lklimek or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.