Skip to content
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 support to read/write the same member from multiple models #2939

Merged
merged 7 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions crates/dojo/core-cairo-test/src/tests/model/model.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,52 @@ fn test_model_ptr_from_entity_id() {
let v1 = world.read_member(ptr, selector!("v1"));
assert!(foo.v1 == v1);
}

#[test]
fn test_read_member() {
let mut world = spawn_foo_world();
let foo = Foo { k1: 1, k2: 2, v1: 3, v2: 4 };
world.write_model(@foo);
let v1: u128 = world.read_member(foo.ptr(), selector!("v1"));
let v2: u32 = world.read_member(foo.ptr(), selector!("v2"));
assert!(foo.v1 == v1);
assert!(foo.v2 == v2);
}

#[test]
fn test_read_members() {
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<u128> = world.read_members(ptrs, selector!("v1"));
let v2s: Array<u32> = world.read_members(ptrs, selector!("v2"));
assert!(v1s == array![foo.v1, foo2.v1]);
assert!(v2s == array![foo.v2, foo2.v2]);
}

#[test]
fn test_write_member() {
let mut world = spawn_foo_world();
let foo = Foo { k1: 1, k2: 2, v1: 3, v2: 4 };
world.write_model(@foo);
world.write_member(foo.ptr(), selector!("v1"), 42);
let foo_read: Foo = world.read_model((foo.k1, foo.k2));
assert!(foo_read.v1 == 42 && foo_read.v2 == foo.v2);
}
#[test]
fn test_write_members() {
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, 43];
world.write_members(ptrs, selector!("v1"), v1s.span());
let v1s_read: Array<u128> = world.read_members(ptrs, selector!("v1"));
let v2s_read: Array<u32> = world.read_members(ptrs, selector!("v2"));
assert!(v1s_read == v1s);
assert!(v2s_read == array![foo.v2, foo2.v2]);
}

2 changes: 1 addition & 1 deletion crates/dojo/core/src/lib.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub mod model {
pub use definition::{ModelIndex, ModelDefinition, ModelDef};

pub mod model;
pub use model::{Model, KeyParser, ModelPtr};
pub use model::{Model, KeyParser, ModelPtr, ModelPtrsTrait};

pub mod model_value;
pub use model_value::{ModelValue, ModelValueKey};
Expand Down
25 changes: 24 additions & 1 deletion crates/dojo/core/src/model/model.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -12,6 +12,29 @@ pub struct ModelPtr<M> {
pub id: felt252,
}

pub trait ModelPtrsTrait<M> {
fn to_indexes(self: Span<ModelPtr<M>>) -> Span<ModelIndex>;
fn to_member_indexes(self: Span<ModelPtr<M>>, field_selector: felt252) -> Span<ModelIndex>;
}

pub impl ModelPtrsImpl<M> of ModelPtrsTrait<M> {
fn to_indexes(self: Span<ModelPtr<M>>) -> Span<ModelIndex> {
let mut ids = ArrayTrait::<ModelIndex>::new();
for ptr in self {
ids.append(ModelIndex::Id(*ptr.id));
};
ids.span()
}

fn to_member_indexes(self: Span<ModelPtr<M>>, field_selector: felt252) -> Span<ModelIndex> {
let mut ids = ArrayTrait::<ModelIndex>::new();
for ptr in self {
ids.append(ModelIndex::MemberId((*ptr.id, field_selector)));
};
ids.span()
}
}

pub trait KeyParser<M, K> {
/// Parses the key from the given model.
fn parse_key(self: @M) -> K;
Expand Down
14 changes: 12 additions & 2 deletions crates/dojo/core/src/model/storage.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,24 @@ pub trait ModelStorage<S, M> {
/// The ptr is mostly used for type inferrence.
fn erase_models_ptrs(ref self: S, ptrs: Span<ModelPtr<M>>);

/// Retrieves a model of type `M` using the provided entity idref .
/// Retrieves a model of type `M` using the provided entity id.
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>>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think read_models_member is better, thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to keep the s at then end since we return an array. Sounds good for you?

self: @S, ptrs: Span<ModelPtr<M>>, field_selector: felt252,
) -> Array<T>;

/// Updates a member of a model.
fn write_member<T, +Serde<T>, +Drop<T>>(
ref self: S, ptr: ModelPtr<M>, field_selector: felt252, value: T,
);

/// Updates a member of multiple models.
fn write_members<T, +Serde<T>, +Drop<T>>(
ref self: S, ptrs: Span<ModelPtr<M>>, field_selector: felt252, values: Span<T>,
);

/// Returns the current namespace hash.
fn namespace_hash(self: @S) -> felt252;
}
Expand Down
36 changes: 35 additions & 1 deletion crates/dojo/core/src/world/storage.cairo
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

use core::panic_with_felt252;
use dojo::world::{IWorldDispatcher, IWorldDispatcherTrait, Resource};
use dojo::model::{Model, ModelIndex, ModelValueKey, ModelValue, ModelStorage, ModelPtr};
use dojo::model::{
Model, ModelIndex, ModelValueKey, ModelValue, ModelStorage, ModelPtr, ModelPtrsTrait,
};
use dojo::event::{Event, EventStorage};
use dojo::meta::Layout;
use dojo::utils::{
Expand Down Expand Up @@ -192,6 +194,22 @@ pub impl ModelStorageWorldStorageImpl<M, +Model<M>, +Drop<M>> of ModelStorage<Wo
),
)
}

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![];
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));
};
values
}

fn write_member<T, +Serde<T>, +Drop<T>>(
ref self: WorldStorage, ptr: ModelPtr<M>, field_selector: felt252, value: T,
) {
Expand All @@ -204,6 +222,22 @@ pub impl ModelStorageWorldStorageImpl<M, +Model<M>, +Drop<M>> of ModelStorage<Wo
);
}

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),
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ohayo sensei! Consider these essential improvements.

The implementation would benefit from two important changes:

  1. Length validation between ptrs and values spans
  2. 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:

  1. Validates input lengths upfront
  2. Uses functional map operation to avoid intermediate array allocation
  3. Maintains the same functionality with better safety and performance

Committable suggestion skipped: line range outside the PR's diff.


fn erase_models_ptrs(ref self: WorldStorage, ptrs: Span<ModelPtr<M>>) {
let mut indexes: Array<ModelIndex> = array![];
for ptr in ptrs {
Expand Down
Loading