feat(schema): support legacy & non legacy ser #3356
Conversation
|
ohayo, sensei WalkthroughSerialization APIs now accept a legacy_storage: bool flag. Member::serialize and Ty::serialize signatures changed to receive and forward the flag into serialize_inner, which threads legacy_storage through recursive serialization and adjusts enum/composite handling and error/zero semantics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Member
participant Ty
participant Serializer as serialize_inner
Caller->>Member: serialize(legacy_storage)
Member->>Ty: serialize(legacy_storage)
Ty->>Serializer: serialize_inner(self, felts, legacy_storage)
loop recurse through nested Ty variants
Serializer->>Serializer: descend(..., legacy_storage)
end
Serializer-->>Ty: Result<Vec<Felt>>
Ty-->>Member: Result<Vec<Felt>>
Member-->>Caller: Result<Vec<Felt>>
note right of Serializer: legacy_storage toggles enum/index handling and error-vs-zero behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/dojo/types/src/schema.rs (1)
crates/dojo/types/src/primitive.rs (1)
serialize(769-831)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/dojo/types/src/schema.rs (1)
crates/dojo/types/src/primitive.rs (1)
serialize(769-831)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/dojo/types/src/schema.rs (1)
crates/dojo/types/src/primitive.rs (1)
serialize(769-831)
| pub fn serialize(&self, legacy_storage: bool) -> Result<Vec<Felt>, PrimitiveError> { | ||
| self.ty.serialize(legacy_storage) | ||
| } |
There was a problem hiding this comment.
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.
| serialize_inner(&selected_option.ty, felts, legacy_storage)?; | ||
| } | ||
| } else { | ||
| // For uninitialized enum in new storage system, use 0 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3 participants
Summary by CodeRabbit