fix(platform)!: load data contracts in their respective versions #2644
Conversation
WalkthroughThe changes introduce version-bound abstractions for contract configuration, enabling platform-version-aware conversions and serialization for data contracts. This includes new traits for infallible conversions, updates to contract version structures, and adjustments to test and helper functions to remove explicit platform version passing. System contract management is refactored for atomic updates and version tracking. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Drive
participant SystemDataContracts
participant ActiveSystemDataContract
Client->>Drive: open(path, config)
Drive->>SystemDataContracts: load_genesis_system_contracts()
SystemDataContracts->>ActiveSystemDataContract: new(contract, protocol_version)
ActiveSystemDataContract-->>SystemDataContracts: ActiveSystemDataContract instance
SystemDataContracts-->>Drive: SystemDataContracts instance
Drive-->>Client: (Drive, Option<PlatformVersion>)
sequenceDiagram
participant DataContract
participant SerializationFormatV0
participant SerializationFormatV1
participant PlatformVersion
DataContract->>SerializationFormatV0: from_platform_versioned(self, platform_version)
DataContract->>SerializationFormatV1: from_platform_versioned(self, platform_version)
Note over SerializationFormatV0,SerializationFormatV1: Config adjusted via config_valid_for_platform_version
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
QuantumExplorer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
packages/rs-drive/tests/query_tests.rs (7)
275-279: Duplicate of the previous remark – same change pattern.
347-351: Duplicate of the previous remark – same change pattern.
823-827: Duplicate of the previous remark – same change pattern.
871-875: Duplicate of the previous remark – same change pattern.
919-923: Duplicate of the previous remark – same change pattern.
957-961: Duplicate of the previous remark – same change pattern.
996-1000: Duplicate of the previous remark – same change pattern.
🧹 Nitpick comments (4)
packages/rs-drive/tests/query_tests.rs (1)
204-208: Signature-update looks correct, but double-check unused import
setup_driveis now invoked with the singleOption<DriveConfig>argument as required by the new signature – good catch.
Minor follow-ups:
- Make sure every other call site across the codebase was updated the same way (grep for
setup_drive().- The explicit import
use std::option::Option::None;(line 47) was previously needed to passNonebut is probably unused now – if it’s no longer required, consider dropping it to avoid a dead import.No functional issues observed here.
packages/rs-dpp/src/data_contract/serialized_version/v0/mod.rs (1)
91-146: Consider refactoring to reduce code duplication.The
FromPlatformVersioned<DataContract>implementation is functionally correct and adds the essential platform version-aware config validation viaconfig.config_valid_for_platform_version(platform_version). However, there's significant code duplication between this implementation and the existingFrom<DataContract>implementation (lines 38-89).Consider extracting the common conversion logic into a helper function that takes an optional platform version parameter to reduce maintenance burden.
Example refactor approach:
impl DataContractInSerializationFormatV0 { fn from_data_contract_with_optional_platform_version( value: DataContract, platform_version: Option<&PlatformVersion> ) -> Self { // Common implementation with conditional config processing } }Then both trait implementations could delegate to this helper.
packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs (1)
105-189: Functional implementation with code duplication concern.The
FromPlatformVersioned<DataContract>implementation correctly handles bothDataContract::V0andDataContract::V1variants with appropriate field mapping. Key improvements over the existingFromimplementation:
- Platform version-aware config validation via
config.config_valid_for_platform_version(platform_version)(lines 119, 163)- Proper handling of V1-specific fields when converting from V0 (defaulting to
NoneorDefault)However, similar to the v0 file, there's significant code duplication between this implementation and the existing
From<DataContract>implementation (lines 191-271).Consider applying the same refactoring approach suggested for the v0 file to extract common conversion logic and reduce maintenance overhead.
packages/rs-drive/src/cache/system_contracts.rs (1)
104-143: Consider documenting or using constants for the protocol versions.The hardcoded protocol versions (1 for most contracts, 9 for token_history and keyword_search) would benefit from either:
- Documentation explaining why these specific versions are used
- Named constants that make the versions more maintainable
Consider defining constants at the module level:
+/// Protocol version when most system contracts became active +const GENESIS_PROTOCOL_VERSION: ProtocolVersion = 1; + +/// Protocol version when token history and keyword search contracts became active +const TOKEN_HISTORY_PROTOCOL_VERSION: ProtocolVersion = 9; + /// System contracts pub struct SystemDataContracts {Then use these constants in the initialization:
- 1, + GENESIS_PROTOCOL_VERSION,- 9, + TOKEN_HISTORY_PROTOCOL_VERSION,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
packages/rs-dpp/src/data_contract/config/mod.rs(3 hunks)packages/rs-dpp/src/data_contract/config/v0/mod.rs(2 hunks)packages/rs-dpp/src/data_contract/serialized_version/mod.rs(7 hunks)packages/rs-dpp/src/data_contract/serialized_version/v0/mod.rs(2 hunks)packages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs(2 hunks)packages/rs-dpp/src/system_data_contracts.rs(2 hunks)packages/rs-drive-abci/src/main.rs(1 hunks)packages/rs-drive-abci/src/platform_types/platform/mod.rs(0 hunks)packages/rs-drive/src/cache/system_contracts.rs(2 hunks)packages/rs-drive/src/drive/credit_pools/epochs/operations_factory.rs(1 hunks)packages/rs-drive/src/drive/credit_pools/storage_fee_distribution_pool/get_storage_fees_from_distribution_pool/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/credit_pools/unpaid_epoch/get_unpaid_epoch_index/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/document/delete/mod.rs(1 hunks)packages/rs-drive/src/drive/document/update/mod.rs(4 hunks)packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/merge_identity_contract_nonce/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/key/fetch/mod.rs(4 hunks)packages/rs-drive/src/drive/identity/update/operations/merge_identity_nonce_operations/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/system/genesis_time/mod.rs(3 hunks)packages/rs-drive/src/open/mod.rs(1 hunks)packages/rs-drive/src/query/mod.rs(2 hunks)packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs(5 hunks)packages/rs-drive/src/util/grove_operations/batch_move/v0/mod.rs(3 hunks)packages/rs-drive/src/util/grove_operations/batch_move_items_in_path_query/v0/mod.rs(3 hunks)packages/rs-drive/src/util/test_helpers/setup.rs(2 hunks)packages/rs-drive/tests/deterministic_root_hash.rs(2 hunks)packages/rs-drive/tests/query_tests.rs(8 hunks)packages/rs-drive/tests/query_tests_history.rs(1 hunks)packages/rs-platform-version/src/lib.rs(1 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs(1 hunks)
💤 Files with no reviewable changes (1)
- packages/rs-drive-abci/src/platform_types/platform/mod.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
packages/rs-dpp/src/data_contract/config/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.
packages/rs-dpp/src/data_contract/serialized_version/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.
🧬 Code Graph Analysis (14)
packages/rs-drive/src/drive/credit_pools/epochs/operations_factory.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/drive/credit_pools/unpaid_epoch/get_unpaid_epoch_index/v0/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/drive/identity/update/operations/merge_identity_nonce_operations/v0/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/drive/system/genesis_time/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/util/grove_operations/batch_move_items_in_path_query/v0/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/util/grove_operations/batch_move/v0/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/merge_identity_contract_nonce/v0/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/drive/document/update/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/tests/deterministic_root_hash.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/drive/identity/key/fetch/mod.rs (1)
packages/rs-drive/src/util/test_helpers/setup.rs (1)
setup_drive(39-46)
packages/rs-drive/src/open/mod.rs (1)
packages/rs-drive/src/cache/system_contracts.rs (1)
load_genesis_system_contracts(104-144)
packages/rs-dpp/src/system_data_contracts.rs (1)
packages/rs-dpp/src/data_contract/config/mod.rs (1)
data_contract(114-136)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Formatting
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (rs-dapi-client) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Formatting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- 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 (38)
packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/mod.rs (1)
25-25: LGTM! Function call correctly updated to match new signature.The change correctly updates the
setup_drivecall to use the simplified signature that only takes an optionalDriveConfigparameter, removing the previously required platform version argument.packages/rs-drive/src/drive/credit_pools/epochs/operations_factory.rs (1)
429-429: LGTM! Function call correctly updated to match new signature.The change correctly updates the
setup_drivecall to align with the simplified function signature that only requires an optionalDriveConfigparameter.packages/rs-drive/src/drive/credit_pools/unpaid_epoch/get_unpaid_epoch_index/v0/mod.rs (1)
60-60: LGTM! Function call correctly updated to match new signature.The change correctly updates the
setup_drivecall to use the simplified signature, maintaining consistency with the broader refactoring effort.packages/rs-drive/src/drive/identity/update/operations/merge_identity_nonce_operations/v0/mod.rs (1)
180-180: LGTM! Test helper function correctly updated to match new signature.The change correctly updates the
setup_drivecall in thesetup_base_testhelper function to align with the simplified signature. This change will benefit all tests that use this helper function.packages/rs-drive/src/drive/credit_pools/storage_fee_distribution_pool/get_storage_fees_from_distribution_pool/v0/mod.rs (1)
55-55: LGTM! Consistent API simplification.The update to
setup_drive(None)correctly aligns with the simplified function signature that removes the explicit platform version parameter. This change maintains the test's functionality while contributing to a cleaner API design.packages/rs-drive/src/drive/identity/contract_info/identity_contract_nonce/merge_identity_contract_nonce/v0/mod.rs (1)
307-307: Excellent! Helper function updated consistently.The change to
setup_drive(None)in thesetup_base_testhelper function correctly implements the API simplification and benefits all the test functions that use this helper (includingmerge_identity_contract_nonce_with_bump,merge_identity_contract_nonce_0_is_invalid, andmerge_identity_contract_nonce_many_updates).packages/rs-drive/src/drive/identity/insert/add_new_identity/mod.rs (1)
193-193: Perfect update to simplified API.The change to
setup_drive(None)correctly implements the function signature update while preserving all test functionality for identity insertion and fetching validation across different platform versions.packages/rs-drive/src/util/grove_operations/batch_move_items_in_path_query/v0/mod.rs (1)
161-161: Excellent consistency across all test functions!All three test functions (
test_batch_move_items_in_path_query_success,test_batch_move_items_in_path_query_no_elements, andtest_batch_move_items_in_path_query_range_query) have been correctly updated to usesetup_drive(None), maintaining comprehensive test coverage for batch move operations while aligning with the simplified API design.Also applies to: 318-318, 365-365
packages/rs-drive/src/util/grove_operations/batch_move/v0/mod.rs (1)
134-134: LGTM! Function signature update is consistent with refactoring.The changes correctly update the
setup_drivecalls to match the new function signature that removes the platform version parameter. This aligns with the broader refactoring effort described in the PR objectives.Also applies to: 240-240, 288-288
packages/rs-drive/src/drive/system/genesis_time/mod.rs (1)
60-60: LGTM! Consistent function signature updates.The changes properly update
setup_drivecalls to use the new single-parameter signature, removing the platform version argument as part of the coordinated refactoring effort.Also applies to: 71-71, 137-137
packages/rs-drive/src/drive/document/update/mod.rs (1)
855-855: LGTM! Function signature updates maintain test functionality.The changes correctly update
setup_drivecalls fromsetup_drive(Some(config), None)tosetup_drive(Some(config)), properly removing the platform version parameter while preserving the drive configuration. This is consistent with the broader refactoring effort.Also applies to: 1162-1162, 1369-1369, 1712-1712
packages/rs-drive-abci/src/main.rs (1)
473-473: LGTM! Drive::open signature update is consistent with refactoring.The change correctly updates the
Drive::opencall to remove the platform version parameter, aligning with the broader codebase refactoring to simplify drive initialization by removing explicit platform version passing.packages/rs-drive/src/drive/document/delete/mod.rs (1)
92-93: LGTM! API simplification aligns with the broader refactor.The removal of the
PlatformVersionparameter from theDrive::openmethod call is consistent with the PR's objective to update data contract loading methods to handle version-specific logic internally. This simplifies the API while maintaining functionality.packages/rs-drive/src/query/mod.rs (2)
2324-2324: Use default platform version in Drive::open test helper
The call toDrive::openinsetup_family_contracthas been updated to passNoneinstead of an explicitPlatformVersion, in line with the refactored signature that uses an internal default. Please verify that the default platform version used internally matches the intended version for all downstream operations (e.g.,create_initial_state_structure).
2357-2357: Omit explicit platform version in withdrawal contract setup
Insetup_withdrawal_contract,Drive::opennow receivesNonefor the platform version argument. This aligns with the updated method signature. Confirm that this change does not introduce any versioning mismatches in the withdrawal contract tests.packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rs (1)
16-16: Excellent architectural improvement for version-bound contract configuration.The change from
FeatureVersiontoFeatureVersionBoundsenables more precise version control by allowing specification of min/max version ranges instead of a single version. This aligns perfectly with the PR objective of loading data contracts in their respective versions and provides better flexibility for version-specific handling.packages/rs-drive/tests/query_tests_history.rs (1)
171-171: LGTM! Function signature simplification improves maintainability.The removal of the second parameter (
Noneforspecific_platform_version) simplifies the test setup by relying on internal defaults for platform version handling. This change is consistent with the broader refactor across the codebase to remove explicit platform version passing, improving code maintainability.packages/rs-drive/tests/deterministic_root_hash.rs (1)
319-319: LGTM! Correct function signature update.The changes properly align with the
setup_drivefunction signature simplification, removing the second parameter as shown in the helper function definition.Also applies to: 338-338
packages/rs-drive/src/util/grove_operations/batch_delete_items_in_path_query/v0/mod.rs (1)
146-146: LGTM! Consistent test setup updates.All test functions correctly updated to use the new
setup_drivesignature. The changes are mechanical and preserve all existing test functionality.Also applies to: 237-237, 382-382, 458-458, 494-494
packages/rs-drive/src/drive/identity/key/fetch/mod.rs (1)
1049-1049: LGTM! Proper test setup function updates.All identity key fetch test functions correctly updated to match the simplified
setup_drivefunction signature. No functional changes to the tests themselves.Also applies to: 1085-1085, 1125-1125, 1165-1165
packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rs (1)
17-21: LGTM! Proper version bounds migration.The change from a simple integer to
FeatureVersionBoundsstructure enhances version handling capabilities while maintaining backward compatibility by setting all bounds to the previous value (0).packages/rs-dpp/src/data_contract/config/v0/mod.rs (2)
2-2: LGTM: Import addition supports version conversion.The import of
DataContractConfigV1is necessary for the new conversion implementation.
197-213: LGTM: Well-implemented backward compatibility conversion.The
From<DataContractConfigV1> for DataContractConfigV0implementation correctly enables version downgrade by performing direct field mapping. This is essential for the version-aware contract loading mechanism described in the PR objectives, allowing V1 configs to be used on platforms that only support V0.The implementation preserves all data without loss, which is appropriate for a backward compatibility conversion.
packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rs (1)
17-21: LGTM: Enhanced version handling with bounds.The migration from a simple config version to
FeatureVersionBoundsprovides better control over version compatibility. The bounds (min: 0, max: 1, default: 1) correctly support both V0 and V1 configurations with V1 as the current default, which aligns perfectly with the backward compatibility conversion implemented in the config module.This change supports the PR objective of loading data contracts in their respective versions by providing precise version range control.
packages/rs-dpp/src/data_contract/serialized_version/mod.rs (2)
20-20: LGTM! Import addition supports platform-versioned conversions.The addition of
IntoPlatformVersionedto the imports is necessary for the explicit platform-version-aware conversions introduced below.
163-164: Excellent refactoring to explicit platform-versioned conversions.The systematic replacement of
.into()calls with.into_platform_versioned(platform_version)makes the versioning semantics explicit and ensures proper version-aware serialization. This change is consistent across allTryFromPlatformVersionedimplementations and aligns with the new infallible conversion traits.Also applies to: 168-169, 195-196, 200-201, 227-228, 232-233, 259-260, 264-265, 291-292, 296-297, 323-324, 328-329
packages/rs-drive/src/open/mod.rs (1)
61-61: LGTM! Simplified system contracts loading aligns with refactoring.The removal of the platform version parameter from
SystemDataContracts::load_genesis_system_contracts()simplifies the API and delegates version management to internal abstractions, as shown in the implementation that now usesPlatformVersion::first()internally and wraps contracts inActiveSystemDataContract.packages/rs-dpp/src/data_contract/config/mod.rs (2)
33-38: LGTM! Version selection logic updated for new FeatureVersionBounds structure.The changes to use
platform_version.dpp.contract_versions.config.default_current_versioninstead of directly matching on the config field align correctly with the updatedFeatureVersionBoundstype that now provides structured bounds withmin_version,max_version, anddefault_current_version.Also applies to: 73-78, 118-123
49-67: Well-designed method for platform version compatibility.The
config_valid_for_platform_versionmethod provides a clean approach to handle version compatibility by:
- Preserving V0 configs unchanged
- Downgrading V1 to V0 only when the platform's max supported version is 0
- Otherwise preserving the original config
This logic ensures backward compatibility while respecting platform version constraints.
packages/rs-drive/src/util/test_helpers/setup.rs (2)
39-39: LGTM! Test helper simplified to align with Drive API changes.The removal of the
specific_platform_versionparameter and the corresponding update to theDrive::opencall simplifies the test setup API and aligns with the broader refactoring that delegates platform version management to internal Drive mechanisms.Also applies to: 42-42
53-56: Good adaptation of dependent function.The
setup_drive_with_initial_state_structurefunction correctly adapts to the simplifiedsetup_driveAPI while maintaining its own platform version handling logic for the initial state structure creation.packages/rs-dpp/src/data_contract/serialized_version/v0/mod.rs (1)
10-11: Good addition of platform version support imports.The imports for
PlatformVersionandFromPlatformVersionedare correctly added to support the new trait implementation.packages/rs-dpp/src/system_data_contracts.rs (2)
70-72: Good addition of serialization test imports.The imports for
DataContractInSerializationFormat,PlatformSerializableWithPlatformVersion, andTryIntoPlatformVersionedare correctly added to support the new serialization tests.
88-137:❓ Verification inconclusive
Comprehensive serialization test with helpful comments.
The test
serialize_withdrawal_contract_v1_vs_v9provides excellent coverage of the platform version-aware serialization behavior. The test correctly:
- Loads contracts for different platform versions and verifies they differ
- Serializes both contracts targeting the same platform version and verifies consistency
- Tests serialization byte length differences across versions
- Includes helpful comments explaining the expected behavior (lines 123, 136)
The hardcoded byte lengths (1747, 1757, 1758) might be somewhat brittle if serialization details change, but they provide valuable regression testing for the specific versioning behavior being implemented.
Consider verifying that the expected byte lengths remain stable across different environments:
🏁 Script executed:
#!/bin/bash # Description: Run the serialization test to verify byte length stability # Expected: Test should pass consistently cargo test serialize_withdrawal_contract_v1_vs_v9 -- --nocaptureLength of output: 17762
Confirm byte length stability locally
The
serialize_withdrawal_contract_v1_vs_v9test inpackages/rs-dpp/src/system_data_contracts.rs(lines 88–137) provides thorough coverage of version-aware serialization, including:
- Loading v1 vs v9 contracts and asserting they differ
- Serializing both against the same platform version to ensure consistent structure
- Verifying hardcoded byte-length differences (1747, 1757, 1758) for regression safety
Since the sandbox environment lacks a C linker (
cc), we couldn’t execute the test here. Please run the following locally to confirm the expected byte lengths remain stable across your environment:cargo test serialize_withdrawal_contract_v1_vs_v9 -- --nocapturepackages/rs-dpp/src/data_contract/serialized_version/v1/mod.rs (1)
17-18: Correct platform version imports added.The imports for
PlatformVersionandFromPlatformVersionedare properly added to support the new trait implementation.packages/rs-platform-version/src/lib.rs (1)
54-79: Well-designed infallible conversion traits.The introduction of
FromPlatformVersioned<T>andIntoPlatformVersioned<T>traits provides a clean, infallible alternative to the existing fallible conversion traits. Key strengths:
- Clear documentation: The traits are well-documented with their purpose and guarantees
- Ergonomic design: The automatic implementation of
IntoPlatformVersioned<U>for types whereU: FromPlatformVersioned<T>(lines 71-79) follows Rust's standard library patterns- Type safety: Infallible conversions ensure guaranteed valid results for supported versions
- Consistent API: Mirrors the existing fallible traits while providing deterministic behavior
This foundation enables the platform version-aware data contract serialization implemented in the other reviewed files.
packages/rs-drive/src/cache/system_contracts.rs (2)
56-68: LGTM! Clean refactoring to use the new abstraction.The consistent replacement of
ArcSwap<DataContract>withActiveSystemDataContractacross all system contract fields provides a uniform interface for version-aware contract management.
92-98: LGTM! Correct usage of the new store method.The removal of explicit
Arc::new()wrapping is correct sinceActiveSystemDataContract::storealready handles the Arc wrapping internally. This simplifies the code and avoids double-wrapping.
Issue being fixed or feature implemented
Fixes the loading mechanism for data contracts to ensure they are loaded in the version they appear in.
What was done?
Updated the methods responsible for loading data contracts to respect their versioning. This includes modifications to the
from_platform_versionedmethods and adjustments in theActiveSystemDataContractstruct to handle version-specific logic.Changed it so data contracts would use the correct config structure when serializing. A data contract in V1 serializing to storage structure V0 was using config structure V1, when it should have been using config structure V0.
How Has This Been Tested?
Changes were verified through existing unit tests that validate the loading and serialization of data contracts. Additional tests were added to cover edge cases related to version handling.
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests