fix(platform): fixed Platform State deserialization issue #2227
Conversation
WalkthroughThe pull request introduces several modifications across multiple modules, focusing on enhancing the encoding and decoding processes for various structs, specifically Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (18)
packages/rs-dpp/src/fee/fee_result/refunds.rs (1)
188-189: LGTM: Test setup updated correctly with a minor suggestion.The initialization of
EPOCH_CHANGE_FEE_VERSION_TESThas been updated to useFeeVersion::first(), which is consistent with the import change and the shift toFeeVersion. This modification simplifies the code while maintaining the same functionality.Consider updating the variable name to reflect the new type:
-static EPOCH_CHANGE_FEE_VERSION_TEST: Lazy<CachedEpochIndexFeeVersions> = +static EPOCH_CHANGE_FEE_VERSIONS_TEST: Lazy<CachedEpochIndexFeeVersions> = Lazy::new(|| BTreeMap::from([(0, FeeVersion::first())]));This change would make the variable name more accurate and consistent with its new content.
packages/rs-dpp/src/core_types/validator/v0/mod.rs (2)
255-296: Excellent addition of serialization/deserialization testThe new test module with the
test_serialize_deserialize_validator_v0function is a valuable addition to ensure the correctness of theValidatorV0serialization and deserialization process. The test comprehensively covers all fields of theValidatorV0struct and verifies that the deserialized instance matches the original, which is crucial for maintaining data integrity.To further enhance the test coverage, consider adding the following:
- Test cases with edge values (e.g., empty strings, minimum/maximum port numbers).
- A test case where
public_keyisNoneto ensure proper handling of optional fields.- Explicit checks for individual field values after deserialization, in addition to the overall struct equality check.
Would you like me to provide example code for these additional test cases?
Line range hint
1-296: Consistent with PR objectives and summaryThe changes in this file align well with the PR objectives of fixing a Platform State deserialization issue. The modifications to the
EncodeandDecodeimplementations forValidatorV0, along with the new test case, directly address potential deserialization problems and ensure data integrity.To further improve the code:
Consider adding a brief comment at the top of the file or above the
ValidatorV0struct explaining its role in the Platform State and how these changes contribute to resolving the deserialization issue mentioned in the PR title. This would provide valuable context for future developers working with this code.Would you like me to provide an example of such a comment?
packages/rs-platform-version/src/version/drive_abci_versions.rs (1)
77-81: Consider adding Rust doc commentsTo improve code documentation and maintainability, consider adding Rust doc comments to the
DriveAbciStructureVersionsstruct and its fields. This will provide better context and understanding for developers working with this code in the future.Example:
/// Represents the versions of various Drive ABCI structures pub struct DriveAbciStructureVersions { /// Version of the platform state structure pub platform_state_structure: FeatureVersion, /// Default version of the platform state structure for saving pub platform_state_for_saving_structure_default: FeatureVersion, // ... (add comments for other fields) }packages/rs-platform-version/src/version/v3.rs (1)
612-612: Approved: Field renaming for clarityThe change from
platform_state_for_saving_structuretoplatform_state_for_saving_structure_defaultimproves clarity by explicitly indicating that this is a default value. This is a good practice for maintaining clear and self-documenting code.Consider updating any related documentation or comments to reflect this change and explain the significance of the "_default" suffix if it's not already clear in the surrounding context.
packages/rs-platform-version/src/version/fee/processing/mod.rs (1)
18-22: Improve clarity of the deserialization issue commentsThe comments explaining the purpose of
FeeProcessingVersionFieldsBeforeVersion1Point4could be clearer for future maintainers. Consider rephrasing them for better understanding.Apply this diff to enhance the comments:
-// This is type only meant for deserialization because of an issue -// The issue was that the platform state was stored with FeeVersions in it before version 1.4 -// When we would add new fields we would be unable to deserialize -// This FeeProcessingVersionFieldsBeforeVersion4 is how things were before version 1.4 was released +// This struct is only meant for deserialization due to a compatibility issue. +// Prior to version 1.4, the platform state was stored with FeeVersions without the `perform_network_threshold_signing` field. +// Adding new fields caused deserialization failures. +// `FeeProcessingVersionFieldsBeforeVersion1Point4` represents the old struct layout before version 1.4.packages/rs-platform-version/src/version/fee/mod.rs (1)
41-74: Consider handling emptyFEE_VERSIONSgracefullyIn the
firstandlatestmethods, usingexpectwill cause a panic ifFEE_VERSIONSis empty. Although it's expected to have at least one fee version, consider adding a more graceful error handling or a descriptive error message to prevent potential runtime panics.packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (3)
333-335: Handle potential decoding errors in testsWhile using
.unwrap()is acceptable in tests, providing a clear error message with.expect()can improve test diagnostics if the decoding fails unexpectedly.Apply this diff to enhance error handling:
// Deserialize the data back into a ValidatorSetV0 instance let decoded: ValidatorSetV0 = bincode::decode_from_slice(&encoded, config::standard()) - .unwrap() + .expect("Failed to decode ValidatorSetV0") .0;
Line range hint
99-126: Ensure consistent handling of decoding errorsIn the
Decodeimplementation, consider providing more detailed error messages or handling specific error cases to aid in debugging if decoding fails.For example, when decoding
ProTxHashandBlsPublicKey, you can include the underlying error message or context.Modify the error handling as follows:
let key = ProTxHash::from_slice(&key_bytes).map_err(|e| { bincode::error::DecodeError::OtherString(format!( - "Failed to decode ProTxHash" + "Failed to decode ProTxHash: {}", + e )) })?;And similarly for
threshold_public_key:let threshold_public_key = BlsPublicKey::from_bytes(&bytes).map_err(|e| { bincode::error::DecodeError::OtherString(format!( - "Failed to decode BlsPublicKey" + "Failed to decode BlsPublicKey: {}", + e )) })?;
140-141: Remove redundant commentsThe comments at lines 140-141 duplicate information that is already clear from the code. Removing them can improve code readability.
Apply this diff:
- // Decode the quorum hash directly as a [u8; 32] array let quorum_hash = <[u8; 32]>::decode(decoder)?; - // Decode quorum_index as Option<u32> let quorum_index = Option::<u32>::decode(decoder)?;packages/rs-drive-abci/src/execution/platform_events/block_fee_processing/tests.rs (6)
Line range hint
54-58: Remove unused variablealtered_documentThe variable
altered_documentis created and modified but never used within thesetup_join_contract_documentfunction. Removing it will improve code clarity and reduce unnecessary code.Apply this diff to remove the unused code:
@@ -54,6 +54,0 @@ - let mut altered_document = document.clone(); - - altered_document.increment_revision().unwrap(); - altered_document.set("displayName", "Ivan".into()); - altered_document.set("avatarUrl", "http://test.com/dog.jpg".into());
Line range hint
128-133: Remove unused variablealtered_documentThe variable
altered_documentis created and modified but never used within thesetup_initial_documentfunction. Removing it will enhance code readability and eliminate unnecessary operations.Apply this diff to remove the unused code:
@@ -128,6 +128,0 @@ - let mut altered_document = document.clone(); - - altered_document.increment_revision().unwrap(); - altered_document.set("displayName", "Samuel".into()); - altered_document.set("avatarUrl", "http://test.com/cat.jpg".into());
Line range hint
303-307: Clarify refund percentage calculation in assertionsThe assertions for refund amounts in
test_document_refund_immediatecompare the refund to be more than 99% and less than 100% of the insertion fee. Consider calculating the exact expected refund percentage to make the test assertions clearer and more precise.You could update the assertions as follows:
-assert!(refund_amount > lower_bound, "expected the refund amount to be more than 99% of the storage cost, as it is for just one out of 2000 epochs"); -assert!( - refund_amount < insertion_fee_result.storage_fee, - "expected the refund amount to be less than the insertion cost" -); +let expected_refund = insertion_fee_result.storage_fee * refund_factor / 100; +assert_eq!( + refund_amount, expected_refund, + "expected the refund amount to be exactly {}% of the storage cost", refund_factor +);
Line range hint
247-248: Avoid magic numbers for identity setupThe value
958used insetup_identity_with_system_creditsmay not be immediately clear. Consider defining a constant or adding a comment to explain its significance.For example:
// 958 is the unique identity index for this test let (identity, signer, key) = setup_identity_with_system_credits(&mut platform, 958, dash_to_credits!(1));
Line range hint
486-490: Consider edge case handling for zero refundsIn
test_document_refund_after_50_years, the refund amount is expected to be zero. It's good to assert this explicitly, but also consider handling any potential edge cases where floating-point precision or integer division could affect the refund calculation.Ensure that the refund calculation correctly handles long durations without unexpected rounding errors.
Line range hint
243-244: Redundant platform state loadingThe
platform_stateis loaded but not used before being reloaded later in the tests. Consider removing the redundant loading to streamline the code.Apply this diff to remove the unused variable:
- let platform_state = platform.state.load();packages/rs-drive/tests/query_tests.rs (2)
Line range hint
149-183: Refactorsetup_family_tests_with_nullsto Eliminate Code DuplicationThe
setup_family_tests_with_nullsfunction shares significant similarities withsetup_family_tests. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring the common code into a shared helper function or parameterizing the differences. This will enhance maintainability and reduce potential errors from duplicated code.
Line range hint
184-221: Refactorsetup_family_tests_only_first_name_indexto Reduce RedundancySimilar to the previous function,
setup_family_tests_only_first_name_indexduplicates much of the logic fromsetup_family_tests. Refactoring common functionality into a shared utility function or using parameters to handle variations can improve code readability and maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
- packages/rs-dpp/src/core_types/validator/v0/mod.rs (5 hunks)
- packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (7 hunks)
- packages/rs-dpp/src/fee/default_costs/mod.rs (3 hunks)
- packages/rs-dpp/src/fee/fee_result/refunds.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/block_fee_processing/tests.rs (1 hunks)
- packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/upgrade_protocol_version/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (4 hunks)
- packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (4 hunks)
- packages/rs-drive/src/drive/document/delete/mod.rs (1 hunks)
- packages/rs-drive/src/drive/document/insert/mod.rs (1 hunks)
- packages/rs-drive/src/drive/document/update/mod.rs (1 hunks)
- packages/rs-drive/tests/query_tests.rs (3 hunks)
- packages/rs-drive/tests/query_tests_history.rs (3 hunks)
- packages/rs-platform-version/src/version/drive_abci_versions.rs (1 hunks)
- packages/rs-platform-version/src/version/fee/mod.rs (3 hunks)
- packages/rs-platform-version/src/version/fee/processing/mod.rs (2 hunks)
- packages/rs-platform-version/src/version/fee/v1.rs (1 hunks)
- packages/rs-platform-version/src/version/mocks/v2_test.rs (1 hunks)
- packages/rs-platform-version/src/version/mocks/v3_test.rs (1 hunks)
- packages/rs-platform-version/src/version/v1.rs (1 hunks)
- packages/rs-platform-version/src/version/v2.rs (1 hunks)
- packages/rs-platform-version/src/version/v3.rs (1 hunks)
- packages/rs-platform-version/src/version/v4.rs (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-version/src/version/mocks/v2_test.rs
- packages/rs-platform-version/src/version/mocks/v3_test.rs
- packages/rs-platform-version/src/version/v1.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-platform-version/src/version/drive_abci_versions.rs (1)
Learnt from: shumkov PR: dashpay/platform#2182 File: packages/rs-platform-version/src/version/drive_abci_versions.rs:116-121 Timestamp: 2024-09-29T13:13:54.230Z Learning: Adding Rust doc comments to structs and fields is recommended to enhance clarity and documentation.
🔇 Additional comments (38)
packages/rs-platform-version/src/version/fee/v1.rs (1)
11-11: LGTM! Verify related code for consistency.The addition of the
fee_version_numberfield is a good practice for versioning and its value is consistent with the constant nameFEE_VERSION1. This change enhances the clarity and maintainability of the fee versioning system.Please ensure that:
- The
FeeVersionstruct definition has been updated to include this new field.- Any code that constructs or uses
FeeVersioninstances has been updated accordingly.- This change is part of a broader versioning strategy for fee-related structures.
Run the following script to verify the
FeeVersionstruct definition and its usage:✅ Verification successful
FeeVersion struct and its usage have been successfully verified.
The
fee_version_numberfield has been correctly added to theFeeVersionstruct inmod.rs, and all instances inv1.rshave been updated accordingly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify FeeVersion struct definition and usage # Test 1: Check FeeVersion struct definition echo "Checking FeeVersion struct definition:" rg --type rust "struct FeeVersion" -A 10 # Test 2: Check FeeVersion usage echo "\nChecking FeeVersion usage:" rg --type rust "FeeVersion\s*[{=]" -A 5Length of output: 5626
packages/rs-dpp/src/fee/fee_result/refunds.rs (2)
185-185: LGTM: Import statement updated correctly.The import statement has been updated to use
FeeVersionfromplatform_version::version::fee, which is consistent with the changes in the test setup. This modification aligns with the shift fromPlatformVersiontoFeeVersionfor fee version management.
185-189: Verify consistency of FeeVersion changes across the codebase.The changes in this file are part of a larger refactoring effort to use
FeeVersioninstead ofPlatformVersion. To ensure consistency and completeness of this refactoring:Run the following script to check for any remaining instances of
PlatformVersionrelated to fee calculations:This script will help identify any remaining instances of
PlatformVersionthat might need to be updated toFeeVersion, ensuring consistency across the codebase.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/upgrade_protocol_version/v0/mod.rs (1)
102-104: Improved fee version comparison logicThe changes to the fee version comparison are well-implemented and offer several benefits:
- Enhanced efficiency: By comparing
fee_version_numberdirectly instead of cloning the entire fee version, unnecessary memory allocation is avoided.- Improved readability: The comparison is now more explicit, making the intention clearer.
- Maintained functionality: The logic still ensures that a new fee version is only inserted when it differs from the last one.
These modifications contribute to better performance and code quality without altering the overall behavior of the method.
Also applies to: 107-107, 114-114
packages/rs-dpp/src/core_types/validator/v0/mod.rs (3)
50-51: Optimization: Improved encoding forpro_tx_hashandnode_idThe changes from
to_byte_array().to_vec()toas_byte_array()forpro_tx_hashandnode_idencoding are good optimizations. This modification avoids unnecessary allocation by returning a reference to the underlying byte array instead of creating a newVec<u8>. This improvement enhances performance and reduces memory usage during the encoding process.Also applies to: 68-69
89-96: Enhanced type safety: Use of fixed-size arrays in decodingThe changes to use fixed-size arrays (
[u8; 32]forpro_tx_hash,[u8; 48]forpublic_key, and[u8; 20]fornode_id) instead ofVec<u8>in the decoding process are excellent improvements. These modifications offer several benefits:
- Improved type safety: The use of fixed-size arrays ensures that the decoded data always has the correct size, preventing potential issues with incorrect data lengths.
- Better alignment with cryptographic practices: Fixed-size arrays are preferred over dynamic-sized vectors when handling cryptographic primitives.
- Potential performance improvements: Fixed-size arrays can lead to better performance as they are stack-allocated and have a known size at compile-time.
These changes contribute to a more robust and reliable decoding process.
Also applies to: 108-110
Line range hint
1-296: Well-structured and complete implementationThe overall structure and completeness of the file are commendable. The changes focus on improving the
EncodeandDecodeimplementations forValidatorV0, with the addition of a comprehensive test. These modifications are consistent and well-integrated with the existing code.The file maintains a clear organization with separate trait implementations for
Encode,Decode,Debug,ValidatorV0Getters, andValidatorV0Setters. This separation of concerns enhances readability and maintainability.The unchanged parts of the file, including the struct definition and other trait implementations, appear to be complete and well-structured. There are no apparent inconsistencies or missing implementations.
packages/rs-platform-version/src/version/drive_abci_versions.rs (2)
80-80: LGTM: Field renamed for clarityThe renaming of
platform_state_for_saving_structuretoplatform_state_for_saving_structure_defaultimproves clarity by indicating that this field represents a default value. This change enhances code readability and understanding.
80-80: Verify impact of field renamingThe renaming of
platform_state_for_saving_structuretoplatform_state_for_saving_structure_defaultmight affect other parts of the codebase that reference this field. Let's verify the impact of this change.✅ Verification successful
Verified: Field Renaming Impact
The renaming of
platform_state_for_saving_structuretoplatform_state_for_saving_structure_defaulthas been successfully reflected across the codebase. No references to the old field name were found, and the new field name is used consistently in the following files:
packages/rs-platform-version/src/version/v4.rspackages/rs-platform-version/src/version/v1.rspackages/rs-platform-version/src/version/v2.rspackages/rs-platform-version/src/version/mocks/v3_test.rspackages/rs-platform-version/src/version/mocks/v2_test.rspackages/rs-platform-version/src/version/v3.rspackages/rs-platform-version/src/version/drive_abci_versions.rspackages/rs-drive-abci/src/platform_types/platform_state/mod.rs🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of the old and new field names across the codebase echo "Searching for old field name usage:" rg --type rust "platform_state_for_saving_structure[^_]" echo "Searching for new field name usage:" rg --type rust "platform_state_for_saving_structure_default"Length of output: 1207
packages/rs-drive/src/drive/document/insert/mod.rs (2)
63-63: LGTM: New import added for FeeVersionThe new import
use dpp::version::fee::FeeVersion;is correctly placed and appears to be necessary for the changes in this file.
67-67: Verify the impact of using FeeVersion::first() instead of PlatformVersionThe initialization of
EPOCH_CHANGE_FEE_VERSION_TESThas been updated to useFeeVersion::first()directly instead ofPlatformVersion::first().fee_version.clone(). While this change simplifies the code, it's important to ensure that it doesn't alter the expected behavior in the rest of the system.Please confirm that this change:
- Maintains the same fee version as before
- Doesn't break any existing functionality that relies on this static variable
- Is consistent with the intended design of the fee version system
You can use the following script to check for any other usages of
PlatformVersion::first().fee_versionthat might need to be updated:✅ Verification successful
Change Verified: No other instances of
PlatformVersion::first().fee_versionfoundThe verification confirms that there are no other usages of
PlatformVersion::first().fee_versionin the codebase. Therefore, usingFeeVersion::first()directly does not affect other parts of the system and maintains the intended behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of PlatformVersion::first().fee_version rg "PlatformVersion::first\(\)\.fee_version" --type rustLength of output: 58
Script:
#!/bin/bash # Search for any occurrences of 'PlatformVersion' accessing 'fee_version' in Rust files rg "PlatformVersion\s*::\s*first\(\)\s*\.s*fee_version" --type rustLength of output: 69
Script:
#!/bin/bash # Search for any occurrences of 'PlatformVersion' accessing 'fee_version' in Rust files with corrected regex rg "PlatformVersion\s*::\s*first\(\)\s*\.\s*fee_version" --type rustLength of output: 70
packages/rs-drive/tests/query_tests_history.rs (3)
56-56: LGTM: New import for FeeVersion.The addition of the
FeeVersionimport is correct and necessary for the changes made in thesetupandtest_query_historicalfunctions.
264-265: LGTM: Consistent FeeVersion initialization in test function.The change to use
FeeVersion::first()in thetest_query_historicalfunction is consistent with the earlier modification in thesetupfunction. This maintains code consistency and simplifies the initialization process.
170-171: LGTM: Simplified FeeVersion initialization.The change to use
FeeVersion::first()directly is an improvement. It simplifies the code and is more efficient.Please verify if similar changes are needed elsewhere in the codebase for consistency. You can use the following script to check for other occurrences:
✅ Verification successful
Verified: Isolated FeeVersion initialization.
No other occurrences of
PlatformVersion::first().fee_versionwere found, indicating that the change is isolated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of PlatformVersion::first().fee_version rg "PlatformVersion::first\(\)\.fee_version" --type rustLength of output: 58
packages/rs-platform-version/src/version/v2.rs (1)
605-605: Approve renaming with a suggestion for impact verification.The renaming of
platform_state_for_saving_structuretoplatform_state_for_saving_structure_defaultlooks good. This change improves clarity by indicating that this is a default structure.To ensure this change doesn't have unintended consequences, please run the following script to check for any other occurrences of the old field name:
This will help verify that all relevant code has been updated to use the new field name.
✅ Verification successful
Renaming Verified Successfully.
All occurrences of
platform_state_for_saving_structurehave been updated toplatform_state_for_saving_structure_default. No references to the old field name were found outside the intended changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of the old field name # Search for the old field name in all Rust files rg --type rust "platform_state_for_saving_structure[^_]" # Search for the new field name to confirm it's used consistently rg --type rust "platform_state_for_saving_structure_default"Length of output: 1045
packages/rs-platform-version/src/version/v3.rs (1)
Line range hint
1-1043: Overall assessment: Minor change with potential wider implicationsThe change in this file is minimal, involving only the renaming of a single field in the
DriveAbciStructureVersionsstruct. While this change improves code clarity, it's important to consider its context within the largerPLATFORM_V3constant, which defines crucial version and configuration information for the platform.To ensure this change doesn't have unintended consequences, please run the following verification steps:
These checks will help ensure that the renaming has been consistently applied across the codebase and that any related tests have been updated accordingly.
✅ Verification successful
Verification Successful: Field Renaming Consistent
All references to
platform_state_for_saving_structurehave been correctly updated toplatform_state_for_saving_structure_defaultacross the codebase and associated tests. No issues or inconsistencies were found during the verification process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any references to the old field name and verify the usage of the new field name # Search for any remaining references to the old field name echo "Searching for old field name references:" rg "platform_state_for_saving_structure[^_]" --type rust # Search for uses of the new field name echo "Searching for new field name usage:" rg "platform_state_for_saving_structure_default" --type rust # Check for any tests that might need updating echo "Checking for potentially affected tests:" rg "platform_state_for_saving_structure" --type rust --glob "*test*.rs"Length of output: 1616
packages/rs-platform-version/src/version/fee/processing/mod.rs (3)
15-15: Verify the reintroduction ofperform_network_threshold_signingThe field
perform_network_threshold_signinghas been reintroduced in theFeeProcessingVersionstruct. Please ensure that this change is intentional and that all serialization and deserialization logic correctly handle this additional field, especially concerning backward compatibility.
33-50: Ensure accurate field mapping in the conversion implementationIn the
impl From<FeeProcessingVersionFieldsBeforeVersion1Point4> for FeeProcessingVersion, verify that all fields are correctly mapped from the old struct to the new one. Specifically, confirm thatperform_network_threshold_signingis set appropriately usingFEE_VERSION1.processing.perform_network_threshold_signing.
47-49: Confirm the default value's suitability for older dataSetting
perform_network_threshold_signingtoFEE_VERSION1.processing.perform_network_threshold_signingmay have implications for data deserialized from versions prior to 1.4. Please confirm that this default value is appropriate and does not introduce inconsistencies or errors with older platform states.packages/rs-platform-version/src/version/fee/mod.rs (4)
23-24: Addition ofFeeVersionNumbertype alias enhances clarityDefining
FeeVersionNumberas a type alias foru32improves code readability and maintainability by providing semantic meaning to the version numbers used.
25-26: Initialization ofFEE_VERSIONSconstant is appropriateThe
FEE_VERSIONSconstant is correctly initialized withFEE_VERSION1, setting up the framework for managing multiple fee versions.
93-105: Confirm hardcodedfee_version_numberin conversion from older versionsIn the
From<FeeVersionFieldsBeforeVersion4>implementation, thefee_version_numberis hardcoded to1. Please verify that this accurately represents the version of the older data and that this is the intended behavior.To ensure that all older serialized
FeeVersioninstances correspond to version1, consider checking the historical data or documentation.
Line range hint
29-38: Ensure backward compatibility with the addedfee_version_numberfieldAdding the
fee_version_numberfield to theFeeVersionstruct may affect serialization and deserialization of existing data. Please verify that backward compatibility is maintained and that older serialized data can be deserialized without errors.To confirm that deserialization handles the new field correctly, you can search for deserialization implementations of
FeeVersionusing the following script:packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (2)
7-8: Imports updated to includePlatformStateForSavingV1The new type
PlatformStateForSavingV1has been added to the imports, which is necessary for handling the new version in the platform state saving logic.
45-46: Added new variantV1toPlatformStateForSavingenumThe addition of
V1(PlatformStateForSavingV1)to thePlatformStateForSavingenum allows the code to handle a new version of platform state data. This maintains consistency in versioning and is appropriate for the extended functionality.packages/rs-drive-abci/src/execution/platform_events/block_fee_processing/tests.rs (3)
798-798: Check insertion intoprevious_fee_versionsIn
test_document_refund_after_10_epochs_on_different_fee_version_increasing_fees, ensure that inserting intoprevious_fee_versionswith key5is correct. Verify that the key corresponds to the intended epoch or version when the fee change occurred.Please confirm that the fee version
5aligns with the platform's versioning for fee changes.
Line range hint
356-359: Verify correctness of epoch advancementWhen fast-forwarding to a future block in
test_document_refund_after_a_year, ensure that the parameters accurately represent a year's advancement based on the platform's epoch duration.Run the following script to confirm the number of epochs in a year:
#!/bin/bash # Description: Verify the number of epochs corresponding to one year # Assuming epoch duration is known, calculate epochs per year echo "scale=2; (365 * 24 * 60 * 60) / <epoch_duration_in_seconds>" | bcReplace
<epoch_duration_in_seconds>with the actual epoch duration used in the platform.
Line range hint
275-280: Ensure consistent usage of platform stateIn the
process_state_transitionscall withintest_document_refund_immediate, theBlockInfo::default()is used. Confirm if the default block info accurately reflects the intended test conditions, especially regarding the block height and time.Run the following script to check for other tests that might need updated block info:
✅ Verification successful
The usage of
BlockInfo::default()is consistent across multiple test files, ensuring standardized block height and time settings in your tests. No issues were found regarding its usage in theprocess_state_transitionscalls withintest_document_refund_immediate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for `process_state_transitions` calls using `BlockInfo::default()` # Test: Find occurrences where `BlockInfo::default()` is used rg --type rust -A 3 $'process_state_transitions' | rg 'BlockInfo::default\(\)'Length of output: 241
Script:
#!/bin/bash # Description: Search for `process_state_transitions` calls using `BlockInfo::default()` # Test: Find occurrences where `BlockInfo::default()` is used in `process_state_transitions` calls rg --type rust 'process_state_transitions.*BlockInfo::default\(\)'Length of output: 68
Script:
#!/bin/bash # Description: Search for all usages of `BlockInfo::default()` in Rust files rg --type rust 'BlockInfo::default\(\)' -A 2Length of output: 114837
packages/rs-drive/src/drive/document/delete/mod.rs (1)
79-83: Ensure Consistent Fee Version InitializationBy replacing
PlatformVersion::first().fee_version.clone()withFeeVersion::first()in the initialization ofEPOCH_CHANGE_FEE_VERSION_TEST, ensure that the fee version used is consistent with the platform's intended configuration. IfFeeVersion::first()andPlatformVersion::first().fee_version.clone()return different values, this might lead to inconsistencies in fee calculations during tests.To confirm that both approaches yield the same fee version, consider comparing their returned values. Here's a shell script to check the outputs:
packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (4)
161-162: Update toprevious_fee_versionsinPlatformStateForSavingV0is AppropriateThe modification of
previous_fee_versionsto useCachedEpochIndexFeeVersionsFieldsBeforeVersion4aligns with the changes in fee version management and ensures backward compatibility.
164-197: Addition ofPlatformStateForSavingV1Struct Enhances SerializationThe new
PlatformStateForSavingV1struct correctly incorporates updated types forprevious_fee_versions, improving versioning and serialization capabilities during state transitions.
Line range hint
200-248:TryFrom<PlatformStateV0>Implementation forPlatformStateForSavingV1is CorrectThe conversion logic properly maps the fields from
PlatformStateV0toPlatformStateForSavingV1, including the transformation ofprevious_fee_versionsinto the new structure.
244-248: Efficient Mapping ofprevious_fee_versionsThe mapping from
CachedEpochIndexFeeVersionstoEpochIndexFeeVersionsForStorageby extractingfee_version_numberis handled correctly, ensuring data integrity.packages/rs-platform-version/src/version/v4.rs (1)
607-607: Ensure all references to the renamed field are updatedThe field
platform_state_for_saving_structurehas been renamed toplatform_state_for_saving_structure_default. Please verify that all usages of the old field name have been updated throughout the codebase to avoid any potential issues.Run the following script to check for any remaining references to
platform_state_for_saving_structure:✅ Verification successful
All references to
platform_state_for_saving_structurehave been successfully updated toplatform_state_for_saving_structure_default.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of `platform_state_for_saving_structure` excluding the current file # Search for the old field name in Rust files, excluding the current file under review rg 'platform_state_for_saving_structure' --type rust --ignore-case --glob '!packages/rs-platform-version/src/version/v4.rs'Length of output: 942
packages/rs-drive/src/drive/document/update/mod.rs (2)
68-68: Adding 'FeeVersion' ImportThe import of
FeeVersionfromdpp::version::feeis necessary due to its usage in the code below. This addition is appropriate and ensures that theFeeVersiontype is available for initializingEPOCH_CHANGE_FEE_VERSION_TEST.
73-73: Updating Initialization of 'EPOCH_CHANGE_FEE_VERSION_TEST'Replacing
PlatformVersion::first().fee_version.clone()withFeeVersion::first()simplifies the initialization by directly accessing the fee version without relying on the platform version. Ensure thatFeeVersion::first()returns the intended initial fee version to maintain consistent behavior.You can verify that both methods return the same fee version by comparing their implementations:
packages/rs-drive/tests/query_tests.rs (2)
78-78: Import ofFeeVersionIs Necessary and AppropriateThe addition of
use dpp::version::fee::FeeVersion;is appropriate asFeeVersionis utilized in the updated initializations ofepoch_change_fee_version_test. This import ensures the code compiles correctly with the new fee version handling.
2309-2309: Simplifyepoch_change_fee_version_testInitializationThe initialization of
epoch_change_fee_version_testhas been updated to useFeeVersion::first(), which simplifies the code. Ensure that this change is consistent with the application's fee versioning logic and that any dependencies are updated accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (3)
7-7: LGTM! Consider updating documentation for the new version.The addition of
PlatformStateForSavingV1and the newV1variant in thePlatformStateForSavingenum are good changes for introducing a new version while maintaining backward compatibility.Consider updating the documentation for
PlatformStateForSavingto explain the differences between V0 and V1, and when each version should be used.Also applies to: 45-46
141-145: LGTM! Consider improving error messages for clarity.The implementation correctly handles the new
PlatformStateForSavingV1format while maintaining compatibility with version 0 of the platform state structure. This aligns with the retroactive nature of the changes, as mentioned in the retrieved learning.Consider updating the error messages in both match arms to include more context about the version mismatch. For example:
Err(Error::Execution(ExecutionError::UnknownVersionMismatch { method: "PlatformState::try_from_platform_versioned(PlatformStateForSavingV1)".to_string(), known_versions: vec![0], received: version, additional_info: "Only version 0 of PlatformState is supported, regardless of PlatformStateForSaving version.".to_string(), })),This would provide more clarity about why only version 0 is handled, even for V1 serialization.
Also applies to: 177-193
551-571: LGTM! Consider expanding test coverage.The addition of this test module is valuable, as it verifies the deserialization process using real testnet data. This helps ensure compatibility with existing serialized states.
Consider the following improvements to enhance the test coverage:
- Add assertions to verify the deserialized state's contents, not just that it deserializes without errors.
- Include tests for error cases, such as attempting to deserialize an invalid or corrupted state.
- If possible, add tests for deserializing both V0 and V1 formats to ensure full compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (5 hunks)
- packages/rs-drive-abci/src/test/fixture/mod.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-drive-abci/src/test/fixture/mod.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive-abci/src/platform_types/platform_state/mod.rs (1)
Learnt from: QuantumExplorer PR: dashpay/platform#2227 File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141 Timestamp: 2024-10-08T13:28:03.529Z Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
|
@shumkov please review when you can. |
QuantumExplorer
changed the title
Issue being fixed or feature implemented
Because of how PlatformStateForSerialization was serialized it would store the fee version. Since fee version can include new fields this was a big no no.
What was done?
Modified PlatformState and introduced a new PlatformStateForSerializationV1 that fixes the issue and also serializes more efficiently.
We also changed validator sets to be serialized with Bincode instead of Serde.
How Has This Been Tested?
Ran tests.
Breaking Changes
Fixing an issue, not breaking.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
ValidatorV0andValidatorSetV0structs.Tests
ValidatorV0,ValidatorSetV0, and platform state structures.Documentation