feat(dojo-core): make the schema upgrade less strict #2925
Conversation
WalkthroughOhayo, sensei! This pull request introduces new enumerations and structures to enhance the Dojo framework's model handling and upgradeability testing. It adds two enumerations, Changes
Possibly related PRs
Suggested Labels
Ohayo, sensei! I hope this breakdown helps illuminate the intricate dance of code evolution in this pull request! 🍵🥷 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/core/src/meta/introspect.cairo (1)
42-85: Optimizeis_an_upgrade_ofLogic inPrimitiveCompareImplThe
is_an_upgrade_ofmethod correctly implements the allowed upgrades for primitive types. However, consider refactoring theallowed_upgradesinto a mapping structure, such as a dictionary or hashmap. This would improve lookup efficiency and enhance code readability by allowing direct access to the allowed upgrades for a given type.crates/dojo/core-cairo-test/src/tests/world/model.cairo (1)
73-76: Consider adding more variants for comprehensive testing.While the single variant enum works for basic testing, adding more variants would help test the full range of enum upgrade scenarios mentioned in the PR objectives.
enum AnotherEnum { X: bool, + Y: u8, + Z: felt252, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(3 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(3 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-hurl
- GitHub Check: ensure-docker
- GitHub Check: dojo-core-test
- GitHub Check: dojo-examples-test
🔇 Additional comments (23)
crates/dojo/core/src/meta/introspect.cairo (5)
38-40: Introduction ofTyCompareTrait<T>Ohayo sensei! The addition of the
TyCompareTrait<T>trait provides a unified approach to type comparison across different type implementations. This enhances the flexibility and maintainability of the type upgrade logic.
87-119: Robust Type Comparison inTyCompareImplThe implementation of
is_an_upgrade_offor theTyenum effectively handles type comparisons for various cases, including primitives, structs, arrays, tuples, byte arrays, and enums. This comprehensive approach ensures accurate validation of type upgrades.
121-148: Correct Handling of Enum Upgrades inEnumCompareImplOhayo sensei! The
EnumCompareImplproperly allows the addition of new variants at the end of an enum and accepts renaming variants without affecting storage. This aligns with the goal of making schema upgrades less strict while maintaining data integrity.
Line range hint
150-165: Efficient Comparison inStructCompareImplThe
StructCompareImplaccurately compares struct definitions by checking names, attributes, and ensuring that the new struct has at least as many members as the old one. The use of theis_an_upgrade_ofmethod on each member maintains consistency in the comparison logic.
174-178: Simplified Member Comparison inMemberCompareImplThe
MemberCompareImplprovides a straightforward comparison of struct members by verifying the name, attributes, and type compatibility. This ensures that member changes adhere to the upgrade rules.crates/dojo/core/src/lib.cairo (1)
32-32: Update Export toTyCompareTraitOhayo sensei! Changing the export from
StructCompareTraittoTyCompareTraitreflects the newly introduced trait and ensures that the correct comparison functionality is accessible throughout the codebase.crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (5)
57-60: Addition ofMyEnumfor Enum Upgrade TestingOhayo sensei! Introducing
MyEnumenhances the test coverage by providing an enum to test valid type upgrades within models. This is a valuable addition for validating enum upgrade logic.
62-69: Definition ofFooModelMemberChangedfor Model Upgrade TestingThe
FooModelMemberChangedstruct usesMyEnumfor the fielda, creating a test case to verify that changing a member's type to a compatible enum is handled correctly during model upgrades.
71-74: Addition ofAnotherEnumfor Invalid Upgrade TestingAdding
AnotherEnumsets up a scenario to test illegal changes in model upgrades, specifically when an enum is changed to an incompatible type.
76-83: Definition ofFooModelMemberIllegalChangeto Test Invalid UpgradesOhayo sensei! Defining
FooModelMemberIllegalChangewith the fieldaof typeAnotherEnumprovides a critical test case to ensure that the upgrade mechanism correctly rejects incompatible type changes in models.
96-97: Including New Models indeploy_world_for_model_upgradesThe models
FooModelMemberChangedandFooModelMemberIllegalChangeare appropriately added to thenamespace_defresources. This inclusion ensures that the new test cases are integrated into the world deployment for comprehensive testing.crates/dojo/core-cairo-test/src/tests/world/model.cairo (5)
58-62: Ohayo sensei! The enum definition looks good!The enum variants follow a natural progression from
u8tou16, which aligns with the PR's objective of allowing upgrades to larger primitive types.
64-71: LGTM! Well-structured model for testing upgrades.The model includes both a key field and test fields, providing good coverage for upgrade scenarios.
78-85: LGTM! Good negative test case model.The model is well-designed for testing illegal type changes, using the same structure as
FooModelMemberChangedbut withAnotherEnumthat should fail the upgrade.
225-251: Well-structured test for valid model upgrades!The test thoroughly verifies:
- Event emission
- Correct selector calculation
- Address updates
- Class hash verification
304-314: LGTM! Good negative test case.The test properly verifies that illegal type changes are rejected during model upgrades.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (6)
312-362: Ohayo! Excellent base type compatibility matrix!The test thoroughly verifies that type upgrades are only allowed between compatible types and prevents cross-type upgrades.
364-420: Comprehensive primitive type upgrade rules, sensei!The test covers all primitive type upgrade paths, ensuring:
- Smaller integers can upgrade to larger ones
felt252can upgrade toClassHashandContractAddress- Bidirectional compatibility between
ClassHashandContractAddress
422-499: Thorough struct upgrade validation!The test covers all struct upgrade scenarios:
- Name changes (rejected)
- Attribute changes (rejected)
- Member name changes (rejected)
- Member attribute changes (rejected)
- Member type upgrades (allowed)
- New member additions (allowed)
501-541: Well-designed enum upgrade tests!The test verifies that:
- Name changes are rejected
- Attribute changes are rejected
- Variant name changes are allowed (aligns with PR objectives)
- Variant type upgrades are allowed
- New variant additions are allowed
543-560: Good tuple upgrade validation!The test ensures:
- Tuple items can be upgraded to compatible types
- Length changes are rejected
- Incompatible type changes are rejected
562-573: Clean array upgrade tests!The test verifies that array item types follow the same upgrade rules as other types.
crates/dojo/core/src/world/world_contract.cairo (1)
45-45: Good trait replacement, sensei!The change from
StructCompareTraittoTyCompareTraitaligns with the PR's objective of implementing less strict schema upgrades.
remybar
force-pushed
the
feat-core-less_strict_layout_update
branch
from
9ac93ff to
663dafd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/dojo/core/src/meta/introspect.cairo (1)
Line range hint
150-173: Add a safety check for field name uniqueness, sensei!While the implementation correctly handles struct upgrades, it should verify that field names are unique to prevent potential storage conflicts.
impl StructCompareImpl of TyCompareTrait<Struct> { fn is_an_upgrade_of(self: @Struct, old: @Struct) -> bool { if self.name != old.name || self.attrs != old.attrs || (*self.children).len() < (*old.children).len() { return false; } + // Verify field name uniqueness + let mut i = 0; + loop { + if i >= (*self.children).len() { + break; + } + let mut j = i + 1; + loop { + if j >= (*self.children).len() { + break; + } + if self.children[i].name == self.children[j].name { + return false; + } + j += 1; + } + i += 1; + }
🧹 Nitpick comments (6)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (4)
57-60: Ohayo! The enum looks good, sensei! Consider adding documentation.The enum is well-structured for testing schema upgrades. Consider adding documentation to explain its role in testing variant additions and renames.
#[derive(Introspect, Copy, Drop, Serde)] +/// Test enum for validating schema upgrades with variant changes enum MyEnum { X: u8, }
62-69: The model structure is solid, sensei! Let's add some docs.The model is well-designed for testing member type changes. Consider adding documentation to explain its role in schema upgrade tests.
#[derive(Introspect, Copy, Drop, Serde)] +/// Test model for validating schema upgrades with member type changes #[dojo::model] struct FooModelMemberChanged { #[key] pub caller: ContractAddress, pub a: MyEnum, pub b: u128, }
71-74: Consider a more descriptive name, sensei!While the enum structure is correct, the name
AnotherEnumcould better reflect its purpose in testing illegal type changes.#[derive(Introspect, Copy, Drop, Serde)] +/// Test enum for validating illegal schema upgrades -enum AnotherEnum { +enum IncompatibleEnum { X: u8, }
76-83: Well-structured for testing invalid upgrades, sensei!The model effectively tests illegal type changes. Consider adding documentation and updating the type if
AnotherEnumis renamed.#[derive(Introspect, Copy, Drop, Serde)] +/// Test model for validating illegal schema upgrades #[dojo::model] struct FooModelMemberIllegalChange { #[key] pub caller: ContractAddress, - pub a: AnotherEnum, + pub a: IncompatibleEnum, pub b: u128, }crates/dojo/core/src/meta/introspect.cairo (2)
87-119: Consider optimizing the array comparison logic, sensei!The array comparison at line 92 could be simplified using pattern matching instead of direct indexing:
- (Ty::Array(n), Ty::Array(o)) => { (*n).at(0).is_an_upgrade_of((*o).at(0)) }, + (Ty::Array(n), Ty::Array(o)) => { + match (n.get(0), o.get(0)) { + (Option::Some(n), Option::Some(o)) => n.is_an_upgrade_of(o), + _ => false + } + },This change would:
- Handle empty arrays gracefully
- Be more idiomatic Cairo code
- Avoid potential panic on empty spans
121-148: Add more documentation to explain the enum upgrade rules, sensei!The implementation correctly allows:
- Adding new variants at the end
- Renaming variants (as noted in the comment)
- Upgrading variant types
Consider adding doc comments to explain:
+/// Implements upgrade rules for enums: +/// - New variants can be added at the end +/// - Existing variants can be renamed +/// - Variant types must follow upgrade rules impl EnumCompareImpl of TyCompareTrait<Enum> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (7)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(3 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(3 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/dojo_dev.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/core-cairo-test/src/tests/world/model.cairo
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (1)
96-97: Clean addition of new test models, sensei!The new models are properly integrated into the test world deployment.
crates/dojo/core/src/meta/introspect.cairo (2)
38-85: Ohayo! The primitive type upgrade paths look solid, sensei!The implementation allows safe upgrades to larger types while maintaining the same felt size, which aligns well with the PR objectives. The upgrade paths are comprehensive and well-defined.
A few notable upgrade paths worth highlighting:
- Unsigned integers follow a natural progression: u8 → u16 → u32 → u64 → u128 → felt252
- Signed integers follow a similar path: i8 → i16 → i32 → i64 → i128 → felt252
- Special handling for ContractAddress ↔ ClassHash ↔ felt252 conversions
174-178: Clean and focused implementation, sensei!The member comparison logic correctly checks all required aspects:
- Name equality
- Attributes equality
- Type compatibility
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
312-573: Excellent test coverage, sensei!The test suite thoroughly covers all upgrade scenarios:
- Type compatibility matrix (test_introspect_upgrade)
- Primitive type upgrades (test_primitive_upgrade)
- Struct upgrades with field additions (test_struct_upgrade)
- Enum upgrades with variant changes (test_enum_upgrade)
- Tuple and array upgrades (test_tuple_upgrade, test_array_upgrade)
There was a problem hiding this comment.
Amazing, thanks for the work here @remybar and emphasis on the tests.
Some use-cases that would be good to confirm:
- For the keys, it should be also supported if we stay in the same serialization scheme. However, for a structure, the serialization will change. So maybe we should forbid the upgrade of keys.
- It seems that we can upgrade a model that derives
IntrospectPacked, is there a way to avoid that and return error if this is attempted? SinceLayout::Fixedis used, wondering if we have a way to detect this early. - Enums without type (using the unit type) are not upgradeable to any type. It seems to work fine, wanted to confirm that, since in DB + storage it's very different.
Other than that, it will be very nice having those two upgrades, enum variants + casting to wider (and compatible) types.
I don't understand this point. What do you mean by "the serialization will change" ? 🤔 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/dojo/core/src/meta/introspect.cairo (1)
98-147: Consider optimizing the primitive type mapping, sensei!The current implementation has two potential issues:
- The series of if-statements could be replaced with a more efficient lookup mechanism.
- Returning 0xFFFFFFFF for unknown types could lead to out-of-bounds access in the ALLOWED_PRIMITIVE_UPGRADES matrix.
Consider using a match expression or a static lookup table for better performance. Also, consider returning an Option to handle unknown types more safely:
-fn primitive_to_index(primitive: felt252) -> u32 { +fn primitive_to_index(primitive: felt252) -> Option<u32> { if primitive == 'bool' { - return 0; + return Option::Some(0); } // ... other cases ... - return 0xFFFFFFFF; + return Option::None; }crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (5)
313-363: Ohayo! Consider refactoring for better maintainability, sensei!The test matrix could be more maintainable by extracting the repetitive pattern into a helper function:
+fn assert_upgrade_compatibility(source: @Ty, targets: Span<Ty>, expected: bool) { + for target in targets { + assert!( + target.is_an_upgrade_of(source) == expected, + "Unexpected upgrade compatibility" + ); + } +} #[test] fn test_introspect_upgrade() { let p = Ty::Primitive('u8'); let s = Ty::Struct(Struct { name: 's', attrs: [].span(), children: [].span() }); let e = Ty::Enum(Enum { name: 'e', attrs: [].span(), children: [].span() }); let t = Ty::Tuple([Ty::Primitive('u8')].span()); let a = Ty::Array([Ty::Primitive('u8')].span()); let b = Ty::ByteArray; - - assert!(p.is_an_upgrade_of(@p)); - assert!(!p.is_an_upgrade_of(@s)); - // ... more assertions + + let all_types = array![p, s, e, t, a, b].span(); + assert_upgrade_compatibility(@p, all_types, false); + // Set the first element's expected to true for self-upgrade
365-425: Add documentation to clarify upgrade rules, sensei!The primitive type upgrade rules are comprehensive but could benefit from documentation explaining the rationale. Consider adding comments to explain:
- Why certain upgrades are allowed (e.g.,
u8tou16)- The relationship between
felt252,ClassHash, andContractAddress- The bidirectional upgrade paths
427-504: Consider adding more test cases for struct upgrades, sensei!The struct upgrade tests are thorough but could be enhanced with:
- Test cases for removing struct members (should fail)
- Test cases for nested struct upgrades
- Test cases for upgrading multiple members simultaneously
579-590: Consider adding more array upgrade test cases, sensei!While the basic cases are covered, consider adding tests for:
- Arrays of structs
- Arrays of enums
- Nested arrays
- Arrays with complex generic types
592-598: Enhance performance testing coverage, sensei!Consider adding more performance test cases:
- Test worst-case upgrade paths
- Test upgrade chains (e.g., u8 -> u16 -> u32)
- Add gas usage assertions to catch performance regressions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(5 hunks)crates/dojo/core/src/meta/introspect.cairo(4 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/world_contract.cairo(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/core/src/world/world_contract.cairo
🔇 Additional comments (11)
crates/dojo/core/src/meta/introspect.cairo (4)
4-96: Ohayo! Let's verify the primitive type upgrade paths.The upgrade matrix is well-structured, but we should verify a few aspects:
- The upgrade paths for
felt252,ClassHash, andContractAddressbeing interchangeable might need additional validation to ensure data integrity.- The upgrade paths for signed integers (
i8->i128) might need bounds checking.Consider adding documentation about:
- The rationale behind allowing/disallowing specific upgrades
- Any potential data loss scenarios
- Runtime validation requirements
183-200: Ohayo! The type comparison trait looks solid!The trait design and primitive type comparison implementation are well-structured and extensible.
236-267: Excellent implementation of enum comparison restrictions!The implementation correctly enforces that enum variants cannot be renamed or reordered, only new variants can be added at the end. This aligns with the previous feedback and prevents potential issues with variant mismatches.
Line range hint
202-297: The trait implementations are well-crafted, sensei!All type comparison implementations follow a consistent pattern and correctly handle their respective cases.
crates/dojo/core/src/world/errors.cairo (1)
85-87: Ohayo! New error message looks good!The error message for packed layouts is clear and follows the existing error message pattern.
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (2)
58-84: Well-structured test models for upgrade scenarios!The test models and enums provide good coverage for testing both legal and illegal upgrade scenarios.
104-110: Good addition of test data initialization!Writing model values before upgrade tests helps verify data integrity during upgrades.
crates/dojo/core-cairo-test/src/tests/world/model.cairo (2)
235-267: Excellent test coverage for successful upgrades!The test verifies both the upgrade process and data integrity after the upgrade.
321-331: Good negative test case for illegal changes!The test properly verifies that illegal type changes are rejected.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (2)
506-558: Excellent test coverage for enum upgrades!The test cases align perfectly with the PR objectives, particularly:
- Allowing new variants at the end (line 539-546)
- Preventing upgrades of unit variants to data variants (line 553-558)
560-577: Well-structured tuple upgrade tests!The test cases cover all essential scenarios for tuple upgrades.
Keys can be anything that implements Let's take this example: struct MyModel {
#[key]
token_id: u256,
d: u32,
}This is valid and accepted by Torii. The key representation in Torii will be So for this reason, we may have structs as keys, which is very complex to update. IMHO on keys, we should only allow primitive updates and adding variant enums (since there are already projects using enums as keys). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (4)
365-425: Consider optimizing the primitive upgrade checks, sensei!While the test is thorough, the nested loop structure could be optimized. Consider using a HashMap for O(1) lookup of allowed upgrades instead of the current O(n) search through the allowed list.
- let mut allowed_upgrades: Span<(felt252, Span<felt252>)> = [ + use core::hash::HashMap; + let mut allowed_upgrades = HashMap::<felt252, Span<felt252>>::new(); - loop { - match allowed_upgrades.pop_front() { - Option::Some((src, allowed)) => { - for dest in primitives { - let expected = if src == dest { - true - } else { - let allowed = *allowed; - let mut i = 0; - loop { - if i >= allowed.len() { - break false; - } - if *allowed.at(i) == *dest { - break true; - } - i += 1; - } - }; + for src in primitives { + for dest in primitives { + let expected = if src == dest { + true + } else { + allowed_upgrades.get(src).map_or(false, |allowed| allowed.contains(dest)) + };
506-558: Consider adding variant reordering test case, sensei!The enum upgrade tests are thorough, but consider adding a test case for variant reordering to ensure the order of variants is preserved during upgrades.
+ // variant reordering + let mut upgraded = e; + upgraded.children = [('y', Ty::Primitive('u16')), ('x', Ty::Primitive('u8'))].span(); + assert!(!upgraded.is_an_upgrade_of(@e), "variant reordering");
579-590: Consider adding nested array upgrade test, sensei!While the basic array upgrade tests are good, consider adding a test case for nested array upgrades to ensure they work correctly.
+ // nested array upgrade + let nested = Ty::Array([Ty::Array([Ty::Primitive('u8')].span())].span()); + let upgraded = Ty::Array([Ty::Array([Ty::Primitive('u16')].span())].span()); + assert!(upgraded.is_an_upgrade_of(@nested));
592-598: Consider expanding performance tests, sensei!The performance test could be more comprehensive. Consider adding more upgrade paths to get a better understanding of performance characteristics across different scenarios.
#[test] #[available_gas(300000)] fn test_primitive_upgrade_performance() { let gas = GasCounterTrait::start(); let _ = Ty::Primitive('ClassHash').is_an_upgrade_of(@Ty::Primitive('ContractAddress')); gas.end("Upgrade from ContractAddress to ClassHash"); + + let gas = GasCounterTrait::start(); + let _ = Ty::Primitive('u128').is_an_upgrade_of(@Ty::Primitive('u8')); + gas.end("Upgrade from u8 to u128"); + + let gas = GasCounterTrait::start(); + let _ = Ty::Primitive('felt252').is_an_upgrade_of(@Ty::Primitive('i128')); + gas.end("Upgrade from i128 to felt252"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/spawn-and-move/dojo_dev.toml
🔇 Additional comments (3)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (3)
313-363: Ohayo! Nice comprehensive type compatibility matrix, sensei!The test thoroughly validates type upgrade compatibility between different types using a well-structured matrix approach. Each type is tested against all other types, ensuring complete coverage.
427-504: Excellent struct upgrade test coverage, sensei!The test comprehensively covers all struct upgrade scenarios including name changes, attribute changes, and member modifications. The test cases are well-organized with clear error messages.
560-577: Clean and concise tuple upgrade tests, sensei!The test effectively covers tuple upgrade scenarios including item upgrades and length changes. The test cases are well-structured with clear error messages.
remybar
force-pushed
the
feat-core-less_strict_layout_update
branch
from
2ca7337 to
a6870e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/core/src/meta/introspect.cairo (2)
98-147: Consider using a match statement for better readability and performance.The current if-else chain could be replaced with a match statement for a more idiomatic and potentially more efficient implementation.
-fn primitive_to_index(primitive: felt252) -> u32 { - if primitive == 'bool' { - return 0; - } - if primitive == 'u8' { - return 1; - } - // ... more if statements ... - return 0xFFFFFFFF; -} +fn primitive_to_index(primitive: felt252) -> u32 { + match primitive { + 'bool' => 0, + 'u8' => 1, + 'u16' => 2, + 'u32' | 'usize' => 3, + 'u64' => 4, + 'u128' => 5, + 'u256' => 6, + 'i8' => 7, + 'i16' => 8, + 'i32' => 9, + 'i64' => 10, + 'i128' => 11, + 'felt252' => 12, + 'ClassHash' => 13, + 'ContractAddress' => 14, + _ => 0xFFFFFFFF, + } +}
202-234: Array comparison could be more robust.The array comparison at line 207 only checks the first element's type. While this works for homogeneous arrays, consider adding a comment explaining this assumption.
- (Ty::Array(n), Ty::Array(o)) => { (*n).at(0).is_an_upgrade_of((*o).at(0)) }, + // Arrays are homogeneous, so checking the first element type is sufficient + (Ty::Array(n), Ty::Array(o)) => { (*n).at(0).is_an_upgrade_of((*o).at(0)) },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core/src/meta/introspect.cairo(3 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/spawn-and-move/dojo_dev.toml
🔇 Additional comments (6)
crates/dojo/core/src/meta/introspect.cairo (5)
4-96: Ohayo sensei! The primitive upgrade matrix looks good!The matrix correctly implements the upgrade rules, allowing:
- Upgrades to larger primitive types (e.g., u8 → u16 → u32)
- Upgrades to felt252 for most types
- Interchangeable upgrades between felt252, ClassHash, and ContractAddress
183-185: Clean and focused trait definition!The trait provides a clear interface for type upgradeability checks.
187-200: Solid implementation of primitive type comparison!The implementation correctly:
- Handles identity upgrades
- Uses the upgrade matrix to validate type changes
236-267: Excellent implementation of enum comparison!The implementation correctly follows the requirements:
- Allows adding new variants at the end
- Prevents variant renaming (as discussed in previous reviews)
- Ensures no variants are removed
293-324: Robust implementation of member comparison with key handling!The implementation correctly handles key members by:
- Allowing primitive and enum upgrades
- Requiring exact matches for struct, array, tuple, and byte array keys
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
313-741: Excellent test coverage, sensei!The test suite is comprehensive and well-structured, covering:
- All upgrade scenarios
- Edge cases and invalid upgrades
- Performance implications
remybar
force-pushed
the
feat-core-less_strict_layout_update
branch
from
a6870e8 to
4cf1f35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (1)
72-84: Consider documenting the illegal change scenario.While the models are well-structured, adding a comment explaining what makes this change "illegal" would improve test clarity.
#[derive(Introspect, Copy, Drop, Serde)] enum AnotherEnum { + // This enum has a different primitive type (bool) compared to MyEnum (u8), + // which makes it an illegal change for testing purposes X: bool, }crates/dojo/core/src/meta/introspect.cairo (2)
4-96: Ohayo sensei! Consider using a more maintainable format for the upgrade matrix.While the matrix is comprehensive, it could be more maintainable. Consider generating it programmatically using a build script or using a more compact representation.
Example build script approach:
def generate_matrix(): types = ['bool', 'u8', 'u16', 'u32', 'u64', 'u128', 'u256', 'i8', 'i16', 'i32', 'i64', 'i128', 'felt252', 'ClassHash', 'ContractAddress'] upgrades = { 'u8': ['u16', 'u32', 'u64', 'u128', 'felt252'], 'u16': ['u32', 'u64', 'u128', 'felt252'], # ... rest of the rules } print("const ALLOWED_PRIMITIVE_UPGRADES: [[bool; 15]; 15] = [") for src in types: allowed = upgrades.get(src, []) row = [t in allowed or t == src for t in types] print(f" [{', '.join(str(x).lower() for x in row)}],") print("];")
98-147: Consider using a match expression for better performance.The current implementation uses multiple if-else statements. A match expression would be more idiomatic and potentially more efficient.
-fn primitive_to_index(primitive: felt252) -> u32 { - if primitive == 'bool' { - return 0; - } - if primitive == 'u8' { - return 1; - } - // ... rest of the conditions +fn primitive_to_index(primitive: felt252) -> u32 { + match primitive { + 'bool' => 0, + 'u8' => 1, + 'u16' => 2, + 'u32' | 'usize' => 3, + // ... rest of the cases + _ => 0xFFFFFFFF, + } }crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
592-598: Consider adding more performance scenarios.While the current performance test is good, consider adding more scenarios to better understand the performance characteristics.
#[test] #[available_gas(300000)] fn test_primitive_upgrade_performance() { let gas = GasCounterTrait::start(); let _ = Ty::Primitive('ClassHash').is_an_upgrade_of(@Ty::Primitive('ContractAddress')); gas.end("Upgrade from ContractAddress to ClassHash"); + + // Test performance of longest upgrade chain + let gas = GasCounterTrait::start(); + let _ = Ty::Primitive('felt252').is_an_upgrade_of(@Ty::Primitive('u8')); + gas.end("Upgrade from u8 to felt252 (longest chain)"); + + // Test performance of struct with multiple members + let gas = GasCounterTrait::start(); + let complex_struct = Struct { + name: 's', + attrs: [].span(), + children: [ + Member { name: 'x', attrs: [].span(), ty: Ty::Primitive('u8') }, + Member { name: 'y', attrs: [].span(), ty: Ty::Primitive('u16') }, + Member { name: 'z', attrs: [].span(), ty: Ty::Primitive('u32') }, + ].span(), + }; + let _ = complex_struct.is_an_upgrade_of(@complex_struct); + gas.end("Upgrade complex struct"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (8)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(5 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(3 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/world_contract.cairo(2 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/world/errors.cairo
- examples/spawn-and-move/dojo_dev.toml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (17)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (2)
58-70: LGTM! Well-structured test models for member type changes.The
MyEnumandFooModelMemberChangedmodels are well-designed for testing member type upgrades, with appropriate attributes and a clear structure.
97-98: LGTM! Good test data initialization.The test setup is comprehensive:
- Models are properly registered in the test world
- Test data is initialized with appropriate values
Also applies to: 104-111
crates/dojo/core-cairo-test/src/tests/world/model.cairo (5)
59-63: LGTM! Well-defined test enum.The
MyEnumis properly defined withPartialEqfor test assertions and includes an upgrade path fromu8tou16.
65-72: LGTM! Good test coverage for type changes.The test models cover both valid and invalid upgrade scenarios:
FooModelMemberChanged: Tests valid type upgradesAnotherEnumandFooModelMemberIllegalChange: Tests invalid type changesAlso applies to: 74-77, 79-86
228-233: LGTM! Thorough value verification.Good addition of value verification after the upgrade to ensure data integrity is maintained.
235-267: LGTM! Comprehensive test for member changes.The test thoroughly verifies:
- Event emission
- Model selector
- Class hash
- Contract address
- Data persistence
321-331: LGTM! Good negative test case.The test properly verifies that illegal changes are rejected with appropriate error messages.
crates/dojo/core/src/meta/introspect.cairo (4)
183-185: LGTM! Clean trait definition and implementation.The trait and its implementation for primitive types are well-designed and handle the upgrade matrix efficiently.
Also applies to: 187-200
202-234: LGTM! Comprehensive type comparison implementation.The implementation handles all type variants correctly and maintains proper recursion for nested types.
236-267: LGTM! Proper enum upgrade validation.The implementation correctly enforces:
- Name preservation
- Attribute preservation
- No variant reordering
- Type compatibility
293-324: LGTM! Careful handling of key members.The implementation properly handles key members with stricter rules for upgrades.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (6)
313-363: LGTM! Thorough type compatibility matrix test.The test comprehensively verifies upgrade compatibility between all type variants.
365-425: LGTM! Extensive primitive type upgrade test.The test thoroughly verifies all allowed and disallowed primitive type upgrades.
427-504: LGTM! Comprehensive struct upgrade test.The test covers all struct upgrade scenarios:
- Name changes
- Attribute changes
- Member changes
- New members
506-558: LGTM! Thorough enum upgrade test.The test properly verifies:
- Variant name preservation
- Attribute preservation
- Type compatibility
- New variants
560-577: LGTM! Good coverage of container types.The tests properly verify upgrade rules for tuple and array types.
Also applies to: 579-590
600-741: LGTM! Thorough key member upgrade test.The test comprehensively verifies upgrade rules for key members across all type variants.
I see, thanks for the explanation, I was not aware of the end of the 'one felt' limitation 👍 If it's fine for you, let's limit key member upgrades to primitives and enums as an enum may evolve when the project evolves. But, as you said, we allow variant adding only 👍 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2925 +/- ##
==========================================
- Coverage 56.89% 56.86% -0.04%
==========================================
Files 423 423
Lines 56070 56097 +27
==========================================
- Hits 31902 31900 -2
- Misses 24168 24197 +29 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/core/src/meta/introspect.cairo (2)
4-96: Ohayo sensei! Consider adding documentation for the upgrade matrix.The
ALLOWED_PRIMITIVE_UPGRADESmatrix would benefit from additional documentation explaining:
- The rationale behind allowed upgrades
- The impact of upgrades on serialization
- Examples of valid and invalid upgrade paths
98-147: Consider using a match expression for better readability.The long if-else chain could be replaced with a match expression, and the magic number could be a named constant.
+const INVALID_TYPE_INDEX: u32 = 0xFFFFFFFF; fn primitive_to_index(primitive: felt252) -> u32 { - if primitive == 'bool' { - return 0; - } - // ... more if statements - return 0xFFFFFFFF; + match primitive { + 'bool' => 0, + 'u8' => 1, + 'u16' => 2, + 'u32' | 'usize' => 3, + 'u64' => 4, + 'u128' => 5, + 'u256' => 6, + 'i8' => 7, + 'i16' => 8, + 'i32' => 9, + 'i64' => 10, + 'i128' => 11, + 'felt252' => 12, + 'ClassHash' => 13, + 'ContractAddress' => 14, + _ => INVALID_TYPE_INDEX, + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core/src/meta/introspect.cairo(3 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/spawn-and-move/dojo_dev.toml
- crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: clippy
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (2)
crates/dojo/core/src/meta/introspect.cairo (2)
236-267: LGTM! Enum upgrade implementation follows requirements.The implementation correctly:
- Prevents variant renaming as discussed in past reviews
- Only allows adding new variants at the end
- Maintains compatibility with existing variants
312-355: Document key member upgrade restrictions.The special handling of key members needs documentation explaining:
- Why only primitive and enum upgrades are allowed
- Why other types must remain unchanged
- The impact on storage layout and serialization
Also, verify that this implementation aligns with the PR objectives regarding key upgrades.
✅ Verification successful
Ohayo sensei! Here's my verification of the key upgrade restrictions:
Key member upgrade implementation is correctly restrictive
The special handling of key members is intentionally strict to maintain storage layout stability and data accessibility. Allowing only primitive type upgrades and enum extensions for key fields while requiring exact matches for other types is the right approach for blockchain storage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for key member usage in models to understand the impact ast-grep --pattern 'struct $name { $$$ #[key] $member: $type $$$ }'Length of output: 77
Script:
#!/bin/bash # Search for key attribute usage and documentation echo "=== Key attribute usage ===" rg "#\[key\]" -B 2 -A 2 echo -e "\n=== Key-related tests ===" rg "key.*upgrade" -i echo -e "\n=== Documentation about keys ===" fd "README|\.md$" --exec rg "key" -i {}Length of output: 130917
glihm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/core/src/meta/introspect.cairo (2)
5-105: Ohayo sensei! The primitive type upgrade matrix looks solid!The implementation efficiently defines valid upgrade paths between primitive types using a boolean matrix. The documentation clearly maps each index to its corresponding type.
Consider adding compile-time validation to ensure the matrix dimensions match the number of primitive types:
const _VALIDATE_MATRIX_SIZE: () = assert(ALLOWED_PRIMITIVE_UPGRADES.len() == 16);
107-161: Consider using a match expression for better readability and maintainability.The current implementation using if-else chains works but could be more elegant.
Consider refactoring to use a match expression:
- if primitive == 'bool' { - return 0; - } - if primitive == 'u8' { - return 1; - } - // ... more if statements ... + match primitive { + 'bool' => 0, + 'u8' => 1, + 'u16' => 2, + 'u32' | 'usize' => 3, + 'u64' => 4, + // ... more cases ... + _ => panic_with_byte_array( + @format!("The introspection of the primitive type {primitive} is not supported."), + ) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core/src/meta/introspect.cairo(5 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/spawn-and-move/dojo_dev.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (6)
crates/dojo/core/src/meta/introspect.cairo (5)
197-199: Clean and focused trait design, sensei!The trait provides a clear and simple interface for type upgradeability checks.
201-248: Solid implementation of primitive and type comparison logic!The implementations handle all type variants correctly and use proper recursion for nested types. The test file shows comprehensive coverage of these implementations.
250-281: Excellent implementation of enum upgrade rules!The implementation correctly prevents variant renaming and allows new variants only at the end, as discussed in the previous review. This ensures type safety during upgrades.
307-370: Well-implemented key member upgrade restrictions!The implementation correctly enforces the PR's requirements:
- Key members can only be upgraded if they are primitives or enums
- Enum variants for key members cannot be modified
- Non-key members follow standard upgrade rules
Line range hint
545-569: Clean up of address type introspection!The implementation now correctly handles ContractAddress, ClassHash, and EthAddress without the starknet:: prefix, fixing the issue mentioned in previous reviews.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
313-765: Impressive test coverage, sensei!The test suite thoroughly validates all upgrade scenarios:
- Type compatibility rules
- Enum variant handling
- Key member restrictions
- Performance considerations
Special appreciation for the comprehensive key member upgrade tests that verify the new restrictions! 🎯
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/dojo/core/src/meta/introspect.cairo (1)
107-161: Consider optimizing the primitive type lookup, sensei.The current implementation uses sequential if-statements. Consider using a match expression or a static hashmap for potentially better performance, especially since this is marked as
inline(always).-fn primitive_to_index(primitive: felt252) -> u32 { - if primitive == 'bool' { - return 0; - } - if primitive == 'u8' { - return 1; - } - // ... more if statements ... +fn primitive_to_index(primitive: felt252) -> u32 { + match primitive { + 'bool' => 0, + 'u8' => 1, + 'u16' => 2, + 'u32' | 'usize' => 3, + 'u64' => 4, + 'u128' => 5, + 'u256' => 6, + 'i8' => 7, + 'i16' => 8, + 'i32' => 9, + 'i64' => 10, + 'i128' => 11, + 'felt252' => 12, + 'ClassHash' | 'starknet::Classhash' => 13, + 'ContractAddress' | 'starknet::ContractAddress' => 14, + 'EthAddress' => 15, + _ => panic_with_byte_array( + @format!("The introspection of the primitive type {primitive} is not supported."), + ) + } +}crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
603-609: Consider enhancing the performance test, sensei.The current performance test only checks one scenario. Consider adding more cases to establish a comprehensive performance baseline.
#[test] #[available_gas(360000)] fn test_primitive_upgrade_performance() { let gas = GasCounterTrait::start(); + // Test primitive to primitive let _ = Ty::Primitive('ClassHash').is_an_upgrade_of(@Ty::Primitive('ContractAddress')); gas.end("Upgrade from ContractAddress to ClassHash"); + + // Test enum with multiple variants + let gas = GasCounterTrait::start(); + let complex_enum = Ty::Enum(Enum { + name: 'e', + attrs: [].span(), + children: [ + ('A', Ty::Primitive('u8')), + ('B', Ty::Primitive('u16')), + ('C', Ty::Primitive('u32')) + ].span() + }); + let _ = complex_enum.is_an_upgrade_of(@complex_enum); + gas.end("Upgrade check for complex enum"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core/src/meta/introspect.cairo(5 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/spawn-and-move/dojo_dev.toml
🔇 Additional comments (5)
crates/dojo/core/src/meta/introspect.cairo (4)
5-105: Ohayo! The primitive upgrades matrix is well-structured and documented, sensei!The matrix provides a clear and maintainable way to define valid upgrade paths between primitive types. The comments effectively document the index-to-type mapping.
197-248: The type comparison trait is well-designed, sensei!The
TyCompareTraitand its implementations provide a clean and extensible way to handle type comparisons. The pattern matching inTyCompareImplis comprehensive and handles all type variants.
250-281: Excellent handling of enum upgrades, sensei!The implementation correctly enforces the rules discussed in past reviews:
- Prevents variant renaming
- Ensures existing variants remain unchanged
- Allows adding new variants only at the end
307-369: Key member upgrade restrictions are properly implemented, sensei!The implementation correctly enforces the PR's requirements:
- Allows primitive type upgrades for key members
- Permits enum upgrades with new variants
- Restricts upgrades for other types (struct, array, tuple)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
313-609: Impressive test coverage, sensei!The test suite thoroughly validates all upgrade scenarios:
- Type compatibility matrix
- Primitive type upgrades
- Enum variant additions
- Struct member changes
- Key member restrictions
remybar
force-pushed
the
feat-core-less_strict_layout_update
branch
from
08bbe3b to
75b2f2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/dojo/core/src/meta/introspect.cairo (2)
5-24: Ohayo! Let's add more detailed documentation for the upgrade matrix, sensei.The primitive type upgrade matrix would benefit from additional documentation explaining:
- The rationale behind allowed upgrades
- The impact on data conversion
- Any potential precision loss considerations
// Each index matches with primitive types in both arrays (main and nested). // The main array represents the source primitive while nested arrays represents // destination primitives. +// +// Upgrade Rules: +// 1. Smaller integers can upgrade to larger ones (e.g., u8 -> u16) +// 2. Integers can upgrade to felt252 for maximum compatibility +// 3. Address types (ContractAddress, ClassHash, EthAddress) are interchangeable +// 4. No downgrades allowed to prevent data loss +// +// Note: When upgrading between different sizes, consider potential +// precision loss and data representation changes.
197-199: Ohayo sensei! Consider optimizing the type comparison trait.The
TyCompareTraitis fundamental to the upgrade system. Consider adding:
- Caching for frequently compared types
- Early returns for common cases
- Performance benchmarks for critical paths
Would you like me to propose specific optimizations for the most common upgrade paths?
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (3)
365-427: Consider improving test readability, sensei!While the test is comprehensive, it could be more maintainable by:
- Breaking down the test into smaller, focused test cases
- Adding helper functions to reduce nesting
- Using test case descriptions that clearly identify which upgrade path failed
Example refactor:
-#[test] -fn test_primitive_upgrade() { +fn assert_primitive_upgrade(src: felt252, dest: felt252, expected: bool) { + assert_eq!( + Ty::Primitive(dest).is_an_upgrade_of(@Ty::Primitive(src)), + expected, + "Unexpected result for upgrade from {} to {}", + src, + dest, + ); +} + +#[test] +fn test_primitive_upgrade_u8() { + // Test all u8 upgrades + assert_primitive_upgrade('u8', 'u16', true); + assert_primitive_upgrade('u8', 'u32', true); + // ... more specific tests +}
439-516: Consider adding more edge cases to struct upgrade tests.While the current test coverage is good, consider adding tests for:
- Multiple member additions in different positions
- Nested struct upgrades
- Complex attribute combinations
604-610: Consider expanding performance testing coverage.The current test only measures ContractAddress to ClassHash upgrade. Consider:
- Testing more upgrade paths
- Adding baseline measurements for comparison
- Testing with different input sizes for complex types
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (8)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(5 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(5 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/world_contract.cairo(2 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/dojo/core/src/world/errors.cairo
- crates/dojo/core/src/world/world_contract.cairo
- examples/spawn-and-move/dojo_dev.toml
- crates/dojo/core/src/lib.cairo
🔇 Additional comments (9)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (4)
58-84: Ohayo! Well-structured test models for upgrade scenarios, sensei!The test models effectively cover different upgrade scenarios:
MyEnumandAnotherEnumfor testing enum upgradesFooModelMemberChangedfor testing tuple and complex type upgradesFooModelMemberIllegalChangefor testing illegal type changes
97-98: LGTM! Proper model registration.The new models are correctly registered in the namespace definition.
104-106: Nice implementation of the requested model value testing, sensei!The world storage initialization allows testing model values after upgrades, as requested in the previous review.
109-110: Good test data initialization!Appropriate test values are written for both simple and complex model types to verify upgrades.
crates/dojo/core-cairo-test/src/tests/world/model.cairo (3)
59-63: Well-designed enum for testing type upgrades!The enum effectively tests upgrades with different numeric types (u8 to u16) and includes PartialEq for value comparison.
228-233: Excellent value verification after upgrade!The test properly verifies that:
- Existing values are preserved during upgrade
- New fields are initialized to zero
321-331: Good test coverage for illegal model changes!The test properly verifies that illegal type changes are rejected with a descriptive error message.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (2)
612-776: Excellent test coverage for key member upgrades!The test comprehensively covers all key member upgrade scenarios with clear assertions and error messages.
518-570: Well-structured enum upgrade tests!Comprehensive coverage of enum upgrade scenarios, including:
- Variant additions
- Type changes
- Unit variant constraints
… a variant with data
…ion + panic if unknown primitive
…ves introspection for backward compatibility
remybar
force-pushed
the
feat-core-less_strict_layout_update
branch
from
47c80d6 to
0d5eb1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (1)
58-84: Ohayo! The model definitions look good, sensei!The new enums and model structs are well-structured for testing schema upgrades. Consider adding doc comments to explain the purpose of each model, especially how they're used in upgrade scenarios.
#[derive(Introspect, Copy, Drop, Serde)] +/// Enum used to test primitive type upgrades in model schemas enum MyEnum { X: u8, } #[derive(Introspect, Copy, Drop, Serde)] #[dojo::model] +/// Model used to test valid member type changes during schema upgrades struct FooModelMemberChanged { #[key] pub caller: ContractAddress, pub a: (MyEnum, u8), pub b: u128, } #[derive(Introspect, Copy, Drop, Serde)] +/// Enum used to test illegal type changes in model schemas enum AnotherEnum { X: bool, } #[derive(Introspect, Copy, Drop, Serde)] #[dojo::model] +/// Model used to test illegal member type changes during schema upgrades struct FooModelMemberIllegalChange { #[key] pub caller: ContractAddress, pub a: AnotherEnum, pub b: u128, }crates/dojo/core-cairo-test/src/tests/world/model.cairo (2)
59-63: Document the relationship with helper model's MyEnum, sensei.This
MyEnumdefinition differs from the one inhelpers/model.cairoby having an additional variantY: u16. Consider adding documentation to explain this intentional difference and its role in testing enum upgrades.#[derive(Introspect, Copy, Drop, Serde, PartialEq)] +/// Extended version of MyEnum from helpers/model.cairo +/// Used to test adding new variants during enum upgrades enum MyEnum { X: u8, Y: u16, }
Line range hint
199-233: Good test coverage for value persistence, sensei!The test verifies that model values persist correctly after upgrades. Consider adding test cases for:
- Edge case values (max/min values)
- Multiple upgrades in sequence
let read: FooModelMemberAdded = world_storage.read_model(caller); assert!(read.a == 123); assert!(read.b == 456); assert!(read.c == 0); + +// Test edge cases +world_storage.write_model(@FooModelMemberAdded { + caller, + a: 0xFFFFFFFF, // max value + b: 0, // min value + c: 0 +}); + +let read: FooModelMemberAdded = world_storage.read_model(caller); +assert!(read.a == 0xFFFFFFFF); +assert!(read.b == 0); +assert!(read.c == 0); + +// Test multiple upgrades +world.upgrade_model("dojo", m_FooModelMemberChanged::TEST_CLASS_HASH.try_into().unwrap()); +world.upgrade_model("dojo", m_FooModelMemberAdded::TEST_CLASS_HASH.try_into().unwrap());crates/dojo/core/src/meta/introspect.cairo (2)
5-105: Excellent primitive type upgrade matrix, sensei!The matrix clearly defines allowed upgrade paths between primitive types. Consider extracting the primitive type indices into named constants for better maintainability.
+const BOOL_INDEX: usize = 0; +const U8_INDEX: usize = 1; +const U16_INDEX: usize = 2; +// ... more index constants const ALLOWED_PRIMITIVE_UPGRADES: [[bool; 16]; 16] = [ - // bool + // BOOL_INDEX [ true, false, false, false, false, false, false, false, false, false, false, false, true, false, false, false, ],
107-167: Consider optimizing the primitive type mapping, sensei!While the current implementation is clear, it could be more efficient using a match expression or a static map.
-fn primitive_to_index(primitive: felt252) -> u32 { +const PRIMITIVE_MAP: Map<felt252, u32> = Map::from([ + ('bool', 0), + ('u8', 1), + // ... more mappings +]); + +fn primitive_to_index(primitive: felt252) -> u32 { + if let Some(index) = PRIMITIVE_MAP.get(primitive) { + return *index; + } + if primitive == 'usize' { panic_with_byte_array( @format!("Prefer using u32 instead of usize as usize size is architecture-dependent."), ) } - - if primitive == 'bool' { - return 0; - } - // ... more if statementscrates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
365-425: Great primitive upgrade test coverage, sensei!The test thoroughly verifies all primitive type upgrade combinations. Consider enhancing the assertion message to provide more context when failures occur.
assert_eq!( Ty::Primitive(*dest).is_an_upgrade_of(@Ty::Primitive(*src)), expected, - "src: {} dest: {}", + "Invalid upgrade path: {} cannot be upgraded to {}. Check ALLOWED_PRIMITIVE_UPGRADES matrix.", *src, *dest, );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (9)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(5 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(5 hunks)crates/dojo/core/src/world/errors.cairo(1 hunks)crates/dojo/core/src/world/world_contract.cairo(2 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/dojo/core/src/world/errors.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/world/world_contract.cairo
- examples/spawn-and-move/dojo_dev.toml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: clippy
- GitHub Check: ensure-wasm
- GitHub Check: docs
🔇 Additional comments (6)
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (2)
97-98: LGTM! New models properly registered.The new models are correctly added to the namespace definition.
104-110: Nice addition of model value initialization, sensei!Writing initial values helps verify that data persists correctly after model upgrades. The test values chosen provide good coverage for verifying the upgrade behavior.
crates/dojo/lang/src/derive_macros/introspect/layout.rs (2)
28-46: Excellent implementation of the usize check, sensei!The check for
usizein member types is well-implemented, with clear error messages guiding users to useu32instead. The use ofCAIRO_DELIMITERSensures accurate type parsing.
201-209: Good addition of the direct usize check, sensei!The direct check for
usizetype complements the member type check, ensuring comprehensive coverage of architecture-dependent types.crates/dojo/core/src/meta/introspect.cairo (1)
Line range hint
203-796: Excellent type comparison implementation, sensei!The type comparison logic is well-implemented with:
- Clear upgrade rules for each type
- Special handling for key members
- Comprehensive test coverage
- Proper enforcement of upgrade restrictions
The implementation aligns perfectly with the PR objectives of making schema upgrades less strict while maintaining safety.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
632-796: Excellent key member upgrade test coverage, sensei!The test suite thoroughly validates the key member upgrade rules with:
- Primitive type upgrades
- Enum variant additions
- Illegal upgrade attempts
- Complex type scenarios
The test cases effectively enforce the design decision to restrict key member upgrades to primitives and enums.
Description
During model/event upgrades, schema changes must respect some conditions to be accepted. This PR updates these conditions to make the upgrade less strict:
u8->u32orfelt252->ContractAddress),Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Refactor
Chores