feat(lang): add members to world store and re added model tests #2611
Conversation
bengineer42
changed the title
WalkthroughOhayo, sensei! The recent changes involve significant modifications across multiple files in the Dojo project, primarily focusing on enhancing the testing modules, model handling, and storage functionalities. Key updates include the reintroduction of the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Suite
participant M as Model Module
participant S as Storage
participant W as World Storage
T->>M: Run tests
M->>S: Access model data
S->>W: Retrieve field layout
W->>S: Return field layout
S->>M: Read/Write member data
M->>T: Return test results
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 3
🧹 Outside diff range and nitpick comments (2)
crates/dojo/core/src/model/model.cairo (1)
44-45: Add documentation for the new field_layout method, sensei!While the method signature looks good, it would be helpful to add documentation explaining:
- Purpose of the method
- What field_selector represents
- When it returns Some(Layout) vs None
Example documentation:
/// Returns the layout of a specific field in the model. /// * `field_selector` - The selector identifying the field /// Returns Some(Layout) if the field exists, None otherwise. fn field_layout(field_selector: felt252) -> Option<Layout>;crates/dojo/core/src/world/storage.cairo (1)
22-27: Consider providing a more descriptive panic message infield_layout_unwrap.Ohayo, sensei! Including additional context in the panic message can aid debugging when the field layout is not found.
You might apply the following change:
fn field_layout_unwrap<M, +Model<M>>(field_selector: felt252) -> Layout { match Model::<M>::field_layout(field_selector) { Option::Some(layout) => layout, - Option::None => panic_with_felt252('bad member id') + Option::None => { + let message = 'Field layout not found for field selector: '; + panic_with_felt252(message + field_selector.to_felt252_string()) + } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core/src/model/model.cairo(3 hunks)crates/dojo/core/src/model/storage.cairo(2 hunks)crates/dojo/core/src/world/storage.cairo(3 hunks)
🔇 Additional comments (12)
crates/dojo/core-cairo-test/src/lib.cairo (2)
26-28: Ohayo! The model module addition looks good, sensei!
The introduction of the model module aligns well with the PR's objective of adding member access functionality, and it follows the established module organization pattern.
26-28: Verify the model test file implementation.
Let's ensure the referenced test file exists and contains the necessary test cases for the new member access functionality.
✅ Verification successful
Let me gather more context about the test implementation to ensure it's comprehensive.
Model test implementation is comprehensive and well-structured, sensei! ✨
The test file contains all necessary test cases covering the model functionality:
- Model definition and metadata verification
- Value serialization and deserialization
- Model read/write operations
- Member access functionality (specifically in
test_read_and_write_field_name) - Model deletion
- Key-based operations
The member access functionality is properly tested with both ID-based and key-based access patterns, validating both read and write operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the model test file
# Test 1: Check if the test file exists
fd "model.cairo" "crates/dojo/core-cairo-test/src/tests/model"
# Test 2: Look for member access related test cases
rg -A 5 "member" "crates/dojo/core-cairo-test/src/tests/model/model.cairo"
Length of output: 544
Script:
#!/bin/bash
# Get the full content of the model test file to verify all test cases
cat "crates/dojo/core-cairo-test/src/tests/model/model.cairo"
Length of output: 4389
crates/dojo/core-cairo-test/src/tests/model/model.cairo (5)
1-3: Ohayo! The imports look well-organized, sensei!
The new imports, especially ModelPtr and test utilities, align perfectly with the PR's objective of enhancing model access functionality.
28-39: Clean helper functions implementation, sensei!
The new helper functions improve test maintainability by:
- Encapsulating namespace definition logic
- Providing a reusable world spawning mechanism
- Reducing boilerplate in test cases
80-98: Test refactoring looks solid, sensei!
The updated test demonstrates good practices with clear arrange-act-assert structure and proper use of the new world spawning mechanism.
123-124: Consider addressing the type inference limitation.
The comment indicates a potential improvement area with type inference. This might affect developer experience.
Let's check if this is a known issue:
Would you like help exploring alternative approaches to improve type inference in this context?
126-129: Verify member access consistency.
The test demonstrates both ID-based and key-based member access. Let's ensure this pattern is consistently used across the codebase.
✅ Verification successful
The previous ast-grep pattern didn't work well for Cairo syntax. Let's try with ripgrep to find member access patterns and analyze the consistency of ID-based vs key-based access.
Member access pattern is correctly implemented, sensei! ٩(◕‿◕｡)۶
The codebase shows consistent implementation of both ID-based and key-based member access:
- The
ModelPtrenum properly handles both variants (IdandKeys) in core implementations:core/src/model/storage.cairocore/src/world/storage.cairo
- The test case demonstrates the correct usage pattern where both access methods return the same value
- The pattern is properly abstracted in the storage trait interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar member access patterns
ast-grep --pattern 'read_member($_, $_)'
Length of output: 42
Script:
#!/bin/bash
# Search for read_member and write_member usage patterns in the codebase
rg -U "read_member|write_member" -A 2
# Also search for ModelPtr usage patterns
rg -U "ModelPtr::" -A 2
Length of output: 3589
crates/dojo/core/src/model/model.cairo (3)
1-4: Ohayo! Clean import organization, sensei!
The new utility import is well-organized and properly grouped with related functionality.
99-102: Clean implementation, sensei!
The implementation is concise and follows good practices:
- Properly delegates to the utility function
- Correctly uses Self::layout() for model layout access
- Maintains the Option return type for error handling
44-45: Verify integration with ModelStorage changes, sensei!
This addition provides the foundation for member-level access, but we should ensure proper integration with the new read_member and write_member methods in ModelStorage.
Let's verify the integration:
Also applies to: 99-102
✅ Verification successful
I'll search more broadly to understand the integration between these components. The previous search might have been too specific, so let's cast a wider net to see how these pieces fit together.
Integration with ModelStorage is properly implemented, sensei!
The verification shows that field_layout is correctly integrated with the storage system:
WorldStorageusesfield_layout_unwrapwhich callsModel::field_layoutin read/write operationsread_memberandwrite_memberimplementations inworld/storage.cairoproperly utilize the layout information- The layout system is thoroughly tested with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of field_layout in read_member and write_member implementations
rg -A 5 "fn (read|write)_member.*field_layout"
Length of output: 48
Script:
#!/bin/bash
# Let's search for read_member and write_member implementations first
rg "fn (read|write)_member" -A 10
# Also search for field_layout usage
rg "field_layout" -A 5
# Let's also check for any ModelStorage references
rg "ModelStorage" -A 5
Length of output: 36855
crates/dojo/core/src/model/storage.cairo (2)
16-19: Addition of ModelPtrTrait<M> is well-structured
Ohayo, sensei! The introduction of ModelPtrTrait<M> with the entity_id method enhances the model pointer functionality nicely. This addition looks good to me.
21-29: Implementation of ModelPtrTrait<M> is correct
Ohayo, sensei! The implementation in ModelPtrImpl<M> correctly retrieves the entity ID from a ModelPtr. The match logic and usage of entity_id_from_keys are appropriate.
| /// Retrieves a model of type `M` using the provided entity idref . | ||
| fn read_member<T, +Serde<T>>(self: @S, ptr: ModelPtr<M>, field_selector: felt252) -> T; | ||
| /// Retrieves a model of type `M` using the provided entity id. | ||
| fn write_member<T, +Serde<T>, +Drop<T>>( | ||
| ref self: S, ptr: ModelPtr<M>, field_selector: felt252, value: T | ||
| ); |
There was a problem hiding this comment.
Correct the documentation comments to match the methods' functionality
Ohayo, sensei! The documentation comments for read_member and write_member don't accurately reflect their purposes. Please update them to describe that read_member retrieves a member of a model, and write_member updates a member of a model.
Apply this diff to correct the documentation comments:
- /// Retrieves a model of type `M` using the provided entity idref .
+ /// Retrieves a member of a model of type `M` using the provided field selector.
fn read_member<T, +Serde<T>>(self: @S, ptr: ModelPtr<M>, field_selector: felt252) -> T;
- /// Retrieves a model of type `M` using the provided entity id.
+ /// Updates a member of a model of type `M` using the provided field selector.
fn write_member<T, +Serde<T>, +Drop<T>>(
ref self: S, ptr: ModelPtr<M>, field_selector: felt252, value: T
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Retrieves a model of type `M` using the provided entity idref . | |
| fn read_member<T, +Serde<T>>(self: @S, ptr: ModelPtr<M>, field_selector: felt252) -> T; | |
| /// Retrieves a model of type `M` using the provided entity id. | |
| fn write_member<T, +Serde<T>, +Drop<T>>( | |
| ref self: S, ptr: ModelPtr<M>, field_selector: felt252, value: T | |
| ); | |
| /// Retrieves a member of a model of type `M` using the provided field selector. | |
| fn read_member<T, +Serde<T>>(self: @S, ptr: ModelPtr<M>, field_selector: felt252) -> T; | |
| /// Updates a member of a model of type `M` using the provided field selector. | |
| fn write_member<T, +Serde<T>, +Drop<T>>( | |
| ref self: S, ptr: ModelPtr<M>, field_selector: felt252, value: T | |
| ); |
| fn read_member<T, +Serde<T>>( | ||
| self: @WorldStorage, ptr: ModelPtr<M>, field_selector: felt252 | ||
| ) -> T { | ||
| deserialize_unwrap( | ||
| IWorldDispatcherTrait::entity( | ||
| *self.dispatcher, | ||
| Model::<M>::selector(*self.namespace_hash), | ||
| ModelIndex::MemberId((ptr.entity_id(), field_selector)), | ||
| field_layout_unwrap::<M>(field_selector) | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Handle deserialization errors in read_member more robustly.
Ohayo, sensei! Currently, read_member uses deserialize_unwrap, which panics if deserialization fails. Consider handling deserialization errors more gracefully to avoid unexpected panics.
You could modify the code to handle errors explicitly:
fn read_member<T, +Serde<T>>(
self: @WorldStorage, ptr: ModelPtr<M>, field_selector: felt252
) -> T {
- deserialize_unwrap(
+ match deserialize(
IWorldDispatcherTrait::entity(
*self.dispatcher,
Model::<M>::selector(*self.namespace_hash),
ModelIndex::MemberId((ptr.entity_id(), field_selector)),
field_layout_unwrap::<M>(field_selector)
- )
+ )
+ ) {
+ Option::Some(value) => value,
+ Option::None => panic_with_felt252('Deserialization failed for read_member')
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn read_member<T, +Serde<T>>( | |
| self: @WorldStorage, ptr: ModelPtr<M>, field_selector: felt252 | |
| ) -> T { | |
| deserialize_unwrap( | |
| IWorldDispatcherTrait::entity( | |
| *self.dispatcher, | |
| Model::<M>::selector(*self.namespace_hash), | |
| ModelIndex::MemberId((ptr.entity_id(), field_selector)), | |
| field_layout_unwrap::<M>(field_selector) | |
| ) | |
| ) | |
| } | |
| fn read_member<T, +Serde<T>>( | |
| self: @WorldStorage, ptr: ModelPtr<M>, field_selector: felt252 | |
| ) -> T { | |
| match deserialize( | |
| IWorldDispatcherTrait::entity( | |
| *self.dispatcher, | |
| Model::<M>::selector(*self.namespace_hash), | |
| ModelIndex::MemberId((ptr.entity_id(), field_selector)), | |
| field_layout_unwrap::<M>(field_selector) | |
| ) | |
| ) { | |
| Option::Some(value) => value, | |
| Option::None => panic_with_felt252('Deserialization failed for read_member') | |
| } | |
| } |
| fn write_member<T, +Serde<T>, +Drop<T>>( | ||
| ref self: WorldStorage, ptr: ModelPtr<M>, field_selector: felt252, value: T | ||
| ) { | ||
| IWorldDispatcherTrait::set_entity( | ||
| self.dispatcher, | ||
| Model::<M>::selector(self.namespace_hash), | ||
| ModelIndex::MemberId((ptr.entity_id(), field_selector)), | ||
| serialize_inline(@value), | ||
| field_layout_unwrap::<M>(field_selector) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure error handling in write_member when the field layout is not found.
Ohayo, sensei! The write_member function relies on field_layout_unwrap, which panics if the field layout is not found. Consider handling this error case to improve robustness.
You might adjust the code to handle the None case explicitly:
fn write_member<T, +Serde<T>, +Drop<T>>(
ref self: WorldStorage, ptr: ModelPtr<M>, field_selector: felt252, value: T
) {
+ let layout_option = Model::<M>::field_layout(field_selector);
+ match layout_option {
+ Option::Some(layout) => {
IWorldDispatcherTrait::set_entity(
self.dispatcher,
Model::<M>::selector(self.namespace_hash),
ModelIndex::MemberId((ptr.entity_id(), field_selector)),
serialize_inline(@value),
- field_layout_unwrap::<M>(field_selector)
+ layout
);
+ },
+ Option::None => panic_with_felt252('Field layout not found in write_member')
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn write_member<T, +Serde<T>, +Drop<T>>( | |
| ref self: WorldStorage, ptr: ModelPtr<M>, field_selector: felt252, value: T | |
| ) { | |
| IWorldDispatcherTrait::set_entity( | |
| self.dispatcher, | |
| Model::<M>::selector(self.namespace_hash), | |
| ModelIndex::MemberId((ptr.entity_id(), field_selector)), | |
| serialize_inline(@value), | |
| field_layout_unwrap::<M>(field_selector) | |
| ); | |
| } | |
| fn write_member<T, +Serde<T>, +Drop<T>>( | |
| ref self: WorldStorage, ptr: ModelPtr<M>, field_selector: felt252, value: T | |
| ) { | |
| let layout_option = Model::<M>::field_layout(field_selector); | |
| match layout_option { | |
| Option::Some(layout) => { | |
| IWorldDispatcherTrait::set_entity( | |
| self.dispatcher, | |
| Model::<M>::selector(self.namespace_hash), | |
| ModelIndex::MemberId((ptr.entity_id(), field_selector)), | |
| serialize_inline(@value), | |
| layout | |
| ); | |
| }, | |
| Option::None => panic_with_felt252('Field layout not found in write_member') | |
| } | |
| } |
bengineer42
changed the title
glihm
changed the title
There was a problem hiding this comment.
Thanks @bengineer42. If you agree, let's rework the ModelPtr in this PR at once. Since it will mostly used with members (and erase).
As discussed, we want to convey the type information alongside the entity id.
So at some point, the model pointer is the entity id, and we don't need the keys here.
What would you think about:
pub enum EntityId<M> {
New: felt252,
}
let v: u32 = world.read_member(EntityId::<Foo>::New(1));Having a trait may also work, but a structure it will be more painful to import I guess.
| /// Deletes a model of type `M` using the provided entity id. | ||
| /// The ptr is mostly used for type inferrence. | ||
| fn erase_model_ptr(ref self: S, ptr: ModelPtr<M>); | ||
|
|
||
| /// Retrieves a model of type `M` using the provided entity idref . |
There was a problem hiding this comment.
| /// Retrieves a model of type `M` using the provided entity idref . | |
| /// Retrieves a model of type `M` using the provided entity id. |
|
Let's move on with what was discussed by adding the |
There was a problem hiding this comment.
Hey @bengineer42! Please if the PRs are expected to be stacked could you do so? If you prefer, you could do one in one PR with several separated commits. Works well too.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2611 +/- ##
==========================================
+ Coverage 54.90% 55.61% +0.70%
==========================================
Files 392 393 +1
Lines 48121 48580 +459
==========================================
+ Hits 26423 27019 +596
+ Misses 21698 21561 -137 ☔ View full report in Codecov by Sentry. |
Description
Added read write member access to models
Related issue
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
New Features
field_layoutandentity_id.Bug Fixes
Refactor
Chores