feat(dpp): add documents_countable to DocumentTypeV2 for O(1) total document counts#3457
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds support for document countability features to the Dash Platform Protocol by introducing two new boolean flags— ChangesDocument Countability Feature
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Collaborator
Review GateCommit:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3457 +/- ##
============================================
+ Coverage 88.29% 88.32% +0.03%
============================================
Files 2479 2486 +7
Lines 301659 302964 +1305
============================================
+ Hits 266347 267593 +1246
- Misses 35312 35371 +59
🚀 New features to boost your workflow:
|
![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: 2
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs (1)
10-39: Random V2 generators never exercisedocuments_countable.Line 16 and Line 32 both build through
DocumentTypeV1, andFrom<DocumentTypeV1> for DocumentTypeV2initializesdocuments_countabletofalse. Any fuzz/property tests using these helpers therefore miss the new CountTree path entirely. Please thread the flag throughRandomDocumentTypeParametersor randomize it when the platform version supports the feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs` around lines 10 - 39, The current helpers random_document_type and invalid_random_document_type call DocumentTypeV1 generators and convert to DocumentTypeV2, but the From<DocumentTypeV1> for DocumentTypeV2 sets documents_countable = false so the CountTree path is never exercised; update either RandomDocumentTypeParameters to include a documents_countable boolean and pass it through into DocumentTypeV1 -> DocumentTypeV2 construction, or (simpler) when platform_version supports CountTree, use the provided rng inside random_document_type and invalid_random_document_type to set DocumentTypeV2.documents_countable randomly before returning; locate the two functions (random_document_type, invalid_random_document_type) and the From<DocumentTypeV1> for DocumentTypeV2 conversion to implement threading or randomized assignment conditioned on platform_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs`:
- Around line 301-307: The match in DocumentTypeRef::schema_owned (and similarly
in DocumentTypeMutRef::schema_owned) contains duplicated DocumentTypeRef::V2
pattern arms causing unreachable code; remove the redundant V2 arm so each
variant (V0, V1, V2) appears exactly once in the match, keeping the correct call
(v2.clone().schema_owned()) for the single V2 arm and run cargo clippy to verify
the warning is gone.
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 51-61: In the v1 parser in try_from_schema
(packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs),
add logic to detect the DOCUMENTS_COUNTABLE field during parsing and, when
full_validation is enabled and platform_version.protocol_version < 12 (i.e.,
v1/protocols 2–3), return the same
ProtocolError::ConsensusError(BasicError::UnsupportedFeatureError(...)) as the
v2 branch does; specifically, mirror the check that sets documents_countable and
returns UnsupportedFeatureError for "documentsCountable" so the v1 path no
longer silently ignores DOCUMENTS_COUNTABLE but fails explicitly under older
protocol versions.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs`:
- Around line 10-39: The current helpers random_document_type and
invalid_random_document_type call DocumentTypeV1 generators and convert to
DocumentTypeV2, but the From<DocumentTypeV1> for DocumentTypeV2 sets
documents_countable = false so the CountTree path is never exercised; update
either RandomDocumentTypeParameters to include a documents_countable boolean and
pass it through into DocumentTypeV1 -> DocumentTypeV2 construction, or (simpler)
when platform_version supports CountTree, use the provided rng inside
random_document_type and invalid_random_document_type to set
DocumentTypeV2.documents_countable randomly before returning; locate the two
functions (random_document_type, invalid_random_document_type) and the
From<DocumentTypeV1> for DocumentTypeV2 conversion to implement threading or
randomized assignment conditioned on platform_version.
🪄 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: 23a22067-a368-4eb1-b3a4-17e2a65e5136
📒 Files selected for processing (15)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
2 times, most recently
from
d3b946a to
1e92143
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs (1)
8-8: Tighten inline comment scope to include both v2 flags.Line 8 and Line 36 comments mention only
documentsCountable, but the v2 parser path also coversblastSyncable. Updating the comment would prevent future confusion.✏️ Suggested comment tweak
-// Introduced in protocol version 12, adds documents_countable feature via try_from_schema v2 +// Introduced in protocol version 12, enables v2 schema parsing for documentsCountable/blastSyncable ... - try_from_schema: 2, // changed: supports documentsCountable + try_from_schema: 2, // changed: supports documentsCountable and blastSyncableAlso applies to: 36-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs` at line 8, Update the inline comment(s) that currently mention only `documentsCountable` to also include `blastSyncable`, clarifying that both feature flags are introduced in protocol version 12 and are set via the `try_from_schema v2` parser path; locate the comments near the `try_from_schema` v2 handling (references: `documentsCountable`, `blastSyncable`, `try_from_schema v2`) in v4.rs and adjust the text on both occurrences so the scope clearly covers both flags.packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (2)
64-79: Consider using a named constant for the protocol version threshold.The literal
12is used here and inv1/mod.rs(line 325) for the same feature gate. WhilePROTOCOL_VERSION_12is defined inpackages/rs-platform-version/src/version/v12.rs, it's not used here. Using a constant would improve maintainability and clarity.💡 Example improvement
+use crate::version::PROTOCOL_VERSION_12; // or appropriate import path #[cfg(feature = "validation")] -if full_validation && needs_v2 && platform_version.protocol_version < 12 { +if full_validation && needs_v2 && platform_version.protocol_version < PROTOCOL_VERSION_12 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs` around lines 64 - 79, The code uses the literal 12 as a protocol threshold when gating features (in the if checking full_validation && needs_v2 && platform_version.protocol_version < 12); replace the magic number with the named constant PROTOCOL_VERSION_12 (import it from the platform-version crate/module where it is defined) and use that constant in the comparison (platform_version.protocol_version < PROTOCOL_VERSION_12); also make the same replacement in the corresponding v1 check to keep behavior consistent and add the necessary use/import for PROTOCOL_VERSION_12 at the top of the module.
96-100: Redundant assignment:documents_countable || blast_syncableis already enforced by the getter.At line 98,
documents_countableis set todocuments_countable || blast_syncable. However, theDocumentTypeV2Getters::documents_countable()getter (inv2/accessors.rslines 202-204) already returnsself.documents_countable || self.blast_syncable. This means the field value doesn't need to encode the implication—the getter handles it.Consider simplifying to just set the parsed values directly:
♻️ Proposed simplification
if needs_v2 { let mut v2: DocumentTypeV2 = v1.into(); - v2.documents_countable = documents_countable || blast_syncable; + v2.documents_countable = documents_countable; v2.blast_syncable = blast_syncable; Ok(v2.into())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs` around lines 96 - 100, The code redundantly sets v2.documents_countable = documents_countable || blast_syncable inside the try_from path even though DocumentTypeV2Getters::documents_countable() already returns self.documents_countable || self.blast_syncable; change the assignment to store the parsed boolean directly (v2.documents_countable = documents_countable) and leave v2.blast_syncable = blast_syncable as-is so the getter enforces the implication, updating the conversion in the DocumentTypeV2 construction path that uses the try_from logic where DocumentTypeV2 is produced.packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs (1)
1-40: Indentation should use 4 spaces per coding guidelines.The file uses 2-space indentation, but Rust files in this repository should use 4-space indentation. Also,
random_document_typeis missing a doc comment whileinvalid_random_document_typehas one—consider adding a brief doc comment for consistency.As per coding guidelines: "Use 4-space indent for Rust files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs` around lines 1 - 40, The file uses 2-space indentation and is missing a doc comment for random_document_type; update the indentation to 4 spaces across the entire file and add a brief doc comment above impl DocumentTypeV2::random_document_type matching the style of invalid_random_document_type (e.g., "Creates a random valid document type" or similar). Ensure you adjust the indentation for the impl block and both methods (random_document_type and invalid_random_document_type) so they use 4-space indents and run rustfmt after changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 64-79: The code uses the literal 12 as a protocol threshold when
gating features (in the if checking full_validation && needs_v2 &&
platform_version.protocol_version < 12); replace the magic number with the named
constant PROTOCOL_VERSION_12 (import it from the platform-version crate/module
where it is defined) and use that constant in the comparison
(platform_version.protocol_version < PROTOCOL_VERSION_12); also make the same
replacement in the corresponding v1 check to keep behavior consistent and add
the necessary use/import for PROTOCOL_VERSION_12 at the top of the module.
- Around line 96-100: The code redundantly sets v2.documents_countable =
documents_countable || blast_syncable inside the try_from path even though
DocumentTypeV2Getters::documents_countable() already returns
self.documents_countable || self.blast_syncable; change the assignment to store
the parsed boolean directly (v2.documents_countable = documents_countable) and
leave v2.blast_syncable = blast_syncable as-is so the getter enforces the
implication, updating the conversion in the DocumentTypeV2 construction path
that uses the try_from logic where DocumentTypeV2 is produced.
In `@packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs`:
- Around line 1-40: The file uses 2-space indentation and is missing a doc
comment for random_document_type; update the indentation to 4 spaces across the
entire file and add a brief doc comment above impl
DocumentTypeV2::random_document_type matching the style of
invalid_random_document_type (e.g., "Creates a random valid document type" or
similar). Ensure you adjust the indentation for the impl block and both methods
(random_document_type and invalid_random_document_type) so they use 4-space
indents and run rustfmt after changes.
In
`@packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs`:
- Line 8: Update the inline comment(s) that currently mention only
`documentsCountable` to also include `blastSyncable`, clarifying that both
feature flags are introduced in protocol version 12 and are set via the
`try_from_schema v2` parser path; locate the comments near the `try_from_schema`
v2 handling (references: `documentsCountable`, `blastSyncable`, `try_from_schema
v2`) in v4.rs and adjust the text on both occurrences so the scope clearly
covers both flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5570ee7c-2eae-487c-9586-dd234a5251f6
📒 Files selected for processing (15)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-platform-version/src/version/v12.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
- packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
1e92143 to
be7f38d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (1)
871-905: Add direct coverage for the new V2-specific accessor dispatch.There are no focused assertions here for
documents_countable()/blast_syncable()on the owned, ref, and mut-ref variants. A smallDocumentType::V2test would lock in these new match arms and the V0/V1 fallback behavior.Also applies to: 907-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs` around lines 871 - 905, Add focused unit tests that exercise the V2-specific match arms on DocumentType by constructing DocumentType::V2 (owned), a &DocumentType::V2 (ref) and a &mut DocumentType::V2 (mut-ref) and asserting the getters DocumentTypeV2Getters::documents_countable and ::blast_syncable return the expected values; also test setters DocumentTypeV2Setters::set_documents_countable and ::set_blast_syncable mutate the V2 state when called on the mut-ref and that calling the getters/setters on V0/V1 remains a no-op/returns false as currently implemented; target the DocumentType, DocumentTypeV2Getters, DocumentTypeV2Setters, documents_countable, blast_syncable, set_documents_countable and set_blast_syncable symbols in your new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs`:
- Around line 211-220: The setter set_documents_countable currently allows
documents_countable to be set to false while blast_syncable may remain true;
update impl DocumentTypeV2Setters for DocumentTypeV2 so
set_documents_countable(&mut self, countable: bool) sets
self.documents_countable = countable and if countable == false also clear
self.blast_syncable to false to preserve the blast_syncable =>
documents_countable invariant (mirror the normalization done in
set_blast_syncable).
In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- Around line 288-319: Existing code allows toggling documents_countable and
blast_syncable on contract updates which corrupts on-disk trees; modify
validate_config to detect any change to DocumentType::documents_countable or
DocumentType::blast_syncable for existing document types and reject the update.
Specifically, in validate_config compare the old and new document type maps (by
name/identifier) and if an existing type's documents_countable or blast_syncable
differs, return a validation error (match the style used for
documents_mutable/documents_keep_history checks). Ensure the same symbols are
referenced: validate_config, DocumentType (or document type map),
documents_countable, blast_syncable, and use the same error type/format used
elsewhere so callers (including update_contract logic) will fail fast.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs`:
- Around line 871-905: Add focused unit tests that exercise the V2-specific
match arms on DocumentType by constructing DocumentType::V2 (owned), a
&DocumentType::V2 (ref) and a &mut DocumentType::V2 (mut-ref) and asserting the
getters DocumentTypeV2Getters::documents_countable and ::blast_syncable return
the expected values; also test setters
DocumentTypeV2Setters::set_documents_countable and ::set_blast_syncable mutate
the V2 state when called on the mut-ref and that calling the getters/setters on
V0/V1 remains a no-op/returns false as currently implemented; target the
DocumentType, DocumentTypeV2Getters, DocumentTypeV2Setters, documents_countable,
blast_syncable, set_documents_countable and set_blast_syncable symbols in your
new tests.
🪄 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: dc904f91-e9e2-4e3d-b430-2cf570fb26de
📒 Files selected for processing (15)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (5)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs
- packages/rs-dpp/src/data_contract/document_type/v2/mod.rs
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
be7f38d to
1e21664
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs`:
- Around line 71-75: The two capability flags documents_countable and
blast_syncable currently have crate-level mutable visibility which allows
bypassing DocumentTypeV2Setters and breaking the invariant that blast_syncable
implies documents_countable; make both fields private (remove pub(in
crate::data_contract)) and enforce the invariant in the setter methods on
DocumentTypeV2 (DocumentTypeV2Setters): when setting blast_syncable = true also
set documents_countable = true, and when setting documents_countable = false
prevent or clear blast_syncable (or return an error) to avoid inconsistency;
update any internal callers to use the provided getters/setters instead of
writing the fields directly.
In `@packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- Around line 288-319: The update path currently always creates a NormalTree for
newly added document types; replicate the same conditional tree selection as in
the insert path by checking document_type.blast_syncable() and
document_type.documents_countable() and creating the matching tree: if
blast_syncable() create a ProvableCountTree (use
Element::empty_provable_count_tree_with_flags /
Element::empty_provable_count_tree and
LowLevelDriveOperation::insert_for_known_path_key_element with path_items and
vec![0]), else if documents_countable() call
LowLevelDriveOperation::for_known_path_key_empty_count_tree with path_items,
vec![0], and storage_flags.as_ref(), otherwise fall back to calling
batch_insert_empty_tree with Key(vec![0]), passing storage_flags.as_ref(), &mut
batch_operations and platform_version.drive; mirror the storage_flags handling
used in the insert code so update and insert behavior are consistent.
🪄 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: 68ecc863-7609-4966-bab9-2ef4bf430a49
📒 Files selected for processing (16)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-platform-version/src/version/v12.rs
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
2 times, most recently
from
9e4cd7c to
4fa6850
Compare
thepastaclaw
suggested changes
Apr 8, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated the current SHA from the completed Codex review outputs plus direct code inspection. Two consensus-critical problems remain: contract updates still create newly added countable / BLAST document types with a plain NormalTree, and several stateless query / fee-estimation paths still hard-code TreeType::NormalTree even when execution now uses CountTree / ProvableCountTree. The other live CodeRabbit threads were either already fixed or already retracted in-thread on the current SHA.
I validated existing CodeRabbit comments separately in-thread instead of restating them here.
Reviewed commit: 4fa6850
🔴 1 blocking
1 additional finding
🔴 blocking: Stateless query and fee-estimation paths still underprice countable / BLAST document trees as `NormalTree`
packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs (lines 44-45)
Primary document storage now executes on CountTree / ProvableCountTree for countable or BLAST-syncable document types, but related stateless paths still hard-code TreeType::NormalTree. The affected code includes insert-time existence checks here, delete-time reads in packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs, delete-cost estimation in packages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rs, and add-cost estimation in packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs. Because CountTree/ProvableCountTree maintain additional count/provable metadata, pricing these operations as NormalTree creates a consensus-accepted undercharge for attacker-chosen document types that enable the new flags.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs`:
- [BLOCKING] lines 44-45: Stateless query and fee-estimation paths still underprice countable / BLAST document trees as `NormalTree`
Primary document storage now executes on `CountTree` / `ProvableCountTree` for countable or BLAST-syncable document types, but related stateless paths still hard-code `TreeType::NormalTree`. The affected code includes insert-time existence checks here, delete-time reads in `packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs`, delete-cost estimation in `packages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rs`, and add-cost estimation in `packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs`. Because CountTree/ProvableCountTree maintain additional count/provable metadata, pricing these operations as NormalTree creates a consensus-accepted undercharge for attacker-chosen document types that enable the new flags.
4fa6850 to
0568dd4
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs (1)
125-148:⚠️ Potential issue | 🟠 MajorKeep the per-document history subtree as
NormalTree.At Line 145 this branch is creating the
document_idsubtree underprimary_key_path, but it passesprimary_key_tree_typeas the type of the new subtree. For history-enabled types, the count/provable-count tree needs to stay onprimary_key_pathso it counts document IDs; the childdocument_idbucket should remainNormalTree. The estimator added inpackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rsalready models that split, so the runtime path is diverging from the estimated layout.🔧 Suggested adjustment
let apply_type = if estimated_costs_only_with_layer_info.is_none() { BatchInsertTreeApplyType::StatefulBatchInsertTree } else { BatchInsertTreeApplyType::StatelessBatchInsertTree { - in_tree_type: TreeType::NormalTree, - tree_type: primary_key_tree_type, + in_tree_type: primary_key_tree_type, + tree_type: TreeType::NormalTree, flags_len: storage_flags .map(|s| s.serialized_size()) .unwrap_or_default(), } }; // we first insert an empty tree if the document is new self.batch_insert_empty_tree_if_not_exists( path_key_info, - primary_key_tree_type, + TreeType::NormalTree, storage_flags, apply_type,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs` around lines 125 - 148, The code is creating the per-document (document_id) subtree using primary_key_tree_type, which makes history-enabled document types get Count/ProvableCount trees under each document_id; instead, ensure the child document_id subtree is always created as TreeType::NormalTree. Update the call to batch_insert_empty_tree_if_not_exists (and any place where a new document_id subtree type is passed) to use TreeType::NormalTree for the document_id subtree while leaving primary_key_tree_type unchanged for the parent primary_key_path; keep the existing apply_type/flags logic intact so the estimator and runtime layout match.
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs (1)
73-79: Consider centralizing primary-key tree selection.This
blast_syncable()/documents_countable()→TreeTypemapping now exists in multiple insert/update/estimation paths. A small helper on the document-type or drive side would make the storage-layout rule single-source and reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs` around lines 73 - 79, The primary-key TreeType selection logic (the conditional using document_type.blast_syncable(), document_type.documents_countable() to set primary_key_tree_type to TreeType::ProvableCountTree / TreeType::CountTree / TreeType::NormalTree) is duplicated across insert/update/estimation code paths; create a single helper method (e.g., DocumentType::primary_key_tree_type() or Drive::primary_key_tree_type_for(document_type)) that encapsulates this mapping and replace the inline conditional in add_estimation_costs_for_add_document_to_primary_storage::v0::mod.rs (and other insert/update/estimation modules) to call that helper, keeping the unique symbols document_type, blast_syncable(), documents_countable(), primary_key_tree_type, and TreeType as the mapping contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs`:
- Around line 125-148: The code is creating the per-document (document_id)
subtree using primary_key_tree_type, which makes history-enabled document types
get Count/ProvableCount trees under each document_id; instead, ensure the child
document_id subtree is always created as TreeType::NormalTree. Update the call
to batch_insert_empty_tree_if_not_exists (and any place where a new document_id
subtree type is passed) to use TreeType::NormalTree for the document_id subtree
while leaving primary_key_tree_type unchanged for the parent primary_key_path;
keep the existing apply_type/flags logic intact so the estimator and runtime
layout match.
---
Nitpick comments:
In
`@packages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rs`:
- Around line 73-79: The primary-key TreeType selection logic (the conditional
using document_type.blast_syncable(), document_type.documents_countable() to set
primary_key_tree_type to TreeType::ProvableCountTree / TreeType::CountTree /
TreeType::NormalTree) is duplicated across insert/update/estimation code paths;
create a single helper method (e.g., DocumentType::primary_key_tree_type() or
Drive::primary_key_tree_type_for(document_type)) that encapsulates this mapping
and replace the inline conditional in
add_estimation_costs_for_add_document_to_primary_storage::v0::mod.rs (and other
insert/update/estimation modules) to call that helper, keeping the unique
symbols document_type, blast_syncable(), documents_countable(),
primary_key_tree_type, and TreeType as the mapping contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57e10670-6fbf-4eda-af45-176e7a525d52
📒 Files selected for processing (21)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (8)
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs
Member
Author
|
@thepastaclaw are things good now? |
Collaborator
|
Not quite yet. I re-checked the current head (
So countable / BLAST document types are still being modeled as There is also still the history-layout mismatch in So my answer is: better, but not good yet. I’d want one more push that fixes those remaining paths before clearing this. |
thepastaclaw
suggested changes
Apr 8, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The earlier storage-layout bugs around contract update and the dedicated estimation helpers are fixed, but this SHA still has three merge-blocking gaps in the new tree-type rollout. Pre-v12 contracts can already carry the new schema keys even though older parsers ignore them, and the remaining insert paths still mis-handle CountTree/ProvableCountTree semantics in ways that keep stateless costing and stateful execution out of sync.
Reviewed commit: 0568dd4
🔴 3 blocking
1 additional finding
🔴 blocking: Stateless non-history inserts still price count/provable primary storage as `NormalTree`
packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs (lines 445-452)
The non-history batch_insert_if_not_exists path still hard-codes in_tree_type: TreeType::NormalTree even though this PR now creates the primary key tree as CountTree or ProvableCountTree for documents_countable / blast_syncable document types. That keeps fee estimation and dry-run validation on the cheaper tree variant while execution writes into the heavier count/provable tree, so flagged document types can still be accepted with underpriced insert costs. The dedicated estimation helper was fixed earlier in the patch, but this inline stateless insert path is still out of sync with the actual storage layout.
💡 Suggested change
let primary_key_tree_type = if document_type.blast_syncable() {
TreeType::ProvableCountTree
} else if document_type.documents_countable() {
TreeType::CountTree
} else {
TreeType::NormalTree
};
let apply_type = if estimated_costs_only_with_layer_info.is_none() {
BatchInsertApplyType::StatefulBatchInsert
} else {
BatchInsertApplyType::StatelessBatchInsert {
in_tree_type: primary_key_tree_type,
target: QueryTargetValue(document_type.estimated_size(platform_version)? as u32),
}
};
🤖 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-dpp/schema/meta_schemas/document/v0/document-meta.json`:
- [BLOCKING] lines 508-514: Pre-v12 contracts can smuggle in tree-type flags that activate after the v12 parser switch
These keys are now accepted by the shared document meta-schema for every protocol version, but pre-v12 parsing still dispatches to the older document-type parsers that validate against this schema and then ignore the fields. Because contracts serialize the raw document schemas and reparse them on deserialization with the current `platform_version`, a contract accepted before v12 can later be reloaded after upgrade with `documentsCountable` / `blastSyncable` suddenly taking effect even though its primary tree was originally created as a normal tree. That creates a storage-layout invariant break across protocol activation and can surface later as fee/proof divergence or block-processing failures for document types that were accepted too early.
In `packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs`:
- [BLOCKING] lines 136-147: Keep-history inserts swap the parent and child tree types
In the `documents_keep_history()` branch, the stateless apply type still treats the parent as `NormalTree` and the child as `primary_key_tree_type`, and the stateful insert creates the per-document history subtree with `primary_key_tree_type` as well. Those are backwards: the parent being inserted into is the primary key tree, which can now be `CountTree` / `ProvableCountTree`, while the per-document history subtree should remain a plain `NormalTree`. As written, stateful execution builds the wrong subtree type and stateless costing models both sides of the insert against the wrong tree variants, so history-enabled countable/BLAST document types no longer preserve a consistent storage-layout/cost invariant.
- [BLOCKING] lines 445-452: Stateless non-history inserts still price count/provable primary storage as `NormalTree`
The non-history `batch_insert_if_not_exists` path still hard-codes `in_tree_type: TreeType::NormalTree` even though this PR now creates the primary key tree as `CountTree` or `ProvableCountTree` for `documents_countable` / `blast_syncable` document types. That keeps fee estimation and dry-run validation on the cheaper tree variant while execution writes into the heavier count/provable tree, so flagged document types can still be accepted with underpriced insert costs. The dedicated estimation helper was fixed earlier in the patch, but this inline stateless insert path is still out of sync with the actual storage layout.
0568dd4 to
5adbe0d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs (1)
40-49: Consider centralizing primary-key tree selection.This
blast_syncable/documents_countable/NormalTreemapping now exists in several insert/delete/estimation paths. One missed call site already caused drift in this PR, so a shared helper would make future consensus-layout changes harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs` around lines 40 - 49, Extract the repeated mapping logic (document_type.blast_syncable -> TreeType::ProvableCountTree, document_type.documents_countable -> TreeType::CountTree, else -> TreeType::NormalTree) into a single helper function (e.g., fn primary_key_tree_type_for(document_type: &DocumentType) -> TreeType) and replace the inline logic in add_document_for_contract_operations::v0 (the block that sets primary_key_tree_type) and all other insert/delete/estimation call sites with calls to that helper; ensure the helper lives in a shared module so all consumers (insert, delete, estimation paths) import and use it to avoid drift.packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs (1)
667-941: Add a V2-backed accessor test.These new
V2branches are not exercised by this module’s tests; the current fixture coverage is still V0-oriented, so regressions in token-cost delegation ordocuments_countable/blast_syncabledispatch would slip through. Please add at least one unit test that constructs aDocumentType::V2and checks owned/ref/mut-ref accessors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs` around lines 667 - 941, Add a unit test that constructs a DocumentType::V2 instance (with deterministic values for token costs and documents_countable/blast_syncable), then verify the V2-tail dispatch for owned, reference, and mutable-reference variants: call DocumentTypeV1Getters methods (document_creation_token_cost, document_replacement_token_cost, document_deletion_token_cost, document_transfer_token_cost, document_update_price_token_cost, document_purchase_token_cost, all_document_token_costs, all_external_token_costs_contract_tokens) and DocumentTypeV2Getters methods (documents_countable, blast_syncable) on DocumentType::V2, DocumentTypeRef::V2, and DocumentTypeMutRef::V2 and assert they return the expected values from the underlying V2 struct; place the test alongside existing tests for this module so V2 branches are exercised.packages/rs-dpp/src/data_contract/document_type/v2/mod.rs (2)
108-140: Consider collapsing the duplicated V0/V1→V2 conversions.These two
Fromimpls copy the same field list almost verbatim, which makes futureDocumentTypeV2additions easy to miss in one path. A small shared constructor/helper withtoken_costsas the only varying input would make this less brittle.Also applies to: 142-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs` around lines 108 - 140, The two From impls for DocumentTypeV2 (From<DocumentTypeV0> and From<DocumentTypeV1>) duplicate the same field mapping; extract a shared constructor/helper (e.g., DocumentTypeV2::from_common or a private fn build_from_fields) that accepts a source struct (or common fields) and a TokenCosts value, move the repeated field assignments (name, schema, indices, index_structure, flattened_properties, properties, identifier_paths, binary_paths, required_fields, transient_fields, documents_keep_history, documents_mutable, documents_can_be_deleted, documents_transferable, trade_mode, creation_restriction_mode, data_contract_id, requires_identity_encryption_bounded_key, requires_identity_decryption_bounded_key, security_level_requirement, #[cfg(feature="validation")] json_schema_validator, documents_countable, blast_syncable) into that helper, and then have both From<DocumentTypeV0> and From<DocumentTypeV1> call it passing the appropriate TokenCosts::V0(Default::default()) or TokenCosts::V1(value) so only the token_costs difference remains in each From impl.
185-195: Pin the test helper to a fixed platform version.Using
PlatformVersion::latest()here makes these unit tests vulnerable to unrelated protocol-profile changes. A concrete test version would keep the V0/V1→V2 conversion coverage stable over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs` around lines 185 - 195, The test helper make_v0 currently calls IndexLevel::try_from_indices with PlatformVersion::latest(), which makes the unit test brittle; replace PlatformVersion::latest() with a pinned, concrete platform version used for tests (e.g., the specific PlatformVersion variant/constructor your test-suite uses) so DocumentTypeV0 -> V2 conversion remains stable; update the call in make_v0 (and any similar helpers) to pass that fixed PlatformVersion instead of PlatformVersion::latest(), using the existing test constant or explicit version constructor used elsewhere in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- Around line 666-696: The new validation block in try_from_schema v1 (the if
full_validation branch that calls Value::inner_optional_bool_value on
property_names::DOCUMENTS_COUNTABLE and property_names::BLAST_SYNCABLE and
returns ProtocolError::ConsensusError/UnsupportedFeatureError) must be removed
or disabled so V1 stays tolerant: V1 should ignore V2-only flags instead of
failing; revert the change that inspects documentsCountable/blastSyncable and
the associated error path (ProtocolError::ConsensusError /
UnsupportedFeatureError) and ensure routing of protocol_version >= 12 schemas to
the V2 parser remains the mechanism that enforces those features, leaving
try_from_schema v1 to simply skip/map only fields present in the V1 output type
(do not rely on full_validation to enforce this).
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs`:
- Around line 667-941: Add a unit test that constructs a DocumentType::V2
instance (with deterministic values for token costs and
documents_countable/blast_syncable), then verify the V2-tail dispatch for owned,
reference, and mutable-reference variants: call DocumentTypeV1Getters methods
(document_creation_token_cost, document_replacement_token_cost,
document_deletion_token_cost, document_transfer_token_cost,
document_update_price_token_cost, document_purchase_token_cost,
all_document_token_costs, all_external_token_costs_contract_tokens) and
DocumentTypeV2Getters methods (documents_countable, blast_syncable) on
DocumentType::V2, DocumentTypeRef::V2, and DocumentTypeMutRef::V2 and assert
they return the expected values from the underlying V2 struct; place the test
alongside existing tests for this module so V2 branches are exercised.
In `@packages/rs-dpp/src/data_contract/document_type/v2/mod.rs`:
- Around line 108-140: The two From impls for DocumentTypeV2
(From<DocumentTypeV0> and From<DocumentTypeV1>) duplicate the same field
mapping; extract a shared constructor/helper (e.g., DocumentTypeV2::from_common
or a private fn build_from_fields) that accepts a source struct (or common
fields) and a TokenCosts value, move the repeated field assignments (name,
schema, indices, index_structure, flattened_properties, properties,
identifier_paths, binary_paths, required_fields, transient_fields,
documents_keep_history, documents_mutable, documents_can_be_deleted,
documents_transferable, trade_mode, creation_restriction_mode, data_contract_id,
requires_identity_encryption_bounded_key,
requires_identity_decryption_bounded_key, security_level_requirement,
#[cfg(feature="validation")] json_schema_validator, documents_countable,
blast_syncable) into that helper, and then have both From<DocumentTypeV0> and
From<DocumentTypeV1> call it passing the appropriate
TokenCosts::V0(Default::default()) or TokenCosts::V1(value) so only the
token_costs difference remains in each From impl.
- Around line 185-195: The test helper make_v0 currently calls
IndexLevel::try_from_indices with PlatformVersion::latest(), which makes the
unit test brittle; replace PlatformVersion::latest() with a pinned, concrete
platform version used for tests (e.g., the specific PlatformVersion
variant/constructor your test-suite uses) so DocumentTypeV0 -> V2 conversion
remains stable; update the call in make_v0 (and any similar helpers) to pass
that fixed PlatformVersion instead of PlatformVersion::latest(), using the
existing test constant or explicit version constructor used elsewhere in the
codebase.
In
`@packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs`:
- Around line 40-49: Extract the repeated mapping logic
(document_type.blast_syncable -> TreeType::ProvableCountTree,
document_type.documents_countable -> TreeType::CountTree, else ->
TreeType::NormalTree) into a single helper function (e.g., fn
primary_key_tree_type_for(document_type: &DocumentType) -> TreeType) and replace
the inline logic in add_document_for_contract_operations::v0 (the block that
sets primary_key_tree_type) and all other insert/delete/estimation call sites
with calls to that helper; ensure the helper lives in a shared module so all
consumers (insert, delete, estimation paths) import and use it to avoid drift.
🪄 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: f4801f72-3c6d-4969-ba1c-d018958e7213
📒 Files selected for processing (25)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_document_from_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rs
- packages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rs
- packages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rs
- packages/rs-drive/src/fees/op.rs
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-dpp/src/data_contract/document_type/mod.rs
- packages/rs-dpp/src/data_contract/document_type/v2/accessors.rs
Collaborator
|
Re-checked the current head ( What changed on the paths I was worried about:
So from my side: yes, things look good now. |
thepastaclaw
suggested changes
Apr 9, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This force-push fixes the earlier CountTree/ProvableCountTree insertion/costing mismatches, but two merge-blocking contract-parsing bugs remain. First, protocol v12 routes document-type parsing through the new v2 parser, yet that parser forwards documentsCountable / blastSyncable straight into the v1 parser with full_validation intact, so full validation rejects the very fields this PR introduces and contract creation cannot use the feature. Second, the new pre-v12 rejection only runs during full validation; contracts already accepted into state before this fix can still be deserialized after the v12 parser switch with those flags taking effect even though their primary storage trees were originally created as normal trees. I also confirmed a smaller coverage gap: the new immutability checks in validate_update do not have matching unit tests like the neighboring config invariants do.
Reviewed commit: 5adbe0d
🔴 2 blocking | 🟡 1 suggestion(s)
🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- [BLOCKING] lines 57-70: V2 document-type parsing forwards the new flags into the V1 rejection path
Protocol v12 switches `document_type.try_from_schema` to version 2, but `DocumentTypeV2::try_from_schema()` only reads `documentsCountable` / `blastSyncable` and then calls `DocumentTypeV1::try_from_schema()` with the original `schema` and the same `full_validation` flag. The new v1 guard at `try_from_schema/v1/mod.rs:670-695` rejects either field whenever full validation is enabled, so any contract-create or contract-update flow that parses a v12 schema with one of these flags set will raise `UnsupportedFeatureError` before the v2 wrapper can ever reattach the values. In other words, the feature is dead on arrival in the production validation path. Strip the two v2-only keys before delegating to v1 (or explicitly bypass that v1 guard from the v2 entry point) so v12 contracts can actually create count/provable document types.
In `packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- [BLOCKING] lines 670-695: The pre-v12 flag rejection still lets already-stored contracts mutate meaning after the v12 parser switch
The new `documentsCountable` / `blastSyncable` rejection only runs when `full_validation` is true. Contracts read back from Drive use `DataContract::versioned_deserialize(..., false, ...)`, which forwards `full_validation = false` through `DataContractV1::try_from_platform_versioned_v1()` into `create_document_types_from_document_schemas()` and then `DocumentType::try_from_schema()`. On a v12 node that deserialization path now uses the v2 parser, so any contract that was previously accepted into chain state with these raw schema keys present will be reloaded as a count/provable document type even though its primary key tree was originally created as a normal tree. The top-level document meta-schema on the base branch did not forbid unknown document-type properties, so this is not just hypothetical future input: old state can already contain the latent keys. The new guard prevents fresh pre-v12 submissions, but it does not protect existing chain data from changing storage semantics after upgrade. The fix needs to sanitize or reject those fields on deserialization as well, not only during full-validation submission paths.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-211: The new countable/blast immutability checks are missing the companion tests the neighboring invariants already have
`validate_config()` now rejects changes to `documents_countable()` and `blast_syncable()`, which is the right invariant because changing those flags after creation would change the primary storage tree shape. However, the test module below still only covers the older config immutability checks (transferable, can_be_deleted, keep_history, mutable, encryption/decryption key requirements, security level, etc.). A quick grep in this file shows no test exercising the new countable/blast branches. Given that these guards protect storage-layout stability, they deserve the same regression coverage as the surrounding checks.
5adbe0d to
b1064f8
Compare
Collaborator
|
Re-checked the current head ( But I still do not think this is fully good yet. The remaining problem is the pre-v12 deserialization path. So from my side the answer is: not quite yet — I still want that old-state migration / sanitize / rejection hole closed before I clear it. |
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
b1064f8 to
cc855a8
Compare
thepastaclaw
suggested changes
Apr 9, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This force-push fixes the previous dead-on-arrival parser bug: v12 contracts can now actually parse documentsCountable / blastSyncable, and the Drive insert/delete/cost paths consistently derive the matching tree type. But the replacement change removed the pre-v12 rejection entirely, which reopens a consensus/storage-layout problem across upgrades: protocol-11 contracts can now persist those keys in raw schema JSON, be created over a NormalTree, and then get reinterpreted after a v12 upgrade as CountTree / ProvableCountTree document types when the stored contract is deserialized with the current platform version. I also confirmed the new immutability guards still have no direct regression tests.
Reviewed commit: b1064f8
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- [BLOCKING] lines 666-670: Relaxing the V1 parser reopens pre-v12 schema smuggling for countable document types
These lines now intentionally ignore `documentsCountable` / `blastSyncable` in the v1 parser. That is unsafe because the shared document meta-schema already allows both keys (`packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json:508-515`), and pre-v12 platform versions still dispatch `DocumentType::try_from_schema()` to the v1 parser (`packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs:35`, `v3.rs:37`). A protocol-11 contract can therefore submit those raw schema fields, pass validation, and get stored with a `DocumentTypeV1` that creates a `NormalTree` at contract insertion time (`packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs:287-307`). Later, Drive fetches stored contracts by deserializing them with the *current* platform version (`packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85`), and that reconstruction path calls `create_document_types_from_document_schemas()` / `DocumentType::try_from_schema()` with the current version (`packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165`). After a network upgrade to v12, the same stored raw schema is reparsed through the v2 parser, which now reads those keys and makes document operations use `CountTree` / `ProvableCountTree` semantics (`packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs:71-77`) even though the underlying tree was created as `NormalTree`. That is a consensus-critical storage-layout mismatch across upgrades. The v1 path still needs to reject or normalize these keys for pre-v12 contracts instead of silently preserving them in serialized state.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-210: The new countable/blast immutability guards still have no direct regression tests
`validate_config()` now correctly rejects changes to `documents_countable` and `blast_syncable`, which is the guard that prevents an update from changing the primary tree type after creation. But the surrounding test module still only covers the older immutability rules (creation restriction, trade mode, transferable, deletable, keep-history, mutable, bounded-key requirements, security level, etc.). There is still no focused test that builds two v2 document types differing only in `documentsCountable` or `blastSyncable` and asserts `DocumentTypeUpdateError` is returned. Because these flags affect storage/proof layout, they deserve the same regression coverage as the neighboring config invariants.
thepastaclaw
suggested changes
Apr 9, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This force-push refactors rs-drive so inserts, deletes, and estimation paths all compute the primary-key tree type through one helper, which does remove some duplication. But it does not fix the actual blocker from the previous SHA: pre-v12 contracts can still persist documentsCountable / blastSyncable in raw schema JSON, and after a v12 upgrade those stored schemas are reparsed into count/provable document types even though the underlying tree was originally created as NormalTree. I also re-checked the non-blocking follow-up and the new immutability guards still lack direct regression tests in validate_update.
Reviewed commit: cc855a8
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- [BLOCKING] lines 666-670: Pre-v12 contracts can still smuggle count-tree flags and be reinterpreted after upgrade
The v1 parser still intentionally ignores `documentsCountable` / `blastSyncable` instead of rejecting or normalizing them, while the shared document meta-schema still allows those keys. That means a pre-v12 contract can carry the raw fields in `document_schemas`, pass validation as a `DocumentTypeV1`, and be inserted with a physical `NormalTree`. Later, stored contracts are reconstructed from serialized schema bytes with the current platform version (`packages/rs-dpp/src/data_contract/v1/serialization/mod.rs:154-165` and `packages/rs-drive/src/drive/contract/get_fetch/fetch_contract/v0/mod.rs:81-85`). Once the node is on protocol v12, that same raw schema is reparsed through `DocumentTypeV2::try_from_schema`, which revives the flags and makes `primary_key_tree_type` select `CountTree` / `ProvableCountTree` for runtime document operations (`packages/rs-drive/src/drive/document/primary_key_tree_type.rs:18-48`, plus the new call sites in the touched insert/delete/estimation files). So this refactor centralizes the tree selection, but it still centralizes the wrong assumption: old on-disk `NormalTree` contracts can be reinterpreted post-upgrade as count/provable trees. That is a storage-layout mismatch in consensus-critical code, and this SHA does not close it.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-210: The countable and blast-syncable immutability guards still have no dedicated tests
`validate_config()` now rejects changes to `documents_countable` and `blast_syncable`, which is the right invariant because those flags determine the physical primary-key tree layout. But the test module in this file still only has explicit regression cases for the older immutable properties; a targeted search shows the new error strings at lines 189 and 204, yet there is still no corresponding test that builds two document types differing only in these flags and asserts `DocumentTypeUpdateError` is returned. Because these guards protect storage layout rather than a cosmetic schema field, they deserve the same direct coverage as the neighboring transferability/history/mutability/security-level checks.
Adds two new fields to DocumentTypeV2: - `documents_countable: bool` — primary key tree uses CountTree for O(1) total document count queries - `range_countable: bool` — primary key tree uses ProvableCountTree for provable range counting. Implies documents_countable = true. Changes: - DocumentTypeV2 struct with both fields - V2 variant added to DocumentType/DocumentTypeRef/DocumentTypeMutRef - DocumentTypeV2Getters/Setters traits - Schema parsing v2 with protocol version 12+ gating - Drive: CountTree for countable, ProvableCountTree for range_countable - DocumentTypePrimaryKeyTreeType extension trait in rs-drive with own versioning (primary_key_tree_type: 0) - Meta-schema: documentsCountable and rangeCountable properties - Update validation: reject changes to countable/range_countable - All stateless insert/delete/estimation paths use correct tree type Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
feat/documents-countable
branch
from
cc855a8 to
51d1808
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs`:
- Around line 77-89: The dispatcher is choosing DocumentType::try_from_schema_v2
based solely on platform_version, which can mis-parse legacy contracts; modify
the dispatcher in try_from_schema so that before calling
DocumentType::try_from_schema_v2 you check data_contract_system_version (the
stored contract system version) and if data_contract_system_version < 2 either
(a) route to the v1 parser (e.g., call the existing v1 path) or (b)
sanitize/remove V2-only flags like documentsCountable/rangeCountable from schema
when full_validation == false, and only allow V2 parsing when
data_contract_system_version >= 2; update uses of full_validation and
validation_operations accordingly to ensure safe deserialization of pre-v12
contracts.
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 45-63: The code currently reads documents_countable and
range_countable from the schema (variables documents_countable and
range_countable in try_from_schema v2) and later applies them unconditionally,
which allows legacy schemas to be reinterpreted; change try_from_schema (mod.rs)
so these two flags are only honored when the persisted contract/parser version
indicates V2 (or newer) — e.g., check the persisted contract/parser version
field available during deserialization and if the version is older than the V2
threshold, ignore/sanitize DOCUMENTS_COUNTABLE and RANGE_COUNTABLE (force false
or use the original NormalTree semantics) before applying them at the
application site (the code path that currently uses the variables around lines
82–83). Ensure the gating uses the persisted version from the contract/metadata
used to parse the schema rather than the current runtime platform version.
🪄 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: 470928c6-ae21-43df-b985-629e49e53ca4
📒 Files selected for processing (30)
packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.jsonpackages/rs-dpp/src/data_contract/document_type/accessors/mod.rspackages/rs-dpp/src/data_contract/document_type/accessors/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/accessors.rspackages/rs-dpp/src/data_contract/document_type/v2/mod.rspackages/rs-dpp/src/data_contract/document_type/v2/random_document_type.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rspackages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_document_from_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/estimation_costs/add_estimation_costs_for_add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/drive/document/primary_key_tree_type.rspackages/rs-drive/src/fees/op.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v4.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/v12.rs
✅ Files skipped from review due to trivial changes (5)
- packages/rs-drive/src/drive/document/delete/delete_document_for_contract_operations/v0/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs
- packages/rs-drive/src/drive/document/insert/add_document_for_contract_operations/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
- packages/rs-drive/src/fees/op.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/rs-drive/src/drive/document/delete/remove_document_from_primary_storage/v0/mod.rs
- packages/rs-drive/src/drive/document/mod.rs
- packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs
- packages/rs-dpp/src/data_contract/document_type/methods/versioned_methods.rs
- packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json
- packages/rs-platform-version/src/version/v12.rs
- packages/rs-drive/src/drive/document/insert/add_document_to_primary_storage/v0/mod.rs
- packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs
- packages/rs-drive/src/drive/contract/update/update_contract/v0/mod.rs
- packages/rs-drive/src/drive/document/primary_key_tree_type.rs
- packages/rs-dpp/src/data_contract/document_type/accessors/mod.rs
- packages/rs-drive/src/drive/document/delete/internal/add_estimation_costs_for_remove_document_to_primary_storage/v0/mod.rs
Collaborator
|
Re-checked the current head ( The remaining blocker is the same old-state/deserialization hole, and it is still live on this SHA:
So a pre-v12 contract can still be accepted/stored with those raw keys ignored by the old parser, get its physical primary tree created as So from my side: not good yet. I'd want the parser/deserialization path gated on the persisted contract system version (or those v2-only keys stripped/rejected for older contracts during reload), and then I'd be happy to clear it. |
thepastaclaw
suggested changes
Apr 10, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Both agents converge on a real storage-layout reinterpretation vulnerability: pre-v12 contracts can carry documentsCountable/rangeCountable in their raw schema, get a physical NormalTree, and then be deserialized through the v2 parser post-upgrade, causing all fee estimation paths to use the wrong tree type. The immutability guards for these flags also lack the regression tests that every other guard in the same file has. Additional architectural concerns around tree-type logic duplication are valid but lower priority.
Reviewed commit: 51d1808
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🔴 blocking: V1 parser preserves smuggled countable flags that the V2 parser will revive after v12 upgrade
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (lines 666-684)
The v1 parser (lines 666-670) explicitly documents that it ignores documentsCountable/rangeCountable, but these keys are preserved in the raw schema Value stored on DocumentTypeV1.schema (line 684) and persisted to GroveDB. Meanwhile, the document meta-schema (document-meta.json:508-514) explicitly declares both fields as allowed boolean properties at any protocol version.
After v12 activation, stored contracts are deserialized using the current platform_version, which dispatches to try_from_schema: 2 (confirmed in v4.rs:36). The v2 parser (try_from_schema/v2/mod.rs:45-63) reads these flags from the raw schema and sets them on DocumentTypeV2 (line 82). This changes the return value of primary_key_tree_type() from NormalTree to CountTree/ProvableCountTree for a tree that was physically created as NormalTree.
The 6+ call sites using primary_key_tree_type() (document insert, delete, estimation cost paths) will then compute fees against the wrong tree type. While all nodes agree on the same incorrect result (no consensus fork), fee calculations are wrong, and future count query support would produce incorrect results on these contracts.
The comment at v2/mod.rs:36-42 claims this is safe because "the primary key tree type is set correctly at contract creation time," but this conflates one-time physical tree creation with per-operation tree type selection during deserialization.
Fix: Strip documentsCountable and rangeCountable from the schema Value in the v1 parser before storing it on DocumentTypeV1.schema. This prevents the flags from reaching persistent storage regardless of which parser later deserializes the contract. Alternatively, have the v2 parser force these flags to false for contracts with created_at_epoch predating v12.
🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- [BLOCKING] lines 666-684: V1 parser preserves smuggled countable flags that the V2 parser will revive after v12 upgrade
The v1 parser (lines 666-670) explicitly documents that it ignores `documentsCountable`/`rangeCountable`, but these keys are preserved in the raw `schema` Value stored on `DocumentTypeV1.schema` (line 684) and persisted to GroveDB. Meanwhile, the document meta-schema (`document-meta.json:508-514`) explicitly declares both fields as allowed boolean properties at any protocol version.
After v12 activation, stored contracts are deserialized using the current `platform_version`, which dispatches to `try_from_schema: 2` (confirmed in `v4.rs:36`). The v2 parser (`try_from_schema/v2/mod.rs:45-63`) reads these flags from the raw schema and sets them on `DocumentTypeV2` (line 82). This changes the return value of `primary_key_tree_type()` from `NormalTree` to `CountTree`/`ProvableCountTree` for a tree that was physically created as `NormalTree`.
The 6+ call sites using `primary_key_tree_type()` (document insert, delete, estimation cost paths) will then compute fees against the wrong tree type. While all nodes agree on the same incorrect result (no consensus fork), fee calculations are wrong, and future count query support would produce incorrect results on these contracts.
The comment at `v2/mod.rs:36-42` claims this is safe because "the primary key tree type is set correctly at contract creation time," but this conflates one-time physical tree creation with per-operation tree type selection during deserialization.
**Fix**: Strip `documentsCountable` and `rangeCountable` from the schema `Value` in the v1 parser before storing it on `DocumentTypeV1.schema`. This prevents the flags from reaching persistent storage regardless of which parser later deserializes the contract. Alternatively, have the v2 parser force these flags to `false` for contracts with `created_at_epoch` predating v12.
In `packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs`:
- [SUGGESTION] lines 183-211: No regression tests for documents_countable and range_countable immutability guards
The `validate_config()` method correctly rejects changes to `documents_countable` (line 183) and `range_countable` (line 198), which is critical because these flags determine the physical primary key tree layout. However, the `validate_config` test module (lines 288-984) has explicit regression tests for all 9 other immutable config properties — `creation_restriction_mode`, `trade_mode`, `transferable`, `can_be_deleted`, `keep_history`, `mutable`, `encryption_key_req`, `decryption_key_req`, `security_level_requirement` — but none for the two new guards.
Because these flags protect physical storage layout (not just schema semantics), they are higher-stakes than most config immutability checks. A future refactor could inadvertently remove them without any test catching the regression. Add two tests following the existing `should_return_invalid_result_when_<property>_is_changed()` pattern.
In `packages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rs`:
- [SUGGESTION] lines 288-315: Tree-type selection logic duplicated instead of using centralized primary_key_tree_type()
The contract insert path (lines 288-315) and update path (`update_contract/v0/mod.rs:352-380`) both inline the same if/else chain for selecting NormalTree vs CountTree vs ProvableCountTree. Document operations use the centralized `DocumentTypePrimaryKeyTreeType::primary_key_tree_type()` from `primary_key_tree_type.rs`, which participates in version dispatch (`platform_version.drive.methods.document.primary_key_tree_type`). The inline copies skip version dispatch entirely — if a v1 of `primary_key_tree_type` is introduced with different selection logic, contract creation will diverge from document operations. Both sites should call the centralized method to maintain a single source of truth.
Comment on lines
+201
to
+208
| impl DocumentTypeV2Getters for DocumentTypeV2 { | ||
| fn documents_countable(&self) -> bool { | ||
| self.documents_countable || self.range_countable | ||
| } | ||
|
|
||
| fn range_countable(&self) -> bool { | ||
| self.range_countable | ||
| } |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Redundant invariant enforcement — documents_countable getter duplicates constructor's OR logic
The documents_countable() getter (line 203) returns self.documents_countable || self.range_countable, but the v2 constructor (try_from_schema/v2/mod.rs:82) already sets v2.documents_countable = documents_countable || range_countable, and set_range_countable() also enforces this invariant. The dual enforcement is harmless but creates ambiguity about which layer is authoritative. If the field always reflects the invariant at construction time, the getter can simply return self.documents_countable. This prevents subtle inconsistencies if a third code path constructs DocumentTypeV2 without the OR.
💡 Suggested change
Suggested change
| impl DocumentTypeV2Getters for DocumentTypeV2 { | |
| fn documents_countable(&self) -> bool { | |
| self.documents_countable || self.range_countable | |
| } | |
| fn range_countable(&self) -> bool { | |
| self.range_countable | |
| } | |
| fn documents_countable(&self) -> bool { | |
| self.documents_countable | |
| } |
source: ['claude']
10 tasks
Resolve conflicts in CONTRACT_VERSIONS_V4 by combining documents_countable (try_from_schema v2) with v1 document meta-schema (additionalProperties:false). - v4.rs: keep try_from_schema=2 with document_type_schema=1 + enrich_with_base_schema=1 - v1 document-meta.json: add documentsCountable / rangeCountable - ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES: include the two new keys - Test helpers: add V2 match arms for DocumentType / DocumentTypeRef Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/methods/mod.rs (1)
356-374: ⚡ Quick winNo test exercises
DocumentTypeV2-specific semantics (documentsCountable/blastSyncable).All seven V2 dispatch arms are mechanically correct, but
build_doc_typepasses a hardcoded1as the document-type schema version, which means every call produces aDocumentTypeV1(or V0). TheV2arms inDocumentTypeTestHelpersare therefore unreachable from any existing test, and the two boolean flags that are the entire raison d'être ofDocumentTypeV2are never asserted against.At a minimum the test suite should verify:
build_doc_type(or a sibling helper) can produce aDocumentTypeRef::V2when the schema contains"documentsCountable": true/"blastSyncable": true.- A
DocumentTypeV2withdocuments_countable = trueis reflected correctly in the accessors.- The
blast_syncable = truepath impliesdocuments_countable = true.🔍 Verification – confirm the hardcoded `1` forces V1 parsing
#!/bin/bash # Locate the try_from_schema signature to confirm the meaning of the `1` argument ast-grep --pattern 'fn try_from_schema($$$)' # Also check if DocumentTypeRef::V2 is ever matched in the test module currently rg -n 'DocumentTypeRef::V2' --type rustAlso applies to: 622-716
🤖 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/methods/mod.rs` around lines 356 - 374, The helper build_doc_type currently hardcodes schema version `1` when calling DocumentType::try_from_schema, so tests never exercise DocumentTypeV2; update build_doc_type (or add a sibling like build_doc_type_with_version) to accept a schema_version parameter and pass that value to try_from_schema (use `2` to produce DocumentTypeRef::V2), then add tests in DocumentTypeTestHelpers that construct schemas containing "documentsCountable": true and "blastSyncable": true and assert the resulting DocumentTypeRef::V2 (or DocumentTypeV2 via your accessors) exposes documents_countable == true and blast_syncable implies documents_countable; ensure you reference try_from_schema, DocumentTypeV2, and the accessor methods (documents_countable / blast_syncable) in the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json`:
- Around line 508-515: The v1 meta-schema must not accept protocol-v12 flags:
remove the "documentsCountable" and "rangeCountable" properties from the v1
document-meta.json schema so they are not structurally valid in legacy schemas;
instead move or declare these flags in the protocol-v12-specific meta-schema (or
external docs) and, if needed, keep only a non-structural note in v1 text (not
as JSON properties). Update references to the identifiers "documentsCountable"
and "rangeCountable" and ensure any validation logic uses the v12 schema (or a
version-gated schema) rather than leaving these keys in the v1 JSON schema.
---
Nitpick comments:
In `@packages/rs-dpp/src/data_contract/document_type/methods/mod.rs`:
- Around line 356-374: The helper build_doc_type currently hardcodes schema
version `1` when calling DocumentType::try_from_schema, so tests never exercise
DocumentTypeV2; update build_doc_type (or add a sibling like
build_doc_type_with_version) to accept a schema_version parameter and pass that
value to try_from_schema (use `2` to produce DocumentTypeRef::V2), then add
tests in DocumentTypeTestHelpers that construct schemas containing
"documentsCountable": true and "blastSyncable": true and assert the resulting
DocumentTypeRef::V2 (or DocumentTypeV2 via your accessors) exposes
documents_countable == true and blast_syncable implies documents_countable;
ensure you reference try_from_schema, DocumentTypeV2, and the accessor methods
(documents_countable / blast_syncable) in the assertions.
🪄 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: 54d5e09e-ada8-4b20-b7a2-3fd5bd84719e
📒 Files selected for processing (5)
packages/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/methods/mod.rspackages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rspackages/rs-dpp/src/data_contract/document_type/v1/mod.rs
✅ Files skipped from review due to trivial changes (3)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
- packages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rs
- packages/rs-dpp/src/data_contract/document_type/v1/mod.rs
Collaborator
|
Re-checked the current head ( The old-state/deserialization storage-layout hole still looks live:
So a pre-v12 contract with those raw keys can still be physically created with a The latest CodeRabbit helper-test suggestion is useful but lower priority; I would not block merge on that alone. The parser/storage-layout invariant above is still the blocker from my side. |
The v11→v12 migration previously used ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES as its allowlist. After documentsCountable / rangeCountable were added to that list (so new v12 contracts can declare them under the v1 meta-schema's additionalProperties: false), the migration would no longer strip those flags from pre-v12 contracts that smuggled them into raw schema bytes. Post-upgrade, the v2 parser would revive the flags and make primary_key_tree_type() return CountTree / ProvableCountTree for a tree that was physically created as NormalTree, causing fee miscalculation across every document insert/delete/estimation path. Add a separate ALLOWED_DOCUMENT_SCHEMA_PRE_V12_PROPERTIES list (V1 minus the v12-introduced flags) and a matching strip helper. The migration uses the pre-v12 helper so smuggled flags are stripped before the v2 parser ever sees them. The existing end-to-end migration test now also injects documentsCountable / rangeCountable and asserts they are absent post- migration both on disk and via the Drive fetch API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The v0 document meta-schema is the validator that admitted every pre-v12 contract on disk; mutating it would change historical validation results and break consensus replay. Mark it frozen / view-only via a top-level \$comment, and remove documentsCountable / rangeCountable, which were inadvertently introduced into v0 by 51d1808. Those properties are v12+ and live in the v1 meta-schema; pre-v12 smuggling of the keys is handled by the v11→v12 migration strip (see commit 409fea9). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a top-level \$comment noting that the v1 document meta-schema is still editable while Platform 3.1 is in development, but freezes once 3.1 ships and v12 activates on mainnet — after that, any change would alter historical validation results and break consensus replay. New top-level properties or rules must go in v2+. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
|
Rechecked current head ( The important change is that the legacy replay problem is handled at the migration boundary rather than in the v2 parser:
I also ran the targeted allowlist regression check locally: So I’m no longer blocking on the old-state/deserialization issue. The remaining CodeRabbit helper-test note is useful coverage, but I wouldn’t treat that as a merge blocker by itself. |
… immutability validate_config rejects changes to documents_countable and range_countable because those flags determine the physical primary-key tree layout (NormalTree vs CountTree vs ProvableCountTree). The validate_config test module already covers the 9 other immutable config properties; add the missing two regression tests in the same should_return_invalid_result_when_<property>_is_changed pattern so a future refactor cannot silently drop the storage-layout guards. The range_countable test pins documentsCountable: true on both old and new so the documents_countable() check above passes equally and validate_config reaches the range_countable branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous shape introduced V12_INTRODUCED_DOCUMENT_SCHEMA_TOP_LEVEL_PROPERTIES, a strip_to_allowlist helper, and a strip_unknown_properties_from_document_schema (v1) helper. None had a production caller — they only powered tests that referred back to themselves. Keep only what the v11→v12 migration actually needs: - ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES (parity-tested against v1 meta-schema JSON, also referenced by rs-scripts). - ALLOWED_DOCUMENT_SCHEMA_PRE_V12_PROPERTIES (= V1 list minus the v12-introduced flags, used by the migration strip). - strip_unknown_properties_from_pre_v12_document_schema (the migration's strip). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…EMA_V1_PROPERTIES The constant's actual semantic is "what survives the v11→v12 migration into v1-document-meta-schema-conforming bytes", not "what v1 meta-schema accepts". The two are deliberately different: v1 meta-schema admits the v12-introduced flags (documentsCountable, rangeCountable) for new v12 contracts, but those keys must NOT survive the migration on pre-v12 contracts — otherwise the v2 parser would revive them and reinterpret a NormalTree as a count tree. - Rename ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES → ALLOWED_TRANSITION_TO_DOCUMENT_SCHEMA_V1_PROPERTIES. - Update the doc comment to spell out the subset relationship. - Relax the parity test from "allowlist == v1 properties" to "allowlist ⊂ v1 properties" — the reverse direction would block the v12 feature. - Update the rs-scripts caller to the new name. Migration call site keeps using strip_unknown_properties_from_document_schema (unchanged); only the constant it consults was renamed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ions The previous subset-of-v1 check only caught one direction of drift: it prevented adding allowlist entries that v1 doesn't accept, but happily allowed v1 to grow new top-level properties without anyone deciding whether the migration should preserve or strip them on pre-v12 contracts. Replace it with a strict equality: allowlist == v0_properties ∪ STANDARD_V1_JSON_SCHEMA_KEYWORDS_KEPT_IN_TRANSITION The new constant lists exactly the JSON-Schema-standard keywords v1 added that we deliberately preserve (required, \$comment, description, minProperties, maxProperties, dependentRequired) — none affect storage layout. Any future v1 addition (like the v12-introduced documentsCountable / rangeCountable that prompted this whole work) breaks both tests until someone explicitly decides: include in the additions list (preserve), or leave out (strip from pre-v12 bytes). Also add a sanity test that every entry in the additions list actually is a v1 addition (in v1, not v0). Also expand the constant's doc comment to spell out why the harmless JSON-Schema-standard keys are kept while documentsCountable / rangeCountable are excluded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
commented
May 8, 2026
Member
Author
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Self Reviewed in depth.
…ty_provable_count_tree
The contract insert and update paths inlined the same documents_countable /
range_countable → tree-variant if/else, calling LowLevelDriveOperation
constructors directly and rebuilding the path Vec themselves. Add Drive
helpers parallel to batch_insert_empty_tree / batch_insert_empty_sum_tree
so call sites just pick the right method:
if document_type.range_countable() {
batch_insert_empty_provable_count_tree(...)
} else if document_type.documents_countable() {
batch_insert_empty_count_tree(...)
} else {
batch_insert_empty_tree(...)
}
- Add for_estimated_path_key_empty_count_tree /
_empty_provable_count_tree on LowLevelDriveOperation so the new helpers
can also handle KeySize cost-estimation paths.
- Add batch_insert_empty_count_tree (mod.rs + v0/mod.rs + 3 unit tests)
and batch_insert_empty_provable_count_tree (mod.rs + v0/mod.rs + 3
unit tests), both versioned through DriveGroveBatchMethodVersions.
- Wire DriveGroveBatchMethodVersions with the two new fields, defaulted
to 0 in v1 of drive_grove_method_versions.
- Update insert_contract/v0 and update_contract/v0 to use the helpers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a `countable_e2e_tests` module in insert_contract/v0 that builds real
v12 contracts via DataContractFactory, applies them to a Drive
(setup_drive_with_initial_state_structure), reads the primary-key tree
element back from grovedb, and asserts the concrete tree variant + count
match what the schema requested. This closes the "feature actually works
end-to-end" gap that was left after the unit-level dispatch tests.
Six new tests:
default_contract_creates_normal_tree_for_primary_key
Sanity baseline: no flags → Element::Tree (NormalTree).
documents_countable_contract_creates_count_tree_for_primary_key
documentsCountable: true → Element::CountTree(_, 0, _) at insertion;
DocumentTypeV2 reports documents_countable=true, range_countable=false.
range_countable_contract_creates_provable_count_tree_for_primary_key
rangeCountable: true → Element::ProvableCountTree(..);
DocumentTypeV2 reports both flags true (range implies countable).
count_tree_count_grows_and_shrinks_with_document_inserts_and_deletes
Insert 3 documents → CountTree count == 3. Delete 1 → count == 2.
provable_count_tree_count_grows_with_document_inserts
Insert 5 documents into a rangeCountable contract → ProvableCountTree
count == 5.
count_tree_contract_supports_document_fetch
Sanity: switching the primary-key tree variant doesn't break the
existing all-items document query path.
These exercise the previously-unverified positive feature path —
schema → CountTree creation → count maintenance through inserts/deletes
— and confirm DocumentTypeV2 invariants are observed end-to-end on a
real grovedb instance.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_tree_type() The contract insert and update paths still inlined their own range_countable / documents_countable selection. Document insert, delete, and estimation paths route through the centralized DocumentTypeRef::primary_key_tree_type() helper, which participates in drive method version dispatch (platform_version.drive.methods.document.primary_key_tree_type). If that helper ever gets a v1 with different selection logic, contract creation and document operations would silently diverge. Match on the helper's TreeType output instead so all five paths (contract insert, contract update, document insert, document delete, estimation) agree on a single source of truth. Variants other than NormalTree / CountTree / ProvableCountTree return a CorruptedCodeExecution error because they're not valid for primary-key trees. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eeType Collapse the explicit `TreeType::NormalTree` arm and the `CorruptedCodeExecution` fallback into a single `_ =>` catch-all that inserts a NormalTree. NormalTree is the safe default for any current or future TreeType variant the dispatcher might return that isn't specifically count/provable-count, and avoids a runtime error path for something that's effectively a code-evolution surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… / rangeCountable Closes the two coverage gaps left after the e2e tests: Rust fee-correctness pinning (packages/rs-drive) Three new tests in countable_e2e_tests assert that switching the primary-key tree variant materially changes the fees Drive reports — if NormalTree / CountTree / ProvableCountTree contracts produced the same FeeResult, the differentiating code path was never exercised. Don't pin exact values (brittle under fee schedule changes); pin the directional invariant. - count_tree_contract_apply_produces_different_fees_than_normal_tree - provable_count_tree_contract_apply_produces_different_fees_than_count_tree - document_insert_into_count_tree_produces_positive_fees JS round-trip coverage (packages/wasm-dpp2) New spec file DataContractCountable.spec.ts asserts that the v12 flags survive the JS DataContract API: - schemas getter exposes documentsCountable / rangeCountable verbatim - toBytes / fromBytes round-trip preserves both flags - toObject / fromObject round-trip preserves documentsCountable - fullValidation accepts both flags together at v12 These pin the wire/serialization contract for SDK consumers — if a future change silently dropped the flags during JS-side serialization, contracts created via the SDK would land on disk with the wrong primary-key tree variant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
May 8, 2026
Resolves conflicts in two auto-generated client files: - packages/dapi-grpc/clients/platform/v0/java/.../PlatformGrpc.java - packages/dapi-grpc/clients/platform/v0/python/platform_pb2.py Both sides expose the same set of underlying RPC methods; this branch's side inserts METHODID_GET_DOCUMENTS_COUNT (15) and METHODID_GET_DOCUMENTS_SPLIT_COUNT (16) and bumps the subsequent IDs by 2, while v3.1-dev kept the original numbering. Take the count-query branch's numbering since it adds the new endpoints these client bindings need. Brings in PR #3457 (documentsCountable / rangeCountable feature) so this branch's count-query handlers can read CountTree / ProvableCountTree primary key trees end-to-end. Test results post-merge: - 3 documents_count tests pass - 4 documents_split_count tests pass - 5 drive count-query tests pass - 10 countable e2e tests pass (incl. v3.1-dev's new range_countable_primary_key_tree_supports_trunk_proof) - cargo check --workspace clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
added a commit
that referenced
this pull request
May 10, 2026
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>
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
3 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
Adds two new fields to
DocumentTypeV2for O(1) document counting:documents_countable: bool— primary key tree usesCountTree, enabling O(1) total document count queriesblast_syncable: bool— primary key tree usesProvableCountTreefor BLAST sync support. Impliesdocuments_countable = true.Changes
V2variant with full match arm coverageDocumentTypeV2Getters/DocumentTypeV2Setterstry_from_schemav2 parses both"documentsCountable"and"blastSyncable"(protocol version 12+ required)CountTreefor countable,ProvableCountTreefor blast_syncableCONTRACT_VERSIONS_V4withtry_from_schema: 2, used in v12Schema example
{ "person": { "type": "object", "documentsCountable": true, "properties": { ... } } }Or with BLAST sync:
{ "person": { "type": "object", "blastSyncable": true, "properties": { ... } } }NOT in this PR
Count query extensions — follow-up to PR #3435.
Test plan
cargo check -p dpp -p drivepassescargo fmt --allclean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores