feat(dojo-core): add support to read/write the same member from multiple models #2939
Conversation
WalkthroughOhayo, sensei! This pull request introduces several enhancements to the Dojo core model handling capabilities. The changes focus on expanding model storage and testing functionalities, adding new methods for reading and writing multiple model members across different components. The modifications span across several files in the core module, introducing traits and implementations that support batch operations on model pointers and storage, alongside new test functions to ensure the correctness of these operations. Changes
Possibly related PRs
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 comments (1)
crates/dojo/core/src/world/storage.cairo (1)
Line range hint
198-239: Ohayo sensei! Don't forget the test implementations.The new batch operations methods need corresponding test implementations in the
ModelStorageTestWorldStorageImpltrait, similar to other methods in this file.Consider adding these test implementations:
#[cfg(target: "test")] fn read_members_test<T, +Serde<T>, +Drop<T>>( self: @WorldStorage, ptrs: Span<ModelPtr<M>>, field_selector: felt252, ) -> Array<T> { let world_test = dojo::world::IWorldTestDispatcher { contract_address: self.dispatcher.contract_address, }; // Implementation similar to read_members but using test dispatcher } #[cfg(target: "test")] fn write_members_test<T, +Serde<T>, +Drop<T>>( ref self: WorldStorage, ptrs: Span<ModelPtr<M>>, field_selector: felt252, values: Span<T>, ) { let world_test = dojo::world::IWorldTestDispatcher { contract_address: self.dispatcher.contract_address, }; // Implementation similar to write_members but using test dispatcher }
🧹 Nitpick comments (5)
crates/dojo/core/src/model/storage.cairo (1)
41-44: Ohayo! The batch operations look solid, sensei!The new methods
read_membersandwrite_membersare well-designed with appropriate generic constraints. They complement the existing single-item operations nicely.Consider adding a note in the documentation about the expected behavior when the lengths of
ptrsandvaluesdon't match inwrite_members.Also applies to: 51-54
crates/dojo/core/src/model/model.cairo (1)
20-36: Implementation looks efficient, but could be optimized further!The implementation is clean and straightforward.
Consider pre-allocating the array size if possible:
- let mut ids = ArrayTrait::<ModelIndex>::new(); + let mut ids = ArrayTrait::<ModelIndex>::with_capacity(self.len());crates/dojo/core-cairo-test/src/tests/model/model.cairo (3)
198-207: Ohayo! Good basic test coverage for read_member, sensei!Consider adding a test case for reading a non-existent member to verify error handling.
209-220: Batch read operations are well-tested!Consider adding test cases for:
- Empty array of pointers
- Array with duplicate pointers
222-230: Write operations test coverage looks good!Consider adding test cases for:
- Writing to non-existent models
- Writing with mismatched ptrs and values lengths
Also applies to: 231-244
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)crates/dojo/core/src/model/storage.cairo(1 hunks)crates/dojo/core/src/world/storage.cairo(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (4)
crates/dojo/core/src/lib.cairo (1)
45-45: Ohayo! LGTM on the model exports update, sensei!The addition of
ModelPtrsTraitto the public interface is well-structured and follows the existing export pattern.crates/dojo/core/src/model/model.cairo (1)
15-18: Ohayo! Clean trait design, sensei!The
ModelPtrsTraitis well-designed with a clear, focused purpose.crates/dojo/core/src/world/storage.cairo (2)
5-7: LGTM! Clean import additions.The new imports
ModelPtrandModelPtrsTraitare properly added to support the new batch operations functionality.
198-211: Ohayo sensei! Consider adding length validation.The implementation efficiently handles batch reading of members, but it would be safer to verify that the number of values returned matches the number of pointers provided.
Consider adding a validation check:
fn read_members<T, +Serde<T>, +Drop<T>>( self: @WorldStorage, ptrs: Span<ModelPtr<M>>, field_selector: felt252, ) -> Array<T> { let mut values: Array<T> = array![]; + let ptrs_len = ptrs.len(); for entity in IWorldDispatcherTrait::entities( *self.dispatcher, Model::<M>::selector(*self.namespace_hash), ptrs.to_member_indexes(field_selector), field_layout_unwrap::<M>(field_selector), ) { values.append(deserialize_unwrap(*entity)); }; + assert(values.len() == ptrs_len, 'Mismatched values count'); values }✅ Verification successful
Ohayo sensei! The length validation suggestion is verified as beneficial.
The test cases show that
read_membersis expected to return values for all provided pointers, maintaining exact correspondence. Adding length validation would help catch contract violations early and make the implicit assumption explicit.• The validation aligns with existing test cases in
core-cairo-test/src/tests/model/model.cairowhich verify exact array contents
• It would help detect issues before they propagate to array content assertions🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Look for similar read operations and patterns ast-grep --pattern 'fn read_$_($$$) { $$$deserialize_unwrap($$$) }' # Search for test cases involving read_members rg "read_members" -A 5 -B 5 # Look for entities implementation ast-grep --pattern 'fn entities($$$) -> $$$'Length of output: 4505
| fn write_members<T, +Serde<T>, +Drop<T>>( | ||
| ref self: WorldStorage, ptrs: Span<ModelPtr<M>>, field_selector: felt252, values: Span<T>, | ||
| ) { | ||
| let mut serialized_values = ArrayTrait::<Span<felt252>>::new(); | ||
| for value in values { | ||
| serialized_values.append(serialize_inline(value)); | ||
| }; | ||
| IWorldDispatcherTrait::set_entities( | ||
| self.dispatcher, | ||
| Model::<M>::selector(self.namespace_hash), | ||
| ptrs.to_member_indexes(field_selector), | ||
| serialized_values.span(), | ||
| field_layout_unwrap::<M>(field_selector), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider these essential improvements.
The implementation would benefit from two important changes:
- Length validation between ptrs and values spans
- Memory optimization by avoiding the intermediate array
Here's the suggested implementation:
fn write_members<T, +Serde<T>, +Drop<T>>(
ref self: WorldStorage, ptrs: Span<ModelPtr<M>>, field_selector: felt252, values: Span<T>,
) {
+ assert(ptrs.len() == values.len(), 'Length mismatch');
- let mut serialized_values = ArrayTrait::<Span<felt252>>::new();
- for value in values {
- serialized_values.append(serialize_inline(value));
- };
IWorldDispatcherTrait::set_entities(
self.dispatcher,
Model::<M>::selector(self.namespace_hash),
ptrs.to_member_indexes(field_selector),
- serialized_values.span(),
+ values.map(|v| serialize_inline(v)),
field_layout_unwrap::<M>(field_selector),
);
}This implementation:
- Validates input lengths upfront
- Uses functional map operation to avoid intermediate array allocation
- Maintains the same functionality with better safety and performance
Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2939 +/- ##
=======================================
Coverage 56.69% 56.70%
=======================================
Files 420 420
Lines 55554 55554
=======================================
+ Hits 31497 31501 +4
+ Misses 24057 24053 -4 ☔ View full report in Codecov by Sentry. |
glihm
changed the title
There was a problem hiding this comment.
Hey @bengineer42, let's rename for something more consistent with the naming, and let's merge. 👍
| 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. | ||
| /// Retrieves a single member from multiple models. | ||
| fn read_members<T, +Serde<T>, +Drop<T>>( |
There was a problem hiding this comment.
Would rename to read_member_models, since we're reading only one member. Would you mind applying this change to all functions added to this PR?
There was a problem hiding this comment.
I think read_models_member is better, thoughts?
There was a problem hiding this comment.
Wanted to keep the s at then end since we return an array. Sounds good for you?
glihm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/dojo/core/src/world/storage.cairo (1)
198-211: Ohayo sensei! Consider adding length validation.The function should validate that the number of values returned matches the number of pointers provided to ensure data integrity.
fn read_member_of_models<T, +Serde<T>, +Drop<T>>( self: @WorldStorage, ptrs: Span<ModelPtr<M>>, field_selector: felt252, ) -> Array<T> { let mut values: Array<T> = array![]; + let expected_len = ptrs.len(); for entity in IWorldDispatcherTrait::entities( *self.dispatcher, Model::<M>::selector(*self.namespace_hash), ptrs.to_member_indexes(field_selector), field_layout_unwrap::<M>(field_selector), ) { values.append(deserialize_unwrap(*entity)); }; + assert(values.len() == expected_len, 'Length mismatch'); values }crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
209-220: Test looks good but could cover more edge cases!The test effectively verifies batch reading functionality. Consider adding edge cases like empty pointer array or single pointer.
+#[test] +fn test_read_members_edge_cases() { + let mut world = spawn_foo_world(); + // Test empty array + let empty_ptrs = array![].span(); + let empty_v1s: Array<u128> = world.read_member_of_models(empty_ptrs, selector!("v1")); + assert!(empty_v1s.len() == 0); + + // Test single model + let foo = Foo { k1: 1, k2: 2, v1: 3, v2: 4 }; + world.write_model(@foo); + let single_ptr = array![foo.ptr()].span(); + let single_v1s: Array<u128> = world.read_member_of_models(single_ptr, selector!("v1")); + assert!(single_v1s == array![foo.v1]); +}
231-244: Test looks good but could handle error cases!The test effectively verifies batch writing functionality. Consider adding error cases like length mismatch between pointers and values.
+#[should_panic(expected: ('Length mismatch'))] +#[test] +fn test_write_members_length_mismatch() { + let mut world = spawn_foo_world(); + let foo = Foo { k1: 1, k2: 2, v1: 3, v2: 4 }; + let foo2 = Foo { k1: 5, k2: 6, v1: 7, v2: 8 }; + world.write_models([@foo, @foo2].span()); + let ptrs = [foo.ptr(), foo2.ptr()].span(); + let v1s = array![42]; // Only one value for two pointers + world.write_member_of_models(ptrs, selector!("v1"), v1s.span()); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(1 hunks)crates/dojo/core/src/model/storage.cairo(1 hunks)crates/dojo/core/src/world/storage.cairo(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/core/src/model/storage.cairo
🔇 Additional comments (3)
crates/dojo/core/src/world/storage.cairo (1)
225-239: Previous review suggestions still apply.The implementation would benefit from length validation and memory optimization as suggested in the previous review.
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
198-207: Ohayo sensei! The test looks good!The test thoroughly verifies reading individual members with proper assertions.
222-230: Ohayo sensei! The write test is well implemented!The test effectively verifies single member write functionality with proper assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (4)
198-207: Ohayo sensei! Consider adding edge cases to strengthen the test.The test verifies basic member reading functionality, but could be enhanced with:
- Reading from a non-existent model
- Reading after model deletion
209-220: Ohayo sensei! Consider enhancing the test robustness.The test effectively verifies bulk reading, but could be strengthened by:
- Testing with an empty array of pointers
- Testing with a mix of existing and non-existing models
222-230: Ohayo sensei! Consider expanding test coverage.The test only verifies writing to v1. Consider:
- Testing writing to v2 as well
- Testing writing invalid values (if applicable)
- Testing writing to a non-existent model
231-244: Ohayo sensei! Consider enhancing the bulk write test.The test effectively verifies bulk writing of v1, but could be improved by:
- Testing writing to both v1 and v2 in the same operation
- Testing with mismatched lengths between ptrs and values
crates/dojo/core/src/world/storage.cairo (1)
213-226: Ohayo sensei! Consider adding input validation.The implementation is efficient, but could be improved by:
- Validating that ptrs is not empty
- Adding error handling for failed deserialization
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (4)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(2 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)crates/dojo/core/src/world/storage.cairo(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/model/model.cairo
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: build
🔇 Additional comments (1)
crates/dojo/core/src/world/storage.cairo (1)
240-254: Previous optimization suggestions still apply.The implementation would benefit from length validation and memory optimization as suggested in the previous review.
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
Tests
Improvements
ModelStoragetrait functionality with new methods for handling multiple models.ModelPtrsTraitto provide more flexible model pointer handling.The updates improve the efficiency and flexibility of model operations in the Dojo framework.