feat(dojo-core): add read schema support #2932
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant enhancements to the Dojo framework's model handling and testing infrastructure. The changes span multiple files across the Dojo core and benchmark examples, focusing on improving model introspection, storage, and testing capabilities. New structures, traits, and methods are added to support more flexible model schema reading, conversion of model pointers, and comprehensive testing scenarios. 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
🧹 Nitpick comments (5)
crates/dojo/core/src/model/storage.cairo (2)
1-1: Ensure consistent import statementsOn line 1, the import statement could be formatted for clarity by separating the imports onto separate lines.
Consider refactoring the import as:
use dojo::{ model::{ModelPtr, model_value::ModelValueKey}, meta::Introspect, };
38-38: Clarify the documentation ofread_membermethodThe comment for
read_memberon line 38 could be improved for clarity. It currently states "Retrieves a model of typeMusing the provided entity id," but the method actually retrieves a member of a model.Consider updating the comment to:
/// Retrieves a member of a model of type `M` using the provided `ModelPtr` and `field_selector`.crates/dojo/core-cairo-test/src/tests/model/model.cairo (3)
39-45: Ohayo! Clean struct definition for AStruct.The struct is well-organized with consistent field types. Consider adding documentation to explain the purpose of these fields.
224-238: Test coverage looks good, but could be enhanced, sensei!The test effectively verifies schema reading functionality. Consider adding negative test cases:
- Reading non-existent model
- Reading with incorrect schema type
241-268: Batch schema reading test is solid!Good test coverage for multiple model reading. Consider adding:
- Test with larger arrays to verify scalability
- Test with empty array
- Test with array containing invalid pointers
📜 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(2 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(2 hunks)crates/dojo/core/src/world/storage.cairo(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/dojo/core/src/model/storage.cairo
[error] 43-46: Missing trailing comma in function parameter list
crates/dojo/core/src/world/storage.cairo
[error] 3-7: Missing trailing comma in use statement list
[error] 31-34: Missing trailing comma in struct initialization
[error] 208-213: Missing trailing commas in function arguments
[error] 215-217: Missing trailing comma in function parameter list
[error] 222-225: Missing trailing comma in function arguments
🔇 Additional comments (11)
crates/dojo/core/src/world/storage.cairo (5)
5-7:⚠️ Potential issueOhayo Sensei, please add a missing trailing comma in the
usestatement listThere is a missing trailing comma in the
usestatement list afterModelPtrsTraiton line 7, which is causing the pipeline to fail.Apply this diff to fix the issue:
use dojo::model::{ Model, ModelIndex, ModelValueKey, ModelValue, ModelStorage, ModelPtr, ModelPtrsTrait, };Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[error] 3-7: Missing trailing comma in use statement list
225-225:⚠️ Potential issueAdd missing trailing comma in function arguments
In the call to
IWorldDispatcherTrait::entitiesat line 225, a trailing comma is missing afterptrs.to_indexes(). This is causing the pipeline to fail.Apply this diff to fix the issue:
for entity in IWorldDispatcherTrait::entities( *self.dispatcher, Model::<M>::selector(*self.namespace_hash), ptrs.to_indexes(), Introspect::<T>::layout() ) {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[error] 222-225: Missing trailing comma in function arguments
211-211:⚠️ Potential issueAdd missing trailing comma in function arguments
In the function call to
IWorldDispatcherTrait::entityat line 211, a trailing comma is missing afterIntrospect::<T>::layout(). Adding it will fix the pipeline failure.Apply this diff to fix the issue:
*self.dispatcher, Model::<M>::selector(*self.namespace_hash), ModelIndex::Id(ptr.id), Introspect::<T>::layout(), )Likely invalid or redundant comment.
217-217:⚠️ Potential issueAdd missing trailing comma in function parameter list
In the function
read_schemasdefinition at line 217, there is a missing trailing comma afterptrs: Span<ModelPtr<M>>. This is causing a pipeline failure.Apply this diff to fix the issue:
fn read_schemas<T, +Drop<T>, +Serde<T>, +Introspect<T>>( self: @WorldStorage, ptrs: Span<ModelPtr<M>>, ) -> Array<T> {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[error] 215-217: Missing trailing comma in function parameter list
34-34:⚠️ Potential issueAdd missing trailing comma in struct initialization
On line 34, in the initialization of
FieldLayout, a trailing comma is missing after the closing brace. This is causing the pipeline to fail due to syntax issues.Apply this diff to fix the issue:
.append( FieldLayout { selector: *selector, layout: field_layout_unwrap::<M>(*selector) }, );Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[error] 31-34: Missing trailing comma in struct initialization
crates/dojo/core/src/lib.cairo (1)
45-45: Ohayo Sensei, the addition ofModelPtrsTraitlooks goodThe inclusion of
ModelPtrsTraitin the exports enhances model pointer management capabilities. Great job on expanding the public interface.crates/dojo/core/src/model/model.cairo (2)
15-17: Ohayo! Clean trait definition for model pointer conversion.The new
ModelPtrsTrait<M>provides a clear and focused interface for converting model pointers to indexes.
19-27: Implementation looks solid, sensei!The
to_indexesimplementation efficiently converts model pointers to indexes using idiomatic Cairo array operations.crates/dojo/core-cairo-test/src/tests/model/model.cairo (3)
47-56: Model struct looks good, sensei!The
Foo4model is well-structured with a clear key field and a mix of primitive and complex types.
58-62: Partial schema struct is well-designed.The
Oostruct efficiently captures a subset ofFoo4fields, demonstrating the purpose of the read schema feature.
69-71: Resource registration looks good!The
Foo4model is properly registered in the test namespace.
| fn read_schema<T, +Serde<T>, +Introspect<T>>(self: @S, ptr: ModelPtr<M>) -> T; | ||
|
|
||
| /// Retrieves part of multiple models, matching a schema. | ||
| fn read_schemas<T, +Drop<T>, +Serde<T>, +Introspect<T>>( | ||
| self: @S, ptrs: Span<ModelPtr<M>> | ||
| ) -> Array<T>; |
There was a problem hiding this comment.
Add missing trailing commas in function definitions
In the definitions of read_schema and read_schemas methods (lines 42-47), trailing commas are missing in the parameter lists, which is causing pipeline failures.
Apply this diff to fix the issue:
fn read_schema<T, +Serde<T>, +Introspect<T>>(self: @S, ptr: ModelPtr<M>) -> T;
/// Retrieves part of multiple models, matching a schema.
fn read_schemas<T, +Drop<T>, +Serde<T>, +Introspect<T>>(
self: @S, ptrs: Span<ModelPtr<M>>,
) -> Array<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.
| fn read_schema<T, +Serde<T>, +Introspect<T>>(self: @S, ptr: ModelPtr<M>) -> T; | |
| /// Retrieves part of multiple models, matching a schema. | |
| fn read_schemas<T, +Drop<T>, +Serde<T>, +Introspect<T>>( | |
| self: @S, ptrs: Span<ModelPtr<M>> | |
| ) -> Array<T>; | |
| fn read_schema<T, +Serde<T>, +Introspect<T>>(self: @S, ptr: ModelPtr<M>,) -> T; | |
| /// Retrieves part of multiple models, matching a schema. | |
| fn read_schemas<T, +Drop<T>, +Serde<T>, +Introspect<T>>( | |
| self: @S, ptrs: Span<ModelPtr<M>>, | |
| ) -> Array<T>; |
🧰 Tools
🪛 GitHub Actions: ci
[error] 43-46: Missing trailing comma in function parameter list
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
226-240: Solid test implementation, sensei!The test thoroughly verifies schema reading functionality. Consider adding error cases to test schema reading with invalid pointers.
243-270: Excellent bulk schema test, sensei!The test effectively validates multiple schema reads. Consider adding edge cases:
- Empty array of pointers
- Array with invalid pointers mixed with valid ones
crates/dojo/core/src/world/storage.cairo (2)
29-38: Konnichiwa! Consider adding input validation, sensei!While the implementation looks solid, it might be good to add validation for empty field selectors to ensure robustness.
Consider adding this validation:
fn make_partial_struct_layout<M, +Model<M>>(field_selectors: Span<felt252>) -> Layout { + assert(!field_selectors.is_empty(), 'field_selectors is empty'); let mut layouts: Array<FieldLayout> = array![]; for selector in field_selectors { layouts .append( FieldLayout { selector: *selector, layout: field_layout_unwrap::<M>(*selector) }, ); }; Layout::Struct(layouts.span()) }
220-234: Arigato for the efficient implementation, sensei!The
read_schemasfunction looks great with its batch processing approach. Consider adding documentation to explain the behavior when some schemas fail to deserialize.Add documentation like this:
+/// Reads multiple schemas for the given model pointers. +/// # Arguments +/// * `ptrs` - Span of model pointers to read schemas for +/// # Returns +/// Array of deserialized schemas +/// # Panics +/// Panics if any schema fails to deserialize fn read_schemas<T, +Drop<T>, +Serde<T>, +Introspect<T>>( self: @WorldStorage, ptrs: Span<ModelPtr<M>>, ) -> Array<T> {examples/benchmark/src/model.cairo (6)
5-26: Ohayo! Consider adding documentation for the model structs, sensei!While the structs are well-designed, adding doc comments would help explain their role in benchmarking different read operations. Consider documenting:
- Purpose of each struct
- Significance of the number of fields
- Why these specific configurations were chosen for benchmarking
Example:
/// Single-value model for basic read operation benchmarks. /// Contains minimal fields to establish a baseline performance metric. #[derive(Copy, Drop, Serde)] #[dojo::model] struct Single { // ... existing fields ... }
28-29: Add const documentation, sensei!The test constants would benefit from doc comments explaining their role in the test suite.
/// Test instance of Single model with predictable sequential values /// k0: 1, v0: 2 const SINGLE: Single = Single { k0: 1, v0: 2 };
31-55: Document the schema progression strategy, sensei!The schema structs show a clear progression from single to sextuple field access, but this strategy should be documented. Also consider more descriptive names that reflect the field count.
Suggestions:
- Add documentation explaining the benchmark strategy
- Consider renaming for clarity:
-struct LargeSingleSchema +struct LargeOneFieldSchema -struct LargeDoubleSchema +struct LargeTwoFieldSchema -struct LargeSextupleSchema +struct LargeSixFieldSchema
58-71: Rename test setup functions for clarity, sensei!The current function names are generic. Consider names that reflect their benchmark-specific purpose.
-fn namespace_def() -> NamespaceDef { +fn benchmark_namespace() -> NamespaceDef { -fn spawn_foo_world() -> WorldStorage { +fn spawn_benchmark_world() -> WorldStorage {
73-252: Ohayo! Let's improve test organization and documentation, sensei!While the tests are comprehensive, we can enhance them:
- Add doc comments explaining the benchmark strategy for each group
- Extract common setup code to reduce duplication
Example refactor:
/// Helper function to setup world and write model fn setup_benchmark_world<T: Model + Copy + Drop>(model: @T) -> (WorldStorage, T) { let mut world = spawn_benchmark_world(); world.write_model(model); (world, *model) } #[test] fn read_model_simple() { let (world, model) = setup_benchmark_world(@SINGLE); let read_model: Single = world.read_model(model.k0); assert!(read_model.v0 == model.v0); }
73-108: Consider grouping related tests using modules, sensei!The tests follow a clear pattern based on the number of fields being read. Consider organizing them into modules for better clarity.
mod tests { mod single_field { #[test] fn read_model_simple() { // existing test } // other single field tests } mod double_field { // double field tests } mod sextuple_field { // sextuple field tests } }Also applies to: 111-145, 148-183, 185-252
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/benchmark/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(2 hunks)crates/dojo/core/src/model/storage.cairo(2 hunks)crates/dojo/core/src/world/storage.cairo(3 hunks)examples/benchmark/Scarb.toml(1 hunks)examples/benchmark/dojo_dev.toml(1 hunks)examples/benchmark/manifest_dev.json(1 hunks)examples/benchmark/src/lib.cairo(1 hunks)examples/benchmark/src/model.cairo(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/benchmark/src/lib.cairo
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/core/src/model/storage.cairo
🧰 Additional context used
🪛 Gitleaks (8.21.2)
examples/benchmark/dojo_dev.toml
10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: ensure-wasm
- GitHub Check: docs
- GitHub Check: clippy
🔇 Additional comments (9)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (4)
39-45: Ohayo! The struct implementation looks great, sensei!The
AStructis well-designed with consistent field types and appropriate trait derivations.
47-56: Excellent model definition, sensei!The
Foo4struct follows consistent patterns with other models and properly integrates the nestedAStruct.
58-62: Nice schema struct, sensei!The
Oostruct effectively serves as a partial schema forFoo4, demonstrating the schema reading capabilities.
70-71: Clean resource registration, sensei!The new model is properly registered in the namespace definition.
examples/benchmark/Scarb.toml (1)
1-23: Clean package configuration, sensei!The configuration is well-structured. Let's verify the Cairo version compatibility.
✅ Verification successful
Ohayo! Cairo version is perfectly aligned, sensei! 🎯
The package uses Cairo v2.9.2, which is the latest stable release in the 2.9.x series. The exact version pinning with '=' ensures reproducible builds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the specified Cairo version exists and is the latest stable curl -s https://api.github.com/repos/starkware-libs/cairo/releases | jq -r '.[].tag_name' | grep -E '^v2\.9\.'Length of output: 146
examples/benchmark/dojo_dev.toml (1)
13-30: Well-organized configuration structure, sensei!The namespace, writers, owners, and migration configurations are clearly defined and properly structured.
crates/dojo/core/src/world/storage.cairo (2)
5-7: Ohayo! The new imports look good, sensei!The additions of
ModelPtrsTrait,FieldLayout, andIntrospectalign perfectly with the PR's objective of adding read schema support.Also applies to: 9-9
209-218: Sugoi implementation, sensei!The
read_schemafunction is well-implemented, following the established patterns and correctly utilizing the Introspect trait for layout information.examples/benchmark/manifest_dev.json (1)
1-1975: Ohayo! The manifest is perfectly structured, sensei!The manifest provides a comprehensive configuration with:
- Well-defined world properties and entrypoints
- Complete ABI definitions
- Proper contract configurations
- Thorough event definitions
examples/benchmark/dojo_dev.toml
Outdated
| account_address = "0x127fd5f1fe78a71f8bcd1fec63e3fe2f0486b6ecd5c86a0466c3a21fa5cfcec" | ||
| private_key = "0xc5b2fcab997346f3ea1c00b002ecf6f382c5f9c9659a3894eb783c5320f912" |
There was a problem hiding this comment.
Security Alert: Exposed Private Key, sensei!
The private key should not be hardcoded in the configuration file, even for development. Consider:
- Using environment variables
- Adding this file to .gitignore
- Providing a template file instead
🧰 Tools
🪛 Gitleaks (8.21.2)
10-10: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2932 +/- ##
=======================================
Coverage 56.70% 56.71%
=======================================
Files 420 420
Lines 55554 55554
=======================================
+ Hits 31502 31506 +4
+ Misses 24052 24048 -4 ☔ View full report in Codecov by Sentry. |
glihm
changed the title
There was a problem hiding this comment.
thanks for the contribution here @bengineer42! Some comments to address please. 🙏
| } | ||
|
|
||
| #[derive(Copy, Drop, Serde, Debug, Introspect)] | ||
| struct Oo { |
There was a problem hiding this comment.
Let's use more descriptive names please.
| } | ||
|
|
||
| pub impl ModelPtrsImpl<M> of ModelPtrsTrait<M> { | ||
| fn to_indexes(self: Span<ModelPtr<M>>) -> Span<ModelIndex> { |
There was a problem hiding this comment.
Could you document this function and why it has been necessary adding it?
examples/benchmark/dojo_dev.toml
Outdated
| mappings = { "ns" = ["c1", "M"], "ns2" = ["c1", "M"] } | ||
|
|
||
| [init_call_args] | ||
| "ns-c1" = ["0xfffe"] | ||
| "ns2-c1" = ["0xfffe"] | ||
|
|
||
| [writers] | ||
| "ns" = ["ns-c1", "ns-c2"] | ||
| "ns-M" = ["ns-c2", "ns-c1", "ns2-c1"] | ||
|
|
||
| [owners] | ||
| "ns" = ["ns-c1"] | ||
|
|
||
| [migration] | ||
| order_inits = ["ns-c2", "ns-c1"] | ||
| skip_contracts = ["ns-c3"] |
There was a problem hiding this comment.
| mappings = { "ns" = ["c1", "M"], "ns2" = ["c1", "M"] } | |
| [init_call_args] | |
| "ns-c1" = ["0xfffe"] | |
| "ns2-c1" = ["0xfffe"] | |
| [writers] | |
| "ns" = ["ns-c1", "ns-c2"] | |
| "ns-M" = ["ns-c2", "ns-c1", "ns2-c1"] | |
| [owners] | |
| "ns" = ["ns-c1"] | |
| [migration] | |
| order_inits = ["ns-c2", "ns-c1"] | |
| skip_contracts = ["ns-c3"] |
examples/benchmark/dojo_dev.toml
Outdated
| description = "Simple world." | ||
| name = "simple" | ||
| seed = "simple" |
There was a problem hiding this comment.
| description = "Simple world." | |
| name = "simple" | |
| seed = "simple" | |
| description = "Benchmark world." | |
| name = "bench" | |
| seed = "bench" |
There was a problem hiding this comment.
Keep this file to the minimum, I think only the world is actually needed and the default namespace.
| 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 part of a model, matching a schema. |
There was a problem hiding this comment.
Let's add a bit more comments here, to show what a schema is.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (1)
226-240: Consider adding negative test cases.The test is thorough for the happy path, but consider adding test cases for:
- Reading a non-existent model
- Reading a schema with mismatched fields
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(2 hunks)crates/dojo/core/src/model/model.cairo(2 hunks)crates/dojo/core/src/model/storage.cairo(2 hunks)examples/benchmark/dojo_dev.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/core/src/model/model.cairo
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (5)
crates/dojo/core/src/model/storage.cairo (3)
1-1: LGTM! Import addition is well-placed.The addition of
Introspectimport is necessary for the new schema reading functionality.
41-47: Add more detailed documentation for schema-related methods.Ohayo sensei! While the methods are well-defined, the documentation could be more descriptive about what a schema represents and its relationship to the model.
Add more context in the documentation:
- /// Retrieves a subset of members in a model, matching a defined schema <T>. + /// Retrieves a subset of members in a model, matching a defined schema <T>. + /// A schema represents a partial view of the model, allowing efficient reading of + /// specific fields while maintaining their structural relationships.
49-49: LGTM! Documentation update is clear.The clarification about member updates improves understanding of the method's purpose.
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
39-62: Consider using more descriptive struct names.Ohayo sensei! While the structs are well-defined, consider using names that better reflect their purpose:
AStructcould bePackedBytesorQuadBytesFoo4could beNestedModelExampleFooSchemacould bePartialNestedModel
70-71: LGTM! Resource registration is correct.The new model is properly registered in the namespace.
examples/benchmark/dojo_dev.toml
Outdated
| [world] | ||
| description = "Simple world." | ||
| name = "simple" | ||
| seed = "simple" |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! Inconsistent world configuration in benchmark directory.
The benchmark directory's configuration should use "bench" instead of "simple" to maintain consistency with other benchmark-related files:
crates/benches/contracts/dojo_dev.tomluses name="bench", seed="bench"- Current file uses name="simple", seed="simple" but is located in benchmark directory
🔗 Analysis chain
Ohayo sensei! Verify world configuration consistency.
The world configuration appears minimal and clean. However, the values differ from those suggested in previous reviews ("simple" vs "bench"). Please ensure these values are consistent with other configuration files and documentation.
Let's check for consistency across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for "simple" and "bench" in configuration files and documentation
echo "Searching for world name/seed usage..."
rg -g '*.{toml,json,md}' -i 'name.*=.*"?(simple|bench)"?'
rg -g '*.{toml,json,md}' -i 'seed.*=.*"?(simple|bench)"?'
Length of output: 803
| #[test] | ||
| fn test_read_schemas() { | ||
| let mut world = spawn_foo_world(); | ||
| let foo = Foo4 { id: 1, v0: 2, v1: 3, v2: 4, v3: AStruct { a: 5, b: 6, c: 7, d: 8 } }; | ||
| let mut foo_2 = foo; | ||
| foo_2.id = 2; | ||
| foo_2.v0 = 12; | ||
|
|
||
| world.write_models([@foo, @foo_2].span()); | ||
|
|
||
| let mut values: Array<FooSchema> = world.read_schemas([foo.ptr(), foo_2.ptr()].span()); | ||
| let schema_1 = values.pop_front().unwrap(); | ||
| let schema_2 = values.pop_front().unwrap(); | ||
| assert!( | ||
| schema_1.v0 == foo.v0 | ||
| && schema_1.v3.a == foo.v3.a | ||
| && schema_1.v3.b == foo.v3.b | ||
| && schema_1.v3.c == foo.v3.c | ||
| && schema_1.v3.d == foo.v3.d, | ||
| ); | ||
| assert!( | ||
| schema_2.v0 == foo_2.v0 | ||
| && schema_2.v3.a == foo_2.v3.a | ||
| && schema_2.v3.b == foo_2.v3.b | ||
| && schema_2.v3.c == foo_2.v3.c | ||
| && schema_2.v3.d == foo_2.v3.d, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add array bounds checking for safer array operations.
While the test is comprehensive, it could be more robust by checking the array length before accessing elements:
let mut values: Array<FooSchema> = world.read_schemas([foo.ptr(), foo_2.ptr()].span());
+ assert!(values.len() == 2, "Expected 2 schemas");
let schema_1 = values.pop_front().unwrap();
let schema_2 = values.pop_front().unwrap();📝 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.
| #[test] | |
| fn test_read_schemas() { | |
| let mut world = spawn_foo_world(); | |
| let foo = Foo4 { id: 1, v0: 2, v1: 3, v2: 4, v3: AStruct { a: 5, b: 6, c: 7, d: 8 } }; | |
| let mut foo_2 = foo; | |
| foo_2.id = 2; | |
| foo_2.v0 = 12; | |
| world.write_models([@foo, @foo_2].span()); | |
| let mut values: Array<FooSchema> = world.read_schemas([foo.ptr(), foo_2.ptr()].span()); | |
| let schema_1 = values.pop_front().unwrap(); | |
| let schema_2 = values.pop_front().unwrap(); | |
| assert!( | |
| schema_1.v0 == foo.v0 | |
| && schema_1.v3.a == foo.v3.a | |
| && schema_1.v3.b == foo.v3.b | |
| && schema_1.v3.c == foo.v3.c | |
| && schema_1.v3.d == foo.v3.d, | |
| ); | |
| assert!( | |
| schema_2.v0 == foo_2.v0 | |
| && schema_2.v3.a == foo_2.v3.a | |
| && schema_2.v3.b == foo_2.v3.b | |
| && schema_2.v3.c == foo_2.v3.c | |
| && schema_2.v3.d == foo_2.v3.d, | |
| ); | |
| } | |
| #[test] | |
| fn test_read_schemas() { | |
| let mut world = spawn_foo_world(); | |
| let foo = Foo4 { id: 1, v0: 2, v1: 3, v2: 4, v3: AStruct { a: 5, b: 6, c: 7, d: 8 } }; | |
| let mut foo_2 = foo; | |
| foo_2.id = 2; | |
| foo_2.v0 = 12; | |
| world.write_models([@foo, @foo_2].span()); | |
| let mut values: Array<FooSchema> = world.read_schemas([foo.ptr(), foo_2.ptr()].span()); | |
| assert!(values.len() == 2, "Expected 2 schemas"); | |
| let schema_1 = values.pop_front().unwrap(); | |
| let schema_2 = values.pop_front().unwrap(); | |
| assert!( | |
| schema_1.v0 == foo.v0 | |
| && schema_1.v3.a == foo.v3.a | |
| && schema_1.v3.b == foo.v3.b | |
| && schema_1.v3.c == foo.v3.c | |
| && schema_1.v3.d == foo.v3.d, | |
| ); | |
| assert!( | |
| schema_2.v0 == foo_2.v0 | |
| && schema_2.v3.a == foo_2.v3.a | |
| && schema_2.v3.b == foo_2.v3.b | |
| && schema_2.v3.c == foo_2.v3.c | |
| && schema_2.v3.d == foo_2.v3.d, | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/dojo/core/src/model/model.cairo (1)
15-24: Ohayo! Let's enhance the trait documentation, sensei.While the documentation is good, it could be more comprehensive by:
- Adding documentation for the
to_member_indexesmethod- Including usage examples for both methods
Add the following to the trait documentation:
/// Trait that defines a method for converting a span of model pointers to a span of model indexes. /// /// # Type Parameters /// - `M`: The type of the model. /// /// # Methods /// - `to_indexes(self: Span<ModelPtr<M>>) -> Span<ModelIndex>`: /// Converts the span of model pointers to a span of model indexes. +/// - `to_member_indexes(self: Span<ModelPtr<M>>, field_selector: felt252) -> Span<ModelIndex>`: +/// Converts the span of model pointers to a span of model member indexes using the provided field selector. +/// +/// # Examples +/// ``` +/// let ptrs = [model1.ptr(), model2.ptr()].span(); +/// let indexes = ptrs.to_indexes(); // For reading full models +/// let member_indexes = ptrs.to_member_indexes(selector!("field")); // For reading specific fields +/// ```crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
39-45: Consider using more descriptive field names, sensei.The current field names (
a,b,c,d) are not very descriptive. Consider using names that reflect their purpose in the test context.struct AStruct { - a: u8, - b: u8, - c: u8, - d: u8, + first_value: u8, + second_value: u8, + third_value: u8, + fourth_value: u8, }
299-313: Enhance test coverage with error cases, sensei.While the happy path is well tested, consider adding test cases for:
- Reading a non-existent model
- Reading a model with invalid schema
+#[test] +#[should_panic(expected: 'entity not found')] +fn test_read_schema_non_existent() { + let mut world = spawn_foo_world(); + let ptr = Model::<Foo4>::ptr_from_id(999); + let _: FooSchema = world.read_schema(ptr); +}crates/dojo/core/src/world/storage.cairo (1)
29-38: Add documentation for the make_partial_struct_layout function, sensei.This utility function deserves documentation explaining its purpose and usage.
+/// Creates a layout for a partial structure based on the provided field selectors. +/// +/// # Arguments +/// * `field_selectors` - A span of field selectors to include in the layout +/// +/// # Returns +/// A Layout::Struct containing the layouts for the selected fields fn make_partial_struct_layout<M, +Model<M>>(field_selectors: Span<felt252>) -> Layout {
📜 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/model/model.cairo(1 hunks)crates/dojo/core/src/model/storage.cairo(2 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
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (5)
crates/dojo/core/src/model/model.cairo (1)
Line range hint
26-38: Implementation looks clean and efficient, sensei!The trait implementation is well-structured and follows best practices:
- Efficient use of array operations
- Clear transformation of model pointers to indexes
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
47-62: Well-structured model and schema definitions, sensei!The relationship between
Foo4andFooSchemais clear, withFooSchemarepresenting a subset ofFoo4's fields. Good use of the#[key]attribute and appropriate field types.
315-342: Previous review comment about array bounds checking is still relevant.The test should include array bounds checking before accessing elements.
crates/dojo/core/src/world/storage.cairo (2)
281-290: Clean and efficient schema reading implementation, sensei!The implementation correctly uses the model's selector and introspection capabilities to read and deserialize the schema.
292-306: Excellent batch schema reading implementation, sensei!The implementation efficiently handles multiple schemas and makes good use of the new
ModelPtrsTraitfor index conversion.
There was a problem hiding this comment.
Thanks @bengineer42, good to have a more optimized way to fetch member data.
glihm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
examples/benchmark/src/model.cairo (5)
5-26: Ohayo! Add documentation for model structs, sensei!While the model structs are well-structured, they would benefit from documentation explaining their purpose and field meanings. This helps other developers understand how to use these models effectively.
Add doc comments like this:
+/// Single represents a basic model with one key and one value. +/// Used for simple read operation benchmarks. #[derive(Copy, Drop, Serde)] #[dojo::model] struct Single { #[key] k0: felt252, v0: felt252, } +/// Large represents a complex model with multiple keys and values. +/// Used for benchmarking read operations with varying schema sizes. #[derive(Copy, Drop, Serde)] #[dojo::model] struct Large { #[key] k0: felt252, #[key] k1: felt252, v0: felt252, v1: felt252, v2: felt252, v3: felt252, v4: felt252, v5: felt252, }
31-55: Add documentation for schema structs to clarify their test purposes, sensei!The schema structs would benefit from documentation explaining their roles in testing different read scenarios.
Add doc comments like this:
+/// Schema for reading a single value from the Single model #[derive(Copy, Drop, Serde, Introspect)] struct SingleSchema { v0: felt252, } +/// Schema for reading one value from the Large model #[derive(Copy, Drop, Serde, Introspect)] struct LargeSingleSchema { v5: felt252, } +/// Schema for reading two specific values from the Large model #[derive(Copy, Drop, Serde, Introspect)] struct LargeDoubleSchema { v0: felt252, v5: felt252, } +/// Schema for reading all values from the Large model #[derive(Copy, Drop, Serde, Introspect)] struct LargeSextupleSchema { v0: felt252, v1: felt252, v2: felt252, v3: felt252, v4: felt252, v5: felt252, }
58-71: Document helper functions for better maintainability, sensei!The helper functions would benefit from documentation explaining their purposes and return values.
Add doc comments like this:
+/// Returns the namespace definition for test models. +/// Registers Single and Large models in the test namespace. fn namespace_def() -> NamespaceDef { // ... existing code ... } +/// Creates and returns a new test world instance with the registered models. fn spawn_foo_world() -> WorldStorage { // ... existing code ... }
73-252: Refactor tests to reduce duplication, sensei!While the test coverage is excellent, there's significant duplication in the setup code. Consider extracting common setup into helper functions.
Here's a suggested refactor:
+/// Helper function to setup test world with Large model +fn setup_large_world() -> WorldStorage { + let mut world = spawn_foo_world(); + world.write_model(@LARGE); + world +} + +/// Helper function to setup test world with Single model +fn setup_single_world() -> WorldStorage { + let mut world = spawn_foo_world(); + world.write_model(@SINGLE); + world +} #[test] fn read_model_simple() { - let mut world = spawn_foo_world(); - world.write_model(@SINGLE); + let world = setup_single_world(); // ... rest of the test ... }This would make the tests more maintainable and reduce code duplication.
1-252: Excellent benchmark implementation overall, sensei!The implementation provides comprehensive coverage of read operations with well-structured models and tests. The code is clean, follows good practices, and effectively demonstrates the new read schema functionality.
Consider these architectural improvements:
- Add performance benchmarks using Cairo's benchmark tools
- Consider parameterized tests for testing different field counts
- Add error case tests (e.g., reading non-existent models)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/benchmark/dojo_dev.toml(1 hunks)examples/benchmark/src/lib.cairo(1 hunks)examples/benchmark/src/model.cairo(1 hunks)scripts/cairo_fmt.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/benchmark/src/lib.cairo
- examples/benchmark/dojo_dev.toml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: docs
- GitHub Check: clippy
- GitHub Check: build
- GitHub Check: ensure-wasm
🔇 Additional comments (2)
scripts/cairo_fmt.sh (1)
12-12: Ohayo! The formatting path addition looks good, sensei! ✨The new path follows the established pattern and ensures consistent formatting for the benchmark examples.
examples/benchmark/src/model.cairo (1)
28-29: LGTM! Well-structured test constants, sensei!The constants provide clear and logical test data for both model types.
Description
Adds the ability to read a partial model efficiently
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
Documentation
Tests
Chores