-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dojo-core): add read schema support #2932
Changes from 11 commits
66c882e
dbe3895
30c7d3c
77ea57a
772f1a7
33b5026
790bc56
b95b94b
5dd114c
1ed7b99
5643b53
95bdb30
b3ea517
89883f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ use dojo::{ | |
utils::{entity_id_from_serialized_keys, find_model_field_layout, entity_id_from_keys}, | ||
}; | ||
|
||
use super::{ModelDefinition, ModelDef}; | ||
use super::{ModelDefinition, ModelDef, ModelIndex}; | ||
/// Trait `KeyParser` defines a trait for parsing keys from a given model. | ||
/// | ||
/// A pointer to a model, which can be expressed by an entity id. | ||
|
@@ -12,6 +12,30 @@ pub struct ModelPtr<M> { | |
pub id: felt252, | ||
} | ||
|
||
|
||
/// 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. | ||
pub trait ModelPtrsTrait<M> { | ||
fn to_indexes(self: Span<ModelPtr<M>>) -> Span<ModelIndex>; | ||
} | ||
|
||
|
||
pub impl ModelPtrsImpl<M> of ModelPtrsTrait<M> { | ||
fn to_indexes(self: Span<ModelPtr<M>>) -> Span<ModelIndex> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you document this function and why it has been necessary adding it? |
||
let mut ids = ArrayTrait::<ModelIndex>::new(); | ||
for ptr in self { | ||
ids.append(ModelIndex::Id(*ptr.id)); | ||
}; | ||
ids.span() | ||
} | ||
} | ||
|
||
pub trait KeyParser<M, K> { | ||
/// Parses the key from the given model. | ||
fn parse_key(self: @M) -> K; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
# Code generated by scarb DO NOT EDIT. | ||
version = 1 | ||
|
||
[[package]] | ||
name = "dojo" | ||
version = "1.0.12" | ||
dependencies = [ | ||
"dojo_plugin", | ||
] | ||
|
||
[[package]] | ||
name = "dojo_benchmark" | ||
version = "0.1.0" | ||
dependencies = [ | ||
"dojo", | ||
"dojo_cairo_test", | ||
] | ||
|
||
[[package]] | ||
name = "dojo_cairo_test" | ||
version = "1.0.12" | ||
dependencies = [ | ||
"dojo", | ||
] | ||
|
||
[[package]] | ||
name = "dojo_plugin" | ||
version = "2.9.2" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
[package] | ||
cairo-version = "=2.9.2" | ||
name = "dojo_benchmark" | ||
version = "0.1.0" | ||
edition = "2024_07" | ||
|
||
[[target.starknet-contract]] | ||
sierra = true | ||
casm = true | ||
build-external-contracts = ["dojo::world::world_contract::world"] | ||
|
||
[dependencies] | ||
dojo = { path = "../../crates/dojo/core" } | ||
starknet = "2.9.2" | ||
|
||
[dev-dependencies] | ||
cairo_test = "2.9.2" | ||
dojo_cairo_test = { path = "../../crates/dojo/core-cairo-test" } | ||
|
||
[features] | ||
default = [] | ||
|
||
[profile.sepolia] |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep this file to the minimum, I think only the |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,4 @@ | ||||||||||||||
[world] | ||||||||||||||
description = "Simple world." | ||||||||||||||
name = "simple" | ||||||||||||||
seed = "simple" | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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:
🔗 Analysis chainOhayo 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 executedThe 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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