Skip to content

feat(schema): support legacy & non legacy ser #3356

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+35 −24
Open

feat(schema): support legacy & non legacy ser #3356

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
779125d
feat(schema): support legacy & non legacy ser
Larkooo Sep 25, 2025
00f214d
avoid u8 overflow
Larkooo Sep 25, 2025
f80751a
keys legacy ser
Larkooo Sep 25, 2025
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
59 changes: 35 additions & 24 deletions crates/dojo/types/src/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ pub struct Member {
}

impl Member {
pub fn serialize(&self) -> Result<Vec<Felt>, PrimitiveError> {
self.ty.serialize()
pub fn serialize(&self, legacy_storage: bool) -> Result<Vec<Felt>, PrimitiveError> {
self.ty.serialize(legacy_storage)
}
Comment on lines +24 to 26
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Respect key semantics in Member::serialize

ohayo sensei, Member::serialize now exposes the legacy_storage flag but ignores self.key. Callers that pass legacy_storage = false when serializing key members will emit the new 1-based encoding, while deserialize on Line 231 still forces legacy semantics via child.key || legacy_storage, breaking round-trips for key enums/structs. Please mirror that predicate here.

         self.ty.serialize(legacy_storage)
+        self.ty.serialize(self.key || legacy_storage)

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

🤖 Prompt for AI Agents 
In crates/dojo/types/src/schema.rs around lines 24 to 26, Member::serialize
currently ignores self.key and always passes legacy_storage through to
ty.serialize; update it to respect key semantics by calling
self.ty.serialize(self.key || legacy_storage) so key members force legacy
encoding and preserve round-trip behavior with deserialize.

}

Expand Down Expand Up @@ -139,52 +139,63 @@ impl Ty {
}
}

pub fn serialize(&self) -> Result<Vec<Felt>, PrimitiveError> {
pub fn serialize(&self, legacy_storage: bool) -> Result<Vec<Felt>, PrimitiveError> {
let mut felts = vec![];

fn serialize_inner(ty: &Ty, felts: &mut Vec<Felt>) -> Result<(), PrimitiveError> {
fn serialize_inner(
ty: &Ty,
felts: &mut Vec<Felt>,
legacy_storage: bool,
) -> Result<(), PrimitiveError> {
match ty {
Ty::Primitive(c) => {
felts.extend(c.serialize()?);
}
Ty::Struct(s) => {
for child in &s.children {
serialize_inner(&child.ty, felts)?;
serialize_inner(&child.ty, felts, child.key || legacy_storage)?;
}
}
Ty::Enum(e) => {
let option = e
.option
.map(|v| Ok(vec![Felt::from(v)]))
.unwrap_or(Err(PrimitiveError::MissingFieldElement))?;
felts.extend(option);

// TODO: we should increment `option` is the model does not use the legacy
// storage system. But is this `serialize` function still
// used ?

for EnumOption { ty, .. } in &e.options {
serialize_inner(ty, felts)?;
if let Some(option) = e.option {
// For new storage system, enum variant indices start from 1
let mut serialized_option = Felt::from(option);
if !legacy_storage {
serialized_option += Felt::ONE;
}
felts.push(serialized_option);

// Only serialize the selected option
if let Some(selected_option) = e.options.get(option as usize) {
serialize_inner(&selected_option.ty, felts, legacy_storage)?;
}
} else {
// For uninitialized enum in new storage system, use 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about that.
When we serialize a Ty:Enum, the enum should always be correctly initialized.
So, if option is None or if we cannot find the selected option among the options we should raise an error because the Ty:Enum has been incorrectly set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But doesn't the new storage system actually allow enums to be None, hence the whole goal of bringing this new layout change? So that, in the case of an uninitialized enum, the option set is correctly None.

Because here we're just mimicking the exact same behaviour as the deserialize, but in the other way.
In the deserialize, when we have a selector of 0, we set the option to None.

So if someone deserializes a new layout Enum with a selector of 0, then tries to serialize it again, it will raise an error and lead to inconsistency across the two functions. We need to either not allow enum options to be None, or allow them.

Copy link
Collaborator

@remybar remybar Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, the real behaviour at dojo-core level, for a DojoStore model (so non legacy one) is:

  • when you read an enum from the world storage and it is not yet initalized, the read variant index is 0.
  • when we deserialize an enum, the variant index is supposed to start from 1. If it is set to 0, we just return the configured default value of the enum. It could be the third variant with some specific data like MyEnum:ThirdVariant(array![1, 2, 3]), whatever.
  • we cannot serialize an uninitialized enum because we call the serialize() function on an enum variable which must be initialized first in Cairo.

Unfortunately, the schema (Ty) provided by the schema() function of a model contract does not contain the default value of an enum.

So, at sozo level, when we deserialize an uninitialized enum (because we may read uninitialized storage when calling sozo model get command), we just display <ENUM_NAME>::default(). See here:
https://github.com/dojoengine/dojo/blob/main/crates/sozo/ops/src/model.rs#L549-L560
There is a TODO to think about how to display the real default value instead of just <ENUM_NAME>::default() which is not so clear but it might not be so easy to do 😅

At torii level, I don't know why you may have to serialize some model data. I thought only the deserialize() function was used. Could you tell me more about that ? But, for me, it doesn't make sense to serialize an uninitialized enum. A possible use case would be to override an existing enum in the world storage to erase it but there is already delete functions at world level for that.

So, yes, if you could tell me the usecases you see in Torii for that, it could help me to understand the needs.

if !legacy_storage {
felts.push(Felt::ZERO);
} else {
return Err(PrimitiveError::MissingFieldElement);
}
}
}
Ty::Tuple(tys) => {
for ty in tys {
serialize_inner(ty, felts)?;
serialize_inner(ty, felts, legacy_storage)?;
}
}
Ty::Array(items_ty) => {
let _ = serialize_inner(
&Ty::Primitive(Primitive::U32(Some(items_ty.len().try_into().unwrap()))),
felts,
legacy_storage,
);
for item_ty in items_ty {
serialize_inner(item_ty, felts)?;
serialize_inner(item_ty, felts, legacy_storage)?;
}
}
Ty::FixedSizeArray((items_ty, size)) => {
let item_ty = &items_ty[0];
for _ in 0..*size {
serialize_inner(item_ty, felts)?;
Ty::FixedSizeArray((items_ty, _size)) => {
for elem in items_ty {
serialize_inner(elem, felts, legacy_storage)?;
}
}
Ty::ByteArray(bytes) => {
Expand All @@ -196,7 +207,7 @@ impl Ty {
Ok(())
}

serialize_inner(self, &mut felts)?;
serialize_inner(self, &mut felts, legacy_storage)?;

Ok(felts)
}
Expand Down
Loading