Re added members and converted ModelPtr to a struct #2634
Conversation
CHanged to match Model convention
WalkthroughOhayo, sensei! This pull request introduces several enhancements across multiple files, primarily focusing on the Changes
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: 0
🧹 Outside diff range and nitpick comments (8)
crates/dojo/core/src/model/storage.cairo (2)
1-3: Ohayo sensei! The TODO comment seems outdated.The comment about defining the right interface for member accesses appears to be outdated since you've now implemented
read_memberandwrite_membermethods in theModelStoragetrait.Consider removing this TODO comment or updating it to reflect any remaining member access interface concerns.
27-28: Consider consolidating the documentation, sensei!The documentation for
erase_model_ptris split across two lines unnecessarily.- /// Deletes a model of type `M` using the provided entity id. - /// The ptr is mostly used for type inferrence. + /// Deletes a model of type `M` using the provided entity id. The ptr is mostly used for type inference.Also, note the spelling correction from "inferrence" to "inference".
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
157-212: Excellent ModelPtr test coverage, sensei!The new tests thoroughly cover ModelPtr functionality with different creation methods. The entity ID verification ensures pointer consistency.
Consider adding tests for error cases, such as:
- Invalid keys
- Non-existent models
- Malformed entity IDs
Would you like me to provide example test cases for these error scenarios?
Ohayo sensei! After analyzing the Model trait implementations and test coverage, I can provide a conclusive response.
Model trait test coverage is comprehensive with minor gaps
The core Model trait methods are well tested:
- ✓
key()andkeys()(tested in multiple ptr tests)- ✓
entity_id()(tested in ptr tests)- ✓
values()(direct test)- ✓
from_values()(direct test including error case)However, there are some gaps in testing the ModelStorage trait methods:
- Missing test for
write_models()(bulk write)- Missing test for
read_models()(bulk read)The existing tests provide good coverage of:
- Model definition
- Value operations
- Field-level operations
- Entity ID operations
- Pointer operations
- Delete operations
🔗 Analysis chain
Line range hint
1-213: Verify complete Model trait coverage.Let's ensure we have tests for all Model trait methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find Model trait definition and compare with test coverage echo "Searching for Model trait methods..." ast-grep --pattern 'trait Model { $$$ }' echo "Searching for corresponding test functions..." rg "^fn test_.*model.*" --type cairoLength of output: 297
Script:
#!/bin/bash # Let's try a different approach to find the Model trait and test functions # Search for trait Model definition echo "=== Searching for Model trait ===" rg "trait Model" -A 10 # Search for test functions in the model test file echo -e "\n=== Searching for model tests ===" rg "^fn test_" "crates/dojo/core-cairo-test/src/tests/model/model.cairo"Length of output: 18726
crates/dojo/core/src/model/model.cairo (2)
163-183: Refactor to reduce code duplication inptrs_from_*methodsOhayo sensei! The implementations of
ptrs_from_key,ptrs_from_keys, andptrs_from_idshare similar logic for constructing arrays ofModelPtr<M>. Consider refactoring this common logic into a generic helper function to enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle.Here's a possible refactor:
fn ptrs_from_key<K, +Serde<K>, +Drop<K>>(keys: Span<K>) -> Span<ModelPtr<M>> { let mut ptrs: Array<ModelPtr<M>> = array![]; for key in keys { - ptrs.append(ModelPtr { id: entity_id_from_key(key) }) + ptrs.append(Self::ptr_from_key(key)) }; ptrs.span() } fn ptrs_from_keys(keys: Span<Span<felt252>>) -> Span<ModelPtr<M>> { let mut ptrs: Array<ModelPtr<M>> = array![]; for _keys in keys { - ptrs.append(ModelPtr { id: entity_id_from_keys(*_keys) }) + ptrs.append(Self::ptr_from_keys(*_keys)) }; ptrs.span() } fn ptrs_from_id(entity_ids: Span<felt252>) -> Span<ModelPtr<M>> { let mut ptrs: Array<ModelPtr<M>> = array![]; for id in entity_ids { - ptrs.append(ModelPtr { id: *id }) + ptrs.append(Self::ptr_from_id(*id)) }; ptrs.span() }This refactor leverages the existing
ptr_from_*methods to eliminate duplicate code and improve clarity.
Line range hint
78-183: Consider adding error handling or validationOhayo sensei! While the methods for generating
ModelPtrinstances are well-implemented, consider adding error handling or validation to account for potential invalid inputs or edge cases. This can enhance the robustness of your code.crates/dojo/core/src/world/storage.cairo (2)
20-25: Ohayo, sensei! Enhance the panic message infield_layout_unwrapfor better debuggingIn the
field_layout_unwrapfunction, consider including thefield_selectorvalue in the panic message to provide more context when a bad member ID is encountered.Apply this diff to improve the panic 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') + Option::None => panic_with_felt252('bad member id: {field_selector}') } }
27-33: Ohayo, sensei! Optimizemodel_ptrs_to_idsby preallocating array capacityConsider preallocating the capacity of the
idsarray to improve performance, especially when dealing with large spans.Apply this diff to preallocate the array:
fn model_ptrs_to_ids<M, +Model<M>>(ptrs: Span<ModelPtr<M>>) -> Span<felt252> { - let mut ids: Array<felt252> = array![]; + let mut ids: Array<felt252> = Array::with_capacity(ptrs.len()); for ptr in ptrs { ids.append(*ptr.id); }; ids.span() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(5 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 (18)
crates/dojo/core-cairo-test/src/lib.cairo (1)
26-28: Ohayo! The model module addition looks good, sensei! 🎋
The inclusion of the model module in the test suite aligns perfectly with the PR's objectives of converting ModelPtr to a struct and re-adding members. This ensures proper test coverage for these significant changes.
Let's verify the model test module structure:
✅ Verification successful
Ohayo! The model test module is perfectly structured and comprehensive, sensei! ✨
The test file contains extensive test coverage for the ModelPtr functionality, including:
- Model definition tests
- Value serialization and deserialization
- Model read/write operations
- Entity ID handling
- Key-based operations
- Proper cleanup tests
The module structure follows the project's conventions and is correctly referenced in lib.cairo. All tests align perfectly with the PR's objective of converting ModelPtr to struct and re-adding members.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the model test module structure and contents
# Expected: Should find the model.cairo test file in the correct location
fd --type f "model.cairo" --exec echo "Found model test file: {}"
Length of output: 283
Script:
#!/bin/bash
# Let's verify the content of the test file to ensure it's properly structured
cat "./crates/dojo/core-cairo-test/src/tests/model/model.cairo"
Length of output: 6242
Script:
#!/bin/bash
# Let's also check if there are any references to this test module in other files
rg -l "mod model" ./crates/dojo/core-cairo-test/
Length of output: 94
crates/dojo/core/src/lib.cairo (2)
37-37: Ohayo sensei! LGTM on adding ModelPtr to model exports.
The addition of ModelPtr to the model module's public exports aligns perfectly with its new implementation as a struct and follows the principle of keeping related functionality together.
49-49: LGTM on removing ModelPtr from storage exports.
Removing ModelPtr from storage exports eliminates duplication and ensures a single source of truth for the ModelPtr type, which is now properly exported from the model module.
crates/dojo/core/src/model/storage.cairo (2)
30-35: Ohayo! Nice addition of member access methods, sensei!
The new read_member and write_member methods are well-designed with:
- Proper generic constraints using
Serde - Consistent parameter patterns using
ModelPtr - Clear separation of read and write operations
1-1: Verify ModelPtr struct conversion implementation.
The PR objectives mention converting ModelPtr to a struct, but we only see the import here. Let's verify the implementation.
crates/dojo/core-cairo-test/src/tests/model/model.cairo (3)
Line range hint 1-39: Ohayo sensei! Clean test setup with proper abstractions!
The new helper functions provide a clean and reusable way to set up test environments. The namespace definition is well-structured and the spawn_foo_world function offers a nice abstraction for test setup.
Line range hint 80-154: Test refactoring improves clarity and coverage!
The renamed test functions better describe their purpose, and the new member access tests provide good coverage of the API. The transition from model-specific methods to more generic read/write operations is well-implemented.
122-123: Address type inference limitation.
The comment indicates a limitation with type inference. Consider documenting the issue in more detail and creating a tracking issue for improvement.
Let's check if this is a known issue:
#!/bin/bash
# Search for related type inference issues or TODOs
rg -i "type inference|inference fails" --type cairoWould you like me to create a GitHub issue to track this type inference limitation?
crates/dojo/core-cairo-test/src/tests/helpers.cairo (1)
203-203: Ohayo! The ModelPtr construction looks good, sensei!
The conversion of ModelPtr to a struct is implemented correctly here, using poseidon hash for the id computation. The implementation maintains data integrity while improving code clarity.
Let's verify consistent ModelPtr usage across the codebase:
✅ Verification successful
Ohayo! The ModelPtr implementation is consistently used across the codebase, sensei!
The verification shows that:
- No instances of the old
ModelPtr::Idstyle were found - All ModelPtr initializations follow the new struct-style pattern with the
idfield - The implementation in the test helper aligns perfectly with the core implementation patterns seen in
crates/dojo/core/src/model/model.cairo
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old-style ModelPtr::Id usage that needs updating
# and verify the new struct-style initialization is used consistently
# Search for any remaining old Id-style initialization
echo "Checking for old-style ModelPtr::Id usage..."
rg "ModelPtr::Id"
# Search for new struct-style initialization pattern
echo "Verifying new struct-style initialization..."
rg "ModelPtr\s*[{<].*id:\s*"
Length of output: 724
crates/dojo/core/src/model/model.cairo (7)
9-12: Definition of ModelPtr<M> is well-structured and appropriate
Ohayo sensei! The ModelPtr<M> struct is properly defined with the necessary derive attributes, ensuring it can be copied, dropped, serialized, and debugged. This provides a solid foundation for model pointers in the codebase.
51-53: Consistent use of field_selector as felt252
Ohayo sensei! The field_layout method accepts field_selector as a felt252, which aligns with the existing codebase where identifiers are often represented as felt252. This consistency enhances maintainability and readability.
62-75: Addition of pointer methods enhances the Model trait functionality
Ohayo sensei! Introducing methods like ptr_from_key, ptr_from_keys, and ptr_from_id significantly improves how models can be referenced and manipulated. This enhancement promotes more flexible and efficient model handling.
121-123: Implementation of field_layout correctly utilizes helper function
Ohayo sensei! The implementation of field_layout correctly calls find_model_field_layout with the model's layout and the given field_selector. This ensures that field layouts are accurately retrieved.
151-153: Efficient generation of ModelPtr from a single key
Ohayo sensei! The ptr_from_key method efficiently creates a ModelPtr by computing the entity ID from the key using entity_id_from_key. This provides a straightforward way to obtain model pointers from keys.
172-173: Correct dereferencing of _keys in the loop
Ohayo sensei! In the ptrs_from_keys method, the dereferencing of _keys using *_keys is appropriate since _keys is a reference to a Span<felt252>. This ensures that the entity_id_from_keys function receives the correct data.
179-180: Confirm necessity of dereferencing *id in ptrs_from_id
Ohayo sensei! The dereferencing of id using *id in the ptrs_from_id method appears necessary since id is a reference to a felt252. This ensures the correct value is used when creating a ModelPtr. Please verify that this aligns with the ownership semantics of your codebase.
If you encounter any issues, consider the following:
- ptrs.append(ModelPtr { id: *id })
+ ptrs.append(ModelPtr { id: id })crates/dojo/core/src/world/storage.cairo (2)
193-204: Ohayo, sensei! Ensure robust error handling in read_member
Verify that deserialize_unwrap properly handles cases where deserialization might fail, to prevent unexpected panics. Consider handling the error gracefully or providing informative error messages.
205-215: Ohayo, sensei! Confirm correct serialization in write_member
Ensure that serialize_inline correctly serializes the value parameter according to the expected layout. This maintains data integrity when writing members.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (1)
156-211: Ohayo! Consider adding error case tests, sensei.While the new model pointer tests provide good coverage for successful scenarios, it would be beneficial to add tests for error cases such as:
- Invalid keys
- Non-existent models
- Malformed entity IDs
Would you like me to provide example test cases for these scenarios?
crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo (1)
200-200: Ohayo sensei! The ModelPtr struct initialization looks good!The change aligns well with the PR objective of converting ModelPtr to a struct. The implementation correctly initializes the struct with an id field derived from the caller's address using poseidon hash.
However, consider adding a comment explaining the hash calculation for better maintainability.
let ptr = ModelPtr::< Foo - > { id: core::poseidon::poseidon_hash_span([get_caller_address().into()].span()) }; + > { + // Generate unique id by hashing the caller's address as the key + id: core::poseidon::poseidon_hash_span([get_caller_address().into()].span()) + };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(5 hunks)crates/dojo/core/src/lib.cairo(2 hunks)crates/dojo/core/src/model/model.cairo(5 hunks)examples/simple/src/lib.cairo(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/core/src/lib.cairo
🔇 Additional comments (12)
examples/simple/src/lib.cairo (2)
42-42: Ohayo! Import changes look good, sensei!
The updated imports align well with the architectural changes of converting ModelPtr to a struct.
94-94: Ohayo! Let's verify the pointer creation pattern, sensei!
The new ptr_from_id syntax is clearer and aligns well with the ModelPtr struct changes.
Let's ensure this pattern is used consistently across the codebase:
✅ Verification successful
The previous search showed the new pattern is used consistently, but let's verify there aren't any other pointer creation patterns we should be aware of.
Let's try with ripgrep to get more context around these patterns, as ast-grep didn't yield results. This will help us ensure we're not missing any variations of pointer creation.
Ohayo! The pointer pattern is consistently used across the codebase, sensei!
Looking at the codebase, we can see that:
- The old
ModelPtr::Idpattern is completely replaced - The new
Model::<M>::ptr_from_idis used consistently in both test and implementation code - The implementation in
core/src/model/model.cairoshows this is the canonical way to create model pointers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining old ModelPtr::Id usage
rg "ModelPtr::<\w+>::Id"
# Search for the new pattern usage
rg "Model::<\w+>::ptr_from_id"
Length of output: 274
Script:
#!/bin/bash
# Let's search for any other model pointer creation patterns to ensure completeness
ast-grep --pattern 'ModelPtr::<$_>::$_($$$)'
# Also search for Model:: patterns to catch any variations
ast-grep --pattern 'Model::<$_>::$_($$$)'
Length of output: 90
Script:
#!/bin/bash
# Let's try a broader search with ripgrep for any ModelPtr usage
rg "ModelPtr::" -A 2 -B 2
# And similarly for Model:: usage
rg "Model::" -A 2 -B 2
Length of output: 53081
crates/dojo/core-cairo-test/src/tests/model/model.cairo (3)
28-39: Ohayo! Nice helper function implementation, sensei!
The new helper functions namespace_def and spawn_foo_world improve test maintainability by centralizing the test setup logic. This is a good practice that reduces code duplication across test cases.
120-122: Consider addressing the inference limitation, sensei.
The comment indicates an inference issue that requires explicit type annotation. This might be worth investigating for a more ergonomic solution in the future.
Let's check if this is a known issue:
Consider creating an issue to track this limitation and explore potential improvements to the type inference system.
Line range hint 1-212: Verify test coverage for model operations.
The test suite looks comprehensive, but let's ensure we haven't missed any model operations.
✅ Verification successful
Test coverage is comprehensive, sensei!
Based on the test file output, we have thorough coverage of all core model operations including:
- Model definition and metadata
- Value serialization/deserialization
- CRUD operations (read, write, update, delete)
- Key and entity ID handling
- Pointer operations
- Error cases
The test suite demonstrates a systematic approach to verifying model functionality with both positive and negative test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all public model operations in the codebase
echo "Searching for model operations that might need tests..."
ast-grep --pattern 'pub fn $FUNC($$$) {
$$$
}'
# Compare with existing test coverage
echo "Checking existing test coverage..."
rg "^fn test_" --type cairo
Length of output: 47842
Script:
#!/bin/bash
# Search for model operations in the codebase
echo "Searching for model operations..."
rg "pub fn" --type rust crates/dojo/model/ -A 1
# Check test coverage in the test file
echo "Checking test coverage..."
rg "^fn test_" crates/dojo/core-cairo-test/src/tests/model/model.cairo
Length of output: 807
Script:
#!/bin/bash
# Let's check the actual model operations in the correct path
echo "Searching for model operations..."
rg "pub fn" --type rust crates/dojo/core/src/model/ -A 1
# Also check model traits to ensure we're testing all required functionality
echo "Checking model traits..."
rg "trait \w+Model" --type rust crates/dojo/core/src/model/ -B 1 -A 5
Length of output: 266
Cargo.toml (1)
195-196: Ohayo sensei! Moving back to the main scarb repository looks good.
The change from remybar's fork to the main dojoengine/scarb repository is a positive move. However, let's verify the impact of this significant version bump.
Let's check for any breaking changes in the new scarb version:
✅ Verification successful
Ohayo sensei! The scarb dependency update looks safe to proceed with.
The codebase extensively uses scarb APIs and utilities, but the changes in the new version (from remybar/scarb to dojoengine/scarb) appear to be minor, containing only "chore: bump dojo" commits. The integration points are well-structured and primarily focused on workspace operations, compilation, and configuration management.
- The scarb dependency is used across multiple critical paths:
- Build and test infrastructure
- Workspace management
- Compilation and code generation
- Configuration handling
However, since the changes in the new version are minimal (just dojo version bumps) and we're moving back to the main repository, this update is safe and follows best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in the scarb dependency
# Compare the old and new commits to see changes
echo "Checking changes between scarb versions..."
gh api repos/dojoengine/scarb/compare/fbad7d8d6bdb763b413055d8d9f8b7995672f145...b9965b7e2f0d97f2a97f18ca9a75bac541de7d84 \
--jq '.commits[].commit.message'
# Look for any BREAKING CHANGE markers in commit messages
echo "Looking for breaking changes..."
gh api repos/dojoengine/scarb/compare/fbad7d8d6bdb763b413055d8d9f8b7995672f145...b9965b7e2f0d97f2a97f18ca9a75bac541de7d84 \
--jq '.commits[].commit.message' | grep -i "breaking"
Length of output: 534
Script:
#!/bin/bash
# Let's check how scarb is used in the codebase to assess impact
echo "Files using scarb dependencies:"
rg "use (scarb|scarb_ui)" -l
echo "\nChecking for any direct scarb API usage:"
rg "scarb::" -A 2
echo "\nLooking for any scarb-specific configurations:"
rg "scarb" Cargo.toml -A 2 -B 2
# Let's also check if there are any scarb-specific test files
echo "\nScarb-related test files:"
fd "test.*\.(rs|toml)" --exec grep -l "scarb" {} \;
Length of output: 21155
crates/dojo/core/src/model/model.cairo (6)
2-3: Ohayo, sensei! Import statements are appropriate
The added imports for Layout, Struct, compute_packed_size, entity_id_from_keys, find_model_field_layout, and entity_id_from_key are necessary for the new functionalities introduced.
10-13: Ohayo, sensei! ModelPtr<M> struct is well-defined
The ModelPtr<M> struct is correctly defined with the necessary derive macros. This will facilitate pointer management for models effectively.
Line range hint 49-74: Ohayo, sensei! Enhancement of the Model trait looks great
The addition of pointer methods and field_layout in the Model trait significantly enhances its functionality. These methods provide flexible ways to interact with models and their fields.
77-79: Ohayo, sensei! Implementation constraints are properly updated
Including +Drop<Array<ModelPtr<M>>> ensures correct memory management for arrays of ModelPtr<M> instances, which is crucial for avoiding memory leaks.
116-119: Ohayo, sensei! field_layout method implementation is correct
The field_layout method correctly utilizes find_model_field_layout to retrieve the layout of a specific field, enhancing introspection capabilities.
145-177: Ohayo, sensei! ModelPtr methods are implemented effectively
The methods ptr_from_key, ptr_from_keys, ptr_from_id, and their counterparts are properly implemented. They provide various ways to obtain ModelPtr<M> instances, improving the API's flexibility.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2634 +/- ##
=======================================
Coverage 56.90% 56.91%
=======================================
Files 397 397
Lines 49461 49461
=======================================
+ Hits 28148 28150 +2
+ Misses 21313 21311 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
crates/dojo/core/src/model/storage.cairo (1)
38-44: Excellent addition of granular member access methods, sensei!The new
read_memberandwrite_membermethods provide fine-grained control over model member access. However, consider adding more detailed documentation:
- Explain the format/structure of the
field_selector- Provide examples of typical usage
- Document any constraints or expectations for the generic type
TExample documentation format:
/// Retrieves a specific member of type `T` from a model using the provided ModelPtr and field selector. /// /// # Arguments /// * `ptr` - Pointer to the model instance /// * `field_selector` - Unique identifier for the field to be read /// /// # Type Parameters /// * `T` - The type of the member to be read, must implement Serde /// /// # Examples /// ``` /// let value = storage.read_member(model_ptr, selector); /// ```crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
79-97: Consider enhancing test failure messages, sensei!While the test logic is solid, adding descriptive messages to assertions would make debugging failures easier.
- assert_eq!(model_value.v1, foo.v1); + assert_eq!(model_value.v1, foo.v1, "Initial v1 value mismatch"); - assert_eq!(model_value.v2, foo.v2); + assert_eq!(model_value.v2, foo.v2, "Initial v2 value mismatch");
156-182: Excellent pointer test coverage, but consider reducing duplication!The new pointer tests provide great coverage of different pointer creation methods. However, there's some duplication in the test setup and verification logic.
Consider extracting the common test logic into a helper function:
fn verify_ptr_functionality<T>(world: WorldStorage, model: T, ptr: ModelPtr) { world.write_model(@model); let v1 = world.read_member(ptr, selector!("v1")); assert!(model.v1 == v1, "v1 value mismatch"); }This would make the tests more concise and maintainable:
#[test] fn test_model_ptr_from_key() { let mut world = spawn_foo_world(); let foo = Foo { k1: 1, k2: 2, v1: 3, v2: 4 }; verify_ptr_functionality(world, foo, Model::<Foo>::ptr_from_key(foo.key())); }crates/dojo/core/src/model/model.cairo (2)
8-13: Ohayo! Consider enhancing the ModelPtr documentation.The current documentation could be more descriptive about the purpose and usage of ModelPtr. Consider adding details about:
- The relationship between ModelPtr and the Model it points to
- The significance of the
idfield- Common use cases
/// A pointer to a model, which can be expressed by an entity id. +/// +/// ModelPtr serves as a lightweight reference to a Model instance, using a felt252 +/// entity id for efficient storage and lookup. It's commonly used for: +/// - Establishing relationships between different models +/// - Storing references without keeping the entire model in memory +/// - Efficient model lookups in storage systems #[derive(Copy, Drop, Serde, Debug, PartialEq)] pub struct ModelPtr<M> { pub id: felt252, }
138-152: Consider documenting type bounds and performance characteristics.The pointer methods look good, but could benefit from some additional documentation:
- Explain the
SerdeandDropbounds on theKtype parameter inptr_from_key- Document the performance implications of entity ID calculations
+ /// Creates a ModelPtr from a key of type K. + /// + /// # Type Parameters + /// * `K` - Must implement Serde for serialization and Drop for cleanup + /// + /// # Performance + /// Involves computing an entity ID using Poseidon hash fn ptr_from_key<K, +Serde<K>, +Drop<K>>(key: K) -> ModelPtr<M>; + /// Creates a ModelPtr from a span of felt252 keys. + /// + /// # Performance + /// Involves computing an entity ID using Poseidon hash fn ptr_from_keys(keys: Span<felt252>) -> ModelPtr<M>; + /// Creates a ModelPtr directly from an entity ID. + /// + /// # Performance + /// O(1) - No hash computation needed fn ptr_from_id(entity_id: felt252) -> ModelPtr<M>; + /// Creates a ModelPtr from the current model instance. + /// + /// # Performance + /// Reuses the already computed entity ID fn ptr(self: @M) -> ModelPtr<M>;crates/dojo/core/src/world/storage.cairo (1)
Line range hint
1-541: Ohayo sensei! Excellent architectural improvements.The conversion of ModelPtr to a struct and the addition of granular member operations have improved the codebase in several ways:
- More efficient operations using entity IDs
- Better error handling with field_layout_unwrap
- Granular control over model fields
- Consistent implementation across production and test code
Consider adding documentation comments for the new member operations to help other developers understand their proper usage and best practices.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(5 hunks)crates/dojo/core/src/model/model.cairo(5 hunks)crates/dojo/core/src/model/storage.cairo(2 hunks)crates/dojo/core/src/world/storage.cairo(6 hunks)
🔇 Additional comments (10)
crates/dojo/core/src/model/storage.cairo (2)
1-1: Ohayo! Clean import organization, sensei!
The nested import structure provides better organization and clarity for the ModelPtr and ModelValueKey imports.
34-35: Documentation clarity improved, sensei!
The updated documentation better explains the purpose of the ptr parameter in type inference.
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
Line range hint 1-39: Ohayo! Clean test infrastructure setup, sensei!
The new helper functions provide a well-organized approach to test world creation and resource registration. The separation of concerns between namespace_def and spawn_foo_world follows good testing practices.
156-182: Consider adding error case tests for pointer creation!
While the happy path is well tested, it would be valuable to add tests for error cases, such as:
- Invalid keys
- Non-existent entity IDs
- Boundary conditions
This would ensure robust error handling in the pointer creation methods.
crates/dojo/core/src/model/model.cairo (2)
49-50: LGTM! Well-structured trait extensions, sensei!
The new methods enhance the Model trait in two valuable ways:
- The field_layout method provides fine-grained introspection capabilities
- The comprehensive set of ptr_* methods offers flexibility in ModelPtr creation
The API design is clean and follows good abstraction principles.
Also applies to: 61-68
108-110: LGTM! Clean implementation of field_layout.
The implementation correctly delegates to the utility function while maintaining a clean API boundary.
crates/dojo/core/src/world/storage.cairo (4)
9-10: Ohayo sensei! Clean implementation of field layout unwrapping.
The new utility function provides proper error handling for field layout retrieval, which is essential for the new member operations.
Also applies to: 20-25
163-173: Ohayo! Improved efficiency in model deletion.
The change to use entity IDs directly is more efficient than processing keys. This aligns well with the new ModelPtr structure.
Line range hint 382-447: Test implementations properly aligned with production changes.
The test implementations have been updated to consistently use ModelPtr and entity IDs, maintaining parity with the production code changes.
185-207: Clean implementation of granular member operations, sensei!
The new read_member and write_member functions provide fine-grained control over model fields with proper error handling through field_layout_unwrap.
Let's verify the member operations are used consistently:
✅ Verification successful
The previous script failed because ripgrep didn't recognize the cairo file type. Let's try without the file type restriction to find any usage of these member operations and also look for the struct definition for more context.
Ohayo! The member operations are well-implemented and properly tested, sensei!
The verification shows that:
- The member operations are defined in the trait interface (
model/storage.cairo) - The implementation in
WorldStoragefollows the trait specification - The operations are thoroughly tested in
core-cairo-test/src/tests/model/model.cairowith multiple test cases covering both read and write operations - The tests verify correct value persistence and retrieval
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of new member operations
rg -A 2 'read_member|write_member' --type cairo
Length of output: 79
Script:
#!/bin/bash
# Search for usage of new member operations without file type restriction
rg -A 2 'read_member|write_member'
# Also search for the struct definition to understand the context
rg -A 5 'struct WorldStorage'
Length of output: 2970
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
Release Notes
New Features
Modeltrait.ModelStorageandWorldStorageimplementations.modelmodule in the test suite for better visibility.Bug Fixes
Chores
Cargo.tomlto align with the latest commit in the repository.