feat: more granular integer document property types #2455
Conversation
WalkthroughThis pull request introduces significant changes to the data contract configuration, including the renaming of constants, the consolidation of boolean parameters into a single configuration object, and the introduction of new methods for handling timestamps and block heights. The changes streamline the handling of integer types and enhance the overall structure of the data contract management. Additionally, the modifications extend to various document type operations, validation logic, and test cases, ensuring consistency and clarity across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentType
participant DataContractConfig
Client->>DocumentType: try_from_schema(schema, config)
DocumentType->>DataContractConfig: Retrieve sized config options
DataContractConfig-->>DocumentType: Return configuration values
DocumentType-->>Client: Return constructed DocumentType
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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.
| documents_keep_history_contract_default: bool, | ||
| documents_mutable_contract_default: bool, | ||
| documents_can_be_deleted_contract_default: bool, |
There was a problem hiding this comment.
There are 4 fields of data contract config that are passing as independent params. I changed it to 1 param - config itself.
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.
| _ => { | ||
| field_type = DocumentPropertyType::try_from_name(type_value)?; | ||
| property_type => { | ||
| if !config.granular_integer_types() && property_type.is_integer() {} |
There was a problem hiding this comment.
Nope. It's an artifact. I remove it
shumkov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (1)
679-750: Consider extracting sorting logic into a separate function.The nested property sorting logic in
insert_values_nestedis quite complex. Consider extracting it into a separate function for better readability and maintainability.+fn sort_properties_by_position(properties: &IndexMap<String, Value>) -> Vec<(&String, &Value)> { + let mut sorted_properties: Vec<_> = properties.iter().collect(); + sorted_properties.sort_by(|(_, value_1), (_, value_2)| { + let pos_1: u64 = value_1 + .get_integer(property_names::POSITION) + .expect("expected a position"); + let pos_2: u64 = value_2 + .get_integer(property_names::POSITION) + .expect("expected a position"); + pos_1.cmp(&pos_2) + }); + sorted_properties +} fn insert_values_nested( document_properties: &mut IndexMap<String, DocumentProperty>, known_required: &BTreeSet<String>, known_transient: &BTreeSet<String>, property_key: String, property_value: &Value, root_schema: &Value, config: &DataContractConfig, ) -> Result<(), DataContractError> { // ... existing code ... let property_type = match DocumentPropertyType::try_from_value_map(&inner_properties, &config.into())? { DocumentPropertyType::Object(_) => { let mut nested_properties = IndexMap::new(); if let Some(properties_as_value) = inner_properties.get(property_names::PROPERTIES) { let properties = properties_as_value .as_map() .ok_or(DataContractError::ValueWrongType( "properties must be a map".to_string(), ))?; - let mut sorted_properties: Vec<_> = properties.iter().collect(); - sorted_properties.sort_by(|(_, value_1), (_, value_2)| { - let pos_1: u64 = value_1 - .get_integer(property_names::POSITION) - .expect("expected a position"); - let pos_2: u64 = value_2 - .get_integer(property_names::POSITION) - .expect("expected a position"); - pos_1.cmp(&pos_2) - }); + let sorted_properties = sort_properties_by_position(properties);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs(15 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/mod.rs(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/v1/mod.rs(1 hunks)packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v0_methods.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/v1/mod.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/v1/v0_methods.rs
- packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/methods/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Formatting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- 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 JS packages / Build JS
🔇 Additional comments (4)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (4)
33-35: LGTM! Clean imports for data contract config.The new imports are well-organized and properly scoped for both v0 and v1 config getters.
84-84: LGTM! Improved configuration management.The refactoring from boolean parameters to a unified
DataContractConfigobject improves code maintainability and reduces parameter coupling. The changes correctly use the config getters to retrieve default values.Also applies to: 169-169, 175-175, 181-181
595-595: LGTM! Enhanced property type handling.The
insert_valuesfunction has been updated to use the new config-based property type creation. The pattern matching is clean and handles both object and non-object types appropriately.Also applies to: 617-653
788-790: LGTM! Test cases properly updated.The test cases have been correctly updated to use the new
DataContractConfigobject. The error handling and assertions remain intact.Also applies to: 819-821, 861-863, 903-905, 929-931
| fn granular_integer_types(&self) -> bool { | ||
| match self { | ||
| DataContractConfig::V0(_) => false, | ||
| DataContractConfig::V1(v1) => v1.granular_integer_types, |
There was a problem hiding this comment.
sized_integer_types or fixed_width_integer_types might be better than granular_integer_types
| .transpose()?; | ||
|
|
||
| let granular_numeric_types = contract | ||
| .get_optional_bool(config::property::GRANULAR_NUMERIC_TYPES)? |
| use crate::prelude::TimestampMillis; | ||
| use crate::ProtocolError; | ||
| use array::ArrayItemType; | ||
| use byteorder::{BigEndian, ReadBytesExt, WriteBytesExt}; | ||
| use indexmap::IndexMap; | ||
| use integer_encoding::{VarInt, VarIntReader}; | ||
| use itertools::Itertools; | ||
| use platform_value::btreemap_extensions::{BTreeValueMapHelper, BTreeValueMapPathHelper}; |
| config.documents_keep_history_contract_default(), | ||
| config.documents_mutable_contract_default(), | ||
| config.documents_can_be_deleted_contract_default(), | ||
| &config, |
There was a problem hiding this comment.
this makes some imports unused
| self.config.documents_keep_history_contract_default(), | ||
| self.config.documents_mutable_contract_default(), | ||
| self.config.documents_can_be_deleted_contract_default(), | ||
| &self.config, |
| config.documents_keep_history_contract_default(), | ||
| config.documents_mutable_contract_default(), | ||
| config.documents_can_be_deleted_contract_default(), | ||
| &config, |
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 13
🔭 Outside diff range comments (4)
packages/rs-dpp/src/errors/consensus/codes.rs (1)
252-268:⚠️ Potential issueFix duplicate error code definition
There's a duplicate definition of
Self::TokenIsPausedError(_) => 40161in the error codes. The error appears on both line 257 and line 264, and the entire section of token-related errors (lines 261-268) appears to be duplicated from the earlier token errors section.Remove the duplicate error code definition and the duplicated section:
// Token errors: 40150-40199 Self::IdentityDoesNotHaveEnoughTokenBalanceError(_) => 40150, Self::NewAuthorizedActionTakerGroupDoesNotExistError(_) => 40158, Self::NewAuthorizedActionTakerMainGroupNotSetError(_) => 40159, Self::InvalidGroupPositionError(_) => 40160, Self::TokenIsPausedError(_) => 40161, // Identity Errors: 40200-40299 - Self::IdentityAlreadyExistsError(_) => 40200, - Self::NewAuthorizedActionTakerGroupDoesNotExistError(_) => 40158, - Self::NewAuthorizedActionTakerMainGroupNotSetError(_) => 40159, - Self::InvalidGroupPositionError(_) => 40160, - Self::TokenIsPausedError(_) => 40161, - Self::IdentityTokenAccountAlreadyFrozenError(_) => 40162, - Self::TokenAlreadyPausedError(_) => 40163, - Self::TokenNotPausedError(_) => 40164, - Self::InvalidTokenClaimPropertyMismatch(_) => 40165,The token-related errors should be consolidated in one place, with the remaining token errors properly sequenced:
// Token errors: 40150-40199 Self::IdentityDoesNotHaveEnoughTokenBalanceError(_) => 40150, Self::NewAuthorizedActionTakerGroupDoesNotExistError(_) => 40158, Self::NewAuthorizedActionTakerMainGroupNotSetError(_) => 40159, Self::InvalidGroupPositionError(_) => 40160, Self::TokenIsPausedError(_) => 40161, + Self::IdentityTokenAccountAlreadyFrozenError(_) => 40162, + Self::TokenAlreadyPausedError(_) => 40163, + Self::TokenNotPausedError(_) => 40164, + Self::InvalidTokenClaimPropertyMismatch(_) => 40165,packages/rs-dpp/src/errors/consensus/state/token/mod.rs (1)
22-33:⚠️ Potential issueDuplicate token error exports detected
There's a duplicate export for
token_is_paused_erroron line 29. Additionally, several other exports appear to be duplicated in this section.Remove the duplicate public use statements to avoid confusion:
pub use identity_does_not_have_enough_token_balance_error::*; pub use new_authorized_action_taker_identity_does_not_exist_error::*; pub use new_authorized_action_taker_main_group_not_set_error::*; pub use new_tokens_destination_identity_does_not_exist_error::*; pub use token_is_paused_error::*; pub use token_mint_past_max_supply_error::*; pub use token_setting_max_supply_to_less_than_current_supply_error::*; pub use unauthorized_token_action_error::*; -pub use new_authorized_action_taker_main_group_not_set_error::*; -pub use new_tokens_destination_identity_does_not_exist_error::*; pub use token_already_paused_error::*; -pub use token_is_paused_error::*; -pub use token_mint_past_max_supply_error::*; pub use token_not_paused_error::*; -pub use token_setting_max_supply_to_less_than_current_supply_error::*; -pub use unauthorized_token_action_error::*;packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs (2)
172-179:⚠️ Potential issueMalformed code segment
There's an inconsistency in this function's implementation - it appears the closing lines of the match statement are mixed with the beginning of another function. This might be a merge conflict or file parsing issue.
- match self { - DocumentTransition::Create(t) => t.data().get(path), - DocumentTransition::Replace(t) => t.data().get(path), - DocumentTransition::Delete(_) => None, - DocumentTransition::Transfer(_) => None, - DocumentTransition::UpdatePrice(_) => None, - DocumentTransition::Purchase(_) => None, - self.base().document_type_name() - } + match self { + DocumentTransition::Create(t) => t.data().get(path), + DocumentTransition::Replace(t) => t.data().get(path), + DocumentTransition::Delete(_) => None, + DocumentTransition::Transfer(_) => None, + DocumentTransition::UpdatePrice(_) => None, + DocumentTransition::Purchase(_) => None, + }
195-198:⚠️ Potential issueDuplicate code segment
This appears to be a duplicate code segment from another method implementation. This should be removed as it creates compilation errors.
- DocumentTransition::UpdatePrice(_) => None, - DocumentTransition::Purchase(_) => None, - } - }
🧹 Nitpick comments (51)
packages/platform-test-suite/lib/test/fixtures/getDataContractFixture.js (1)
154-154: Consider adding input validationWhile
BigInt()can accept strings, numbers, and bigints, it will throw an error if given an invalid input (like non-numeric strings or objects). Consider adding validation before the conversion to prevent potential runtime errors.- return factory.create(ownerId, BigInt(identityNonce), documents, config); + // Ensure identityNonce can be safely converted to BigInt + const nonce = typeof identityNonce === 'bigint' ? identityNonce : BigInt(identityNonce); + return factory.create(ownerId, nonce, documents, config);packages/js-dapi-client/lib/BlockHeadersProvider/BlockHeadersProvider.js (2)
8-11: Clarify the property description forfromBlockHeightThe JSDoc description for
fromBlockHeightcurrently states "a target batch size per stream", which doesn't match what the property name suggests. The property name indicates it's related to a block height, not a batch size.- * @property {number} [fromBlockHeight=1] a target batch size per stream + * @property {number} [fromBlockHeight=1] starting block height for synchronization
138-140: Simplify parameter documentation for readHistoricalThe parameter documentation for both parameters contains redundant wording.
- * @param {number} fromBlockHeight height block height - * @param {number} toBlockHeight height block height + * @param {number} fromBlockHeight starting block height + * @param {number} toBlockHeight ending block heightpackages/wasm-dpp/src/voting/state_transition/masternode_vote_transition/mod.rs (2)
233-236: Method name inconsistency between JS binding and Rust functionThere's a slight inconsistency between the JS binding name
getIdentityContractNonceand the Rust function nameget_identity_nonce. Consider making these consistent to improve maintainability.-#[wasm_bindgen(js_name=getIdentityContractNonce)] -pub fn get_identity_nonce(&self) -> u64 { +#[wasm_bindgen(js_name=getIdentityNonce)] +pub fn get_identity_nonce(&self) -> u64 {or
-#[wasm_bindgen(js_name=getIdentityContractNonce)] -pub fn get_identity_nonce(&self) -> u64 { +#[wasm_bindgen(js_name=getIdentityContractNonce)] +pub fn get_identity_contract_nonce(&self) -> u64 {
289-291: Consider adding error handling for bincode encodingThe current implementation uses
expect()which will panic if encoding fails. While this might be fine in most scenarios, consider propagating the error up the call chain for more robust error handling.-bincode::encode_to_vec(value, config) - .expect("expected to encode value in path"), +bincode::encode_to_vec(value, config) + .map_err(|e| JsError::new(&format!("Failed to encode value: {}", e)).into())?,Note that this would require changing the method's return type to include
Result.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_transfer_transition_action/state_v0/mod.rs (1)
101-119: Properly handling paused tokensThe new check ensures tokens cannot be transferred while paused, returning a
TokenIsPausedErrorfor clarity. This aligns with typical token lifecycle restrictions. Consider verifying whether a missing token status should also raise an error, but otherwise this is an excellent addition.packages/rs-dpp/src/block/pool_credits.rs (2)
1-9: Consider adding validation for total_credits consistencyThe
StorageAndProcessingPoolCreditsstruct provides a clean representation of pool credits, but there's no guarantee thattotal_creditswill always equal the sum ofstorage_pool_creditsandprocessing_pool_creditssince all fields are public and can be modified independently.Consider either:
- Making the fields private and providing a constructor that ensures consistency:
- pub struct StorageAndProcessingPoolCredits { - pub storage_pool_credits: Credits, - pub processing_pool_credits: Credits, - pub total_credits: Credits, - } + pub struct StorageAndProcessingPoolCredits { + storage_pool_credits: Credits, + processing_pool_credits: Credits, + total_credits: Credits, + } + + impl StorageAndProcessingPoolCredits { + pub fn new(storage_pool_credits: Credits, processing_pool_credits: Credits) -> Self { + let total_credits = storage_pool_credits + processing_pool_credits; + Self { + storage_pool_credits, + processing_pool_credits, + total_credits, + } + } + + pub fn storage_pool_credits(&self) -> Credits { + self.storage_pool_credits + } + + pub fn processing_pool_credits(&self) -> Credits { + self.processing_pool_credits + } + + pub fn total_credits(&self) -> Credits { + self.total_credits + } + }
- Or, if you need public fields, consider making
total_creditsa computed property:- pub struct StorageAndProcessingPoolCredits { - pub storage_pool_credits: Credits, - pub processing_pool_credits: Credits, - pub total_credits: Credits, - } + pub struct StorageAndProcessingPoolCredits { + pub storage_pool_credits: Credits, + pub processing_pool_credits: Credits, + } + + impl StorageAndProcessingPoolCredits { + pub fn total_credits(&self) -> Credits { + self.storage_pool_credits + self.processing_pool_credits + } + }
11-19: Update Display implementation if struct is refactoredIf you implement either of the suggested refactorings for the struct, don't forget to update the
Displayimplementation accordingly.For example, if using the computed property approach:
impl fmt::Display for StorageAndProcessingPoolCredits { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, "Storage: {}, Processing: {}, Total: {}", - self.storage_pool_credits, self.processing_pool_credits, self.total_credits + self.storage_pool_credits, self.processing_pool_credits, self.total_credits() ) } }packages/js-dapi-client/lib/test/fixtures/getStatusFixture.js (1)
1-63: Well-structured status fixture with comprehensive data.This new fixture provides a complete representation of system status information with appropriate data types. The use of
Uint8Arrayfor binary data like hashes and IDs is correct. The fixture covers all necessary aspects of the status (version, node, chain, network, stateSync, time) which will be valuable for testing.One minor suggestion: consider adding JSDoc comments to document the purpose of the fixture function and possibly the structure of the returned object to help other developers understand its usage.
-/** - * - */ +/** + * Returns a fixture representing a complete system status + * including software versions, network state, blockchain info and timestamps. + * + * @returns {Object} A status object fixture with version, node, chain, network, stateSync and time information + */packages/js-dapi-client/lib/methods/platform/getStatus/StateSyncStatus.js (2)
60-65: JSDoc comment needs correctionThe JSDoc comment for
getSnapshotHeightincorrectly states it returns "Chunk process average time" instead of "Snapshot height"./** - * @returns {bigint} Chunk process average time + * @returns {bigint} Snapshot height */
67-72: JSDoc comment needs correctionThe JSDoc comment for
getSnapshotChunkCountincorrectly states it returns "Chunk process average time" instead of "Snapshot chunks count"./** - * @returns {bigint} Chunk process average time + * @returns {bigint} Snapshot chunks count */packages/rs-dpp/src/data_contract/associated_token/token_keeps_history_rules/v0/mod.rs (1)
7-10: Documentation looks good but could be more explicitThe documentation for the token history rules is clear, but could benefit from explaining how these rules interact with the actual token events in the system. It might be helpful to mention which document types are created when each type of history is enabled.
packages/js-dash-sdk/src/SDK/Client/Platform/NonceManager/NonceManager.spec.ts (2)
54-55: Consider using proper BigInt arithmeticThe TypeScript ignore comment is used to suppress type checking for BigInt addition. Consider refactoring to use proper BigInt arithmetic without needing to ignore TypeScript checks.
- // @ts-ignore - .to.equal(prevNonce + BigInt(1)); + .to.equal(prevNonce + BigInt(1));And update your TypeScript configuration or add proper type definitions to support BigInt arithmetic operations.
99-100: Consider using proper BigInt arithmeticSame issue as above with TypeScript ignore comment for BigInt addition. Consider refactoring to properly support BigInt arithmetic.
- // @ts-ignore - .to.equal(prevNonce + BigInt(1)); + .to.equal(prevNonce + BigInt(1));packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (2)
20-43: Potential cryptographic weakness in Random variant.This uses a custom PRF-like routine based on a simple SplitMix64 approach. If this random output is used for distribution or has security implications, you might want a cryptographically secure RNG. Otherwise, for purely pseudo-random needs, this is acceptable.
45-83: Check for potential floating-point inaccuracies in StepDecreasingAmount.Because the code calculates
factor = reduction.powf(steps as f64), large steps can cause floating-point drift. This can lead to unexpected rounding. If this function controls critical token amounts, consider a piecewise integer-based approach or a rational exponent method to reduce floating errors.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (4)
17-30: Ensure consistent naming for parameteramountvs.n.
InDistributionFunction::FixedAmount, the parameter is namedamountbut validated asn. While functionally correct, using inconsistent names can be confusing. Consider renaming one to match the other for clarity and consistency.
112-125: Avoid repeated max-bound checks for optional fields.
Fors(and similar optional parameters), the code re-implements the same bounding logic for multiple variants. Extracting these repeated validations into a small helper function would minimize code duplication and reduce the chance of future inconsistencies.
390-566: Consider refactoring repeated mandatorymax_valuechecks forExponentialdistributions.
In exponential distributions (m > 0), code requiresmax_valueto be set. This is repeated across the block. Factor them into a single check near the start of the match arm or a helper method for readability.
568-713: Improve clarity on permissible parameter ranges for Logarithmic distributions.
While you handle zero denominators, negative arguments, and large parameter values, it might help to unify these checks into a smaller set of grouped logic blocks or structured error messages. This helps maintain readability and simplifies future expansions.packages/js-dapi-client/lib/methods/platform/getIdentityNonce/GetIdentityNonceResponse.js (1)
19-19: Document potential conversion edge cases.
Returning abigintfromgetIdentityNonce()is correct for large integer support. However, code using this method must avoid accidentally using number-based arithmetic. Consider adding usage notes (or code examples) in the docstring.packages/js-dapi-client/lib/methods/platform/getDataContractHistory/GetDataContractHistoryResponse.js (1)
18-19: Ensure docs are updated outside the code.
You’ve updated the JSDoc to reflectDataContractHistoryEntry[]forgetDataContractHistory(). Confirm that any external documentation or integration references are also updated to reflect this new return type.packages/js-dapi-client/test/unit/methods/platform/getIdentityKeys/getIdentityKeysFactory.spec.js (1)
110-154: Redundant request assignment.
Lines 120–127 and 128–134 both set the sameGetIdentityKeysRequestV0. Consolidate these into a single assignment to avoid confusion:request.setV0( new GetIdentityKeysRequestV0() .setIdentityId(identityId) .setRequestType(new KeyRequestType().setAllKeys(new AllKeys())) .setLimit(new UInt32Value([limit])) .setProve(false), - ); - request.setV0( - new GetIdentityKeysRequestV0() - .setIdentityId(identityId) - .setRequestType(new KeyRequestType().setAllKeys(new AllKeys())) - .setLimit(new UInt32Value([limit])) - .setProve(false), );packages/wasm-dpp/src/identity/state_transition/identity_update_transition/identity_update_transition.rs (1)
173-181: Inconsistent method naming for identity nonce accessorsThe getter is named
get_identity_noncewhile the setter is namedset_identity_contract_nonce, creating an API inconsistency.Consider renaming one of the methods to ensure naming consistency:
-#[wasm_bindgen(js_name=getIdentityContractNonce)] -pub fn get_identity_nonce(&self) -> u64 { +#[wasm_bindgen(js_name=getIdentityContractNonce)] +pub fn get_identity_contract_nonce(&self) -> u64 { self.0.nonce() }Or alternatively:
-#[wasm_bindgen(js_name=setIdentityContractNonce)] -pub fn set_identity_contract_nonce(&mut self, identity_nonce: u64) -> () { +#[wasm_bindgen(js_name=setIdentityNonce)] +pub fn set_identity_nonce(&mut self, identity_nonce: u64) -> () { self.0.set_nonce(identity_nonce) }packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h (1)
6714-6761: Unused message classes retained in generated codeBoth
PersonalEncryptedNoteandSharedEncryptedNoteclasses are still defined in the code but are no longer referenced in theTokenEventenum after the removal ofTransferEvent. Since this is an auto-generated file, the persistence of these classes suggests they might still be referenced elsewhere in the protocol definition.Consider reviewing the original .proto file to ensure these classes are either still used elsewhere or can be safely removed in a future update to maintain a clean API surface.
packages/rs-dapi-client/src/transport/wasm_channel.rs (3)
25-30: Consider using_settingsor removing it.Currently,
_settingsis an unused parameter increate_channel. Either integrate it (e.g., for timeouts, retries) or remove to avoid confusion.
46-72: Assess concurrency strategy for heavy loads.While implementing
GrpcServicein a Wasm environment is valid, consider potential concurrency constraints if numerous requests are made simultaneously. If large numbers of tasks are spawned, you may need a robust concurrency strategy or an internal queue.
74-82: Expand error handling.A TODO comment notes missing or partial error handling in
wasm_client_error_to_status. Consider handling more specific error variants (e.g., network failures) for clearer feedback to callers.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_recipient.rs (1)
37-77: Improve error messaging for unsupported distribution.In
simple_resolve_with_distribution_type, the error message for attempted evonode distribution underPreProgrammedis acceptable. You might consider clarifying steps for the caller to handle or avoid this scenario.packages/wasm-dpp/src/document/state_transition/batch_transition/document_transition/mod.rs (3)
50-70: Refine return ofnullfor unsupported transitions.
get_datareturnsnullfor non-Create/Replace transitions. If future transitions need partial data, consider an extensible representation. For now, this is acceptable.
89-94: Clarify naming or usage forget_identity_contract_nonce.The method is straightforward but might warrant additional doc comments clarifying the meaning of “contract nonce” within the WASM context.
148-172: Optional refactoring for single-field object.
get_prefunded_voting_balancereturns a JS object with a single field. Consider directly returning a structured type or a single numeric value unless future enhancements require a broader object.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_moment/mod.rs (2)
11-19: Consider verifying cross-variant ordering requirements.Deriving
PartialOrdandOrdon an enum with multiple variants can produce implicit ordering rules. If you rely on ordering comparisons across different variants, consider verifying whether the default behavior aligns with your intended logic. Otherwise, this derived ordering might be unexpected.
191-199: Note varying byte lengths among variants.
to_be_bytes_vec()returns different-length byte vectors for block-, time-, and epoch-based moments. This is acceptable if you're aware of the variant, but be sure the calling code doesn't assume identical lengths for all moment types.packages/rs-dpp/src/block/finalized_epoch_info/v0/getters.rs (1)
36-36: Return slices instead of&Vec<(Identifier, u64)>.It's often more idiomatic to return
&[(Identifier, u64)](a slice) rather than a reference to aVec. This provides additional flexibility for future changes and avoids exposing the concrete vector type.- fn block_proposers(&self) -> &Vec<(Identifier, u64)>; + fn block_proposers(&self) -> &[(Identifier, u64)];packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (7)
12-15: Consider additional validation forFixedAmount.
AlthoughTokenAmountcan handle large values, consider confirming that the amount is non-negative to avoid unexpected underflows or negative distribution behavior.
37-40: Unify naming consistency betweenStepwiseandStepDecreasingAmount.
Currently, these variants both handle step-based distributions. Consider more consistent variant names to reduce confusion.
41-48: Guard optional boundaries inLinearvariant.
With optional fieldsmin_valueandmax_value, ensure constraints (e.g.,min_value <= max_valuewhen both exist) are kept in sync.
57-78: Validate polynomial exponent ranges.
mandninPolynomialcan lead to large exponents or negative powers. Consider bounding these fields or handling overflows gracefully.
79-100: Confirm exponential growth constraints.
Exponentialdistributions can explode quickly. Ensurea,m, andoare validated or capped, to prevent unexpected large expansions.
123-144: ReviewInvertedLogarithmicfor negative or zero arguments.
Inverted logarithms can be undefined for certain inputs. Ensure usage with safe numeric domains to avoid runtime errors.
303-304: Maintain consistency betweendecodeandborrow_decodelogic.
Both decode the same fields. If further changes or validations are introduced, ensure thatdecodeandborrow_decoderemain aligned to prevent mismatches.Also applies to: 307-310, 311-325, 328-329, 333-345, 348-367, 370-389, 392-411, 414-433
packages/rs-dpp/Cargo.toml (2)
58-59: Optionalrust_decimalandrust_decimal_macrosdependencies introduced.
By making these optional, code relying on decimal arithmetic might break if features are not enabled. Double-check that all relevant features are set in the pipeline.
217-221: Addedjson-schema-compatibility-validatordependency injson-schema-validation.
Reference to an external validator can help ensure schema correctness. Provide test coverage demonstrating how these validators integrate.packages/rs-dpp/src/data_contract/accessors/v1/mod.rs (1)
49-65: New timestamp, block height, and epoch getters.
These provide clearer metadata access. Consider whether there's a default or fallback mechanism if any optional fields are absent.packages/js-dash-sdk/src/SDK/Client/Platform/NonceManager/NonceManager.ts (1)
157-160: Concurrency caution for incrementing contract nonce.
If multiple calls to bump occur concurrently for the same identity & contract, a race condition may overwrite the nonce. Consider adding a locking mechanism or atomic operation to avoid inconsistent updates.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (4)
7-10: New modules for evaluation and validation.
Ensure thorough unit tests are present inevaluate.rs,evaluate_interval.rs, andvalidation.rsto align with the documented formulas.
76-106: StepDecreasingAmount includes step-based reductions.
The parameters are well-documented. Consider validating that(decrease_per_interval_denominator != 0)to prevent division by zero in evaluation.
108-120: Stepwise variant uses a mapping of intervals to amounts.
This approach is straightforward. Check that intervals inBTreeMapdo not overlap or conflict during evaluation.
242-325: Polynomial variant with rational exponents.
Well-structured doc for varied emission curves. Watch for potential overflow when(x - s + o)^(m/n)grows large. Consider safe exponentiation or robust checks.packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1)
58-91: Consider documenting this significant change in type representation.The change from
numbertostringfor numeric values is a breaking change that will require clients to update their code to handle these fields as strings. Consider adding documentation comments to highlight this change and provide guidance on how to properly handle these string-typed numeric values (e.g., using BigInt or libraries like bn.js for arithmetic operations).Also applies to: 537-563, 606-632, 675-701, 1447-1463, 2590-2610, 5132-5274, 5703-5735, 5784-5827, 5861-5904
🛑 Comments failed to post (13)
packages/js-dash-sdk/tsconfig.mocha.json (1)
7-8:
⚠️ Potential issueFix JSON syntax error
There's a trailing comma after the
outDirproperty which is causing a syntax error. JSON doesn't allow trailing commas.- "outDir": "./build", + "outDir": "./build"📝 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."outDir": "./build" },🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Expected a property but instead found '}'.
Expected a property here.
(parse)
packages/js-dapi-client/lib/methods/platform/getEpochsInfo/EpochInfo.js (1)
5-7: 💡 Verification agent
🧩 Analysis chain
Promote consistent usage of bigint in constructor.
Shifting
firstBlockHeightandstartTimetobigintis a potential breaking change. Please verify all call sites are updated to pass BigInts (instead of numbers) to prevent runtime errors.
🏁 Script executed:
#!/bin/bash # Description: Verify any `EpochInfo` constructor calls pass BigInt rg --multiline "new EpochInfo\(" -A 3 -B 1Length of output: 3291
Action Required: Update EpochInfo Call Sites for BigInt Consistency
The change to use BigInts for
firstBlockHeightandstartTimeis not uniformly applied across all call sites. Our grep results show:
- In packages/js-dapi-client/test/integration/methods/platform/PlatformMethodsFacade.spec.js, the builder method call
.setFirstBlockHeight(1)is still passing a number.- In packages/js-dapi-client/test/unit/methods/platform/getEpochsInfo/getEpochsInfoFactory.spec.js, the direct constructor call uses a number for the first argument (
new EpochInfo(1, BigInt(1), 1, BigInt(Date.now()), 1.1)), where the expected type forfirstBlockHeightis now a BigInt.Please update these call sites to ensure that BigInts are consistently passed for parameters intended as such (e.g., using
BigInt(1)or literal suffix1nas appropriate) to prevent any runtime errors from type mismatches.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (1)
1-3: 🛠️ Refactor suggestion
General note: floating-point arithmetic might reduce precision.
Your distribution function variants rely heavily on floating-point (
f64). If absolute mathematical precision is critical (especially for large token amounts over many intervals), consider integer arithmetic or rational approximations. Floating-point can introduce unexpected rounding or large magnitude errors.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/validation.rs (1)
260-388: 🛠️ Refactor suggestion
Validate exponent with caution for Polynomial distributions.
Polynomial distributions with largemandncould produce expensive computations or overflow. Currently, you handle denominator zero checks, but there's no direct safeguard for extremely large exponents. If user input can go arbitrarily high, consider adding explicit upper bounds for the exponent to prevent potential performance or overflow issues at runtime.packages/js-dapi-client/lib/methods/platform/getIdentityNonce/GetIdentityNonceResponse.js (1)
8-8: 🛠️ Refactor suggestion
Switch to
bigintmay affect downstream consumers.
Changing the@paramfromnumbertobigintis a breaking change for JavaScript code that previously passed numeric literals. Confirm that client code has been updated or can handlebigintseamlessly to avoid unexpected runtime errors.packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs (1)
6-70: 🛠️ Refactor suggestion
Consider using checked arithmetic for the step increment.
Whiletotalaccumulation already useschecked_addto prevent overflow, the incrementx += stepat line 67 could overflow ifx + stepexceedsu64::MAX. As recommended in the retrieved learnings, you may wish to replace it with a checked approach, for example:x = x.checked_add(step).ok_or_else(|| { ProtocolError::Overflow("Block interval overflow in evaluate_interval".into()) })?;packages/rs-dapi-client/src/transport/wasm_channel.rs (1)
100-119: 🛠️ Refactor suggestion
Avoid panicking in
into_send.In a Wasm environment, panicking can terminate execution abruptly. Consider returning an error or status instead of panicking if the receiver is dropped. This can enhance overall robustness.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (3)
21-36: 🛠️ Refactor suggestion
Check for zero or negative denominator in
StepDecreasingAmount.
decrease_per_interval_denominatorcan potentially be zero. This might cause division by zero in downstream calculations. Ensure there's a protective check or validation.
101-122: 🛠️ Refactor suggestion
Check domain of
Logarithmicvariants.
The variablesm,n, andofor log-based distributions risk invalid inputs (log of zero or negative). Confirm that minimal positivity constraints are enforced upstream.
156-164: 🛠️ Refactor suggestion
Harmonize decoding checks across all variants.
Each variant decodes its fields independently. For robust code, consider centralizing or reusing a validation method to ensure consistent numeric range checks.Also applies to: 165-179, 181-184, 186-199, 201-221, 223-243, 245-245, 246-265, 267-287
packages/rs-dpp/src/data_contract/accessors/v1/mod.rs (1)
81-98: 🛠️ Refactor suggestion
New setter methods for contract timestamps and block data.
Aligns well with the new getters. Ensure internal state updates are validated (e.g.,updated_atshould not precedecreated_at).packages/js-dash-sdk/src/SDK/Client/Platform/NonceManager/NonceManager.ts (1)
40-40: 💡 Verification agent
🧩 Analysis chain
Confirm that dapiClient returns a
bigint.
Currently, the code assumesidentityNonceis abigint. Verify that theplatform.getIdentityNonce()call actually returns abigintor convert the returned value accordingly.
🏁 Script executed:
#!/bin/bash # Searching for getIdentityNonce within the codebase to confirm return type rg "getIdentityNonce" -A 10Length of output: 97696
Action Required: Convert the Returned Nonce Value to BigInt
Our investigation shows that the dapi client’s
getIdentityNonce()—as defined in the generated TypeScript definitions (e.g. inpackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts)—returns a string rather than abigint. This means the current assumption in the SDK’sNonceManager.tsis incorrect. To fix this, please explicitly convert the returned nonce (e.g. viaBigInt(value)) before using or returning it.
- Verify that the value from
platform.getIdentityNonce()is converted from string to bigint.- Update type annotations accordingly to reflect that the raw response is a string if not converted.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (1)
122-240: 🛠️ Refactor suggestion
Linear variant with fractional slopes.
The thorough examples and clamp (min_value,max_value) add clarity. Confirm runtime checks handle negative or zerodto avoid division by zero.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
packages/js-dapi-client/test/unit/methods/platform/getIdentityKeys/getIdentityKeysFactory.spec.js (1)
110-154:⚠️ Potential issueFixed duplicate request setup in "all keys" test
There's duplicate code in the request setup for the "all keys" test case. Lines 128-134 repeat the exact same code as lines 121-127.
Remove the duplicate request setup:
request.setV0( new GetIdentityKeysRequestV0() .setIdentityId(identityId) .setRequestType(new KeyRequestType().setAllKeys(new AllKeys())) .setLimit(new UInt32Value([limit])) .setProve(false), ); - request.setV0( - new GetIdentityKeysRequestV0() - .setIdentityId(identityId) - .setRequestType(new KeyRequestType().setAllKeys(new AllKeys())) - .setLimit(new UInt32Value([limit])) - .setProve(false), - );
🧹 Nitpick comments (45)
packages/js-dapi-client/lib/test/fixtures/getStatusFixture.js (3)
1-63: Well-structured test fixtureThe new
getStatusFixturefunction is well-structured and comprehensive, providing a complete mock of system status for testing purposes. It includes detailed representations of version information, node details, chain status, network information, state synchronization metrics, and timestamps.Consider adding JSDoc comments to document the purpose and return type of this fixture function, especially since it's likely to be used across multiple test files.
/** - * + * Returns a fixture object that represents a complete system status response + * for testing purposes. Includes version, node, chain, network, state sync, + * and time information with realistic test values. + * + * @returns {Object} A mock status object with all required properties */ function getStatusFixture() {
24-25: Consider using a more maintainable approach for binary dataThe current implementation creates
Uint8Arrayobjects from base64-encoded strings, which works but makes the test data harder to modify or understand. Consider using a helper function to make this more readable and maintainable.+ /** + * Converts a base64 string to a Uint8Array for test fixtures + * @param {string} base64String - The base64 encoded string + * @returns {Uint8Array} - The resulting binary data + */ + function base64ToUint8Array(base64String) { + return new Uint8Array(Buffer.from(base64String, 'base64')); + } + function getStatusFixture() { return { version: { // ... }, node: { - id: new Uint8Array(Buffer.from('QbMI9zfKnjn2e1UxWJAxmKiMUW4=', 'base64')), - proTxHash: new Uint8Array(Buffer.from('s7V0hXG2D+mtEScV1qUXJdblpSqcOvX9NqFyTPUNhi8=', 'base64')), + id: base64ToUint8Array('QbMI9zfKnjn2e1UxWJAxmKiMUW4='), + proTxHash: base64ToUint8Array('s7V0hXG2D+mtEScV1qUXJdblpSqcOvX9NqFyTPUNhi8='), }, chain: { catchingUp: false, - latestBlockHash: new Uint8Array(Buffer.from('mVDwGtY2oJSaLLgv3WpLp2dFDyFEtqhD4z1gl2OJceY=', 'base64')), - latestAppHash: new Uint8Array(Buffer.from('jHgEBK8aZ74TUKcUGN58EFzUNvNsLboOgYe6eH/JetU=', 'base64')), + latestBlockHash: base64ToUint8Array('mVDwGtY2oJSaLLgv3WpLp2dFDyFEtqhD4z1gl2OJceY='), + latestAppHash: base64ToUint8Array('jHgEBK8aZ74TUKcUGN58EFzUNvNsLboOgYe6eH/JetU='), latestBlockHeight: '94461', - earliestBlockHash: new Uint8Array(Buffer.from('CPoCwn7AOQujAeT8fj1+rbNQyBk+PmKgk2iXBuOiC/o=', 'base64')), - earliestAppHash: new Uint8Array(Buffer.from('vwzLnKBxugGubmegwJD5eAPSbVbWddzVExeBy8rI7I8=', 'base64')), + earliestBlockHash: base64ToUint8Array('CPoCwn7AOQujAeT8fj1+rbNQyBk+PmKgk2iXBuOiC/o='), + earliestAppHash: base64ToUint8Array('vwzLnKBxugGubmegwJD5eAPSbVbWddzVExeBy8rI7I8='),Also applies to: 29-34
31-32: Inconsistent height representationsSome height values are represented as strings (
'94461') while others are integers (1187358). Consider standardizing the representation for consistency and to prevent potential type coercion issues.chain: { catchingUp: false, latestBlockHash: new Uint8Array(Buffer.from('mVDwGtY2oJSaLLgv3WpLp2dFDyFEtqhD4z1gl2OJceY=', 'base64')), latestAppHash: new Uint8Array(Buffer.from('jHgEBK8aZ74TUKcUGN58EFzUNvNsLboOgYe6eH/JetU=', 'base64')), - latestBlockHeight: '94461', + latestBlockHeight: 94461, earliestBlockHash: new Uint8Array(Buffer.from('CPoCwn7AOQujAeT8fj1+rbNQyBk+PmKgk2iXBuOiC/o=', 'base64')), earliestAppHash: new Uint8Array(Buffer.from('vwzLnKBxugGubmegwJD5eAPSbVbWddzVExeBy8rI7I8=', 'base64')), - earliestBlockHeight: '1', + earliestBlockHeight: 1, - maxPeerBlockHeight: '94461', + maxPeerBlockHeight: 94461, coreChainLockedHeight: 1187358,Also applies to: 35-36
packages/rs-dpp/src/data_contract/associated_token/token_keeps_history_rules/mod.rs (1)
15-16: Consider moving imports before their usageThe import of
TokenKeepsHistoryRulesV0comes after its usage in the enum definition. Consider moving this import to the top of the file with the other imports to follow Rust's conventional ordering.use bincode::{Decode, Encode}; use derive_more::From; use serde::{Deserialize, Serialize}; +use crate::data_contract::associated_token::token_keeps_history_rules::v0::TokenKeepsHistoryRulesV0; +use std::fmt; pub mod accessors; pub mod v0; #[derive(Serialize, Deserialize, Encode, Decode, Debug, Clone, Copy, PartialEq, Eq, From)] #[serde(tag = "$format_version")] pub enum TokenKeepsHistoryRules { #[serde(rename = "0")] V0(TokenKeepsHistoryRulesV0), } -use crate::data_contract::associated_token::token_keeps_history_rules::v0::TokenKeepsHistoryRulesV0; -use std::fmt;packages/rs-dapi-client/src/lib.rs (1)
29-32: Conditional exports based on target architecture and featuresThe conditional exports of the
Uritype handle different build configurations correctly. This approach is appropriate for supporting both WASM targets and mocking scenarios.Consider adding a brief comment explaining why different
Uriimplementations are needed based on the target and features, which would help future maintainers understand the rationale.+// Use different Uri implementations based on target and features: +// - For WASM or when mocks are disabled, use the standard http::Uri +// - When mocks are enabled and not on WASM, use the serializable version from http_serde #[cfg(any(target_arch = "wasm32", not(feature = "mocks")))] pub use http::Uri; #[cfg(all(feature = "mocks", not(target_arch = "wasm32")))] pub use http_serde::http::Uri;packages/rs-dpp/src/data_contract/associated_token/token_configuration_localization/v0/mod.rs (1)
7-13: Well-structured localization configuration structThis struct provides a clean data structure for token configuration localization with appropriate serialization annotations and necessary traits. The fields capture the essential localization properties.
Consider adding documentation comments to explain the purpose and usage of this struct and its fields, which would improve code maintainability.
+/// Represents localization configuration for token properties in version 0. +/// Contains settings for capitalization and language-specific forms. #[derive(Serialize, Deserialize, Decode, Encode, Debug, Clone, PartialEq, Eq, PartialOrd)] #[serde(rename_all = "camelCase")] pub struct TokenConfigurationLocalizationV0 { + /// Whether the token name should be capitalized pub should_capitalize: bool, + /// The singular form of the token name pub singular_form: String, + /// The plural form of the token name pub plural_form: String, }packages/js-dapi-client/test/unit/methods/platform/getDataContract/GetDataContractResponse.spec.js (1)
153-158: Missing metadata properties in InvalidResponseError testFor consistency, consider adding the new metadata properties (
timeMsandprotocolVersion) to this test case as well.const metadata = new ResponseMetadata(); metadata.setHeight(metadataFixture.height); metadata.setCoreChainLockedHeight(metadataFixture.coreChainLockedHeight); +metadata.setTimeMs(metadataFixture.timeMs); +metadata.setProtocolVersion(metadataFixture.protocolVersion);packages/js-dapi-client/test/unit/methods/platform/waitForStateTransitionResult/waitForStateTransitionResultFactory.spec.js (1)
122-123: Test refactoring improves consistency.Extracting the CBOR-encoded data into a variable improves test maintainability by ensuring the same data is used both for setting the error and validating expectations. This is a good practice to prevent inconsistencies in tests.
Also applies to: 127-127, 140-140
packages/js-dapi-client/lib/methods/platform/getStatus/StateSyncStatus.js (4)
39-44: Incorrect JSDoc commentThe JSDoc return comment for
getRemainingTime()is incorrect. It says "Total synced time" but should say "Remaining time to sync" to match the actual functionality./** - * @returns {bigint} Total synced time + * @returns {bigint} Remaining time to sync */
60-65: Incorrect JSDoc commentThe JSDoc return comment for
getSnapshotHeight()is incorrect. It says "Chunk process average time" instead of "Snapshot height"./** - * @returns {bigint} Chunk process average time + * @returns {bigint} Snapshot height */
67-72: Incorrect JSDoc commentThe JSDoc return comment for
getSnapshotChunkCount()is incorrect. It says "Chunk process average time" instead of "Snapshot chunks count"./** - * @returns {bigint} Chunk process average time + * @returns {bigint} Snapshot chunks count */
81-86: Method name inconsistencyThe method name is
getBackfilledBlockTotal()but it returns the property namedbackfillBlocksTotal. Consider making them consistent to improve code clarity./** - * @returns {bigint} Backfill blocks total + * @returns {bigint} Backfilled blocks total count */ - getBackfilledBlockTotal() { + getBackfillBlocksTotal() { return this.backfillBlocksTotal; }packages/rs-dapi-client/src/executor.rs (1)
37-52:❓ Verification inconclusive
Custom PartialEq implementation for better type safety
Replacing the derived
PartialEqwith a custom implementation ensures thatExecutionError<E>is only comparable when the inner error typeEalso implementsPartialEq. This provides better type safety and explicit control over equality comparison.However, based on the retrieved learning from user lklimek, standard traits like
Debug,Clone,Eq, andPartialEqshould always be present (not conditional), while only serde derives should be feature-gated.You might want to keep both the derive and the manual implementation for consistency with the learned preferences:
-#[derive(Debug, Clone, thiserror::Error, Eq)] +#[derive(Debug, Clone, thiserror::Error, Eq, PartialEq)] #[error("{inner}")] pub struct ExecutionError<E> { /// The cause of error pub inner: E, /// How many times the request was retried pub retries: usize, /// The address of the node that was used for the request pub address: Option<Address>, } -impl<E: PartialEq> PartialEq for ExecutionError<E> { - fn eq(&self, other: &Self) -> bool { - self.inner == other.inner && self.retries == other.retries && self.address == other.address - } -}Let me verify that the current implementation correctly restricts
PartialEqto cases whereE: PartialEq:
🏁 Script executed:
#!/bin/bash # Check if there are any usage instances where an ExecutionError with a non-PartialEq type is being compared # Find all comparisons with ExecutionError rg -t rust "ExecutionError.*==|==.*ExecutionError" --no-headingLength of output: 65
Update Derived Traits & Remove Custom PartialEq Implementation
The current manual implementation of
PartialEqrestricts comparisons only whenE: PartialEq, but per our learned preferences (lklimek), standard traits—Debug,Clone,Eq, andPartialEq—should always be included via derivation. Using the derive macro forPartialEqautomatically enforces thatEimplementsPartialEqwithout needing a custom implementation.Please update the code as follows:
File:
packages/rs-dapi-client/src/executor.rs(lines 37–52)Suggested Diff:
-#[derive(Debug, Clone, thiserror::Error, Eq)] +#[derive(Debug, Clone, thiserror::Error, Eq, PartialEq)] #[error("{inner}")] pub struct ExecutionError<E> { /// The cause of error pub inner: E, /// How many times the request was retried pub retries: usize, /// The address of the node that was used for the request pub address: Option<Address>, } -impl<E: PartialEq> PartialEq for ExecutionError<E> { - fn eq(&self, other: &Self) -> bool { - self.inner == other.inner && self.retries == other.retries && self.address == other.address - } -}The shell script used to search for comparison usages of
ExecutionErrorproduced no output, which is inconclusive. Please manually verify that this change won’t affect any comparisons in the codebase.packages/js-dapi-client/test/unit/methods/platform/getStatus/GetStatusResponse.spec.js (1)
21-83: Code duplication in test setupThere's significant code duplication in the test setup. The same protocol buffer initialization code appears twice (once here and once at lines 131-182).
Consider extracting the protocol buffer setup to a helper method:
beforeEach(async () => { statusFixture = getStatusFixture(); const { GetStatusResponseV0 } = GetStatusResponse; proto = new GetStatusResponse(); + + // Extract to reusable helper method + function setupProtoResponse() { + return new GetStatusResponseV0() + .setVersion(new GetStatusResponseV0.Version() + // ... existing setup code + ); + } - - proto.setV0( - new GetStatusResponseV0() - .setVersion(new GetStatusResponseV0.Version() - // ... rest of the setup - ), - ); + proto.setV0(setupProtoResponse()); // ... rest of setup code // Remove duplicate setup - proto.setV0( - new GetStatusResponseV0() - // ... duplicate code - ), - );packages/wasm-dpp/src/identity/state_transition/identity_credit_withdrawal_transition/transition.rs (1)
139-142: New getUserFeeIncrease API.
If the underlying field supports values above 65535, consider range checks or clarifying documentation to avoid potential overflow or truncation.packages/js-dapi-client/lib/methods/platform/getIdentityContractNonce/GetIdentityContractNonceResponse.js (2)
37-39: Consider adding BigInt type validationWhile the code checks for undefined or null values, it might be worth adding validation that the value is a valid BigInt before proceeding, especially since this represents a breaking change in the API.
- if ((typeof identityContractNonce === 'undefined' || identityContractNonce === null) && !proof) { + if ((typeof identityContractNonce === 'undefined' || identityContractNonce === null) && !proof) { throw new InvalidResponseError('Contract nonce data is not defined'); } + if (identityContractNonce !== undefined && typeof identityContractNonce !== 'bigint') { + throw new InvalidResponseError('Contract nonce must be a bigint'); + }
1-50: Document JSON serialization handling for BigInt valuesBigInt values cannot be directly serialized to JSON. If this response is meant to be converted to JSON at any point, you'll need to handle the serialization of BigInt values.
Consider adding a toJSON method to properly handle BigInt serialization:
toJSON() { return { ...super.toJSON(), identityContractNonce: this.identityContractNonce.toString(), }; }.pnp.cjs (4)
9062-9078: Diff Package Dependencies Update (Hunk 28)
Two dependency blocks for the"diff"package are present, one for"npm:5.1.0"and another for"npm:5.2.0". Consider verifying if both are required or if consolidation is possible.
18190-18203: Terser Package Update (Hunk 33)
Two blocks for the"terser"package—versions"npm:5.31.6"and"npm:5.39.0"—have been added. Verify that both are needed and that your bundler selects the correct version.
18348-18368: Virtual Dependency Block for terser-webpack-plugin (Hunk 35)
A complex virtual dependency block concerning"terser-webpack-plugin"and related modules has been updated. It is crucial to ensure no conflicts arise from these multi-package virtual entries.
20462-20478: Yargs-parser Dependency Revision (Hunk 39)
The dependency blocks foryargs-parserhave been updated to include versions"npm:20.2.9"and"npm:21.1.1". Verify that these changes do not lead to version conflicts and that the resolver picks the intended version.packages/js-dapi-client/test/unit/methods/platform/getDataContractHistory/GetDataContractHistoryResponse.spec.js (2)
28-35: Consider renaming the "date" field for clarity.You are using
BigInt('10000')and'20000'for thedatefield, which might be interpreted as a timestamp or block height. If it truly represents time or height, consider using a more descriptive name (e.g.,timestamp,blockHeight) to better convey its meaning.
137-138: Metadata fields added.Including
timeMsandprotocolVersionin metadata is useful, but double-check coverage in other tests to confirm thorough validation of these new fields.packages/dapi-grpc/build.rs (2)
16-27: Validate feature combinations for clarity.
These cfg-based blocks look correct individually but consider clearly documenting what happens if multiple features are enabled (e.g., bothserverandclient) so future maintainers understand the intended behavior.
237-242: Remove or clarify the#[allow(unused)]attribute.
The enumImplTypeis actually used, so the#[allow(unused)]attribute might be unnecessary. Removing it can help prevent accidental ignoring of actual unused code.-#[allow(unused)] enum ImplType { Server, Client, Wasm, }packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rs (3)
34-34: Include a comment on using the v1 config getters.
The import fromv1::DataContractConfigGettersV1is fine, but it might help readers if there’s a brief doc comment hinting how v1 differs from v0.
587-657: Empty conditional block can be refined.
Insideif !config.granular_integer_types() && property_type.is_integer() {}, the block is empty. Either remove the conditional or implement any planned logic.-if !config.granular_integer_types() && property_type.is_integer() {} +// if !config.granular_integer_types() && property_type.is_integer() { +// // TODO: implement integer-specific fallback logic +// }
659-765: Consider splitting the nested logic into helper functions.
Theinsert_values_nestedfunction has grown fairly large. Extracting parts into smaller helpers would improve readability and maintainability.packages/wasm-dpp/src/voting/state_transition/masternode_vote_transition/mod.rs (1)
241-301: Avoid usingexpectin production
.expect("expected to encode value in path")can lead to a panic on encoding errors.Consider returning a
Resultinstead of panicking:- .expect("expected to encode value in path") + .map_err(|e| ProtocolError::SerializationError(e.to_string()))?packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate.rs (1)
45-83: Floating-precision caution in StepDecreasingAmount
Utilizingf64exponentiation may introduce numerical imprecision for largesteps.Consider an integer- or rational-based approach if precision is critical.
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_recipient.rs (4)
37-78: Validation of distribution types.Overall logic is sound—and returning an error on unsupported combinations (e.g.,
EvonodesByParticipationwithPreProgrammed) is good. If you anticipate expansions to distribution logic, consider implementing an intermediate validation layer or specialized error structs.
80-80: Name clarity.
TokenDistributionWeight = u64;is well-defined, though consider naming it more specifically (e.g.,DistributionWeightType) if you foresee more numeric types.
98-98: Retain useful commented code if needed.Commented out structs (
EpochProposedBlocks) could cause confusion. If you plan to restore it soon, consider adding aTODO:note with context. Otherwise, remove it to keep the codebase clean.
168-183: Display trait usage.Providing explicit
DisplayforTokenDistributionResolvedRecipientimproves debugging. Ensure these string formats (e.g.,Evonode({})) align with any user-visible logs or telemetry.packages/dapi-grpc/Cargo.toml (1)
48-50: Target-specific dependencies for wasm vs. non-wasm.This approach is appropriate for cross-platform builds, separating
wasm32from conventional Rust dependencies. Verify that your CI pipeline checks thewasm32target if you plan to support it fully.Would you like sample CI configuration to ensure both targets (wasm32 and non-wasm32) are tested?
Also applies to: 51-60
packages/rs-dapi-client/src/transport/wasm_channel.rs (4)
24-30: Consider utilizing request settings.
The_settingsparameter is currently unused. If there's no plan to use it, consider removing it. Otherwise, leveraging it for custom timeouts or credentials can improve flexibility.
46-72: Evaluateclone()performance impact incall().
self.client.clone()is invoked each time a request is made. If requests are frequent, consider whether the underlying state is shared or if this clone operation is negligible.
74-82: Expand error mapping.
The function only handles theTonicStatusErrorvariant. For broader resiliency, consider matching other error cases and converting them to specifictonic::Statuscodes.
84-98: EnsureWasmBackonSleepercovers all retry scenarios.
The current implementation is sufficient for basic sleeps, but if advanced retry logic or jitter is needed, you may consider layering more functionality or referencing existing crates.packages/wasm-dpp/src/document/state_transition/batch_transition/document_transition/mod.rs (1)
148-172: Consider returning structured data inget_prefunded_voting_balance.
Dynamically setting the key on a JS object using theindex_namecan lead to confusion. If multiple balances are returned, keys can differ per call. A more explicit field name can improve clarity.packages/rs-dpp/src/data_contract/associated_token/token_keeps_history_rules/accessors/v0/mod.rs (1)
16-29: Consider consolidating setters.
Maintaining separate setter methods for each flag is fine, but a single setter or a builder-like API could reduce boilerplate if these flags are frequently set together.packages/js-dash-sdk/src/SDK/Client/Platform/NonceManager/NonceManager.ts (2)
74-79: Consider concurrent usage concerns inbumpIdentityNonce.If multiple concurrent calls bump the same identity's nonce, local caching could drift from the actual network state. Consider adding synchronization or re-fetching logic to minimize race conditions.
157-160: Remove the TypeScript ignore directive if possible.Line 159 has
// @ts-ignore. IfidentityContractNonceis truly abigint, the ignore directive is unnecessary.- // @ts-ignore const nextIdentityContractNonce = identityContractNonce + BigInt(1);packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/mod.rs (2)
76-106:StepDecreasingAmountvariant improvements.The discrete step pattern mirrors many popular block reward schedules. Double-check rounding if floating-point math is involved to avoid precision errors.
386-453:Logarithmicapproach is concise yet effective.The real-world cases (slowing emission rate) are well-covered. Ensure runtime logs or debug checks handle invalid inputs (e.g., log of zero or negative).
🛑 Comments failed to post (11)
packages/rs-dpp/src/errors/consensus/codes.rs (1)
257-257:
⚠️ Potential issueDuplicate error code definition.
The
TokenIsPausedErrorwith code40161is defined twice in the match statement - here and also at line 264. This duplication will cause compilation errors.- Self::TokenIsPausedError(_) => 40161,Either remove this line or the duplicate entry at line 264, but make sure to keep one instance.
Committable suggestion skipped: line range outside the PR's diff.
packages/rs-dpp/src/errors/consensus/state/token/mod.rs (1)
22-22:
⚠️ Potential issueDuplicate module import detected
The same module
token_is_paused_erroris imported twice at lines 22 and 29. This duplication is unnecessary and should be removed.pub use identity_does_not_have_enough_token_balance_error::*; pub use new_authorized_action_taker_identity_does_not_exist_error::*; pub use new_authorized_action_taker_main_group_not_set_error::*; pub use new_tokens_destination_identity_does_not_exist_error::*; pub use token_is_paused_error::*; pub use token_mint_past_max_supply_error::*; pub use token_setting_max_supply_to_less_than_current_supply_error::*; pub use unauthorized_token_action_error::*; pub use new_authorized_action_taker_main_group_not_set_error::*; pub use new_tokens_destination_identity_does_not_exist_error::*; pub use token_already_paused_error::*; -pub use token_is_paused_error::*; pub use token_mint_past_max_supply_error::*;Also applies to: 29-29
packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.spec.ts (1)
60-60: 💡 Verification agent
🧩 Analysis chain
Correct implementation of DataContractHistoryEntry in test response.
The code now uses an array of DataContractHistoryEntry objects instead of the previous object structure with numeric keys. This change reflects a more structured and type-safe approach to representing contract history entries.
Make sure that all code that consumes this response is updated to handle the new array structure instead of the previous object format.
🏁 Script executed:
#!/bin/bash # Check if any code still expects the old contract history response format rg -g "*.ts" -g "*.js" "contract\w*History\[\d+\]" --no-ignoreLength of output: 314
Action Required: Update Test Access to New Array Structure
The test in
packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.spec.tsstill accesses the contract history using the old object key (contractHistory[1000]). Since the new implementation now returns an array ofDataContractHistoryEntryobjects, please update all references accordingly (for example, if there's only one entry, usecontractHistory[0]).
- File Affected:
packages/js-dash-sdk/src/SDK/Client/Platform/methods/contracts/history.spec.ts- Action: Replace any occurrence of
contractHistory[1000]with the proper index that reflects the new array structure.- Verification: Ensure that all consumers of this response are updated to access the array format without relying on numeric keys as object properties.
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_transition.rs (1)
123-124:
⚠️ Potential issueCode duplication detected in trait definition.
The
entropymethod is declared twice in the trait. There appears to be a duplication between lines 123-124 and lines 131-132. One of these declarations should be removed.packages/js-dapi-client/test/unit/methods/platform/getStatus/GetStatusResponse.spec.js (1)
131-182: 🛠️ Refactor suggestion
Remove duplicate protocol buffer initialization code
This entire block duplicates the initialization done in the
beforeEachblock (lines 27-78). This redundancy should be eliminated.Consider removing this duplicate initialization since it's already done in the
beforeEachblock:- proto.setV0( - new GetStatusResponseV0() - .setVersion(new GetStatusResponseV0.Version() - // ... all of this duplicate code - )), - );📝 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.packages/js-dash-sdk/tsconfig.mocha.json (1)
8-8:
⚠️ Potential issueFix the trailing comma syntax error.
There's a trailing comma before the closing brace which causes a JSON syntax error.
- "outDir": "./build", + "outDir": "./build"📝 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."outDir": "./build" },🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Expected a property but instead found '}'.
Expected a property here.
(parse)
packages/wasm-dpp/src/document/state_transition/batch_transition/document_transition/mod.rs (1)
50-70:
⚠️ Potential issueHandle serialization failures in
get_data.
.unwrap()may cause a runtime panic if data serialization fails. Consider returning aResult<JsValue, JsValue>or a fallback in case of unforeseen conversion issues.- let json_value = document_create_transition.data().to_json_value().unwrap(); + let json_value = match document_create_transition.data().to_json_value() { + Ok(v) => v, + Err(e) => return JsValue::from_str(&format!("Serialization error: {}", e)), + };📝 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.#[wasm_bindgen(js_name=getData)] pub fn get_data(&self) -> JsValue { match &self.0 { DocumentTransition::Create(document_create_transition) => { - let json_value = document_create_transition.data().to_json_value().unwrap(); + let json_value = match document_create_transition.data().to_json_value() { + Ok(v) => v, + Err(e) => return JsValue::from_str(&format!("Serialization error: {}", e)), + }; json_value .serialize(&serde_wasm_bindgen::Serializer::json_compatible()) .unwrap() } DocumentTransition::Replace(document_replace_transition) => { let json_value = document_replace_transition.data().to_json_value().unwrap(); json_value .serialize(&serde_wasm_bindgen::Serializer::json_compatible()) .unwrap() } DocumentTransition::Delete(_) => JsValue::null(), DocumentTransition::Transfer(_) => JsValue::null(), DocumentTransition::UpdatePrice(_) => JsValue::null(), DocumentTransition::Purchase(_) => JsValue::null(), } }packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_moment/mod.rs (1)
124-154:
⚠️ Potential issuePotential data truncation in
PartialEq<&u16>andPartialEq<u16>.
When comparingBlockBasedMomentorTimeBasedMomentagainst au16, casting larger values will truncate and could incorrectly produce a match. To avoid silent mismatches, consider an explicit check before casting:-match self { - RewardDistributionMoment::BlockBasedMoment(value) => *value as u16 == **other, - RewardDistributionMoment::TimeBasedMoment(value) => *value as u16 == **other, - RewardDistributionMoment::EpochBasedMoment(value) => *value == **other, -} +match self { + RewardDistributionMoment::BlockBasedMoment(value) => { + if *value > u16::MAX.into() { + false + } else { + (*value as u16) == **other + } + } + RewardDistributionMoment::TimeBasedMoment(value) => { + if *value > u16::MAX.into() { + false + } else { + (*value as u16) == **other + } + } + RewardDistributionMoment::EpochBasedMoment(value) => *value == **other, +}📝 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.impl PartialEq<&u16> for RewardDistributionMoment { fn eq(&self, other: &&u16) -> bool { match self { RewardDistributionMoment::BlockBasedMoment(value) => { if *value > u16::MAX.into() { false } else { (*value as u16) == **other } } RewardDistributionMoment::TimeBasedMoment(value) => { if *value > u16::MAX.into() { false } else { (*value as u16) == **other } } RewardDistributionMoment::EpochBasedMoment(value) => *value == **other, } } } impl PartialEq<u16> for RewardDistributionMoment { fn eq(&self, other: &u16) -> bool { self == &other } } impl PartialEq<&usize> for RewardDistributionMoment { fn eq(&self, other: &&usize) -> bool { match self { RewardDistributionMoment::BlockBasedMoment(value) => *value as usize == **other, RewardDistributionMoment::TimeBasedMoment(value) => *value as usize == **other, RewardDistributionMoment::EpochBasedMoment(value) => *value as usize == **other, } } } impl PartialEq<usize> for RewardDistributionMoment { fn eq(&self, other: &usize) -> bool { self == &other } }packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/encode.rs (3)
23-25:
⚠️ Potential issueStepDecreasingAmount encode - watch out for zero denominator
Splitting the decrease factor into numerator and denominator is flexible. However, consider validating or rejecting a zero denominator to avoid potential runtime faults.
Would you like to add validation to ensure
decrease_per_interval_denominatorremains non-zero?Also applies to: 27-27, 29-29, 31-33, 35-35
166-168:
⚠️ Potential issueStepDecreasingAmount decode - validate denominator
Same note as in encode: consider adding checks for zero denominators.
Also applies to: 169-169, 171-171, 173-179
312-317:
⚠️ Potential issueStepDecreasingAmount borrow_decode - zero denominator risk
Same concerns about a zero denominator apply here.
Also applies to: 319-325
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-dpp/src/data_contract/config/v1/mod.rs (1)
136-139:⚠️ Potential issueCritical copy-paste error in property retrieval.
The block for
requires_identity_decryption_bounded_keyreferencesREQUIRES_IDENTITY_ENCRYPTION_BOUNDED_KEYinstead ofREQUIRES_IDENTITY_DECRYPTION_BOUNDED_KEY. As a result, both fields are pulled from the same property.Apply this fix:
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()?;
🧹 Nitpick comments (3)
packages/rs-drive-abci/src/rpc/core.rs (1)
273-273: Method name changes should be documentedThe implementation is now calling new "reversed" versions of the quorum methods, but the trait still defines methods with the original names. This appears to be an adaptation layer to maintain API compatibility.
Consider adding a brief comment explaining why the underlying methods have changed to the "reversed" versions, which would help future developers understand the purpose of this adaptation.
fn get_quorum_listextended( &self, height: Option<CoreHeight>, ) -> Result<ExtendedQuorumListResult, Error> { + // Using the reversed version from the newer API while maintaining compatibility retry!(self.inner.get_quorum_listextended_reversed(height)) }Also applies to: 284-284
packages/rs-dpp/src/data_contract/config/v1/mod.rs (1)
141-143: Ensure consistent naming between value retrieval and struct field assignment.The value is retrieved using
GRANULAR_NUMERIC_TYPES, but assigned to the struct fieldgranular_integer_types. While the code works, it might cause confusion if field names differ from their corresponding property names.For better alignment, consider either renaming the constant in the fields.rs file or making the struct field name match:
let granular_numeric_types = contract .get_optional_bool(config::property::GRANULAR_NUMERIC_TYPES)? .unwrap_or(DEFAULT_GRANULAR_NUMERIC_TYPES);packages/rs-dpp/src/data_contract/document_type/property/mod.rs (1)
2134-2160: Consider optimizing enum type detection.The nested conditions and type determination for enum values could potentially be refactored for better readability.
Consider extracting the enum type determination into a separate function:
- // If enum is defined, we can try to figure out type based on minimal and maximal values - let enum_type = if let Some(enum_values) = - value.get_optional_inner_value_array::<Vec<_>>(property_names::ENUM)? - { - match enum_values - .into_iter() - .filter_map(|v| v.as_integer()) - .minmax() - { - itertools::MinMaxResult::MinMax(min, max) => { - Some(find_integer_type_for_min_and_max_values(min, max)) - } - itertools::MinMaxResult::OneElement(val) => { - Some(find_unsigned_integer_type_for_max_value(val)) - } - _ => None, - } - } else { - None - }; + let enum_type = find_integer_type_from_enum_values(value)?; // ... + fn find_integer_type_from_enum_values( + value: &BTreeMap<String, &Value> + ) -> Result<Option<DocumentPropertyType>, DataContractError> { + if let Some(enum_values) = value.get_optional_inner_value_array::<Vec<_>>(property_names::ENUM)? { + match enum_values + .into_iter() + .filter_map(|v| v.as_integer()) + .minmax() + { + itertools::MinMaxResult::MinMax(min, max) => { + Ok(Some(find_integer_type_for_min_and_max_values(min, max))) + } + itertools::MinMaxResult::OneElement(val) => { + Ok(Some(find_unsigned_integer_type_for_max_value(val))) + } + _ => Ok(None), + } + } else { + Ok(None) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockCargo.lockis excluded by!**/*.lockCargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
packages/rs-dpp/src/data_contract/document_type/property/mod.rs(3 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs(1 hunks)packages/rs-dpp/src/data_contract/document_type/property/mod.rs(3 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs(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/property/mod.rs(5 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)packages/rs-dpp/src/data_contract/config/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/config/v1/mod.rs(1 hunks)packages/rs-dpp/Cargo.toml(6 hunks)packages/rs-dpp/Cargo.toml(1 hunks)packages/rs-drive-abci/Cargo.toml(1 hunks)packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/rpc/core.rs(4 hunks)packages/rs-sdk/Cargo.toml(2 hunks)packages/simple-signer/Cargo.toml(1 hunks)packages/rs-dpp/src/data_contract/config/fields.rs(2 hunks)packages/rs-dpp/src/data_contract/config/mod.rs(1 hunks)packages/rs-dpp/src/data_contract/config/v1/mod.rs(5 hunks)packages/rs-dpp/src/data_contract/document_type/property/mod.rs(2 hunks)packages/rs-drive/tests/query_tests.rs(1 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/rs-dpp/src/data_contract/config/mod.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs
- packages/rs-dpp/src/data_contract/config/fields.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs
- packages/rs-dpp/src/data_contract/config/fields.rs
- packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs
- packages/rs-dpp/src/data_contract/document_type/property/mod.rs
- packages/rs-dpp/src/data_contract/config/v1/mod.rs
- packages/rs-dpp/Cargo.toml
- packages/rs-dpp/src/data_contract/config/mod.rs
- packages/rs-dpp/src/data_contract/document_type/property/mod.rs
- packages/rs-dpp/Cargo.toml
🧰 Additional context used
🪛 GitHub Check: Rust packages (wasm-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
🪛 GitHub Check: Rust packages (drive) / 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
🪛 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
🪛 GitHub Check: Rust packages (dash-sdk) / 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
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (18)
packages/rs-drive-abci/src/rpc/core.rs (1)
171-203: Improved retry logic with more idiomatic RustThe refactoring of the retry logic to use a functional approach with
find_mapis a nice improvement. The new implementation is more concise, better separates success and error cases, and follows Rust's functional programming patterns. The explicit delay comment also adds clarity.packages/simple-signer/Cargo.toml (1)
11-11: Dependency Version Update: dashcore-rpc
The dashcore-rpc dependency has been updated to version v0.38.0. This change is consistent with similar dependency updates across the repository. Please ensure that the new version is fully compatible with the simple-signer package and does not introduce any breaking changes.packages/rs-drive-abci/Cargo.toml (1)
31-31: Dependency Version Update: dashcore-rpc
The update to dashcore-rpc v0.38.0 in the drive-abci package is aligned with the rest of the repository. Verify that the new version integrates seamlessly into the package without affecting dependent functionalities.packages/rs-sdk/Cargo.toml (1)
41-41: Dependency Version Update: dashcore-rpc
The dashcore-rpc dependency in the rs-sdk package has been updated to tag v0.38.0. This is consistent with the other package updates. Please validate that the changes in this dependency version do not break any integrations or APIs within the SDK, especially where dashcore-rpc functionalities are directly or indirectly invoked.packages/rs-dpp/src/data_contract/config/mod.rs (2)
229-233: Improved naming from 'granular_integer_types' to 'sized_integer_types'This rename provides a clearer description of the feature's purpose and aligns with the previous reviewer's suggestion. The term "sized" better reflects that these are fixed-width integer types with specific constraints.
238-242: Consistent renaming in the setter methodThe setter method has been correctly renamed to match the getter, maintaining consistency throughout the codebase.
packages/rs-drive/tests/query_tests.rs (3)
62-62: Import added for the new data contract configuration feature.This import provides access to the setters needed for configuring granular integer types in data contracts.
83-84: Import statement reorganization.The imports for
LazyandStdRngwere moved to improve code organization. This change doesn't affect functionality.
883-896: Contract variable now mutable to allow configuration updates.The contract is now created as mutable to support the explicit disabling of granular integer types. This ensures consistent behavior for existing tests when the new granular integer types feature is introduced in the codebase.
This change aligns with the PR objective of introducing more granular integer document property types while maintaining backward compatibility for existing contracts.
packages/rs-dpp/src/data_contract/config/v1/mod.rs (4)
48-51: Verify getter name consistency with implementation and usage.The getter is named
granular_integer_typeswhich should match the field name in the struct. Ensure the naming is consistent with usage in other parts of the codebase.
54-57: Verify setter name consistency with implementation and usage.The setter method is named
set_granular_integer_types_enabledwhich follows the structure of other setters in the codebase. Make sure this is used consistently.
44-44: Documentation should match the property's purpose.The documentation comment describes the field as "Use granular integer Rust types for
integerproperty type" which accurately describes what the field controls.
16-45: Well-structured data contract configuration with clear documentation.The struct is well-designed with comprehensive documentation for each field, making it clear what each configuration option controls. The annotations for serialization and deserialization are properly applied.
packages/rs-dpp/src/data_contract/document_type/property/mod.rs (5)
19-19: Remove unused import.The
BTreeValueMapPathHelperimport is unused according to multiple linting warnings.- use platform_value::btreemap_extensions::{BTreeValueMapHelper, BTreeValueMapPathHelper}; + use platform_value::btreemap_extensions::BTreeValueMapHelper;🧰 Tools
🪛 GitHub Check: Rust packages (wasm-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🪛 GitHub Check: Rust packages (drive) / 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🪛 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🪛 GitHub Check: Rust packages (dash-sdk) / 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
2017-2029: Good addition of helper method for type checking.The
is_integermethod provides a clean way to check if a property type is an integer type, which will help with type-specific operations throughout the codebase.
2031-2086: Well-implemented property type parser with granular integer support.The
try_from_value_mapmethod is a significant improvement over the deprecatedtry_from_namemethod. It properly handles different property types and incorporates the granular integer types option to determine the appropriate type based on schema constraints.
2089-2108: Clean implementation of parsing options struct with appropriate defaults.The
DocumentPropertyTypeParsingOptionsstruct and its implementations provide a clear way to configure property type parsing, with sensible defaults and convenient conversion fromDataContractConfig.
2117-2164: Well-designed integer type selection logic.The
find_integer_type_for_subschema_valuefunction correctly handles various scenarios for determining the appropriate integer type based on minimum, maximum, and enum values, ensuring optimal type selection when granular integer types are enabled.
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
chore: update to latest dash core 37 (#2483) feat(platform)!: token advanced distribution and updates (#2471) fix: token history contract (#2474) Co-authored-by: Ivan Shumkov <[email protected]> Co-authored-by: QuantumExplorer <[email protected]> fix(drive): using new rust dash core methods for reversed quorum hash to maintain backwards compatibility (#2489) feat: more granular integer document property types (#2455) Co-authored-by: Quantum Explorer <[email protected]> docs: update comment for data contract code range (#2476) feat: validate token name localizations (#2468) feat(sdk): get identity by non-unique keys build(deps): update grovedb to current develop test: test identity by non-unique pubkey hashes fix(sdk): dash core client fails to get quorum chore: minor fixes test(drive-abci): identity by non-unique pubkey start after chore: minor changes to verify feat(sdk): token and group queries (#2449) chore: revert limit 1 => limit none chore: add non-unique key to test identities test(sdk): test vectors for test_fetch_identity_by_non_unique_public_keys fix(platform)!: token distribution fixes and tests (#2494) chore(platform): bump to version 2.0.0-dev.1 (#2495) test: update assertion fix(sdk): make some things public (#2496) feat(platform): require token for document actions (#2498) fix: data contract proof doesn't work with new auto fields (#2501)
chore: update to latest dash core 37 (#2483) feat(platform)!: token advanced distribution and updates (#2471) fix: token history contract (#2474) Co-authored-by: Ivan Shumkov <[email protected]> Co-authored-by: QuantumExplorer <[email protected]> fix(drive): using new rust dash core methods for reversed quorum hash to maintain backwards compatibility (#2489) feat: more granular integer document property types (#2455) Co-authored-by: Quantum Explorer <[email protected]> docs: update comment for data contract code range (#2476) feat: validate token name localizations (#2468) feat(sdk): get identity by non-unique keys build(deps): update grovedb to current develop test: test identity by non-unique pubkey hashes fix(sdk): dash core client fails to get quorum chore: minor fixes test(drive-abci): identity by non-unique pubkey start after chore: minor changes to verify feat(sdk): token and group queries (#2449) chore: revert limit 1 => limit none chore: add non-unique key to test identities test(sdk): test vectors for test_fetch_identity_by_non_unique_public_keys fix(platform)!: token distribution fixes and tests (#2494) chore(platform): bump to version 2.0.0-dev.1 (#2495) test: update assertion fix(sdk): make some things public (#2496) feat(platform): require token for document actions (#2498) fix: data contract proof doesn't work with new auto fields (#2501)
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
New Features
RewardDistributionMomentenum for handling different types of reward distribution moments with robust type safety and error handling.Refactor
Metadataobjects by transitioning from object literals to direct positional arguments.Tests
BigIntfor numeric comparisons, ensuring accurate handling of larger integer values.