feat!: more granular integer document property types #2455
Conversation
WalkthroughThis pull request refines type annotations and error handling in several JavaScript and Rust modules. In the JavaScript client libraries, type definitions and error processing in state transition methods have been updated. In the Rust code, multiple functions related to document type creation and schema validation have been modified to replace several boolean parameters with a single, unified configuration object ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocCreator as "Document Types Creator"
participant Config as DataContractConfig
Client->>DocCreator: call create_document_types_from_document_schemas(config)
DocCreator->>Config: Retrieve default document properties
DocCreator->>DocCreator: Call try_from_schema(config) internally
DocCreator-->>Client: Return created Document Types map
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
shumkov
changed the title
There was a problem hiding this comment.
Add 1 to known_versions on line 60
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/rs-drive/tests/query_tests.rs (1)
883-895:⚠️ Potential issueContract configuration needs error handling
The contract configuration modification lacks error handling for the
config_mut()andset_granular_integer_types_enabled()calls.Add error handling:
- contract - .config_mut() - .set_granular_integer_types_enabled(false); + contract + .config_mut() + .set_granular_integer_types_enabled(false) + .map_err(|e| Error::ContractError(format!("Failed to set granular integer types: {}", e)))?;
🧹 Nitpick comments (11)
packages/rs-dpp/src/data_contract/config/v1/mod.rs (1)
16-45: Suggestion: Unify naming across code and doc references.The code uses the field name
granular_integer_typeswhile the constant isDEFAULT_GRANULAR_NUMERIC_TYPES. Consider renaming one for consistency (e.g., rename the constant toDEFAULT_GRANULAR_INTEGER_TYPESor rename the field togranular_numeric_types).packages/rs-dpp/src/data_contract/config/mod.rs (2)
65-66: Remove or address leftover TODO.The comment
// TODO: Remove, it's not usingindicates potential dead code or a pending task. If the method is truly unused, consider removing it to keep the module clean.
228-245: Clarify no-op behavior for V0.Retrieving or enabling
granular_integer_typeson a V0 config has no effect. While this maintains backward compatibility, adding a comment or guard clause helps future maintainers understand why the calls are effectively disabled for V0.packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (2)
617-618: Avoid extra conversion if feasible.Calling
DocumentPropertyType::try_from_value_map(&inner_properties, &config.into())may be unnecessary if the method can directly accept&DataContractConfig. Consider updatingtry_from_value_mapto reduce complexity.
644-644: Empty conditional block could be clarified or removed.Inside
if !config.granular_integer_types() && property_type.is_integer() {}, there's no implemented logic. If this is intentional, add a comment; otherwise, consider removing it to reduce confusion.packages/rs-dpp/src/data_contract/document_type/property/mod.rs (3)
19-19: Unused import:BTreeValueMapPathHelper.The static analysis tool warns that
BTreeValueMapPathHelperis never used. Consider removing it to keep the imports clean:use platform_value::btreemap_extensions::{ BTreeValueMapHelper, - BTreeValueMapPathHelper };🧰 Tools
🪛 GitHub Check: Rust packages (dpp) / Linting
[warning] 19-19: unused import:
BTreeValueMapPathHelper
warning: unused import:BTreeValueMapPathHelper
--> packages/rs-dpp/src/data_contract/document_type/property/mod.rs:19:64
|
19 | use platform_value::btreemap_extensions::{BTreeValueMapHelper, BTreeValueMapPathHelper};
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by default
2088-2092: Add a brief doc comment for improved clarity.Consider documenting the purpose of
DocumentPropertyTypeParsingOptionsto help future developers quickly understand its usage and defaults.
2181-2192: Collapse nestedelse { if ... }for cleaner control flow.As suggested by the linter, you can refactor this section for clarity:
- } else { - if min >= i8::MIN as i64 && max <= i8::MAX as i64 { - DocumentPropertyType::I8 - } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 { - DocumentPropertyType::I16 - } else if min >= i32::MIN as i64 && max <= i32::MAX as i64 { - DocumentPropertyType::I32 - } else { - DocumentPropertyType::I64 - } - } + } else if min >= i8::MIN as i64 && max <= i8::MAX as i64 { + DocumentPropertyType::I8 + } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 { + DocumentPropertyType::I16 + } else if min >= i32::MIN as i64 && max <= i32::MAX as i64 { + DocumentPropertyType::I32 + } else { + DocumentPropertyType::I64 + }🧰 Tools
🪛 GitHub Check: Rust packages (dpp) / Linting
[warning] 2181-2191: this
else { if .. }block can be collapsed
warning: thiselse { if .. }block can be collapsed
--> packages/rs-dpp/src/data_contract/document_type/property/mod.rs:2181:12
|
2181 | } else {
| _______^
2182 | | if min >= i8::MIN as i64 && max <= i8::MAX as i64 {
2183 | | DocumentPropertyType::I8
2184 | | } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 {
... |
2190 | | }
2191 | | }
| |^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
= note:#[warn(clippy::collapsible_else_if)]on by default
help: collapse nested if block
|
2181 ~ } else if min >= i8::MIN as i64 && max <= i8::MAX as i64 {
2182 + DocumentPropertyType::I8
2183 + } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 {
2184 + DocumentPropertyType::I16
2185 + } else if min >= i32::MIN as i64 && max <= i32::MAX as i64 {
2186 + DocumentPropertyType::I32
2187 + } else {
2188 + DocumentPropertyType::I64
2189 + }
|packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/mod.rs (1)
16-32: Update documentation to reflect new configuration parameter.The documentation still references the old boolean parameters (
documents_keep_history_contract_default,documents_mutable_contract_default) but should describe the newdata_contact_configparameter instead./// # Parameters /// /// * `data_contract_id`: Identifier for the data contract. /// * `contract_document_types_raw`: Vector representing the raw contract document types. /// * `definition_references`: BTreeMap representing the definition references. -/// * `documents_keep_history_contract_default`: A boolean flag that specifies the document's keep history contract default. -/// * `documents_mutable_contract_default`: A boolean flag that specifies the document's mutable contract default. +/// * `data_contact_config`: Configuration object containing document contract defaults and settings. /// * `platform_version`: The platform version being used.packages/rs-drive/tests/deterministic_root_hash.rs (1)
303-307: Version-specific hash updates reflect breaking changes in data serialization.The expected application hash values have been updated to account for different platform versions, indicating breaking changes in how data is serialized. Please ensure these changes are documented in the migration guide.
Consider adding a comment explaining why the hash values differ between versions to help future maintainers understand the changes.
packages/rs-drive/tests/query_tests.rs (1)
1-7038: Test coverage could be improvedWhile the test file has good coverage for various query scenarios, it would benefit from additional test cases:
- Edge cases for granular integer types
- Error cases for invalid configurations
- Boundary conditions for integer values
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
packages/js-dapi-client/lib/methods/platform/waitForStateTransitionResult/ErrorResult.js(2 hunks)packages/js-dapi-client/lib/methods/platform/waitForStateTransitionResult/WaitForStateTransitionResultResponse.js(1 hunks)packages/js-dash-sdk/src/SDK/Client/Platform/IStateTransitionResult.ts(1 hunks)packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.ts(1 hunks)packages/rs-dpp/src/data_contract/config/fields.rs(2 hunks)packages/rs-dpp/src/data_contract/config/mod.rs(9 hunks)packages/rs-dpp/src/data_contract/config/v1/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/mod.rs(4 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v0/mod.rs(4 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs(15 hunks)packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs(22 hunks)packages/rs-dpp/src/data_contract/document_type/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/property/mod.rs(4 hunks)packages/rs-dpp/src/data_contract/v0/methods/schema.rs(2 hunks)packages/rs-dpp/src/data_contract/v0/serialization/mod.rs(2 hunks)packages/rs-dpp/src/data_contract/v1/methods/schema.rs(2 hunks)packages/rs-dpp/src/data_contract/v1/serialization/mod.rs(2 hunks)packages/rs-dpp/src/util/json_schema.rs(2 hunks)packages/rs-drive-abci/src/query/document_query/v0/mod.rs(5 hunks)packages/rs-drive/src/query/test_index.rs(2 hunks)packages/rs-drive/tests/deterministic_root_hash.rs(1 hunks)packages/rs-drive/tests/query_tests.rs(4 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Rust packages (dpp) / Linting
packages/rs-dpp/src/data_contract/document_type/property/mod.rs
[warning] 19-19: unused import: BTreeValueMapPathHelper
warning: unused import: BTreeValueMapPathHelper
--> packages/rs-dpp/src/data_contract/document_type/property/mod.rs:19:64
|
19 | use platform_value::btreemap_extensions::{BTreeValueMapHelper, BTreeValueMapPathHelper};
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= note: #[warn(unused_imports)] on by default
[warning] 2181-2191: this else { if .. } block can be collapsed
warning: this else { if .. } block can be collapsed
--> packages/rs-dpp/src/data_contract/document_type/property/mod.rs:2181:12
|
2181 | } else {
| _______^
2182 | | if min >= i8::MIN as i64 && max <= i8::MAX as i64 {
2183 | | DocumentPropertyType::I8
2184 | | } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 {
... |
2190 | | }
2191 | | }
| |^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
= note: #[warn(clippy::collapsible_else_if)] on by default
help: collapse nested if block
|
2181 ~ } else if min >= i8::MIN as i64 && max <= i8::MAX as i64 {
2182 + DocumentPropertyType::I8
2183 + } else if min >= i16::MIN as i64 && max <= i16::MAX as i64 {
2184 + DocumentPropertyType::I16
2185 + } else if min >= i32::MIN as i64 && max <= i32::MAX as i64 {
2186 + DocumentPropertyType::I32
2187 + } else {
2188 + DocumentPropertyType::I64
2189 + }
|
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (wasm-dpp) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
🔇 Additional comments (36)
packages/rs-dpp/src/data_contract/config/v1/mod.rs (2)
59-73: Confirm impact of changing defaults in new version.The default implementation sets
granular_integer_typestotrue, which may alter behavior compared to the earlier version. Verify whether enabling this by default is intended, and ensure it won't introduce breaking changes for existing client code.
141-155: Initialization logic looks good.Setting
granular_integer_types: granular_numeric_typesensures the new contract property fully controls granular integer parsing. Just confirm that downstream usage relies on this flag consistently.packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (1)
169-181: Unified config usage is a welcome improvement.Replacing multiple boolean parameters with calls to
data_contact_configmakes the code more maintainable and self-documenting, particularly when defaults are versioned.packages/rs-dpp/src/data_contract/document_type/property/mod.rs (8)
1-1: No specific issues identified.
9-11: Imports appear valid.
18-18: Imports appear valid.
2016-2029: Consider i128 and u128 in integer matching.This method treats only 8/16/32/64-bit signed/unsigned as integers, excluding i128/u128. If you intend 128-bit types to be recognized as integers, consider adding them:
pub fn is_integer(&self) -> bool { matches!( self, DocumentPropertyType::I8 | DocumentPropertyType::I16 | DocumentPropertyType::I32 | DocumentPropertyType::I64 | DocumentPropertyType::U8 | DocumentPropertyType::U16 | DocumentPropertyType::U32 | DocumentPropertyType::U64 | DocumentPropertyType::I128 | DocumentPropertyType::U128 ) }
2031-2087: Logic looks good.The new
try_from_value_mapmethod introduces robust parsing conditions and aligns with the updated integer parsing approach.
2094-2100: Default implementation is clear.
2102-2107: Conversion fromDataContractConfigis straightforward.
2116-2180: Overall logic for integer type resolution is coherent.packages/js-dash-sdk/src/SDK/Client/Platform/IStateTransitionResult.ts (1)
10-10: Improved type safety for error data.Changing
datafromanyto aBuffer(optional) clarifies how error details are structured.packages/js-dapi-client/lib/methods/platform/waitForStateTransitionResult/ErrorResult.js (2)
5-5: Refined parameter type annotation.Specifying
Buffer|undefinedimproves clarity and ensures better type safety for thedataparameter.
28-28: Updated return type annotation.Changing
getData()return type from*toBuffer|undefinedaligns with the refined constructor parameter type.packages/rs-dpp/src/data_contract/config/fields.rs (2)
7-7: LGTM! Default configuration for granular numeric types.The default value of
trueindicates this feature will be enabled by default, which aligns with the PR objective to enhance integer type handling.
21-21: LGTM! Property name for granular numeric types configuration.The constant follows Rust naming conventions and is appropriately placed in the property module.
packages/js-dapi-client/lib/methods/platform/waitForStateTransitionResult/WaitForStateTransitionResultResponse.js (1)
39-42: LGTM! Simplified error data handling.The change improves error handling by directly converting error data to a Buffer without CBOR decoding, making the code simpler and more maintainable.
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs (2)
18-18: LGTM! Improved configuration management.Consolidating multiple boolean parameters into a single
data_contact_configparameter improves code maintainability and readability.
43-43: LGTM! Version 1 added to known versions.Version 1 has been added to the known versions list as requested in the previous review.
packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs (1)
13-17: LGTM! Version updates for granular numeric types support.The version updates are well-documented and align with the PR objectives:
- Serialization version defaulting to 1
- Structure version incremented to 1
- Config version updated to 1 for granular number types
- Document types method version updated to 1
Also applies to: 30-30
packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs (2)
2-2: LGTM! Configuration parameter consolidation improves maintainability.The replacement of multiple boolean parameters with a single
DataContractConfigparameter is a good refactoring that enhances code maintainability and reduces parameter complexity.Also applies to: 18-18, 44-44
26-30:❓ Verification inconclusive
Verify token-based document schema validation.
The code allows empty document schemas when tokens are present (
has_tokensis true). This is a significant change in behavior that warrants verification.
🏁 Script executed:
#!/bin/bash # Search for test cases that verify empty document schemas with tokens rg -l "has_tokens.*true.*empty.*schema" --type rustLength of output: 53
Attention: Manual Verification Required for Token-Based Validation
The updated behavior allowing empty document schemas when tokens are present is not clearly covered by any detected test cases. In the file
packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs(lines 26–30), the error is only triggered if both the document schemas are empty and there are no tokens. However, our search for tests (using a regex for patterns involvinghas_tokens,true, andemptyschemas) returned no relevant results. This absence of evidence suggests that token-based document schema validation isn’t automatically verified in the existing test suite.
- Review Location:
packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v1/mod.rs
Lines 26–30- Action Required:
- Manually verify that the intended behavior when
has_tokensis true (i.e., permitting empty document schemas) is correct and intentional.- If the behavior is desired, please add or update test cases to explicitly cover this scenario, ensuring that edge cases tied to token-based validations are handled properly.
packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/mod.rs (1)
42-42: LGTM! Consistent parameter updates across version handlers.The changes consistently replace boolean parameters with the new configuration object across all version handlers (v0 and v1).
Also applies to: 59-59, 69-69
packages/js-dash-sdk/src/SDK/Client/Platform/broadcastStateTransition.ts (1)
92-96: LGTM! Improved error handling robustness.The addition of the null check for
error.dataand base64 encoding of the error data enhances the robustness of the error handling mechanism.packages/rs-dpp/src/data_contract/document_type/class_methods/create_document_types_from_document_schemas/v0/mod.rs (1)
80-81: LGTM! Test case properly updated with new configuration.The test case has been correctly updated to use
DataContractConfig::default_for_version()with the latest platform version.Also applies to: 87-87
packages/rs-drive/src/query/test_index.rs (2)
7-7: LGTM!The import of
DataContractConfigis correctly added to support the configuration object usage.
80-81: LGTM!The changes correctly implement the new configuration-based approach:
- Creating a default config for the platform version
- Passing the config to
DocumentType::try_from_schemaAlso applies to: 88-88
packages/rs-dpp/src/data_contract/v1/methods/schema.rs (1)
25-25: LGTM!The changes effectively consolidate configuration parameters by using
&self.configinstead of individual boolean flags, improving code maintainability.Also applies to: 48-48
packages/rs-dpp/src/data_contract/v0/methods/schema.rs (1)
26-26: LGTM!The changes maintain consistency with v1 implementation by using
&self.config, ensuring a unified approach across versions.Also applies to: 49-49
packages/rs-dpp/src/data_contract/v0/serialization/mod.rs (1)
94-94: LGTM!The changes consistently apply the configuration-based approach in serialization methods, aligning with the broader refactoring effort.
Also applies to: 134-134
packages/rs-dpp/src/data_contract/v1/serialization/mod.rs (2)
89-98: LGTM! Configuration parameter consolidation improves code maintainability.The change to use
&configinstead of individual boolean parameters makes the code more maintainable and consistent with the rest of the codebase.
132-141: LGTM! Configuration parameter consolidation improves code maintainability.Similar to the previous change, using
&confighere improves code maintainability and consistency.packages/rs-dpp/src/util/json_schema.rs (1)
148-148: LGTM! Test configuration updated to use the new config object.The test configuration has been updated to use
DataContractConfiginstead of individual boolean parameters, maintaining consistency with the codebase changes.Also applies to: 210-212
packages/rs-dpp/src/data_contract/document_type/mod.rs (1)
57-57: LGTM! Added ENUM constant for enhanced integer type validation.The addition of the
ENUMconstant supports the new functionality for defining integer types with validation rules, consistent with the PR objectives.packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v0/mod.rs (1)
250-251: LGTM! The changes to useDataContractConfigare consistent and well-structured.The refactoring to use
DataContractConfiginstead of direct boolean parameters improves configuration management while preserving the test coverage and assertions.Also applies to: 277-279, 349-351, 376-378, 421-423, 448-450, 493-495, 520-522, 565-567, 592-594, 637-639, 664-666, 709-711, 736-738, 778-780, 847-849, 921-923, 936-938, 977-979
packages/rs-drive/tests/query_tests.rs (1)
62-62: LGTM! Import addition for DataContractConfigSettersV1The addition of
DataContractConfigSettersV1import aligns with the PR objectives to support granular integer document property types.
| let requires_identity_encryption_bounded_key = contract | ||
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)? | ||
| .map(|int| int.try_into()) | ||
| .transpose()?; | ||
|
|
||
| let requires_identity_decryption_bounded_key = contract | ||
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)? | ||
| .map(|int| int.try_into()) | ||
| .transpose()?; | ||
|
|
There was a problem hiding this comment.
Critical copy-paste error in property retrieval.
The block for requires_identity_decryption_bounded_key references REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY instead of REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY. As a result, both fields are pulled from the same property.
Apply this fix:
- .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)?
+ .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY)?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let requires_identity_encryption_bounded_key = contract | |
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)? | |
| .map(|int| int.try_into()) | |
| .transpose()?; | |
| let requires_identity_decryption_bounded_key = contract | |
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)? | |
| .map(|int| int.try_into()) | |
| .transpose()?; | |
| let requires_identity_encryption_bounded_key = contract | |
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)? | |
| .map(|int| int.try_into()) | |
| .transpose()?; | |
| let requires_identity_decryption_bounded_key = contract | |
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY)? | |
| .map(|int| int.try_into()) | |
| .transpose()?; |
There was a problem hiding this comment.
This also needs to be fixed in V0.
There was a problem hiding this comment.
🧩 Analysis chain
@QuantumExplorer You're absolutely right! I found the same issue in V0. Here's the fix needed for both versions:
V0 (packages/rs-dpp/src/data_contract/config/v0/mod.rs):
let requires_identity_decryption_bounded_key = contract
- .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)?
+ .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY)?
.map(|int| int.try_into())
.transpose()?;V1 (packages/rs-dpp/src/data_contract/config/v1/mod.rs):
let requires_identity_decryption_bounded_key = contract
- .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)?
+ .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY)?
.map(|int| int.try_into())
.transpose()?;This will ensure both versions correctly read the decryption key from the right property.
🐰 thoughtfully reviewing the code
🏁 Script executed:
#!/bin/bash
# Search for V0 implementation with similar pattern
ast-grep --pattern 'let requires_identity_decryption_bounded_key = contract
.get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)?'
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Check V0 implementation file
cat packages/rs-dpp/src/data_contract/config/v0/mod.rs
Length of output: 8548
@QuantumExplorer Yes, I can now confirm the exact same issue in V0! Here's the fix needed for both versions:
V0 (packages/rs-dpp/src/data_contract/config/v0/mod.rs line ~196):
let requires_identity_decryption_bounded_key = contract
- .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)?
+ .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY)?
.map(|int| int.try_into())
.transpose()?;V1 (packages/rs-dpp/src/data_contract/config/v1/mod.rs line ~131):
let requires_identity_decryption_bounded_key = contract
- .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)?
+ .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY)?
.map(|int| int.try_into())
.transpose()?;Both versions are currently reading the encryption key property instead of the decryption key property for the requires_identity_decryption_bounded_key field. This fix ensures they read from the correct property.
🐰 thoughtfully reviewing both versions
| properties.insert("status".to_string(), Value::U8(0)); // Always queued | ||
| properties.insert("pooling".to_string(), Value::U8(0)); // Always 0 | ||
| properties.insert("coreFeePerByte".to_string(), Value::U32(1)); // Always 1 | ||
| properties.insert("amount".to_string(), Value::U64(1000)); // Set a minimum amount of 1000 |
There was a problem hiding this comment.
Fix the mismatch between test data and query conditions.
The test data uses unsigned types:
properties.insert("status".to_string(), Value::U8(0));
properties.insert("pooling".to_string(), Value::U8(0));
properties.insert("coreFeePerByte".to_string(), Value::U32(1));
properties.insert("amount".to_string(), Value::U64(1000));But the query conditions at lines 825, 833, and 840 still use Value::I64. This mismatch could lead to incorrect query results.
Apply this diff to fix the query conditions:
- value: Value::I64(0),
+ value: Value::U8(0),
- value: Value::I64(0),
+ value: Value::U8(0),
- value: Value::I64(1),
+ value: Value::U32(1),| properties.insert("status".to_string(), Value::U8(i as u8 % 4)); // Always queued | ||
| properties.insert("pooling".to_string(), Value::U8(0)); // Always 0 | ||
| properties.insert("coreFeePerByte".to_string(), Value::U32(1)); // Always 1 | ||
| properties.insert("amount".to_string(), Value::U64(1000)); // Set a minimum amount of 1000 |
There was a problem hiding this comment.
Fix the mismatch between test data and query conditions in multiple tests.
The test data uses unsigned types in multiple test cases:
properties.insert("status".to_string(), Value::U8(i as u8 % 4));
properties.insert("pooling".to_string(), Value::U8(0));
properties.insert("coreFeePerByte".to_string(), Value::U32(1));
properties.insert("amount".to_string(), Value::U64(1000));But the query conditions at lines 989, 997, 1004, 1476, 1490, and 1498 still use Value::I64. This mismatch could lead to incorrect query results.
Apply this diff to fix the query conditions:
- value: Value::I64(0),
+ value: Value::U8(0),
- value: Value::I64(0),
+ value: Value::U8(0),
- value: Value::I64(1),
+ value: Value::U32(1),
- Value::I64(0),
+ Value::U8(0),
- Value::I64(1),
+ Value::U8(1),
- Value::I64(2),
+ Value::U8(2),
- Value::I64(3),
+ Value::U8(3),
- Value::I64(4),
+ Value::U8(4),Also applies to: 1260-1263, 1431-1434
| documents_keep_history_contract_default: bool, | ||
| documents_mutable_contract_default: bool, | ||
| documents_can_be_deleted_contract_default: bool, |
| let requires_identity_encryption_bounded_key = contract | ||
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)? | ||
| .map(|int| int.try_into()) | ||
| .transpose()?; | ||
|
|
||
| let requires_identity_decryption_bounded_key = contract | ||
| .get_optional_integer::<u8>(config::property::REQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEY)? | ||
| .map(|int| int.try_into()) | ||
| .transpose()?; | ||
|
|
There was a problem hiding this comment.
This also needs to be fixed in V0.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/config/mod.rs (1)
65-65: Remove unused method.The TODO comment indicates that
get_contract_configuration_propertiesis not used. Consider removing this method to maintain codebase cleanliness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-dpp/src/data_contract/config/mod.rs(9 hunks)packages/rs-dpp/src/data_contract/config/v1/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (5)
packages/rs-dpp/src/data_contract/config/v1/mod.rs (3)
16-45: LGTM! Well-structured configuration type with clear documentation.The struct is well-documented with comprehensive field descriptions and appropriate derive macros for serialization, deserialization, and other common traits.
85-157: LGTM! Robust configuration property retrieval.The implementation correctly handles all configuration properties with appropriate default values and error handling.
59-73:❓ Verification inconclusive
Verify the impact of enabling granular integer types by default.
Setting
granular_integer_typestotrueby default is a breaking change that will affect how integer properties are handled in new contracts. Please ensure this aligns with the migration strategy.
🏁 Script executed:
#!/bin/bash # Search for existing integer property type usage rg -A 5 'type.*integer' --type rustLength of output: 22999
Confirm Migration Strategy: Impact of Enabling Granular Integer Types
The default implementation now sets
granular_integer_typestotrue, which is a deliberate breaking change in how integer properties are managed. The search results confirm that this flag is referenced across multiple modules—affecting property type handling in documents, schemas, and state transitions. Please ensure that:
- The migration strategy explicitly addresses this breaking change.
- Existing contracts and related tests are updated to accommodate the new behavior.
- All references to integer property types are reviewed to prevent unintended side effects in legacy systems.
packages/rs-dpp/src/data_contract/config/mod.rs (2)
20-27: LGTM! Clean version enum extension.The V1 variant is correctly added with appropriate serialization tags.
228-235: LGTM! Safe version handling for granular integer types.The implementation correctly defaults to
falsefor V0, ensuring backward compatibility.
Issue being fixed or feature implemented
TokenAmountand other system types are defined as unsigned integers. Data contract schema, in turn, supports defining such types using theintegertype and validation rules (i.e.minimum = 0). Unfortunately, the document type parser still recognizes such fields as i64.What was done?
DocumentPropertyType::try_from_nameDocumentPropertyType::try_from_value_mapthat accepts a property subschema map and considers the validation rules to create more granular numeric types:minimum,maximum, andenum.How Has This Been Tested?
Updated existing tests
Breaking Changes
The
integerproperties in data contract v1 (newly created) will be represented as different signed and unsigned integer types. Already existing contracts will usei64as before.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit