fix(core): upgrade checks for key members - #3285
Conversation
WalkthroughOhayo sensei! Inserts Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant Test as Test Runner
participant Helpers as Test Helpers
participant World as World Instance
participant Contract as Model Contract
end
Test->>Helpers: deploy legacy model resource & write entity (serialized MyNestedEnum key)
Test->>World: upgrade model to ModelWithNestedEnumKey
Test->>Contract: read model entity by nested-enum key
Contract-->>Test: return model fields (assert a == 42)
sequenceDiagram
rect rgb(255,250,240)
participant Macro as Macro Codegen
participant Formatter as Formatter Helpers
participant ModelParser as Generated ModelParser
participant Serde as Serde Deserializer
participant Test as Test Runner
end
Macro->>Formatter: generate deserialization for keys and values
Macro->>ModelParser: implement deserialize(ref keys, ref values)
alt legacy storage
ModelParser->>Serde: concat keys+values → deserialize(combined)
else new storage
ModelParser->>Serde: deserialize keys and values separately, build instance
end
Test->>ModelParser: call deserialize(keys, values)
ModelParser-->>Test: return model instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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 (1)
crates/dojo/core/src/meta/introspect.cairo (1)
295-301: Nice refactoring to idiomatic loops!The loop-based implementations are cleaner and less error-prone than manual index tracking.
Also applies to: 343-358
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (13)
crates/dojo/core-tests/src/lib.cairo(1 hunks)crates/dojo/core-tests/src/tests/helpers/helpers.cairo(1 hunks)crates/dojo/core-tests/src/tests/helpers/model.cairo(5 hunks)crates/dojo/core-tests/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-tests/src/tests/model/model.cairo(2 hunks)crates/dojo/core-tests/src/tests/world/model.cairo(3 hunks)crates/dojo/core/src/meta/introspect.cairo(4 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)crates/dojo/macros/src/attributes/event.rs(1 hunks)crates/dojo/macros/src/attributes/model.rs(7 hunks)crates/dojo/macros/src/derives/introspect/structs.rs(1 hunks)crates/dojo/macros/src/helpers/formatter.rs(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
crates/dojo/core-tests/src/tests/model/model.cairo (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within crates/dojo/core/src/world/storage.cairo, length checks between entity_ids and values are not required in test API functions like write_values_from_ids_test.
examples/spawn-and-move/dojo_dev.toml (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
crates/dojo/core-tests/src/lib.cairo (2)
Learnt from: glihm
PR: #2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within crates/dojo/core/src/world/storage.cairo, length checks between entity_ids and values are not required in test API functions like write_values_from_ids_test.
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
crates/dojo/core-tests/src/tests/world/model.cairo (2)
Learnt from: glihm
PR: #2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within crates/dojo/core/src/world/storage.cairo, length checks between entity_ids and values are not required in test API functions like write_values_from_ids_test.
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
crates/dojo/core-tests/src/tests/helpers/helpers.cairo (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within crates/dojo/core/src/world/storage.cairo, length checks between entity_ids and values are not required in test API functions like write_values_from_ids_test.
crates/dojo/core-tests/src/tests/meta/introspect.cairo (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within crates/dojo/core/src/world/storage.cairo, length checks between entity_ids and values are not required in test API functions like write_values_from_ids_test.
crates/dojo/core/src/meta/introspect.cairo (1)
Learnt from: glihm
PR: #2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within crates/dojo/core/src/world/storage.cairo, length checks between entity_ids and values are not required in test API functions like write_values_from_ids_test.
crates/dojo/core-tests/src/tests/helpers/model.cairo (2)
Learnt from: glihm
PR: #2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within crates/dojo/core/src/world/storage.cairo, length checks between entity_ids and values are not required in test API functions like write_values_from_ids_test.
Learnt from: glihm
PR: #2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in crates/dojo/world/src/contracts/abigen/ is auto-generated and should be excluded from code reviews.
🔇 Additional comments (37)
examples/spawn-and-move/dojo_dev.toml (1)
72-72: Ohayo sensei! LGTM on the world address update.This configuration change aligns perfectly with the new test infrastructure for model upgrades and key member checks.
crates/dojo/macros/src/derives/introspect/structs.rs (1)
262-262: Ohayo sensei! Excellent alignment with the new deserialization interface.The addition of the "values" parameter correctly implements the separation of key and value handling in the deserialization process, which addresses the DojoStore vs Serde serialization inconsistencies mentioned in the PR objectives.
crates/dojo/core-tests/src/lib.cairo (1)
39-42: Ohayo sensei! Nice organizational improvement for the enum exports.The reordering of
MyNestedEnumto be adjacent toMyEnummakes perfect sense given their relationship, and supports the new nested enum key testing infrastructure cleanly.crates/dojo/core-tests/src/tests/model/model.cairo (2)
596-600: Ohayo sensei! Perfect adaptation to the new deserialization interface.The test correctly splits the serialized data into separate keys and values spans, which aligns with the updated
ModelParser::deserializesignature that now accepts two spans instead of one.
680-684: Ohayo sensei! Consistent implementation across both model types.The DojoStoreModel test follows the same pattern as the LegacyModel test, correctly separating keys and values for the new deserialization interface. This consistency validates the robustness of the new approach.
crates/dojo/core-tests/src/tests/helpers/helpers.cairo (2)
12-12: Ohayo sensei! Good addition of the Copy trait to MyEnum.Adding the
Copytrait toMyEnumenables its use within the newMyNestedEnum, which is essential for the nested enum key testing infrastructure.
19-25: Ohayo sensei! Excellent design for the nested enum test cases.The
MyNestedEnumprovides comprehensive test coverage with variants containing different types (MyEnum, u16, u32), which will be valuable for testing the key upgrade compatibility rules defined in the PR objectives. The derive attributes are appropriate and the default variant is properly marked.crates/dojo/macros/src/attributes/event.rs (1)
192-199: Ohayo! The deserialization logic looks good, sensei!The implementation correctly handles the new signature by concatenating keys and values before passing to Serde, which is appropriate for events that are never stored in world storage.
crates/dojo/core/src/model/model.cairo (2)
52-54: Ohayo! Clean trait signature update, sensei!The separation of keys and values in the deserialization method provides better type safety and clarity.
129-131: The implementation correctly forwards the separated spans.Good consistency with the updated trait signature.
crates/dojo/core-tests/src/tests/world/model.cairo (3)
6-8: Ohayo! Import additions look good, sensei!The new imports properly support the nested enum key test functionality.
80-86: Nice model structure with nested enum key!The Debug derive is appropriately added for test debugging purposes.
249-264: Comprehensive test for nested enum key upgrades!The test properly validates the upgrade process and data integrity. Good use of println! for debugging during test execution.
crates/dojo/macros/src/helpers/formatter.rs (1)
41-64: Ohayo! Flexible deserialization input naming, sensei!The addition of the
input_nameparameter elegantly supports deserializing from either keys or values spans. Clean implementation that maintains backward compatibility.crates/dojo/macros/src/attributes/model.rs (4)
31-35: Ohayo! Good struct field additions for the new deserialization approach.The new fields properly support the separation of key and value deserialization logic.
Also applies to: 52-56
115-128: Clean separation of key and value deserialization!The iteration correctly identifies key members and generates appropriate deserialization code with the correct input span names.
241-269: Excellent handling of legacy vs non-legacy storage, sensei!The deserialize_body generation correctly:
- Concatenates spans for legacy storage (maintaining backward compatibility)
- Deserializes keys and values separately for non-legacy storage
- Properly constructs the model instance with all fields
390-392: Signature update properly integrated into the generated code!The ModelParser implementation correctly uses the new deserialize signature with the generated body.
crates/dojo/core-tests/src/tests/helpers/model.cairo (5)
3-3: Ohayo! Import additions look good, sensei!The new imports are properly organized and necessary for the nested enum key functionality.
Also applies to: 7-7
38-47: Clean layout implementation!The layout correctly represents a struct with a single u8 field, matching the model's schema.
60-60: Model registration looks perfect!The new model is properly added to the test resources for deployment.
87-96: Excellent nested enum key setup, sensei!The entity write logic correctly handles the complex nested enum key serialization and storage.
318-395: Ohayo! Comprehensive model implementation!The contract correctly implements the legacy model with nested enum keys. The manual schema construction is well-structured and the comments clearly explain the nested enum type hierarchy.
crates/dojo/core/src/meta/introspect.cairo (5)
211-211: Clean trait extension, sensei!The new
is_a_key_upgrade_ofmethod provides a clear separation of concerns for key-specific upgrade semantics. The primitive implementation correctly delegates to the existing logic.Also applies to: 216-218
235-270: Ohayo! Excellent key upgrade semantics!The implementation correctly enforces stricter rules for key types:
- Fixed arrays and tuples maintain exact dimensions
- Arrays allow item upgrades while preserving flexibility
- Proper delegation to type-specific implementations
This ensures entity key stability across upgrades.
311-334: Solid enum key upgrade logic!The implementation correctly balances stability and extensibility:
- Existing variants preserve compatibility
- New variants can be added for future features
- Recursive type checking ensures deep compatibility
363-393: Clean struct upgrade implementations, sensei!The differentiation between key and general upgrades is spot-on:
- Key upgrades maintain exact structure
- General upgrades allow growth
- Loop refactoring improves code clarity
397-424: Ohayo! Elegant member upgrade logic!The implementation correctly applies different upgrade rules based on the 'key' attribute. The refactored loop for attribute checking is cleaner and more maintainable.
crates/dojo/core-tests/src/tests/meta/introspect.cairo (9)
537-570: Ohayo! Excellent test refactoring, sensei!The loop-based approach is much cleaner and tests both upgrade methods to ensure primitive upgrades behave consistently for keys and general members.
791-794: Simple but essential test!ByteArray self-upgrade compatibility confirmed.
804-880: Comprehensive struct key upgrade test coverage!The test thoroughly validates all struct key upgrade rules, especially the critical constraint that new members are not allowed.
882-940: Ohayo! Thorough enum key upgrade testing!Great coverage including the important case of allowing new variants while maintaining existing variant compatibility.
942-953: Clean array key upgrade tests!Correctly validates that array keys follow item upgrade rules.
955-976: Excellent tuple key constraint testing, sensei!The test properly validates that tuple keys maintain exact length while allowing item upgrades.
978-997: Solid fixed array key upgrade validation!Correctly enforces that fixed array keys maintain exact dimensions.
999-1002: ByteArray key upgrade confirmed!Simple validation that ByteArray keys follow the same rules.
1004-1159: Ohayo! Outstanding nested enum test coverage, sensei!This complex test excellently validates that key upgrade rules apply recursively through nested structures. The test cases cover both allowed upgrades (u16→u32) and disallowed ones (u16→u8) at the nested level.
a577fc1 to
8947b69
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (12)
crates/dojo/core-tests/src/lib.cairo(1 hunks)crates/dojo/core-tests/src/tests/helpers/helpers.cairo(1 hunks)crates/dojo/core-tests/src/tests/helpers/model.cairo(5 hunks)crates/dojo/core-tests/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-tests/src/tests/model/model.cairo(2 hunks)crates/dojo/core-tests/src/tests/world/model.cairo(3 hunks)crates/dojo/core/src/meta/introspect.cairo(4 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)crates/dojo/macros/src/attributes/event.rs(1 hunks)crates/dojo/macros/src/attributes/model.rs(7 hunks)crates/dojo/macros/src/derives/introspect/structs.rs(1 hunks)crates/dojo/macros/src/helpers/formatter.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/dojo/core-tests/src/lib.cairo
- crates/dojo/macros/src/derives/introspect/structs.rs
- crates/dojo/core-tests/src/tests/helpers/helpers.cairo
- crates/dojo/macros/src/attributes/event.rs
- crates/dojo/core-tests/src/tests/world/model.cairo
- crates/dojo/core/src/model/model.cairo
- crates/dojo/core-tests/src/tests/model/model.cairo
- crates/dojo/macros/src/helpers/formatter.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in the cairo codebase for the dojo project, within `crates/dojo/core/src/world/storage.cairo`, lengt...
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within `crates/dojo/core/src/world/storage.cairo`, length checks between `entity_ids` and `values` are not required in test API functions like `write_values_from_ids_test`.
Applied to files:
crates/dojo/core-tests/src/tests/meta/introspect.cairocrates/dojo/core-tests/src/tests/helpers/model.cairocrates/dojo/core/src/meta/introspect.cairo
📚 Learning: code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code...
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Applied to files:
crates/dojo/core-tests/src/tests/helpers/model.cairo
🔇 Additional comments (23)
crates/dojo/macros/src/attributes/model.rs (3)
31-31: Ohayo sensei! New fields added for separate key deserialization.The addition of
deserialized_keysanddeserialize_bodyfields properly supports the new separate key/value deserialization architecture described in the PR objectives.Also applies to: 34-34
115-128: Key deserialization logic properly separated from values.The refactored iteration logic correctly handles key members separately by pushing them to
deserialized_keyswith the "keys" input name, while non-key members go todeserialized_valueswith the "values" input name. This aligns perfectly with the PR's goal to address the DojoStore serialization mismatch issue.
241-269: Well-structured deserialize_body generation for both legacy and modern storage.The conditional logic properly handles both storage types:
- Legacy storage concatenates keys and values for backwards compatibility
- Modern storage deserializes keys and values separately using Serde for keys and DojoStore for values
This addresses the core deserialization issue mentioned in the PR objectives where keys were serialized using Serde while values used DojoStore.
crates/dojo/core-tests/src/tests/helpers/model.cairo (4)
3-3: Ohayo! Proper imports added for nested enum testing.The addition of
serialize_inlineandMyNestedEnumimports supports the new nested enum key testing functionality as described in the PR objectives.Also applies to: 7-7
38-47: Well-defined layout function for legacy nested enum model.The
old_model_with_nested_enum_key_layout()function correctly defines a fixed layout with an 8-bit field, which matches the expected layout for the nested enum key testing scenario.
60-60: Comprehensive legacy model setup for upgrade testing.The addition of "OldModelWithNestedEnumKey" to the test resources and the corresponding entity write operation properly sets up the test data for verifying nested enum key upgrade compatibility. The use of
serialize_inlineandentity_id_from_serialized_keyscorrectly handles the complex key serialization.Also applies to: 87-96
320-395: Detailed schema implementation for nested enum introspection.Sensei, this contract implementation provides comprehensive introspection data for the nested enum structure, correctly defining the
MyNestedEnumwith variantAcontainingMyEnumwith variantXof typeu8, and variantBof typeu16. This detailed schema is essential for the upgrade compatibility testing.crates/dojo/core/src/meta/introspect.cairo (7)
211-211: Ohayo sensei! Essential new method for key-specific upgrade semantics.The addition of
is_a_key_upgrade_ofmethod to the trait provides the foundation for implementing stricter upgrade compatibility checks specifically for key members, as outlined in the PR objectives.
216-218: Sensible delegation for primitive key upgrades.For primitive types, delegating key upgrade checks to the existing
is_an_upgrade_ofmethod is correct since primitive upgrade rules (size compatibility) should be consistent between keys and values.
235-270: Well-implemented key upgrade rules for different type categories.The implementation correctly enforces stricter rules for key upgrades:
- Fixed arrays and tuples require exact size matches (stricter than general upgrades)
- Arrays still allow item type upgrades while maintaining structure
- ByteArray upgrades are always allowed
- Proper recursive checking for nested types
This perfectly aligns with the PR's objective of defining explicit upgrade rules for key members.
311-334: Strict enum key upgrade rules properly implemented.The key upgrade implementation for enums correctly enforces:
- Exact name and attribute matching
- Existing variants must follow key upgrade rules
- New variants are allowed (line 323 comment confirms this)
This stricter approach for keys compared to general enum upgrades is appropriate for maintaining key compatibility.
363-377: Appropriately strict struct key upgrade requirements.The requirement for exact member count equality (line 366) in struct key upgrades is more restrictive than general struct upgrades that allow new members. This makes sense for keys where structural changes could break entity identification.
295-301: Excellent refactoring to use for loops.The conversion from manual iteration patterns to for loops significantly improves code readability and maintainability. The logic remains functionally equivalent while being more idiomatic.
Also applies to: 343-359, 379-393, 410-417
420-420: Smart integration of key upgrade checks in member comparison.The conditional logic that uses
is_a_key_upgrade_offor key members andis_an_upgrade_offor regular members elegantly integrates the new stricter key semantics into the existing upgrade system.crates/dojo/core-tests/src/tests/meta/introspect.cairo (9)
537-571: Ohayo sensei! LGTM on the primitive upgrade test enhancement.The addition of
is_a_key_upgrade_oftesting alongsideis_an_upgrade_ofis well-implemented. Both methods correctly have identical behavior for primitive types since upgrade rules for atomic types should be consistent regardless of usage context.
791-794: Simple and effective ByteArray upgrade test, sensei!This test properly verifies the basic ByteArray upgrade compatibility. Given that ByteArray upgrades are always allowed according to the PR objectives, this serves as a good baseline test.
804-880: Excellent implementation of stricter struct key upgrade rules, sensei!The rename to
test_struct_key_upgradeand the switch tois_a_key_upgrade_ofcorrectly reflects the new stricter semantics. The key insight here is that new members are NOT allowed for key upgrades (line 879), which is the opposite of general struct upgrades. This makes perfect sense for maintaining key compatibility in stored data.
882-940: Comprehensive enum key upgrade test coverage, sensei!This new test function properly validates enum key upgrade rules. The logic correctly allows new variants (maintaining backward compatibility) while enforcing upgrade rules on existing variant data types. The inclusion of the Option special case is a nice touch.
942-953: Clean array key upgrade test, sensei!The test correctly verifies that array key upgrades follow the same rules as general array upgrades - the item type must be upgradeable. This is logical since the array structure remains consistent.
955-976: Perfect implementation of stricter tuple key upgrade rules, sensei!This test correctly enforces that tuple key upgrades must maintain the same length (lines 967-975), unlike general tuple upgrades. This stricter rule makes sense for key compatibility - changing tuple length would alter the key structure and break existing storage compatibility.
978-997: Solid fixed array key upgrade test with proper restrictions, sensei!The test correctly enforces that fixed array key upgrades must maintain the exact same size (lines 990-996), which is stricter than general fixed array upgrades. This restriction is essential for maintaining key structure compatibility in storage.
1004-1159: Exceptional nested enum key upgrade test coverage, sensei!This comprehensive test brilliantly handles the complex scenario of nested enums in struct keys. The three test cases are well-chosen:
- New variant addition (lines 1042-1080): Correctly allowed for backward compatibility
- Valid data upgrade (lines 1082-1119): u16 → u32 follows primitive upgrade rules
- Invalid data downgrade (lines 1121-1158): u16 → u8 properly rejected
The deeply nested enum structure accurately reflects real-world usage patterns and ensures the upgrade logic works correctly at multiple nesting levels. Well-structured test, sensei!
999-1002: ByteArray key upgrade test looks good, sensei!Simple and effective test ensuring ByteArray key upgrades maintain the same "always allowed" behavior as general ByteArray upgrades.
8947b69 to
d112ee2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
examples/spawn-and-move/dojo_dev.toml (1)
72-72: Optional: avoid hard-coding ephemeral addresses to reduce churnIf supported by your tooling, consider sourcing world_address from an env var or a per-profile overlay (e.g., .env, workspace profile). This avoids frequent commits whenever the dev world is re-deployed.
crates/dojo/macros/src/helpers/formatter.rs (1)
66-68: Minor: clarify parameter intent in docs for future maintainers.Adding a brief doc comment that input_name should be the span identifier in generated Cairo ("keys" or "values") helps prevent misuse.
Proposed doc additions:
pub fn deserialize_primitive_member_ty( member_name: &String, member_ty: &String, use_serde: bool, - input_name: &str, + // input_name: the span identifier to read from in generated Cairo, e.g. "keys" or "values". + input_name: &str, ) -> String {crates/dojo/core/src/meta/introspect.cairo (1)
318-341: Style/consistency: deref the Span items like in is_an_upgrade_of.In is_a_key_upgrade_of(Enum), you destructure children with n.children[i] and o.children[i] without deref, whereas is_an_upgrade_of uses *self.children[i]. For consistency and to avoid surprises around snapshots, mirror the latter’s pattern.
Apply:
- let (new_name, new_ty) = n.children[i]; - let (old_name, old_ty) = o.children[i]; + let (new_name, new_ty) = *n.children[i]; + let (old_name, old_ty) = *o.children[i];crates/dojo/core-tests/src/tests/meta/introspect.cairo (1)
1010-1079: Add explicit is_a_key_upgrade_of assertions for nested-enum key tests.These cases exercise key semantics via Struct::is_an_upgrade_of (which internally routes key members through is_a_key_upgrade_of). For clarity and direct coverage, also assert Struct::is_a_key_upgrade_of in the three upgraded cases.
Proposed additions:
@@ - assert!(upgraded.is_an_upgrade_of(@s), "nested enum upgrade (new variant added)"); + assert!(upgraded.is_an_upgrade_of(@s), "nested enum upgrade (new variant added)"); + assert!(upgraded.is_a_key_upgrade_of(@s), "key: nested enum upgrade (new variant added)"); @@ - assert!(upgraded.is_an_upgrade_of(@s), "nested enum upgrade (modify variant data u16 -> u32)"); + assert!(upgraded.is_an_upgrade_of(@s), "nested enum upgrade (modify variant data u16 -> u32)"); + assert!(upgraded.is_a_key_upgrade_of(@s), "key: nested enum upgrade (modify variant data u16 -> u32)"); @@ - assert!(!upgraded.is_an_upgrade_of(@s), "nested enum upgrade (modify variant data u16 -> u8)"); + assert!(!upgraded.is_an_upgrade_of(@s), "nested enum upgrade (modify variant data u16 -> u8)"); + assert!(!upgraded.is_a_key_upgrade_of(@s), "key: nested enum upgrade (modify variant data u16 -> u8)");Also applies to: 1082-1115, 1121-1159
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (14)
crates/dojo/core-tests/src/lib.cairo(1 hunks)crates/dojo/core-tests/src/tests/helpers/helpers.cairo(1 hunks)crates/dojo/core-tests/src/tests/helpers/model.cairo(5 hunks)crates/dojo/core-tests/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-tests/src/tests/model/model.cairo(2 hunks)crates/dojo/core-tests/src/tests/world/model.cairo(3 hunks)crates/dojo/core/src/meta/introspect.cairo(4 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)crates/dojo/macros/src/attributes/event.rs(1 hunks)crates/dojo/macros/src/attributes/model.rs(7 hunks)crates/dojo/macros/src/derives/dojo_store.rs(1 hunks)crates/dojo/macros/src/derives/introspect/structs.rs(0 hunks)crates/dojo/macros/src/helpers/formatter.rs(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
💤 Files with no reviewable changes (1)
- crates/dojo/macros/src/derives/introspect/structs.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/dojo/core-tests/src/tests/helpers/helpers.cairo
- crates/dojo/macros/src/attributes/event.rs
- crates/dojo/core-tests/src/lib.cairo
- crates/dojo/core/src/model/model.cairo
- crates/dojo/core-tests/src/tests/world/model.cairo
- crates/dojo/core-tests/src/tests/helpers/model.cairo
- crates/dojo/core-tests/src/tests/model/model.cairo
- crates/dojo/macros/src/attributes/model.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-05T04:29:12.288Z
Learnt from: glihm
PR: dojoengine/dojo#2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within `crates/dojo/core/src/world/storage.cairo`, length checks between `entity_ids` and `values` are not required in test API functions like `write_values_from_ids_test`.
Applied to files:
crates/dojo/core-tests/src/tests/meta/introspect.cairo
🔇 Additional comments (12)
crates/dojo/macros/src/helpers/formatter.rs (1)
49-50: Ohayo sensei! All call sites now include the newinput_nameparameterBoth
DojoFormatter::deserialize_member_tyanddeserialize_primitive_member_tyinvocations in the codebase pass the fourth argument:
- crates/dojo/macros/src/attributes/model.rs (lines 119–125)
–.deserialize_member_ty(db, member_ast, true, "keys")
–.deserialize_member_ty(db, member_ast, model.use_legacy_storage, "values")- crates/dojo/macros/src/derives/dojo_store.rs (lines 54–58)
–.deserialize_primitive_member_ty(&member_name, &member_ty, false, "values")Looks good to merge!
crates/dojo/macros/src/derives/dojo_store.rs (1)
55-59: LGTM: correct integration with the updated DojoFormatter API.Passing "values" as input_name keeps the derive consistent with the single-span DojoStore::deserialize signature in this context. No issues.
crates/dojo/core/src/meta/introspect.cairo (5)
211-214: Solid addition: key-aware upgrade pathway on the trait.Introducing is_a_key_upgrade_of alongside is_an_upgrade_of is aligned with the PR objectives and keeps key semantics explicit and testable.
217-220: Primitive key-upgrade delegation looks correct.For primitives, key upgrades follow the same widening rules as non-key upgrades. Matches the spec.
236-274: Key-upgrade semantics for Ty are spot on.
- Arrays/tuples delegate element checks to key-upgrade.
- FixedArray enforces equal length (as required).
- ByteArray always permitted.
This faithfully implements the rules described in the PR.
302-309: Loop refactor improves readability.Switching to for loops for tuple comparisons removes manual indexing boilerplate and matches the rest of the file’s style.
404-428: Member key-routing logic is correct.
- Name and attrs equality checked up-front guards against key attr drift.
- Using the key-aware path when the member has 'key' ensures nested checks propagate proper constraints.
crates/dojo/core-tests/src/tests/meta/introspect.cairo (5)
537-569: Cleaner primitive upgrade test and added key-upgrade parity.Iterating allowed_upgrades with (src, allowed) reduces noise, and asserting both is_an_upgrade_of and is_a_key_upgrade_of ensures parity for primitives. Note: including 'usize' only in the allowed set (not in primitives) avoids the intentional panic path — good.
791-795: ByteArray upgrade tests: good coverage.Explicit tests for both non-key and key paths ensure ByteArray rules remain permissive as intended.
Also applies to: 1000-1003
804-881: Struct key-upgrade tests match the stricter rules.
- Name/attrs equality enforced.
- No new members permitted.
- Widening-only changes accepted.
Great coverage of the key constraints.
883-941: Enum key-upgrade tests: clear and comprehensive.Covers name/attr equality, variant name invariance, per-variant type widening, new variants allowance, unit-variant constraints, and Option special-case. Nicely done.
943-953: Array/tuple/fixed-array key-upgrade tests align with rules.
- Arrays: element widening only.
- Tuples: element widening and identical size.
- Fixed arrays: element widening and identical length.
All consistent with the spec.Also applies to: 956-976, 979-997
| account_address = "0x2af9427c5a277474c079a1283c880ee8a6f0f8fbf73ce969c08d88befec1bba" | ||
| private_key = "0x1800000000300000180000000000030000000000003006001800006600" | ||
| world_address = "0x653644e1085578a22addf74edbd3a211cd41e7323f9ac249e3ffbf04bcbb86b" | ||
| world_address = "0x46e3f86feb22601de72852319d7e155b8b22d15a13d89c751515fcd408a3f29" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ohayo, sensei — world_address update looks good; please ensure it matches the deployed dev world
The new value is a 32-byte hex and fits the expected format. Given the PR’s test harness and world-dependent flows, double-check that this address corresponds to the currently deployed world your tests target.
Run this script to validate the format and check for drift across other config files:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Extract and validate the updated world_address format (0x + 64 hex chars)
ADDR=$(rg -nP '^\s*world_address\s*=\s*"(?<addr>0x[0-9a-fA-F]+)"' -or '$addr' examples/spawn-and-move/dojo_dev.toml | head -n1)
echo "Configured world_address: $ADDR"
if [[ "$ADDR" =~ ^0x[0-9a-fA-F]{64}$ ]]; then
echo "OK: Address format is valid (32 bytes)."
else
echo "ERROR: world_address is not a 32-byte hex value." >&2
exit 1
fi
# 2) Surface other world_address entries to keep environments consistent
echo
echo "Scanning for other world_address entries in the repo:"
fd -a 'dojo_dev.toml' | while read -r f; do
rg -nP '^\s*world_address\s*=' "$f" | sed "s|^|$f: |"
doneLength of output: 634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Extract the world_address value without line numbers
addr_line=$(rg -P '^\s*world_address\s*=' examples/spawn-and-move/dojo_dev.toml | head -n1)
addr=$(echo "$addr_line" | sed -E 's/.*"([^"]+)".*/\1/')
# Strip the 0x prefix and measure length
hex=${addr#0x}
len=$(echo -n "$hex" | wc -m)
echo "Extracted address: $addr"
echo "Hex digit length: $len (expected 64)"Length of output: 820
Ohayo, sensei — world_address length mismatch
The configured world_address in examples/spawn-and-move/dojo_dev.toml is only 63 hex digits (31.5 bytes) instead of the required 64 (32 bytes). Please correct the value by adding the missing hex character and then double-check it against your deployed dev world.
• File: examples/spawn-and-move/dojo_dev.toml
• Line: the world_address = "0x46e3f86f…fcd408a3f29" entry
🤖 Prompt for AI Agents
In examples/spawn-and-move/dojo_dev.toml around line 72, the world_address value
is 0x46e3f86feb22601de72852319d7e155b8b22d15a13d89c751515fcd408a3f29 which is 63
hex digits (31.5 bytes) instead of the required 64 (32 bytes); fix it by adding
the missing hex character so the string after 0x is exactly 64 hex characters,
save the file, and then verify the corrected address matches your deployed dev
world (replace with the exact correct hex from the deployment).
2 participants
Description
model upgrade with enum/option key
In the way we check if a model can be upgraded, some cases for
keymembers are not well managed.Here are the
keymember upgrade rules that are checked according to thekeymember type:Primitive: same than for avaluemember, the new type must have the same or a bigger size than the old type (u8->u16),Struct: new members are NOT allowed. Existing member types must follow thekeymember upgrade rules,Enum: new variants are allowed. Existing variant data types must follow thekeymember upgrade rules,Tuple: tuple items must follow thekeymember upgrade rules and the tuple size must remain the same,Fixed-Size Array: the array item type must follow thekeymember upgrade rules and the array size must remain the same,Array: the array item type must follow thekeymember upgrade rules,ByteArray: always allowed.model reading with enum/option key
Since the introduction of
DojoStore, when a model is read, keys are serialized usingSerdebut read values were serialized withDojoStore. That means, when we try to deserialize the whole data keys+values usingDojoStoreit fails due to the fact thatSerdeandDojoStoredon't handle variant indexes the same way.So, this PR updates the
ModelParser::deserialize()function to:Serdeto deserialize events and legacy models,Serdefor key fields andDojoStorefor value fields.Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores