feat(drive): add array element indexing support #2974
Conversation
📝 WalkthroughWalkthroughAdds indexed-array support: schema validation for indexed array properties, array item encoding/decoding, a Document API to return deduplicated encoded array elements, per-element index insert/remove/update logic in Drive, a new Query Contains operator, platform-version flags, tests, and visibility/API adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant DriveOp as Drive Operation
participant Document as DocumentV0
participant DPP as DocumentType/ArrayItemType
participant Drive as Drive Indexer
DriveOp->>Document: get_raw_array_elements_for_document_type(key_path, doc_type)
Document->>DPP: flatten properties, lookup array item type
DPP-->>Document: ArrayItemType
Document->>Document: encode each non-null element via ArrayItemType.encode_element_for_tree_keys()
Document-->>DriveOp: Vec<encoded elements> (deduplicated)
DriveOp->>Drive: for each encoded element -> add/remove index entry (push key, create/remove subtree, recurse)
Drive-->>DriveOp: result
sequenceDiagram
participant Updater as Update Flow
participant Drive as Drive
participant Document as DocumentV0
Updater->>Drive: check if any index contains array
Drive-->>Updater: yes
Updater->>Document: get_raw_array_elements_for_document_type(old_key)
Document-->>Updater: old elements
Updater->>Drive: remove_indices_for_top_index_level per old element
Updater->>Document: get_raw_array_elements_for_document_type(new_key)
Document-->>Updater: new elements
Updater->>Drive: add_indices_for_top_index_level per new element
Drive-->>Updater: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (13)📚 Learning: 2024-10-29T14:40:54.727ZApplied to files:
📚 Learning: 2025-06-18T03:44:14.385ZApplied to files:
📚 Learning: 2024-11-20T20:43:41.185ZApplied to files:
📚 Learning: 2024-11-20T16:05:40.200ZApplied to files:
📚 Learning: 2025-05-28T16:22:26.334ZApplied to files:
📚 Learning: 2024-11-25T01:17:02.001ZApplied to files:
📚 Learning: 2024-10-21T01:03:42.458ZApplied to files:
📚 Learning: 2025-10-09T15:59:28.329ZApplied to files:
📚 Learning: 2024-10-22T10:53:12.111ZApplied to files:
📚 Learning: 2024-11-03T10:39:11.242ZApplied to files:
📚 Learning: 2025-10-01T08:37:32.168ZApplied to files:
📚 Learning: 2025-04-11T09:08:05.652ZApplied to files:
📚 Learning: 2025-04-11T09:08:05.652ZApplied to files:
🧬 Code graph analysis (1)packages/rs-dpp/src/data_contract/document_type/property/array.rs (4)
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @packages/rs-dpp/src/data_contract/document_type/property/array.rs:
- Around line 274-278: The Date branch in ArrayItemType::Date currently uses
value.to_integer(), causing encoding to i64 which is inconsistent with
encode_value_with_size; change it to use value.into_float() (as
encode_value_with_size does for DocumentPropertyType::encode_date_timestamp) and
encode the f64 bytes (e.g., f64::to_be_bytes) so index keys and stored values
use the same f64-based date encoding.
- Around line 284-288: The ArrayItemType::Number branch in
encode_element_for_tree_keys incorrectly serializes floats with
value.to_float().to_be_bytes(), which breaks lexicographic ordering; replace
that serialization by calling the existing
DocumentPropertyType::encode_float(...) helper (the same function used by scalar
F64 encoding) to produce the ordered byte representation, ensuring the branch
uses value.to_float().map_err(ProtocolError::ValueError)? as input to
DocumentPropertyType::encode_float and returns its Vec<u8> result so array
element Number encoding matches scalar F64 tree-key ordering.
- Around line 279-283: ArrayItemType::Integer currently encodes i64 values with
plain to_be_bytes() which breaks lexicographic ordering for negative integers;
replace that encoding with the same sign-bit-flipped big-endian encoding used by
DocumentPropertyType::encode_i64 by calling
DocumentPropertyType::encode_i64(value_as_i64) (or replicating its wtr[0] ^=
0b1000_0000 flip) instead of to_be_bytes(), ensuring ArrayItemType::Integer
produces the same sortable byte representation as scalar integers.
In
@packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rs:
- Around line 110-121: The validation currently sets is_array_property by
matching only DocumentPropertyType::Array(_), but DocumentPropertyType also has
DocumentPropertyType::VariableTypeArray(_) which should be treated the same;
update the match in the is_array_property computation (the call chain starting
from document_type.flattened_properties().get(name).map(...)) to include
DocumentPropertyType::VariableTypeArray(_) alongside Array(_) so that both array
variants cause the NotSupported error when creating contested indexes.
🧹 Nitpick comments (9)
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (1)
176-183: Consider extracting duplicated array detection logic.The array property detection logic at lines 177-183 is duplicated at lines 501-507. Consider extracting this into a helper function or closure to improve maintainability.
♻️ Suggested refactor
// Extract helper closure at the start of the function let index_contains_array = |index: &Index| -> bool { index.properties.iter().any(|prop| { document_type .flattened_properties() .get(&prop.name) .map(|p| matches!(p.property_type, DocumentPropertyType::Array(_))) .unwrap_or(false) }) }; // Then use: if index_contains_array(index) { continue; } // And: let any_index_has_array = document_type.indexes().values().any(index_contains_array);packages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rs (1)
139-142: Redundant operation:all_fields_null &= truehas no effect.The expression
all_fields_null &= trueis equivalent toall_fields_null = all_fields_null & true, which doesn't change the value. If the intent is to preserveall_fields_nullunchanged, the line can be removed.🔧 Suggested simplification
if array_elements.is_empty() { // Empty array - track as null any_fields_null = true; - all_fields_null &= true; } else {packages/rs-drive/tests/query_tests.rs (6)
7003-7140: Hardcoded post IDs diverge from the new JSON fixtures; validate Identifier encoding + reduce drift.
The helper embeds$id/$ownerIdvalues directly (Lines 7039-7129) while separate fixtures were added undertests/supporting_files/contract/array-index/.
- Please confirm the
$id/$ownerIdstrings in both the fixtures and these inline JSON values decode to the expected Identifier type (and are 32 bytes if that’s the constraint). A quick alphabet check script is in thepost1.jsoncomment.- Consider loading
post0.json/post1.jsonfixtures here (or deriving IDs from known 32-byte hex) to avoid fixture/code divergence.
7227-7275: Serialization test is fine; avoidunwrap()afteris_ok()to keep failure output.
Right now the test checksis_ok()thenserialized.unwrap()(Lines 7266-7274), which will lose the original context if it ever regresses.Proposed tweak
- assert!( - serialized.is_ok(), - "Serialization should succeed, got: {:?}", - serialized.err() - ); - assert!( - serialized.unwrap().len() > 0, - "Serialized bytes should not be empty" - ); + let bytes = serialized.expect("serialization should succeed"); + assert!(!bytes.is_empty(), "Serialized bytes should not be empty");
7340-7388: Test naming doesn’t follow repo guideline (“should …”).
New tests usetest_*naming (Lines 7341+). If you want to follow**/tests/**/*.rsguidance, rename the new ones.Based on coding guidelines, tests should start with “should …”.
7390-7500:containsquery tests: add assertions on returned document IDs to prevent false positives.
Currently you only assertresults.len()(Lines 7427-7431, 7460-7464, 7493-7497). A regression could return the wrong 2 docs and still pass.Example follow-up assertion (pattern)
assert_eq!( results.len(), 2, "expected 2 posts containing 'dash' hashtag" ); + // TODO: also assert returned $id set matches expected (order-independent)
7502-7542: Array element encoding assertion may be brittle if “tree key encoding” changes.
The test assumes returned bytes equalb"alpha"etc. (Lines 7539-7541). If element encoding includes type markers/normalization, this will fail even if behavior is correct.
- Consider asserting via the same encoding routine used for indexing (e.g.,
ArrayItemType.encode_element_for_tree_keys(...)) so the test tracks intended encoding semantics.
7544-7584: Dedup test: also assert which elements remain.
Right now it only checkslen() == 2(Lines 7578-7582). Verifying membership prevents accidental “dedup to wrong set” bugs.packages/rs-drive/tests/supporting_files/contract/array-index/array-index-contract.json (1)
1-58: Contract fixture looks correct for array-element indexing constraints.
Array index is standalone or last in compound index, anditemsis a bounded scalar type.Consider adding
namefields for the indexes to make failures/debug output easier to interpret.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rspackages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/mod.rspackages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/v0/mod.rspackages/rs-dpp/src/document/document_methods/mod.rspackages/rs-dpp/src/document/mod.rspackages/rs-dpp/src/document/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/util/object_size_info/document_info.rspackages/rs-drive/tests/query_tests.rspackages/rs-drive/tests/supporting_files/contract/array-index/array-index-contract.jsonpackages/rs-drive/tests/supporting_files/contract/array-index/post0.jsonpackages/rs-drive/tests/supporting_files/contract/array-index/post1.jsonpackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v2.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v3.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/mod.rspackages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v1.rspackages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/mod.rspackages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/mod.rspackages/rs-dpp/src/document/mod.rspackages/rs-drive/tests/query_tests.rspackages/rs-dpp/src/document/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v2.rspackages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rspackages/rs-dpp/src/document/document_methods/mod.rspackages/rs-drive/src/query/conditions.rspackages/rs-drive/src/util/object_size_info/document_info.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v3.rs
**/tests/**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'
Unit and integration tests should not perform network calls; mock dependencies
Files:
packages/rs-drive/tests/query_tests.rs
🧠 Learnings (14)
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2024-12-10T10:56:26.215Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2030
File: packages/rs-sdk/tests/vectors/test_asset_lock_proof/quorum_pubkey-100-4ce7fd81273c2b394c0f32367374fc5b09ba912e017aacb366d2171e9ca6f9d5.json:1-1
Timestamp: 2024-12-10T10:56:26.215Z
Learning: In the `packages/rs-sdk/tests/vectors/test_asset_lock_proof/` directory, files with the `.json` extension are mock data that may not follow standard JSON format; this is intentional and acceptable within the project's testing framework.
Applied to files:
packages/rs-drive/tests/supporting_files/contract/array-index/post1.json
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/mod.rspackages/rs-dpp/src/document/mod.rspackages/rs-dpp/src/document/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v2.rspackages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/document/document_methods/mod.rspackages/rs-drive/src/query/conditions.rspackages/rs-drive/src/util/object_size_info/document_info.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2024-11-28T13:49:17.301Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2317
File: packages/rs-dapi-client/src/address_list.rs:175-180
Timestamp: 2024-11-28T13:49:17.301Z
Learning: In Rust code in `packages/rs-dapi-client/src/address_list.rs`, do not change the interface of deprecated methods like `add_uri`, even to fix potential panics.
Applied to files:
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/mod.rspackages/rs-dpp/src/document/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v2.rs
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-drive/tests/query_tests.rs
📚 Learning: 2024-11-25T01:17:02.001Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2347
File: packages/rs-drive/tests/query_tests.rs:438-460
Timestamp: 2024-11-25T01:17:02.001Z
Learning: In Rust test files (`packages/rs-drive/tests/query_tests.rs`), when code is used only in tests, defining explicit enums for fields (like the `status` field in the `Withdrawal` struct) may not be necessary; using primitive types is acceptable.
Applied to files:
packages/rs-drive/tests/query_tests.rs
📚 Learning: 2025-03-07T12:37:44.555Z
Learnt from: ogabrielides
Repo: dashpay/platform PR: 2486
File: packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs:1-29
Timestamp: 2025-03-07T12:37:44.555Z
Learning: GroveDB operations like `insert()` return a tuple that includes a cost value which needs to be unwrapped with `.unwrap()` before error handling with `.map_err()`.
Applied to files:
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts is already handled in the data contract validation process, specifically in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts (ensuring it's between 3 and 100 characters) is already handled at the data contract validation level in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs, making additional checks in the update operations redundant.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs
📚 Learning: 2025-05-28T16:22:26.334Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2644
File: packages/rs-drive/src/cache/system_contracts.rs:18-19
Timestamp: 2025-05-28T16:22:26.334Z
Learning: In packages/rs-drive/src/cache/system_contracts.rs, the `active_since_protocol_version` field in `ActiveSystemDataContract` struct is intentionally added for future use, not current implementation. QuantumExplorer confirmed this is "meant for later" when questioned about the `#[allow(unused)]` attribute.
Applied to files:
packages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v2.rs
📚 Learning: 2025-02-03T23:34:43.081Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2450
File: packages/rs-drive/src/drive/tokens/distribution/queries.rs:57-57
Timestamp: 2025-02-03T23:34:43.081Z
Learning: In Rust query implementations, the `..` operator after a value creates a `RangeFrom` bound rather than performing vector slicing, which is a safe operation for constructing range-based queries.
Applied to files:
packages/rs-drive/src/query/conditions.rs
📚 Learning: 2024-11-20T14:14:54.721Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2337
File: packages/rs-dapi-client/src/executor.rs:161-212
Timestamp: 2024-11-20T14:14:54.721Z
Learning: In `packages/rs-dapi-client/src/executor.rs`, the `WrapWithExecutionResult` trait supports on-the-fly type conversions using the `From` trait, which is useful when using the `?` operator to return errors.
Applied to files:
packages/rs-drive/src/query/conditions.rs
🧬 Code graph analysis (9)
packages/rs-dpp/src/data_contract/document_type/property/array.rs (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (4)
value(2451-2451)value(2452-2452)value(2467-2467)get_field_type_matching_error(2441-2446)
packages/rs-drive/tests/query_tests.rs (4)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-45)packages/rs-drive/src/drive/contract/test_helpers.rs (1)
add_init_contracts_structure_operations(8-10)packages/rs-dpp/src/tests/json_document.rs (1)
json_document_to_contract(66-74)packages/rs-dpp/src/document/serialization_traits/platform_serialization_conversion/mod.rs (1)
serialize(19-30)
packages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
name(113-135)
packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rs (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
name(113-135)
packages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rs (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
name(113-135)
packages/rs-dpp/src/document/document_methods/mod.rs (2)
packages/rs-drive/src/util/object_size_info/document_info.rs (4)
get_raw_array_elements_for_document_type(65-70)get_raw_array_elements_for_document_type(261-282)get_raw_for_document_type(54-61)get_raw_for_document_type(156-258)packages/rs-dpp/src/document/mod.rs (4)
get_raw_array_elements_for_document_type(147-175)get_raw_for_contract(82-113)get_raw_for_document_type(116-145)hash(177-200)
packages/rs-drive/src/util/object_size_info/document_info.rs (2)
packages/rs-dpp/src/document/document_methods/mod.rs (1)
get_raw_array_elements_for_document_type(41-46)packages/rs-dpp/src/document/mod.rs (1)
get_raw_array_elements_for_document_type(147-175)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (2)
name(113-135)max_size(260-291)
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rs (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
name(113-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image
🔇 Additional comments (49)
packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rs (1)
13-13: LGTM! Import is necessary and correctly placed.The
DocumentPropertyTypeimport is required for the array property type check introduced below and follows standard Rust conventions.packages/rs-drive/src/query/conditions.rs (8)
76-77: LGTM: Contains operator properly documented.The doc comment clearly explains the operator's purpose for array field queries.
95-95: LGTM: Flip behavior correctly disabled for Contains.Contains cannot be meaningfully flipped (unlike comparison operators), so returning false in
allows_flip()and an error inflip()is correct.Also applies to: 125-127
138-139: LGTM: Contains correctly categorized as a range operator.Since Contains requires scanning multiple index entries (one per array element), treating it as a range operator is appropriate.
241-244: LGTM: Contains evaluation logic is correct.The implementation properly checks if the document's array field contains the query value, returning false if the field is not an array.
255-272: LGTM: Comprehensive type validation for Contains operator.The implementation correctly validates that the query value's type matches the array's element type, covering all
ArrayItemTypevariants. This ensures type safety at query construction time.
668-668: LGTM: Contains correctly classified as non-groupable.Like StartsWith and Between variants, Contains cannot be meaningfully combined with other range operators, so the non-groupable classification is correct.
Also applies to: 686-686
1702-1703: LGTM: Contains operator correctly restricted to Array types.The implementation properly limits Contains to
DocumentPropertyType::Array, preventing misuse on scalar fields. Note thatVariableTypeArraycurrently has no allowed operators, which may be a known limitation.
1202-1224: Clarify what "elsewhere" means for array element path construction in Contains queries.The implementation correctly serializes the search element and inserts it as a key. However, the comment is imprecise about path adjustment happening "elsewhere." The array element subtree navigation actually occurs through the standard
recursive_insert_on_query()processing, which handles remaining index properties sequentially. For clarity, the comment should explain that the field name becomes the subquery key in the path building logic (viaquery.set_subquery_key(first.name.as_bytes().to_vec())), allowing the serialized element value to naturally navigate to the correct element subtree within the index structure.packages/rs-drive/src/query/filter.rs (1)
538-538: LGTM: Contains correctly rejected for price clause validation.Since price is a scalar numeric field (
U64), the Contains operator (designed for array fields) is correctly marked as invalid, consistent with how StartsWith is rejected for numeric contexts.packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/mod.rs (1)
32-32: LGTM: Visibility widened to support array indexing.The visibility change from
pub(super)topub(crate)appropriately extends access to this function within the crate, aligning with the array indexing feature's requirements.packages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v2.rs (1)
24-31: LGTM: Consistent version initialization.The addition of
get_raw_array_elements_for_document_type: 0properly tracks the new document method version, consistent with the initialization pattern for other method versions.packages/rs-dpp/src/data_contract/document_type/mod.rs (1)
69-69: LGTM: Standard JSON Schema constant.The addition of the
ITEMSconstant follows the established pattern and aligns with the JSON Schema standard for defining array item types.packages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v1.rs (1)
24-31: LGTM: Consistent version initialization.The addition of
get_raw_array_elements_for_document_type: 0properly tracks the new document method version, maintaining consistency with the version initialization pattern.packages/rs-dpp/src/data_contract/document_type/property/array.rs (1)
260-262: The collision concern is partially addressed; empty strings are already mitigated but empty byte arrays are not.The code already handles null-to-empty-vec collisions for string types by encoding empty strings as
[0](see lines 1155-1157 in mod.rs and array.rs). However,ByteArraytypes inencode_element_for_tree_keys(array.rs line 289) return raw bytes without collision mitigation, meaning null values and empty byte arrays both encode to an empty vector. This should either be explicitly documented as safe or mitigated consistently with string handling.packages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/mod.rs (1)
24-24: LGTM!The new version tracking field follows the established pattern and naming conventions for document method versions.
packages/rs-dpp/src/document/document_methods/mod.rs (3)
6-6: LGTM!Module declaration follows the established pattern.
12-12: LGTM!Re-export visibility is correctly scoped to
crate::document.
38-46: LGTM!The method signature is well-documented and follows the pattern of similar methods. The return type
Vec<Vec<u8>>appropriately represents multiple encoded array elements, and the absence ofowner_idparameter is reasonable since array elements are encoded for index keys independently.packages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/mod.rs (1)
1-3: LGTM!Module structure and re-export pattern are consistent with sibling document method modules.
packages/rs-platform-version/src/version/dpp_versions/dpp_document_versions/v3.rs (1)
29-29: LGTM!Version initialization to
0is correct for the new method, and placement withinDocumentMethodVersionsis logical.packages/rs-dpp/src/document/v0/mod.rs (2)
22-23: LGTM!Import follows the established pattern and maintains the logical grouping of document method traits.
180-182: LGTM!Trait implementation follows the established pattern used by sibling document method traits, with the "automatically done" comment indicating a default or blanket implementation.
packages/rs-dpp/src/document/mod.rs (1)
147-175: LGTM!The new
get_raw_array_elements_for_document_typemethod follows the established version-dispatch pattern consistently with other methods in this file. The error handling for unknown versions is correctly implemented.packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rs (2)
111-114: Verify empty array handling in update scenarios.When an array is empty, the removal loop is skipped entirely. This is correct if no index entries were created for an empty array. However, in update scenarios where an array transitions from non-empty to empty, the old index entries need to be removed.
Please verify that the update path (in
update_document_for_contract_operations) correctly handles this case by ensuring the old document's array elements are passed for removal, not the new document's (potentially empty) array.
100-184: LGTM for array property removal logic.The per-element removal flow correctly:
- Retrieves array elements via
get_raw_array_elements_for_document_type- Iterates each element to construct element-specific paths
- Handles cost estimation per element
- Recursively removes lower-level index entries
packages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs (1)
167-203: LGTM for per-element array indexing.The per-element insertion logic correctly:
- Creates a tree for each element value
- Pushes the element to the path
- Recursively indexes sub-levels for each element
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs (1)
113-211: LGTM for top-level array index insertion.The array handling correctly:
- Skips empty arrays (no index entries created)
- Creates per-element trees and paths
- Handles cost estimation per element
- Recursively processes sub-levels for each element
The pattern is consistent with the removal logic in the corresponding module.
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (2)
457-540: LGTM for array item type validation.The validation logic correctly:
- Validates that array item types are indexable scalars (String, ByteArray, Integer, Number, Identifier, Date)
- Enforces bounded sizes for String and ByteArray elements
- Rejects Boolean arrays as not meaningful for indexing
- Limits to one array property per compound index
600-616: LGTM for array position validation.The check correctly enforces that array properties must be the last property in a compound index, which is essential for the per-element indexing strategy to work correctly with prefix-based queries.
packages/rs-dpp/src/document/document_methods/get_raw_array_elements_for_document_type/v0/mod.rs (1)
11-70: LGTM!The implementation correctly:
- Looks up the array item type from the document type definition
- Returns empty vec for non-array or missing properties (graceful fallback)
- Encodes each non-null element using the item type's encoder
- Deduplicates elements while preserving first-occurrence order
- Propagates encoding errors appropriately
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (1)
497-540: The code correctly avoids double-processing. The main loop iterates over indexes (not individual properties) and skips any index that contains an array property entirely—not just the array properties themselves. The remove/add functions then handle all properties of these skipped indexes, processing both scalar and array properties as appropriate. No scalar properties are processed twice.Likely an incorrect or invalid review comment.
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (3)
11-12: LGTM!The conditional import of
ArrayItemTypeunder thevalidationfeature flag is correctly placed alongside other validation-specific imports.
432-556: LGTM!The array property validation logic is well-structured:
- Properly validates each
ArrayItemTypevariant with appropriate constraints- Correctly enforces single array property per index
- Tracks array position for the "must be last" validation
- Consistent with the v0 schema validation approach
The use of
enumerateand position tracking is clean.
617-633: LGTM!The "array must be last" constraint is correctly enforced after iterating through all index properties. This ensures the array property validation is complete before checking its position.
packages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rs (4)
7-8: LGTM!The new imports for
DocumentTypeV0Getters,DocumentPropertyType, andKeyare correctly added to support array property detection and per-element key construction.Also applies to: 18-18
92-97: LGTM!The array property detection safely falls back to
falsewhen the property isn't found inflattened_properties(). This correctly handles system properties like$ownerIdwhich won't be arrays.
146-168: LGTM!The per-element iteration correctly:
- Creates a
Keyfrom each encoded element value- Clones the path and appends the element key
- Recurses with
all_fields_null = falsesince each element represents a non-null valueThis mirrors the add_indices logic for consistency.
170-204: LGTM!The scalar property removal logic is correctly preserved in the
elsebranch, maintaining the existing behavior for non-array properties.packages/rs-drive/src/util/object_size_info/document_info.rs (2)
62-70: LGTM!The trait method signature correctly mirrors the
Documentmethod frompackages/rs-dpp/src/document/document_methods/mod.rs, enabling consistent array element retrieval across allDocumentInfovariants.
260-282: LGTM with a note on estimation behavior.The implementation correctly:
- Delegates to the document's method for all concrete document variants
- Maps
ProtocolErrortoError::Protocolconsistently with other methods- Returns an empty
VecforDocumentEstimatedAverageSizesince actual array elements aren't availableNote: For fee estimation involving array-indexed properties, the empty return may underestimate costs since per-element operations won't be counted. If fee accuracy becomes a concern, consider documenting this limitation or adding estimated element counts.
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (3)
14-14: LGTM!The public re-export of
ArrayItemTypeenables external modules to access this type for array element handling.
1227-1236: LGTM!The
Arraycase now correctly delegates toitem_type.encode_element_for_tree_keys(value)for Contains query support, whileVariableTypeArrayappropriately remains unsupported for tree key encoding.
2329-2403: LGTM!The array parsing logic is comprehensive and well-structured:
- Correctly parses typed arrays via the
itemsmap with support for String, Integer, Number, Boolean, and nested ByteArray- Falls back to ByteArray handling when
itemsis not present- Properly validates that
byteArraymust betrueif defined (lines 2388-2392)- Handles Identifier detection via
contentMediaTypeThe fact that
ArrayItemType::Booleancan be parsed here while being rejected during index validation (intry_from_schema) is appropriate - parsing represents the schema structure, while index validation enforces indexing constraints separately.packages/rs-drive/tests/supporting_files/contract/array-index/post0.json (1)
1-6: LGTM: solid fixture for array-index tests.
Shape matches the intended contract (contentstring +hashtagsstring array).packages/rs-drive/tests/query_tests.rs (3)
7142-7192: Good coverage: validates array property types in bothflattened_properties()andproperties().
This catches the common “flattened vs nested schema mismatch” failure mode.
7194-7225: Nice: asserts CBOR round-trip yieldsValue::Arraywith textual elements.
7277-7338: Good: reproduces serialization using a contract loaded viasetup_contract.
This is a useful regression guard if contract-loading paths differ.packages/rs-drive/tests/supporting_files/contract/array-index/post1.json (1)
1-6: Remove or update post1.json fixture—unused file with invalid base58 identifiers.post1.json is an orphaned fixture file not referenced anywhere in the codebase. While the
$idfield does contain non-base58 characters (I, O, 0), these don't break tests since the file is never parsed. Either delete the unused fixture or replace the invalid identifiers if the file serves documentation purposes.Likely an incorrect or invalid review comment.
.../src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rs
Show resolved
Hide resolved
...rive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/rs-dpp/src/data_contract/document_type/property/array.rs:
- Around line 275-279: ArrayItemType::Date conversion casts an i64 to u64
without checking for negative values, which can wrap and corrupt lexicographic
ordering; update the array encoding path to validate the integer from
value.to_integer() (the local value_as_i64) is non-negative and return a
ProtocolError::ValueError when it's negative, otherwise call
DocumentPropertyType::encode_date_timestamp with the safe cast to u64; reference
ArrayItemType::Date, value.to_integer(), value_as_i64 and
DocumentPropertyType::encode_date_timestamp when making the change.
In
@packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs:
- Around line 163-167: The error message string used in the FeeError::Overflow
when checking document_top_field_estimated_size (> u8::MAX as u16) is incorrect
for the add_indices operation; update the message to refer to "add_indices"
(e.g., "document field is too big for being an index on add_indices" or similar)
in the branch with the condition checking document_top_field_estimated_size and
make the identical change for the second occurrence around the other check (the
similar FeeError::Overflow at the later location).
🧹 Nitpick comments (2)
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (1)
176-195: Array detection logic is correct but duplicated.The array property detection pattern is repeated at lines 177-188 and again at lines 505-518. Consider extracting this into a helper function or caching the result to avoid redundant iteration.
Suggested refactor
+ // Pre-compute which indexes contain array properties + let indexes_with_arrays: Vec<_> = document_type + .indexes() + .values() + .filter(|index| { + index.properties.iter().any(|prop| { + document_type + .flattened_properties() + .get(&prop.name) + .map(|p| { + matches!( + p.property_type, + DocumentPropertyType::Array(_) | DocumentPropertyType::VariableTypeArray(_) + ) + }) + .unwrap_or(false) + }) + }) + .collect(); + // fourth we need to store a reference to the document for each index for index in document_type.indexes().values() { - // Check if this index contains an array property - let index_has_array = index.properties.iter().any(|prop| { - document_type - .flattened_properties() - .get(&prop.name) - .map(|p| { - matches!( - p.property_type, - DocumentPropertyType::Array(_) | DocumentPropertyType::VariableTypeArray(_) - ) - }) - .unwrap_or(false) - }); - - // For indexes with array properties, skip the inline scalar update logic. - if index_has_array { + // Skip array-containing indexes - handled separately below + if indexes_with_arrays.iter().any(|i| std::ptr::eq(*i, index)) { continue; }packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs (1)
151-181: Cost estimation inserted redundantly inside element loop.The
estimated_costs_only_with_layer_info.insertat line 169-180 usesindex_path.clone()as the key, which is the same for all elements. This means the same entry is overwritten on each iteration. Consider moving this block before the element loop.Suggested refactor
+ if let Some(estimated_costs_only_with_layer_info) = + estimated_costs_only_with_layer_info + { + let document_top_field_estimated_size = document_and_contract_info + .owned_document_info + .document_info + .get_estimated_size_for_document_type( + name, + document_type, + platform_version, + )?; + + if document_top_field_estimated_size > u8::MAX as u16 { + return Err(Error::Fee(FeeError::Overflow( + "document field is too big for being an index", + ))); + } + + estimated_costs_only_with_layer_info.insert( + KeyInfoPath::from_known_owned_path(index_path.clone()), + EstimatedLayerInformation { + tree_type: TreeType::NormalTree, + estimated_layer_count: PotentiallyAtMaxElements, + estimated_layer_sizes: AllSubtrees( + document_top_field_estimated_size as u8, + NoSumTrees, + storage_flags.map(|s| s.serialized_size()), + ), + }, + ); + } + // For each array element, create an index entry for element_value in array_elements { let element_key = Key(element_value); let path_key_info = element_key.clone().add_path::<0>(index_path.clone()); // Insert tree for this element value self.batch_insert_empty_tree_if_not_exists( path_key_info.clone(), TreeType::NormalTree, storage_flags, apply_type, transaction, previous_batch_operations, batch_operations, drive_version, )?; - if let Some(estimated_costs_only_with_layer_info) = - estimated_costs_only_with_layer_info - { - // ... move this block before the loop - } - let any_fields_null = false; // Not null since we have an element
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
🧠 Learnings (15)
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-05-28T16:22:26.334Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2644
File: packages/rs-drive/src/cache/system_contracts.rs:18-19
Timestamp: 2025-05-28T16:22:26.334Z
Learning: In packages/rs-drive/src/cache/system_contracts.rs, the `active_since_protocol_version` field in `ActiveSystemDataContract` struct is intentionally added for future use, not current implementation. QuantumExplorer confirmed this is "meant for later" when questioned about the `#[allow(unused)]` attribute.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-25T01:17:02.001Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2347
File: packages/rs-drive/tests/query_tests.rs:438-460
Timestamp: 2024-11-25T01:17:02.001Z
Learning: In Rust test files (`packages/rs-drive/tests/query_tests.rs`), when code is used only in tests, defining explicit enums for fields (like the `status` field in the `Withdrawal` struct) may not be necessary; using primitive types is acceptable.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-10-09T15:59:28.329Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs:181-187
Timestamp: 2025-10-09T15:59:28.329Z
Learning: In `packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs`, the maintainer requires logging full state transition bytes (`tx_bytes = hex::encode(st_bytes)`) at debug level when a state transition passes CheckTx but is removed from the block by the proposer, to facilitate debugging of this rare edge case.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-10-22T10:53:12.111Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/dapi_client.rs:137-139
Timestamp: 2024-10-22T10:53:12.111Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when passing data into asynchronous code, ensure that data structures are `Send + Sync`. Using `Arc<AtomicUsize>` is necessary for the retry counter.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts (ensuring it's between 3 and 100 characters) is already handled at the data contract validation level in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs, making additional checks in the update operations redundant.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts is already handled in the data contract validation process, specifically in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2025-03-07T12:37:44.555Z
Learnt from: ogabrielides
Repo: dashpay/platform PR: 2486
File: packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs:1-29
Timestamp: 2025-03-07T12:37:44.555Z
Learning: GroveDB operations like `insert()` return a tuple that includes a cost value which needs to be unwrapped with `.unwrap()` before error handling with `.map_err()`.
Applied to files:
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
🧬 Code graph analysis (2)
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs (3)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
name(113-135)packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rs (2)
PathInfo(169-169)PathInfo(250-250)packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rs (1)
PathInfo(99-99)
packages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs (4)
packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rs (2)
PathInfo(169-169)PathInfo(250-250)packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs (3)
PathInfo(196-196)PathInfo(291-291)element_key(137-137)packages/rs-drive/src/drive/document/insert_contested/add_contested_indices_for_contract_operations/v0/mod.rs (1)
PathInfo(99-99)packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
name(113-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (7)
packages/rs-dpp/src/data_contract/document_type/property/array.rs (1)
255-308: Well-structured tree key encoding implementation.The method correctly delegates to
DocumentPropertyTypeencoding helpers for proper lexicographic ordering of numeric types, handles null values appropriately, and prevents empty string collision with null by usingvec![0].packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (1)
520-550: Remove+add approach for array indexes looks correct.The pattern of removing old index entries first (with
&Nonefor previous batch operations) and then adding new entries (with properprevious_batch_operations) is appropriate. The asymmetric handling makes sense: removal doesn't need conflict checking against pending operations, but insertion does.packages/rs-drive/src/drive/document/delete/remove_indices_for_top_index_level_for_contract_operations/v0/mod.rs (2)
156-157: Hardcoded null flags for array elements.Setting
any_fields_null = falseandall_fields_null = falsefor array elements assumes that the presence of an element means the field is not null. This is semantically correct - if we have an element value to remove, the field itself isn't null.
105-189: Array element removal logic is well-structured.The per-element iteration correctly:
- Skips empty arrays (no elements to remove)
- Creates proper path segments for each element value
- Uses consistent
PathInfoconstruction matching the scalar path- Recurses to handle sub-level indices
packages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs (1)
153-208: Per-element tree insertion for arrays is correctly implemented.The logic properly:
- Retrieves all array elements (with deduplication handled by the source method)
- Skips empty arrays (lines 164-169)
- Creates a subtree for each element value (lines 176-189)
- Pushes the element to the path and recurses (lines 191-207)
The comment on line 199 (
// Not all fields null since we have an element) correctly explains thefalsevalue.packages/rs-drive/src/drive/document/delete/remove_indices_for_index_level_for_contract_operations/v0/mod.rs (1)
133-173: Array element removal at index level is consistent with insertion logic.The implementation mirrors
add_indices_for_index_level_for_contract_operations_v0:
- Same array detection pattern
- Same empty array handling (continue without action)
- Same per-element path construction and recursion
- No tree insertion needed (removal only)
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs (1)
118-216: Array indexing at top level is correctly implemented.The implementation correctly:
- Detects array properties using the consistent pattern
- Skips empty arrays (lines 129-132)
- Creates index trees for each element value
- Constructs proper
PathInfobased on stateful vs stateless operation mode- Recurses with appropriate null flags
packages/rs-dpp/src/data_contract/document_type/property/array.rs
Outdated
Show resolved
Hide resolved
.../src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs:
- Around line 175-197: The post-loop remove+add functions are re-processing
scalar-only indexes (causing duplicate updates); modify the logic so post-loop
removal/addition runs only for indexes that contain array properties: inside the
first loop that checks index_has_array (the loop iterating
document_type.indexes() and computing index_has_array), collect or mark which
indexes have arrays and set a flag if any exist, then change the later calls to
remove_indices_for_top_index_level_for_contract_operations and
add_indices_for_top_index_level_for_contract_operations to either accept/receive
only the array-containing index set or be invoked only when the “any
index_has_array” flag is true so scalar-only indexes are not processed again
(adjust function signatures or add a parameter to filter to array-only indexes
if needed).
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (1)
176-189: DRY: array-property detection logic is duplicatedThe same “is any indexed property an array?” predicate appears twice. Consider extracting a small helper (or compute once and reuse) to avoid divergence bugs and repeated lookups.
Also applies to: 506-520
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/data_contract/document_type/property/array.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
🧠 Learnings (5)
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts (ensuring it's between 3 and 100 characters) is already handled at the data contract validation level in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs, making additional checks in the update operations redundant.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts is already handled in the data contract validation process, specifically in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Detect immutable structure changes
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp2) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (1)
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (1)
22-22: Import looks appropriate for the new behaviorNo concerns with bringing
DocumentPropertyTypeinto scope here.
...s-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs (1)
151-181: Consider moving cost estimation outside the element loop.The cost estimation block (lines 169-180) uses
index_path.clone()as the HashMap key, which is the same for all array elements. This means the same entry is being overwritten on each iteration with identical values. While functionally correct, this is redundant work.♻️ Suggested refactor to move estimation before the loop
if array_elements.is_empty() { // Empty array - nothing to index at top level continue; } + if let Some(estimated_costs_only_with_layer_info) = + estimated_costs_only_with_layer_info + { + let document_top_field_estimated_size = document_and_contract_info + .owned_document_info + .document_info + .get_estimated_size_for_document_type( + name, + document_type, + platform_version, + )?; + + if document_top_field_estimated_size > u8::MAX as u16 { + return Err(Error::Fee(FeeError::Overflow( + "document field is too big for being an index", + ))); + } + + estimated_costs_only_with_layer_info.insert( + KeyInfoPath::from_known_owned_path(index_path.clone()), + EstimatedLayerInformation { + tree_type: TreeType::NormalTree, + estimated_layer_count: PotentiallyAtMaxElements, + estimated_layer_sizes: AllSubtrees( + document_top_field_estimated_size as u8, + NoSumTrees, + storage_flags.map(|s| s.serialized_size()), + ), + }, + ); + } + // For each array element, create an index entry for element_value in array_elements { let element_key = Key(element_value); let path_key_info = element_key.clone().add_path::<0>(index_path.clone()); // Insert tree for this element value self.batch_insert_empty_tree_if_not_exists( path_key_info.clone(), TreeType::NormalTree, storage_flags, apply_type, transaction, previous_batch_operations, batch_operations, drive_version, )?; - if let Some(estimated_costs_only_with_layer_info) = - estimated_costs_only_with_layer_info - { - // ... move this block before the loop - } let any_fields_null = false;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/data_contract/document_type/property/array.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
🧠 Learnings (8)
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts (ensuring it's between 3 and 100 characters) is already handled at the data contract validation level in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs, making additional checks in the update operations redundant.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts is already handled in the data contract validation process, specifically in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-09-30T11:55:43.856Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2185
File: packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs:47-62
Timestamp: 2024-09-30T11:55:43.856Z
Learning: In `rebroadcast_expired_withdrawal_documents_v1`, the variable `expired_withdrawal_indices` needs to be a `Vec<WithdrawalTransactionIndex>` rather than a `BTreeSet<WithdrawalTransactionIndex>`, because a vector is necessary for subsequent processing.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs
📚 Learning: 2025-03-07T12:37:44.555Z
Learnt from: ogabrielides
Repo: dashpay/platform PR: 2486
File: packages/rs-drive/src/drive/platform_state/store_reduced_platform_state_bytes/v0/mod.rs:1-29
Timestamp: 2025-03-07T12:37:44.555Z
Learning: GroveDB operations like `insert()` return a tuple that includes a cost value which needs to be unwrapped with `.unwrap()` before error handling with `.map_err()`.
Applied to files:
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Rust packages (wasm-dpp2) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (13)
packages/rs-drive/src/drive/document/insert/add_indices_for_top_index_level_for_contract_operations/v0/mod.rs (3)
6-16: LGTM!New imports for
KeyandDocumentPropertyTypeare correctly added and used in the array handling logic below.
106-116: LGTM!Array property detection correctly identifies both
ArrayandVariableTypeArraytypes, with safe fallback tofalsefor missing properties.
217-312: LGTM!Scalar property handling correctly preserves the existing logic, including proper null flag tracking and recursive index processing.
packages/rs-drive/src/drive/document/insert/add_indices_for_index_level_for_contract_operations/v0/mod.rs (5)
6-11: LGTM!New imports for array handling are correctly added:
Key/KeyReffor key construction,DocumentTypeV0Gettersfor property access, andDocumentPropertyTypefor type checking.
96-106: LGTM!Array property detection is consistent with the top-level implementation.
108-151: LGTM!Property name tree insertion and cost estimation are correctly placed before the array/scalar branching, ensuring the structure is created regardless of property type.
153-208: LGTM!Array handling correctly:
- Documents the semantic difference between empty arrays and null scalars (lines 165-167)
- Preserves
any_fields_nullfrom parent scope while settingall_fields_nullto false for elements- Creates per-element index entries and recurses appropriately
209-259: LGTM!Scalar property handling correctly preserves existing behavior with proper null flag updates using
|=and&=operators.packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (5)
22-22: LGTM!Import is correctly added and used for the array property type matching.
206-226: LGTM - Remove+add approach for array indexes.The approach of removing old index entries and adding new ones is sound for handling array-containing indexes. This correctly handles element-level changes in arrays without the complexity of inline diffing.
The use of
&Noneforprevious_batch_operationsduring removal is appropriate since the removal phase should operate unconditionally on the old document state.
173-190: LGTM - Array property detection logic.The upfront check correctly identifies whether any index contains array-typed properties by looking up each indexed property in the flattened properties map. The
unwrap_or(false)fallback is a safe default for properties that may not be found.
228-228: LGTM!The comment clearly documents that the following optimized inline update logic only applies when no indexes contain array properties.
196-203: No action needed. The code at lines 196-203 correctly handles index removal by usingowner_id: None, which causes$ownerIdreferences in indexes to resolve to the document's embedded owner_id. The comment accurately describes this behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/rs-dpp/src/data_contract/document_type/property/mod.rs:
- Around line 2347-2395: The array item type match for item_type_str in the
items_map parsing is missing the "date" branch; add a case matching "date" that
returns ArrayItemType::Date so schemas with items.type == "date" are accepted
(update the match arm in the block that maps item_type_str to ArrayItemType
inside the items_map handling where ArrayItemType variants like String, Integer,
Number, Boolean, Array are matched, and ensure the function still returns
DocumentPropertyType::Array(array_item_type)).
🧹 Nitpick comments (5)
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (1)
173-190: LGTM - array detection logic is sound.The upfront check correctly identifies whether any index contains array properties by matching against
DocumentPropertyType::ArrayandDocumentPropertyType::VariableTypeArray. Usingunwrap_or(false)safely handles edge cases where properties might not be found inflattened_properties().For future optimization, consider caching this check at the
DocumentTypelevel (e.g., ahas_array_indexed_property()method) to avoid repeated iteration on every update. This is a minor concern for typical document types but could matter at scale.packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
772-776: Remove redundant conditional.Both branches return the same value. The
if elements.is_empty()check serves no purpose here.♻️ Proposed fix
- if elements.is_empty() { - Ok((Some(Value::Array(elements)), false)) - } else { - Ok((Some(Value::Array(elements)), false)) - } + Ok((Some(Value::Array(elements)), false))packages/rs-drive/tests/query_tests.rs (3)
7003-8264: Rename new tests to follow “should …” naming convention (guidelines).All newly-added tests are
test_*, but**/tests/**/*.rsshould have descriptive names starting withshould …. Please rename the tests in this module accordingly. As per coding guidelines, ...Proposed rename pattern (example)
- fn test_query_documents_with_contains_operator() { + fn should_query_documents_with_contains_operator() {(Apply similarly across the other
test_*functions in this module.)
7009-7140: Consider extracting/reusing a shared “contracts tree + contract load” helper to reduce duplication.
setup_array_index_testsrepeats the same boilerplate used elsewhere in this file (drive setup, transaction, init contracts tree, apply batch, setup_contract, commit). Given how many tests depend on it, a small helper (e.g.,setup_drive_with_contract(path, platform_version)) would cut repetition and lower maintenance cost.
7277-7338:test_array_document_serialize_with_drive_contract: simplify scope + remove explicitdrop(drive)unless it’s solving a real leak.The extra block scope and
drop(drive)read like leftovers; in tests they typically don’t add value, and can make future refactors noisier.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rspackages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-drive/tests/query_tests.rspackages/rs-drive/tests/supporting_files/contract/array-index/array-index-with-types-contract.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rspackages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/tests/query_tests.rs
**/tests/**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'
Unit and integration tests should not perform network calls; mock dependencies
Files:
packages/rs-drive/tests/query_tests.rs
🧠 Learnings (18)
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts (ensuring it's between 3 and 100 characters) is already handled at the data contract validation level in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs, making additional checks in the update operations redundant.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rspackages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts is already handled in the data contract validation process, specifically in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/property/mod.rs
📚 Learning: 2024-09-30T11:55:43.856Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2185
File: packages/rs-drive-abci/src/execution/platform_events/withdrawals/rebroadcast_expired_withdrawal_documents/v1/mod.rs:47-62
Timestamp: 2024-09-30T11:55:43.856Z
Learning: In `rebroadcast_expired_withdrawal_documents_v1`, the variable `expired_withdrawal_indices` needs to be a `Vec<WithdrawalTransactionIndex>` rather than a `BTreeSet<WithdrawalTransactionIndex>`, because a vector is necessary for subsequent processing.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2024-11-22T08:19:14.448Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs:93-99
Timestamp: 2024-11-22T08:19:14.448Z
Learning: In `packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs`, the `insert_contract` method requires an owned `BlockInfo`, so cloning `block_info` is necessary when calling it.
Applied to files:
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-05-28T16:22:26.334Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2644
File: packages/rs-drive/src/cache/system_contracts.rs:18-19
Timestamp: 2025-05-28T16:22:26.334Z
Learning: In packages/rs-drive/src/cache/system_contracts.rs, the `active_since_protocol_version` field in `ActiveSystemDataContract` struct is intentionally added for future use, not current implementation. QuantumExplorer confirmed this is "meant for later" when questioned about the `#[allow(unused)]` attribute.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-25T01:17:02.001Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2347
File: packages/rs-drive/tests/query_tests.rs:438-460
Timestamp: 2024-11-25T01:17:02.001Z
Learning: In Rust test files (`packages/rs-drive/tests/query_tests.rs`), when code is used only in tests, defining explicit enums for fields (like the `status` field in the `Withdrawal` struct) may not be necessary; using primitive types is acceptable.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/tests/query_tests.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-10-09T15:59:28.329Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs:181-187
Timestamp: 2025-10-09T15:59:28.329Z
Learning: In `packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs`, the maintainer requires logging full state transition bytes (`tx_bytes = hex::encode(st_bytes)`) at debug level when a state transition passes CheckTx but is removed from the block by the proposer, to facilitate debugging of this rare edge case.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-10-22T10:53:12.111Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/dapi_client.rs:137-139
Timestamp: 2024-10-22T10:53:12.111Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when passing data into asynchronous code, ensure that data structures are `Send + Sync`. Using `Arc<AtomicUsize>` is necessary for the retry counter.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-12-10T10:56:26.215Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2030
File: packages/rs-sdk/tests/vectors/test_asset_lock_proof/quorum_pubkey-100-4ce7fd81273c2b394c0f32367374fc5b09ba912e017aacb366d2171e9ca6f9d5.json:1-1
Timestamp: 2024-12-10T10:56:26.215Z
Learning: In the `packages/rs-sdk/tests/vectors/test_asset_lock_proof/` directory, files with the `.json` extension are mock data that may not follow standard JSON format; this is intentional and acceptable within the project's testing framework.
Applied to files:
packages/rs-drive/tests/supporting_files/contract/array-index/array-index-with-types-contract.json
📚 Learning: 2024-11-25T07:49:05.419Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2345
File: packages/wallet-utils-contract/schema/v1/wallet-utils-contract-documents.json:49-55
Timestamp: 2024-11-25T07:49:05.419Z
Learning: In the schema definitions for the `wallet-utils-contract` (file `packages/wallet-utils-contract/schema/v1/wallet-utils-contract-documents.json`), the `$createdAt` field is a system field augmented by DPP and does not need to be defined explicitly in the schema's properties.
Applied to files:
packages/rs-drive/tests/supporting_files/contract/array-index/array-index-with-types-contract.json
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-drive/tests/query_tests.rs
🧬 Code graph analysis (1)
packages/rs-dpp/src/data_contract/document_type/property/array.rs (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (7)
value(2469-2469)value(2470-2470)value(2485-2485)get_field_type_matching_error(2459-2464)encode_date_timestamp(1507-1509)encode_i64(1655-1678)encode_float(1981-2018)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (wasm-dpp2) / Tests
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (12)
packages/rs-drive/src/drive/document/update/internal/update_document_for_contract_operations/v0/mod.rs (3)
22-22: LGTM!The import of
DocumentPropertyTypeis necessary for the pattern matching onArrayandVariableTypeArrayvariants in the new array detection logic.
242-243: LGTM!The comment clearly documents that the following code path is specifically for scalar-only indexes, providing good context for future maintainers.
192-240: Two-phase remove+add approach is sound.The strategy of applying removes first, then adds, correctly handles overlapping array elements between old and new documents by preventing GroveDB conflicts when the same key is both removed and inserted.
Key implementation details verified:
- Line 199:
owner_id: Nonecorrectly signals "use owner from the document" (consistent with line 272)- Line 213: Passing
&Noneforprevious_batch_operationsproperly isolates removal logic from pending operations- Lines 221–227:
apply_batch_low_level_drive_operationsexecutes remove operations within the transaction context before add operations proceed, maintaining operation ordering and atomicitypackages/rs-dpp/src/data_contract/document_type/property/array.rs (4)
1-7: LGTM!The new imports are appropriate for the added functionality:
DocumentPropertyTypefor delegating tree-key encoding to scalar type helpers,VarIntReaderfor decoding length-prefixed data inread_from, andstd::io::{BufRead, Read}for the buffer reading interface.
256-318: Well-designed tree-key encoding for array elements.The implementation correctly:
- Mirrors the scalar encoding in
DocumentPropertyType::encode_value_for_tree_keys- Uses proper lexicographic ordering helpers (
encode_i64,encode_float,encode_date_timestamp)- Handles edge cases (null → empty vec, empty string →
[0]to avoid collision)- Validates date timestamps are non-negative
346-355: Verify Date type encoding/decoding symmetry.The
read_frommethod decodes Date asf64::from_be_bytesand returnsValue::Float. However, looking atencode_value_ref_with_size(line 216-219), Date is encoded withvalue.to_float()?.to_be_bytes(). This appears consistent.However, the AI summary and PR objectives mention that dates should be timestamps (integers). If dates are intended to be stored as
u64timestamps elsewhere (as seen in the tree-key encoding usingencode_date_timestamp), there may be an inconsistency between serialization and tree-key encoding formats. Please verify this is the intended behavior.
410-418: Boolean decoding is correct.The decoding logic (
bool_byte[0] != 0meanstrue) correctly matches the encoding inencode_value_ref_with_sizewheretrue → [1]andfalse → [0].packages/rs-dpp/src/data_contract/document_type/property/mod.rs (3)
14-14: LGTM!The public re-export of
ArrayItemTypeappropriately exposes the type for downstream crates that need to work with array element encoding for indexing.
1245-1254: LGTM!The implementation correctly delegates single-element tree-key encoding to
ArrayItemType::encode_element_for_tree_keysfor Contains query support. The error handling forVariableTypeArrayis appropriately explicit.
2396-2421: LGTM!The fallback logic correctly handles the byte array case with clear error messages when the schema is malformed. The
contentMediaTypecheck for Identifier detection is consistent with the existing pattern.packages/rs-drive/tests/query_tests.rs (1)
7502-7542: The test assertions are correct for String arrays.For
ArrayItemType::String,encode_element_for_tree_keysreturnsvalue_as_text.as_bytes().to_vec()for non-empty strings (lines 268-273 inpackages/rs-dpp/src/data_contract/document_type/property/array.rs), which is identical to raw UTF-8 bytes. The assertionelements.iter().any(|e| e == b"alpha")correctly matches the encoding used.Note: This is specific to String arrays. Non-string array types (Integer, Date, Number, Boolean) use different encodings (sign-bit flip, timestamp encoding, float encoding, etc.), but this test only uses String arrays, so the assertions are valid.
packages/rs-drive/tests/supporting_files/contract/array-index/array-index-with-types-contract.json (1)
1-82: Well-structured test contract covering multiple array types.This test fixture provides good coverage for the new array indexing feature:
- String arrays (
tags)- Integer arrays (
scores)- Byte array arrays (
identifierswith 32-byte items)All indices are single-property, satisfying the constraints that arrays must be last in compound indexes and only one array per index.
One minor observation: the indices don't have explicit
namefields. If the system auto-generates names this is fine, but explicit names can aid debugging in test failures.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @packages/rs-dpp/src/data_contract/document_type/property/array.rs:
- Around line 257-318: encode_element_for_tree_keys currently returns an empty
Vec for null and returns raw bytes for ArrayItemType::ByteArray, causing empty
byte arrays to collide with null; in the ArrayItemType::ByteArray branch of
encode_element_for_tree_keys, check if bytes.is_empty() after calling
value.to_binary_bytes() and return Ok(vec![0]) for empty byte arrays (mirroring
the String branch) otherwise return Ok(bytes), so empty byte arrays don’t
collide with the null encoding.
In @packages/rs-drive/tests/query_tests.rs:
- Around line 7228-7275: The test test_array_document_serialize consumes the
Result by calling serialized.err(), causing a move so the later
serialized.unwrap() is a use-after-move; change the assertion to borrow the
error instead (e.g., use serialized.as_ref().err() or match on &serialized) so
the Result is not moved, then you can safely call serialized.unwrap()
afterwards; refer to the serialized variable and
DocumentPlatformConversionMethodsV0::serialize to locate the change.
- Around line 7003-8278: Tests violate naming guideline and include redundant
imports; rename the test functions to start with should_ (e.g.,
test_array_index_contract_types -> should_have_array_index_contract_types,
test_array_document_values ->
should_preserve_array_document_values_after_cbor_deserialize,
test_array_document_serialize -> should_serialize_document_with_array_field,
test_array_document_serialize_with_drive_contract ->
should_serialize_with_drive_contract, test_array_index_contract_creation ->
should_create_contract_with_array_index,
test_query_documents_with_contains_operator ->
should_query_documents_with_contains_operator,
test_get_raw_array_elements_for_document_type ->
should_get_raw_array_elements_for_document_type,
test_array_elements_deduplication -> should_deduplicate_array_elements,
test_delete_document_with_array_field ->
should_remove_array_index_entries_on_delete, test_update_document_array_field ->
should_update_array_index_entries_on_update, test_empty_array_field ->
should_not_index_empty_arrays, test_array_with_integer_elements ->
should_index_integer_array_elements, test_compound_index_with_array ->
should_query_compound_index_with_array) and remove redundant in-test imports
that duplicate module-level imports (for example remove the inner uses of
dpp::... in the bodies of should_have_array_index_contract_types,
should_serialize_document_with_array_field,
should_serialize_with_drive_contract,
should_preserve_array_document_values_after_cbor_deserialize, etc.), leaving
only imports that are not already provided by use super::*; this will align
names with the repo guideline and eliminate clippy warnings about redundant
imports.
- Around line 7502-7583: Add assertions in test_array_elements_deduplication to
verify the actual deduplicated element byte values, not just the count: after
calling document.get_raw_array_elements_for_document_type(...) on "hashtags" and
storing into elements, assert that elements contains b"dash" and b"crypto"
(matching how encode_element_for_tree_keys produces raw UTF‑8 bytes) and nothing
else; keep using elements.iter().any(|e| e == b"dash") / elements.iter().any(|e|
e == b"crypto") or equivalent exact-match checks to ensure correct deduplication
and encoding.
🧹 Nitpick comments (2)
packages/rs-drive/tests/query_tests.rs (2)
7009-7140:setup_array_index_testslooks fine; consider validating base58 IDs are 32 bytes to avoid “mystery” failures.Since
$id/$ownerIdare hardcoded strings (and deserialization will fail if any aren’t valid 32-byte identifiers), adding an explicit decode/assert once in setup can make failures much easier to diagnose.
7390-7500: Contains-operator coverage is solid; consider asserting returned document IDs (not just counts).Right now these tests only assert
results.len(). Adding ID assertions would better catch accidental duplicate/ordering regressions (especially with per-element index entries).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/tests/query_tests.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/tests/query_tests.rs
**/tests/**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'
Unit and integration tests should not perform network calls; mock dependencies
Files:
packages/rs-drive/tests/query_tests.rs
🧠 Learnings (11)
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-06-18T03:44:14.385Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-05-28T16:22:26.334Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2644
File: packages/rs-drive/src/cache/system_contracts.rs:18-19
Timestamp: 2025-05-28T16:22:26.334Z
Learning: In packages/rs-drive/src/cache/system_contracts.rs, the `active_since_protocol_version` field in `ActiveSystemDataContract` struct is intentionally added for future use, not current implementation. QuantumExplorer confirmed this is "meant for later" when questioned about the `#[allow(unused)]` attribute.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-25T01:17:02.001Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2347
File: packages/rs-drive/tests/query_tests.rs:438-460
Timestamp: 2024-11-25T01:17:02.001Z
Learning: In Rust test files (`packages/rs-drive/tests/query_tests.rs`), when code is used only in tests, defining explicit enums for fields (like the `status` field in the `Withdrawal` struct) may not be necessary; using primitive types is acceptable.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rspackages/rs-drive/tests/query_tests.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2025-10-09T15:59:28.329Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2716
File: packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs:181-187
Timestamp: 2025-10-09T15:59:28.329Z
Learning: In `packages/rs-dapi/src/services/platform_service/broadcast_state_transition.rs`, the maintainer requires logging full state transition bytes (`tx_bytes = hex::encode(st_bytes)`) at debug level when a state transition passes CheckTx but is removed from the block by the proposer, to facilitate debugging of this rare edge case.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-11-03T10:39:11.242Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2305
File: packages/rs-drive-abci/src/abci/handler/finalize_block.rs:81-0
Timestamp: 2024-11-03T10:39:11.242Z
Learning: In `packages/rs-drive-abci/src/abci/handler/finalize_block.rs`, the use of `.expect("commit transaction")` after `app.commit_transaction(platform_version)` is intentional due to the provided comment explaining its necessity. Do not flag this usage in future reviews.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-10-22T10:53:12.111Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2259
File: packages/rs-dapi-client/src/dapi_client.rs:137-139
Timestamp: 2024-10-22T10:53:12.111Z
Learning: In `packages/rs-dapi-client/src/dapi_client.rs`, when passing data into asynchronous code, ensure that data structures are `Send + Sync`. Using `Arc<AtomicUsize>` is necessary for the retry counter.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/array.rs
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-drive/tests/query_tests.rs
🧬 Code graph analysis (1)
packages/rs-drive/tests/query_tests.rs (3)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-45)packages/rs-drive/src/drive/contract/test_helpers.rs (1)
add_init_contracts_structure_operations(8-10)packages/rs-dpp/src/tests/json_document.rs (1)
json_document_to_contract(66-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (2)
packages/rs-drive/tests/query_tests.rs (1)
7003-8278: No action needed. CI already runs tests with theserverfeature enabled. The test workflow usescargo test --all-features, which includes all features defined in the crate (includingserver). The module and its tests will be compiled and executed in CI.packages/rs-dpp/src/data_contract/document_type/property/array.rs (1)
1-7: Imports look consistent with the new APIs.
DocumentPropertyType,VarIntReader, andBufRead/Readare all used by the added helpers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/rs-dpp/src/data_contract/document_type/property/mod.rs:
- Around line 757-776: The DocumentPropertyType::Array branch deserializes
array_len and allocates a Vec without size checks, allowing DoS via huge
lengths; add a maxItems bound to DocumentPropertyType::Array (or pull the schema
max if stored elsewhere) and before Vec::with_capacity validate that array_len
<= max_items (and optionally <= a global MAX_ARRAY_ITEMS constant), returning an
appropriate DataContractError::CorruptedSerialization (or a new error) if
exceeded; keep using array_item_type.read_from in the element loop unchanged but
only after the length check to avoid large allocations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-dpp/src/data_contract/document_type/property/mod.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2761
File: packages/rs-drive/src/query/filter.rs:268-272
Timestamp: 2025-09-05T06:00:56.491Z
Learning: For document subscription filters in rs-drive, validation should not require index availability checks like queries do. Subscription filtering has different validation requirements than query optimization.
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/mod.rs
📚 Learning: 2025-04-11T09:08:05.652Z
Learnt from: pauldelucia
Repo: dashpay/platform PR: 2523
File: packages/rs-drive/src/drive/contract/update/update_contract/v1/update_description/v1/mod.rs:147-151
Timestamp: 2025-04-11T09:08:05.652Z
Learning: Description length validation for data contracts is already handled in the data contract validation process, specifically in packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs.
Applied to files:
packages/rs-dpp/src/data_contract/document_type/property/mod.rs
🔇 Additional comments (3)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (3)
14-14: LGTM!The public re-export of
ArrayItemTypeis appropriate and follows Rust module conventions for exposing types that are used by other modules for array element encoding/decoding.
1241-1250: LGTM!The change correctly implements the key semantic for array indexing: encoding a single element value (not the entire array) for tree key construction. This enables Contains queries to use the searched value as the key to look up in the index. The delegation to
item_type.encode_element_for_tree_keysis a clean abstraction.
2343-2418: Well-structured array parsing implementation.The enhanced parsing logic correctly handles:
- Typed arrays with
itemsproperty (string, integer, number, boolean, date)- Nested byte arrays with proper validation
- Fallback to byte array semantics when
itemsis absentThe validation is appropriately strict - nested arrays must be byte arrays, and unrecognized item types produce clear error messages.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Issue being fixed or feature implemented
Adds support for indexing individual elements of array fields in document types. This enables efficient queries like "find all posts where hashtags contains 'dash'" by creating separate index entries for each array element, rather than treating the entire array as a single indexed value.
What was done?
Schema & Validation (rs-dpp):
itemsdefinition intry_from_value_mapITEMSconstant to property_names moduleEncoding (rs-dpp):
ArrayItemType.encode_element_for_tree_keys()for encoding individual array elements as index keysDocument.get_raw_array_elements_for_document_type()to extract and encode all array elements with deduplicationSerialization (rs-dpp):
ArrayItemType.read_from()for deserializing array elements from storageDocumentPropertyType::read_optionally_fromIndex Operations (rs-drive):
add_indices_for_top_index_level_for_contract_operationsto detect array properties and create index entries for each elementadd_indices_for_index_level_for_contract_operationswith same array handling pattern for nested index levelsremove_indices_for_top_index_level_for_contract_operationswith same array handling patternremove_indices_for_index_level_for_contract_operationswith same array handling pattern for nested index levelsQuery Support (rs-drive):
Containsquery operator (aliased asinfor SQL compatibility)How Has This Been Tested?
query_tests.rs:array-index-with-types-contract.jsonfor testing different array element typesBreaking Changes
None - this is additive functionality. Existing contracts and documents are unaffected.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Query
Behavior & Validation
Drive
Tests
✏️ Tip: You can customize this high-level summary in your review settings.