Feat(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
Suggested Reviewers
Ohayo, sensei! I hope this breakdown helps illuminate the intricate dance of code evolution in this pull request! 🍵🥷 🪧 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.
| #[generate_trait] | ||
| pub impl StructCompareImpl of StructCompareTrait { | ||
| pub trait TyCompareTrait<T> { | ||
| fn is_an_upgrade_of(self: @T, old: @T) -> bool; |
There was a problem hiding this comment.
We will need to update the doc for the possible conversions, since here it's a bit difficult to propagate the actual member that causes the issue.
There was a problem hiding this comment.
I will add a upgrades page in the Dojo Book - "models" chapter to explain all this stuff ;-)
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
… a variant with data
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2925 +/- ##
==========================================
- Coverage 55.75% 55.75% -0.01%
==========================================
Files 445 445
Lines 57627 57627
==========================================
- Hits 32129 32128 -1
- Misses 25498 25499 +1 ☔ 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
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