Model ptr to stuct #2612
Model ptr to stuct #2612
Conversation
WalkthroughOhayo, sensei! The pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (4)
crates/dojo/core-cairo-test/src/lib.cairo (1)
Line range hint
41-43: Consider activating additional test modules, sensei! 📝I notice there are still commented-out modules under the
worldsection. If these are ready for testing, consider activating them as well.crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
80-98: Ohayo! Consider enhancing test readability, sensei! 📚While the test logic is solid, we could make it even better with more descriptive assertions.
Consider this enhancement:
- assert_eq!(model_value.v1, foo.v1); - assert_eq!(model_value.v2, foo.v2); + assert_eq!( + model_value.v1, + foo.v1, + "Initial v1 value should match" + ); + assert_eq!( + model_value.v2, + foo.v2, + "Initial v2 value should match" + );
Line range hint
146-154: Consider additional test cases, sensei! 🎯While the basic deletion test is good, consider adding edge cases:
- Deleting non-existent models
- Deleting already deleted models
- Concurrent deletion scenarios
Would you like me to provide example implementations for these test cases?
crates/dojo/core/src/model/model.cairo (1)
50-51: Documentation needed for new trait methods, sensei!The new methods
field_layout,ptr,ptr_from_id, andinstance_ptrwould benefit from documentation explaining their purpose and usage patterns.Consider adding documentation like this:
+ /// Returns the layout of a specific field in the model given its selector. fn field_layout(field_selector: felt252) -> Option<Layout>; + /// Creates a model pointer from a given key. fn ptr<K, +Serde<K>, +Drop<K>>(key: K) -> ModelPtr<M>; + /// Creates a model pointer directly from an entity ID. fn ptr_from_id(entity_id: felt252) -> ModelPtr<M>; + /// Returns a pointer to this model instance. fn instance_ptr(self: @M) -> ModelPtr<M>;Also applies to: 62-67
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
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/lib.cairo(2 hunks)crates/dojo/core/src/model/model.cairo(5 hunks)crates/dojo/core/src/model/storage.cairo(1 hunks)crates/dojo/core/src/world/storage.cairo(6 hunks)
🔇 Additional comments (16)
crates/dojo/core-cairo-test/src/lib.cairo (1)
26-28: Ohayo! The model module activation looks good, sensei! 🎋
The uncommented model module aligns well with the test suite organization.
Let's verify the test coverage of the newly activated module:
✅ Verification successful
I'll examine the test implementations to ensure they provide adequate coverage and are properly structured before making a final determination.
Ohayo! The model module is well-tested and ready for action, sensei! 🎋
The test file contains comprehensive coverage with 9 test functions that verify:
- Model definition and metadata
- Value serialization and deserialization
- CRUD operations (Create, Read, Update, Delete)
- Field-level operations
- Error handling for invalid data
Plus two helper functions for test setup. All looking sugoi! ✨
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the model.cairo test file exists and contains test functions
# Test: Verify the existence and content of the model test file
rg -l "use test::test_utils::assert" "crates/dojo/core-cairo-test/src/tests/model/model.cairo"
# Test: Look for test functions in the model tests
rg "#\[test\]" "crates/dojo/core-cairo-test/src/tests/model/model.cairo"
Length of output: 239
Script:
#!/bin/bash
# Let's check the actual content of the test file to verify the test coverage
rg -A 5 "#\[test\]" "crates/dojo/core-cairo-test/src/tests/model/model.cairo"
# Also check for any test helper functions or setup code
rg "fn " "crates/dojo/core-cairo-test/src/tests/model/model.cairo"
Length of output: 2116
crates/dojo/core/src/lib.cairo (2)
49-49: Clean formatting of storage exports, sensei!
The storage-related exports are now well-organized and logically grouped, maintaining a clear separation of concerns.
37-37: Ohayo! The ModelPtr export addition looks good, sensei!
The addition of ModelPtr to the model module exports is logically placed alongside related types Model and KeyParser.
Let's verify the ModelPtr implementation:
✅ Verification successful
Ohayo! The ModelPtr export is properly implemented and used across the codebase, sensei!
The verification shows:
- ModelPtr is correctly defined in
model/model.cairoas a struct with anidfield - It's properly integrated into the Model trait with ptr-related methods
- Used consistently across the codebase in storage operations and world interactions
- The export in lib.cairo logically groups it with related model types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ModelPtr struct definition and its usage
# Expected: Find the ModelPtr struct definition and its usages
# Search for ModelPtr struct definition
echo "=== ModelPtr Definition ==="
ast-grep --pattern 'struct ModelPtr<$_> { $$$ }'
# Search for ModelPtr usages
echo -e "\n=== ModelPtr Usages ==="
rg "ModelPtr" -A 2
Length of output: 4747
crates/dojo/core/src/model/storage.cairo (2)
19-24: 🛠️ Refactor suggestion
The new member access methods need more guardrails, sensei! 🛡️
A few suggestions to make these methods more robust:
- The
field_selector: felt252parameter needs documentation explaining valid values and format - Consider adding constraints on type parameter
Tto ensure type safety - Consider using a strongly-typed enum for field selection instead of raw felt252 to prevent runtime errors
Here's a suggested improvement:
+/// Represents valid field selectors for model members
+#[derive(Drop, Serde)]
+enum FieldSelector {
+ // Add your field selectors here
+}
/// 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;
+fn read_member<T, +Serde<T>, +Drop<T>>(
+ self: @S,
+ ptr: ModelPtr<M>,
+ field_selector: FieldSelector
+) -> 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,
+ ptr: ModelPtr<M>,
+ field_selector: FieldSelector,
value: T
);Let's check if there are any existing field selector implementations we should align with:
#!/bin/bash
# Search for existing field selector patterns
ast-grep --pattern 'enum $name {
$$$
}'1-3: Ohayo! The TODO comment needs attention, sensei! 🎋
The comment suggests that the member access interface design isn't finalized. Since you're adding member access methods below, we should clarify the interface design first to avoid potential breaking changes later.
Let's check if there are any related discussions:
Would you like help designing and documenting the member access interface?
✅ Verification successful
Ohayo! The TODO comment is actually fine here, sensei! 🍜
The TODO comment is appropriate as it documents an ongoing design consideration. Looking at the codebase:
ModelPtris actively used across multiple files including world/storage.cairo and model/model.cairo- Member access methods (
read_member,write_member) are consistently implemented in both model/storage.cairo and world/storage.cairo - The interface is already well-structured and in use, just pending final design decisions
The TODO serves as a helpful reminder for future interface refinements while the current implementation is functional and consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related discussions about member access interface
rg -i "member access.*interface" --type md
Length of output: 137
Script:
#!/bin/bash
# Let's search for any related discussions or documentation about member access
rg -i "member\s+access" --type md
# Look for existing member access implementations
ast-grep --pattern 'fn read_member($$$)'
# Check for any related TODOs
rg -i "todo.*member"
# Look for ModelPtr usage to understand the context better
rg "ModelPtr"
Length of output: 2498
crates/dojo/core-cairo-test/src/tests/model/model.cairo (4)
1-3: Ohayo! Clean imports looking good, sensei! 🎌
The imports are well-organized and provide all necessary dependencies for the test module.
28-35: Excellent test namespace setup, sensei! ✨
The namespace_def function provides a clean way to define test resources. The implementation is clear and follows good practices for test organization.
37-39: Clean world spawning helper, sensei! 🌍
The spawn_foo_world function nicely encapsulates the world creation logic, making tests more maintainable.
115-130: Heads up about type inference, sensei! 🤔
The comment on line 122 indicates a potential improvement area regarding type inference. This might need attention in the future.
Let's check for similar inference issues:
Also, consider adding error messages to assertions for better debugging:
- assert!(foo.v1 == v1);
+ assert!(foo.v1 == v1, "Member read value should match original");✅ Verification successful
The previous command failed due to incorrect file extension. Let's try with a more comprehensive search to understand the context better and find similar patterns in the codebase.
Ohayo sensei! The type inference comment appears to be isolated 🍃
The search results show that this is the only occurrence of inference-related comments in the codebase. Since this is a test file and the comment is documenting a known limitation rather than a bug, the current implementation is acceptable.
The suggestion about adding error messages to assertions remains valid for better debugging experience:
crates/dojo/core-cairo-test/src/tests/model/model.cairo: Consider adding descriptive messages to assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential type inference issues
rg -g '*.cairo' 'Inference fails|need.*better' --type cairo
Length of output: 91
Script:
#!/bin/bash
# Search for type inference related comments with proper file extension
rg -g '*.{cairo,cairo.txt}' 'Inference|inference' -B2 -A2
# Also search for similar member access patterns
ast-grep --pattern 'world.read_member($_, selector!($_))'
Length of output: 598
crates/dojo/core/src/model/model.cairo (3)
8-12: Ohayo! Clean and well-structured pointer type implementation.
The ModelPtr<M> struct is elegantly minimal while providing all necessary functionality. The derived traits are appropriate for this use case.
111-113: Clean implementation of pointer operations, sensei!
The implementations are concise and make good use of helper functions. The pointer creation methods follow a consistent pattern and handle the entity ID conversions appropriately.
Also applies to: 141-149
8-12: Ohayo! Let's verify the pointer usage across the codebase.
The introduction of ModelPtr is a significant change. Let's ensure it's being used consistently.
Also applies to: 141-149
✅ Verification successful
I'll analyze these patterns to ensure ModelPtr is being used consistently and no direct felt252 entity IDs are being used where ModelPtr would be more appropriate.
Ohayo! ModelPtr implementation looks solid, sensei!
After analyzing the codebase, the ModelPtr introduction is well-structured and properly integrated:
- It's consistently used across the storage layer through
erase_model_ptr,read_member, and other storage operations - Direct
felt252entity_id usage is properly contained within internal implementation details (storage, world contract) - Public interfaces properly use
ModelPtrfor type safety and abstraction
The pointer changes are safe and maintain a clean separation between the public model interface and internal storage implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct felt252 usage that might need to be replaced with ModelPtr
rg -g '*.cairo' 'fn.*->.*felt252.*#\[.*storage_access'
# Look for existing ModelPtr usage patterns
rg -g '*.cairo' 'ModelPtr'
Length of output: 2368
Script:
#!/bin/bash
# Let's check for any direct entity_id usage that might need ModelPtr
rg -g '*.cairo' 'entity_id:.*felt252' -A 2
# Also check for any potential storage access patterns that might need updating
ast-grep --pattern 'trait $_ {
fn $_($_, entity_id: felt252) {
$$$
}
}'
# And look for any existing model implementations
ast-grep --pattern 'impl $_ of Model {
$$$
}'
Length of output: 7648
crates/dojo/core/src/world/storage.cairo (4)
110-110: Ohayo sensei! Using ptr.id simplifies the erase_model_ptr function
Directly using ptr.id improves readability and aligns with the updated logic.
114-136: Ohayo sensei! The read_member and write_member functions enhance member access functionality
The implementation of these functions is correct and adds valuable functionality for model member access.
213-213: Ohayo sensei! Added +Drop<M> constraint to ModelStorageTestWorldStorageImpl
Including the +Drop<M> constraint ensures proper memory management for model instances.
249-249: Ohayo sensei! Consistent use of ptr.id in erase_model_ptr_test
Using ptr.id here aligns the test implementation with the main implementation, improving consistency.
| 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') | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider improving the error message in field_layout_unwrap function
Including the field_selector value in the panic message can aid in debugging and provide more context.
Apply this diff to enhance the error message:
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'
+ 'Field layout not found for field_selector: ' + field_selector
)
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (3)
28-39: Well-structured test helpers, sensei! Consider adding documentation.The helper functions provide a clean way to set up test environments. The
namespace_deffunction properly registers both models, andspawn_foo_worldprovides a convenient wrapper.Consider adding documentation comments to explain the purpose of these helpers:
+/// Defines the test namespace with Foo and Foo2 models fn namespace_def() -> NamespaceDef { +/// Creates a test world with the Foo models namespace fn spawn_foo_world() -> WorldStorage {
101-112: Consider enhancing deletion verification.The test could be more robust by checking if the model exists before assuming it's zeroed out.
ModelStorage::<WorldStorage, Foo>::erase_model(ref world, @foo); +// First verify the model no longer exists +let exists = world.model_exists(foo.key()); +assert!(!exists, "Model should not exist after deletion"); + let read_values: FooValue = world.read_value_from_id(entity_id); assert!(read_values.v1 == 0 && read_values.v2 == 0);
157-164: Consider expanding model pointer test coverage.While the basic functionality is tested, consider adding tests for:
- Invalid pointer handling
- Multiple field access
- Pointer comparison
Example addition:
// Test invalid pointer let invalid_key = (42, 42); let invalid_ptr = Model::<Foo>::ptr(invalid_key); // Add assertions for expected error handling
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(5 hunks)
🔇 Additional comments (3)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (3)
1-3: LGTM! Clean imports for test requirements.
Ohayo! The imports are well-organized and include all necessary components for model testing.
80-98: LGTM! Clean test implementation using new API.
The test properly validates both read and write operations using the updated API methods.
123-123: TODO comment needs attention.
The comment about inference issues suggests there might be room for improvement in the type system.
Let's check if this is a known issue:
✅ Verification successful
The previous search was too specific. Let's broaden our search to find any related type inference issues or TODO comments that might give us more context about the situation.
Ohayo sensei! After reviewing the code, I can now provide a clear response about the TODO comment.
TODO comment is a known limitation with a workaround in place
The comment is actually documenting a specific type inference limitation in the test_read_and_write_field_name test function, but the code has a working solution implemented. The comment explains that while type inference fails, they've implemented a working approach that avoids excessive generics which would also fail.
The code is functioning correctly as evidenced by:
- The test passes and properly verifies the read/write functionality
- The implemented solution using
read_memberandwrite_memberworks as intended - The comment serves as documentation rather than indicating a problem that needs fixing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar inference comments or related issues
rg -g '!target' -g '!*.lock' "inference fails" .
Length of output: 50
Script:
#!/bin/bash
# Let's search for TODO comments and type inference related patterns more broadly
rg -g '!target' -g '!*.lock' -i "TODO.*type.*infer" .
# Also search for type inference related comments
rg -g '!target' -g '!*.lock' -i "type.*inference" .
# Check the specific file for TODO comments for more context
cat "crates/dojo/core-cairo-test/src/tests/model/model.cairo"
Length of output: 4740
There was a problem hiding this comment.
Thanks for the work here @bengineer42!
Could you please merge the two PRs in one, and rebase at the same time? Will merge once we have the single PR. 👍
| fn ptr<K, +Serde<K>, +Drop<K>>(key: K) -> ModelPtr<M> { | ||
| ModelPtr { id: entity_id_from_key(@key) } | ||
| } | ||
| fn ptr_from_id(entity_id: felt252) -> ModelPtr<M> { | ||
| ModelPtr::<M> { id: entity_id } | ||
| } | ||
| fn instance_ptr(self: @M) -> ModelPtr<M> { | ||
| ModelPtr::<M> { id: self.entity_id() } | ||
| } |
There was a problem hiding this comment.
Could you add a line break between functions please?
CHanged to match Model convention
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/dojo/core/src/model/model.cairo (2)
8-12: Ohayo! The ModelPtr struct looks clean and well-designed, sensei!The struct is minimal and focused, with appropriate trait derivations. Consider adding a doc comment example to show typical usage patterns.
Add usage example:
/// # Example /// ``` /// let ptr = ModelPtr { id: 123 }; /// ```
62-69: Well-structured pointer interface design, sensei!The four methods provide a comprehensive and flexible API for obtaining model pointers. This design allows for efficient pointer creation in various contexts.
Consider adding a caching mechanism for frequently accessed pointers to improve performance in high-throughput scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(5 hunks)crates/dojo/core/src/model/model.cairo(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/core-cairo-test/src/tests/model/model.cairo
🔇 Additional comments (2)
crates/dojo/core/src/model/model.cairo (2)
143-154: Implementation looks solid but needs formatting adjustment, sensei!
The pointer method implementations are correct and use appropriate utility functions.
Two suggestions:
- Add line breaks between functions as requested in the previous review
- Consider optimizing
ptr_from_keysfor large key spans:
fn ptr_from_keys(keys: Span<felt252>) -> ModelPtr<M> {
- ModelPtr { id: entity_id_from_keys(keys) }
+ // Early return for empty or single key cases
+ match keys.len() {
+ 0 => panic_with_felt252('empty_keys'),
+ 1 => ModelPtr { id: *keys.at(0) },
+ _ => ModelPtr { id: entity_id_from_keys(keys) }
+ }
}50-51: Clean implementation of field_layout, sensei!
The method provides a safe way to access field layouts with proper error handling through Option type.
Let's verify the field selector usage:
Also applies to: 113-115
✅ Verification successful
Field layout implementation is consistent across the codebase
The field_layout method is properly integrated with the rest of the codebase:
- Correctly uses
find_model_field_layoututility function - Consistent with storage operations in
world/storage.cairoandstorage/layout.cairo - Well-tested in
core-cairo-test/src/tests/utils/layout.cairo - Properly used by model store implementations for member access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for field selector usage patterns to ensure consistency
rg "field_layout|field_selector" -A 2
Length of output: 17769
Description
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
modelmodule in the test suite for improved testing capabilities.Modeltrait, enhancing functionality for handling model pointers and field layouts.ModelStoragetrait for improved model member manipulation.Bug Fixes
field_layout_unwrapfunction to ensure proper layout retrieval.Refactor
erase_model_ptrfunction for simplified logic in entity ID handling.Documentation