-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: make the model key a tuple and simplified models code #28
Conversation
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.
Amazing work, crazy reduction of the generated code with that.
The new approach with the world looks great, as discussed with you and @remybar on discord it's a matter of making sure we support every use case since the compiler inference might be tricky.
Some general comments:
- Add some documentation in the new modules and the need of each new traits.
- Would be awesome to have in the
examples/dojo_simple
new cairo modules to showcase all the possible combinations (using the trait, using the world, etc..) - Adding some tests where we manipulate multiple models type in the same scope.
crates/compiler/src/plugin/attribute_macros/templates/model_store.generate.cairo
Outdated
Show resolved
Hide resolved
MemberId: (felt252, felt252) | ||
} | ||
|
||
pub trait ModelAttributes<T> { |
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.
Not sure if Attribute
is the word for that. We have a concept of definition
recently added which regroup all those data, we may re-use this term here.
|
||
pub trait EntityKey<E, K> {} | ||
|
||
// Needs to be generated |
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.
// Needs to be generated |
@@ -456,7 +456,7 @@ fn test_delete_entity_by_id() { | |||
|
|||
world.set_entity(selector, ModelIndex::Id(entity_id), values, layout); | |||
|
|||
world.delete_entity(selector, ModelIndex::Id(entity_id), layout); | |||
IWorldDispatcherTrait::delete_entity(world, selector, ModelIndex::Id(entity_id), layout); |
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.
May be worthy to rename the delete_entity
in the new implementation to avoid conflicts with the world trait.
use super::DOJO_MODEL_ATTR; | ||
|
||
const MODEL_CODE_STRING: &str = include_str!("./templates/model_store.generate.cairo"); |
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.
const MODEL_CODE_STRING: &str = include_str!("./templates/model_store.generate.cairo"); | |
const MODEL_CODE_PATCH: &str = include_str!("./templates/model_store.generate.cairo"); |
Let's call them patch
or template
.
.name(db) | ||
.as_syntax_node() | ||
.get_text(db) | ||
.trim() | ||
.to_string(); | ||
let model_name_snake = model_type.to_case(Case::Snake); |
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.
let model_name_snake = model_type.to_case(Case::Snake); | |
let model_type_snake = model_type.to_case(Case::Snake); |
to be consistent with the change proposed here.
Co-authored-by: glihm <[email protected]>
…ore.generate.cairo Co-authored-by: glihm <[email protected]>
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.
Awesome work Ben ! 🚀 Thanks for this contribution !
I need to practice a little bit with this new way to manipulate models and entities to fully understand the impact, but it looks pretty great 👍
As @glihm said, we should play a little bit more with these new interfaces to be sure there won't be too many issues with compiler inferences even if there is always the possibility to explicitly specify the data types (but in this case, the DevX is degraded).
On another hand and on a general point of view, I'm wondering if we don't provide too many ways to set models/entities/members and if we should be more opinionated 🤔 Maybe we should select one way for each operation (probably the one which provides the best DevX) and document it well through examples and documentation. What do you think ?
use super::DOJO_MODEL_ATTR; | ||
|
||
const MODEL_CODE_PATCH: &str = include_str!("./templates/model_store.generate.cairo"); | ||
const MODEL_FIELD_CODE_STRING: &str = include_str!("./templates/model_field_store.generate.cairo"); |
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.
To be consistent, I would use _PATCH
instead of _STRING
.
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.
Yeah missed that, will change
@@ -56,16 +58,17 @@ impl DojoModel { | |||
&mut diagnostics, | |||
); | |||
|
|||
let model_name = struct_ast | |||
let model_type = struct_ast |
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.
As in ModelDefinition
we still use name()
to get the model name, I would keep model_name
instead of model_type
here.
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.
I think Type is more descriptive though as it is the type, even if we do use it in the contract
@@ -176,6 +172,13 @@ impl DojoModel { | |||
); | |||
|
|||
// Ensures models always derive Introspect if not already derived. | |||
let derive_tags = derive_attr_names |
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.
Instead of derive_tags
I would name this variable entity_derive_attr_names
as it will be used to expand derive attributes of the underlying $model_type$Entity
struct.
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.
Okay will change
} | ||
} | ||
|
||
impl $model_type$EntityKey of dojo::model::entity::EntityKey<$model_type$Entity, $model_type$KeyType> { |
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.
I don't understand what is the goal of this empty trait (and the generic one EntityKey<E, K>
) 🤔
Could you explain me please ?
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.
Its to make sure that when you put the key in the get_entity function its the correct key type for the model Its a bit hacky, If you know a better way please let me know
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.
Hum ok, thanks for the explanation 👍
I need to have a look but maybe using Associated types ?
} | ||
|
||
// Impl to get the static definition of a model | ||
pub mod $model_type_snake$_definition { |
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.
I like the idea to split model traits in 2 parts: definition and entity. At the end, we can probably use the same Definition
trait for both models and events.
fn delete_entity(self: IWorldDispatcher, entity: @E); | ||
fn delete_from_id(self: IWorldDispatcher, entity_id: felt252); | ||
fn get_member_from_id<T, +MemberStore<E, T>>( | ||
self: @IWorldDispatcher, member_id: felt252, entity_id: felt252 |
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.
I would put entity_id
first and then member_id
, to have a general-to-specific approach.
Same for update_member_from_id
.
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.
Sure
fn values(self: @E) -> Span<felt252>; | ||
fn from_values(entity_id: felt252, ref values: Span<felt252>) -> Option<E>; | ||
|
||
fn name() -> ByteArray; |
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.
Do we really need these definition functions here ? Would it be possible to directly use the ModelDefinition::xxx()
or EntityDefinition::xxx()
when needed ?
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.
I did this to be more compatable with impls, I quite like them being part of model as it means useers only ned to use the Model Trait and the rest can be hidden but I'm not that attached to it
use core::panic_with_felt252; | ||
|
||
pub trait MemberStore<M, T> { | ||
fn get_member(self: @IWorldDispatcher, member_id: felt252, entity_id: felt252,) -> T; |
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.
Same than before, I would put entity_id
first and then member_id
.
Same for update_member
.
} | ||
|
||
|
||
fn update_serialized_member( |
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.
Here I would put parameters in this order: model_id
, entity_id
, member_id
, layout
and values
, with the idea to first define the targeted member with a general-to-specific approach, and then provide the layout and values.
Same for other functions.
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.
I think layout shoould be after model
|
||
fn get_position(world: @IWorldDispatcher) -> Point { | ||
let caller = starknet::get_caller_address(); | ||
let entity: PositionEntity = world.get_entity(caller); |
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.
If we can do this:
let mut position: Position = world.get(caller);
position.x = new_x;
position.y = new_y;
world.set(@position);
I'm wondering if we still really need this concept of Entity
🤔
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.
Hi @bengineer42 and @glihm ! I tried to list all the ways to get/set/update/delete models/entities/members. Have I forgotten any possibilites ? I would say that we should remove #[derive(Drop, Serde)]
#[dojo::model]
pub struct Attack {
#[key]
pub player: ContractAddress,
#[key]
pub monster_id: u32,
pub damage: u8,
pub factor: u8,
}
use super::models::{Attack, AttackEntity, AttackMembersStore};
fn spawn(ref world: IWorldDispatcher) {
let caller = starknet::get_caller_address();
let entity_id = entity_id_from_key(@(caller, 12));
// ---- get ----
// ModelStore::get();
let attack: Attack = world.get((caller, 12));
// get! macro
let _ = get!(world, (caller, 12), (Attack,));
// EntityStore::get_entity()
let attack_entity: AttackEntity = world.get_entity((caller, 12));
// EntityStore::get_entity_from_id()
let _: AttackEntity = world.get_entity_from_id(entity_id);
// ---- get members ----
// generated field accessors (MembersStore)
let _attack_damage: u8 = world.get_damage((caller, 12));
let _: u8 = world.get_damage_from_id(entity_id);
// from ModelStore
let _attack_damage3 = ModelStore::<Attack>::get_member::<u8, (ContractAddress, u32)>(
@world, selector!("damage"), (caller, 12)
);
// from EntityStore
let _attack_damage4 = EntityStore::<AttackEntity>::get_member_from_id::<u8>(
@world, selector!("damage"), entity_id
);
// ---- update ----
// ModelStore::set()
world.set(@attack);
// EntityStore::update()
world.update(@attack_entity);
// set! macro
set!(world, (attack,));
// --- update members ----
// from MembersStore
world.update_damage((caller, 12), 42_u8);
world.update_damage_from_id(entity_id, 42_u8);
// from ModelStore
ModelStore::<Attack>::update_member::<u8, (ContractAddress, u32)>(
world, selector!("damage"), (caller, 12), 42_u8
);
// from EntityStore
EntityStore::<AttackEntity>::update_member_from_id::<u8>(
world, selector!("damage"), entity_id, 42_u8
);
// ---- delete ----
// from ModelStore
world.delete(@attack);
// from EntityStore
world.delete_entity(@attack_entity);
// from ModelStore
ModelStore::<Attack>::delete_from_key(world, (caller, 12));
// from EntityStore
EntityStore::<AttackEntity>::delete_from_id(world, entity_id);
} |
I noticed that the compiler is lost with the 2 fn EntityStore::delete_entity(self: IWorldDispatcher, entity: @E);
fn IWorld::delete_entity(ref self: T, model_selector: felt252, index: ModelIndex, layout: Layout); I played a little bit more with this world-centric solution and I think it should be fine for the type inference for |
Yeah I saw this but I felt that both functions are used so rarely and also the fact that the IWorldDispactureTrait will not need to be imported as much now that I think it can be left (also I don't know what to rename it to) |
@remybar the get_<member_name> was also an issue with the previous versions, you can access it with MemberStore::get_<member_name>() if there are mutiple avliable implimenations but I'm also happy to change it to get_<model_name>_<field_name> (I actually had this format in a prevous commit a couple of months ago) |
Yes, you're right ! Maybe something like I think the most important point will be to well document in examples and the Dojo book how to do these things ^^ |
MemberId: (felt252, felt252) | ||
} | ||
|
||
pub trait ModelDefinition<T> { |
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.
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.
Hi @bengineer42 !
It is the same trait than the one you added in your PR but coming from this merged PR (#10).
I think there is no conflict, it should be the same trait 🤔
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.
Isnt the ModelDefinition
in that PR a struct not a trait?
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.
Isnt the
ModelDefinition
in that PR a struct not a trait?
Ah yes, understood 👍
Yes, this ModelDefinition
struct is used to return the full model definition from the model contract, so you can get the whole definition with only one contract call instead of calling all the functions one by one.
Maybe we can juste rename the struct ModelDefinition
to something else ?
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.
Any suggestions? ModelAttributes, @glihm I know you didn't like this for the trait but I'm lost for ideas
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.
Any suggestions? ModelAttributes, @glihm I know you didn't like this for the trait but I'm lost for ideas
ModelDef
? 😅
In fact, it provides the same information than the ModelDefinition
trait ... So quite difficult to find a different name for that.
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.
I think it is better to keep ModelDefinition
for the trait as it will be used directly, while the struct name won't be used. We will do something like:
let definition = dispatcher.definition();
println!("{}", definition.name);
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.
I will change ti to ModelDef for now just to keep it moving but if anyone has any better suggestions let me know
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.
@remybar there are some inconsistencies between struct and trait, eg fn namespace_hash() -> felt252;
vs pub namespace_selector: felt252,
pub trait ModelDefinition<T> {
fn name() -> ByteArray;
fn namespace() -> ByteArray;
fn tag() -> ByteArray;
fn version() -> u8;
fn selector() -> felt252;
fn name_hash() -> felt252;
fn namespace_hash() -> felt252;
fn layout() -> Layout;
}
#[derive(Drop, Serde, Debug, PartialEq)]
pub struct ModelDef {
pub name: ByteArray,
pub namespace: ByteArray,
pub namespace_selector: felt252,
pub version: u8,
pub layout: Layout,
pub schema: Ty,
pub packed_size: Option<u32>,
pub unpacked_size: Option<u32>
}
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.
@remybar there are some inconsistencies between struct and trait, eg
fn namespace_hash() -> felt252;
vspub namespace_selector: felt252,
pub trait ModelDefinition<T> { fn name() -> ByteArray; fn namespace() -> ByteArray; fn tag() -> ByteArray; fn version() -> u8; fn selector() -> felt252; fn name_hash() -> felt252; fn namespace_hash() -> felt252; fn layout() -> Layout; } #[derive(Drop, Serde, Debug, PartialEq)] pub struct ModelDef { pub name: ByteArray, pub namespace: ByteArray, pub namespace_selector: felt252, pub version: u8, pub layout: Layout, pub schema: Ty, pub packed_size: Option<u32>, pub unpacked_size: Option<u32> }
You can change to namespace_hash
in the struct, thanks 👍 Maybe we should also add name_hash
as it exists in the ModelDefinition
trait.
Hi @bengineer42, @glihm ! From what Ben did on his branch, I tried to rearrange traits to have less traits to handle/generate. It's more an experiment than a fixed solution. The result is there: https://github.com/remybar/dojo-core/tree/make-key-tuple-bar1 The idea is to just use the following traits: // use an Index to avoid having _from_key/_from_id functions
#[derive(Drop)]
pub enum Index<T> {
Key: T,
Id: felt252
}
// kind of merge of `ModelParser` and `EntityParser`.
// used to get index data from a model.
pub trait IndexParser<M, K, +Serde<K>> {
fn key(self: @M) -> K;
fn entity_id(self: @M) -> felt252 {
entity_id_from_key(@Self::key(self))
}
}
pub fn to_entity_id<K, +Serde<K>>(self: @Index<K>) -> felt252 {
match self {
Index::Key(key) => dojo::utils::entity_id_from_key(key),
Index::Id(id) => *id
}
}
fn to_model_index<K, +Serde<K>>(self: @Index<K>) -> ModelIndex {
match self {
Index::Key(key) => ModelIndex::Keys(dojo::utils::serialize_inline(key)),
Index::Id(id) => ModelIndex::Id(*id),
}
}
// kind of merge of `ModelParser` and `EntityParser`.
// trait to implement for Model and ModelEntity
// `deserialize` replaces the `from_values` function.
pub trait DojoSerde<T, +Serde<T>> {
fn serialize_keys(self: @T) -> Span<felt252>;
fn serialize_values(self: @T) -> Span<felt252>;
fn deserialize(ref serialized_keys: Span<felt252>, ref serialized_values: Span<felt252>) -> Option<T> {
let mut serialized: Array<felt252> = serialized_keys.into();
serialized.append_span(serialized_values);
let mut span = serialized.span();
Serde::<T>::deserialize(ref span)
}
}
// kind of merge of `ModelStore`, `EntityStore` and `MemberStore`.
// if we need model definition, we can directly use ModelDefinition<M> or eventually generates an impl `<ModelName>Definition` as it is done in the PR of Ben.
pub trait Model<M, K, E> {
fn get(self: @IWorldDispatcher, key: K) -> M;
fn get_entity(self: @IWorldDispatcher, index: Index<K>) -> E;
fn set(self: IWorldDispatcher, model: @M);
fn update(self: IWorldDispatcher, entity: @E);
fn delete(self: IWorldDispatcher, model: @M);
fn delete_entity(self: IWorldDispatcher, entity: @E);
fn delete_from_index(self: IWorldDispatcher, index: Index<K>);
fn update_serialized_member(
world: IWorldDispatcher,
entity_id: felt252,
member_id: felt252,
values: Span<felt252>,
);
fn get_serialized_member(
world: IWorldDispatcher,
entity_id: felt252,
member_id: felt252,
) -> Span<felt252>;
} Then, the generated code directly calls the At the end, we can use all this stuff like that: #[derive(Drop, Serde)]
#[dojo::model]
pub struct Attack {
#[key]
pub player: ContractAddress,
#[key]
pub monster_id: u32,
pub damage: u32,
pub effect: u32,
}
let key = (caller, 12);
let attack = AttackModel::get(@world, key);
let attack_entity = AttackModel::get_entity(@world, Index::Id(42));
let _ = AttackModel::get_entity(@world, Index::Key(key));
AttackModel::set(world, @attack);
// I don't know why the compiler does not accept world.set(@attack); ...
AttackModel::update(world, @attack_entity);
AttackModel::delete(world, @attack);
AttackModel::delete_entity(world, @attack_entity);
AttackModel::delete_from_index(world, Index::Id(42));
AttackModel::delete_from_index(world, Index::Key(key));
let _ = AttackModel::get_damage(world, key);
let _ = AttackModel::get_damage_from_index(world, Index::id(42));
let _ = AttackModel::get_damage_from_index(world, Index::Key(key)); It would be nice to be able to do Don't hesitate to tell me what you think about that ;-) |
I think I might know why, but its a bit complex, are you free for a call to discuss? I have sent you an email but feel free to add me on discord! |
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.
Awesome work @bengineer42, let's iterate on that to push that further. 🔥
Thank you @remybar for the reviews and the time taken also on discussions for that.
Make the model key a tuple and simplified models