fix(core): FixedArray Ty/Layout improvements - #3296
Conversation
WalkthroughOhayo sensei! This change refactors the Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
kariy
left a comment
There was a problem hiding this comment.
i think we should add some sanity checks to make sure the size of Span<Ty> is exactly the same as the size in the tuple ie Ty::FixedSizeArray((Span<Ty>, u32)).
But it's not the case on dojo-core side. The Span always contains only one item which is the Ty of array items. But on Torii it seems that the Span is then used to store values. |
Ok yes you're right! I keep forgetting about that. It is pretty confusing when the type definition is inconsistent with what it's trying to represent. |
Yes, I agree! That's why I proposed to try to use a Box instead of a Span but it would break on Torii side 🫤 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/dojo/world/src/contracts/abigen/model.rs (2)
484-492: Ty: FixedArray tuple payload and discriminants look correctOhayo sensei — Ty::FixedArray((Vec, u32)) with ByteArray=5 and FixedArray=6 is consistently reflected across size/serialize/deserialize branches and uses the tuple serde helpers.
Given the ongoing confusion discussed in the PR about whether the inner Vec represents a single element type (core) vs values (Torii), consider adding a short doc comment in the non-generated API surface (e.g., types/schema.rs or a README) clarifying the semantics expected by core vs Torii. Avoid putting checks in generated files.
Also applies to: 497-507, 541-546, 566-569
393-393: Optional: Add sanity checks for FixedArray inner Vec length outside generated codeOhayo sensei — if core guarantees the inner Span/Vec holds exactly one Ty/Layout (the element type), a lightweight runtime assert (or constructor) in the hand-written layers could prevent misuse: e.g., enforce vec.len() == 1 when constructing FixedArray on the core path.
I can sketch a small helper/newtype for FixedArray element type to centralize the invariant if you’d like.
Also applies to: 491-491
crates/dojo/world/src/contracts/abigen/world.rs (1)
3681-3689: Nit: Consider documenting the meaning of the inner Vec for FixedArray in consumer docsOhayo sensei — since abigen maps Span to Vec, a note in user-facing docs clarifying whether Vec is “element type (len=1)” vs “values” would save future head-scratching. Keep generated code unchanged.
Also applies to: 3694-3703
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (3)
crates/dojo/world/src/contracts/abigen/model.rs(8 hunks)crates/dojo/world/src/contracts/abigen/world.rs(4 hunks)crates/sozo/ops/src/model.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/sozo/ops/src/model.rs
🔇 Additional comments (4)
crates/dojo/world/src/contracts/abigen/model.rs (2)
386-394: FixedArray moved to end and payload switched to tuple — LGTMOhayo sensei — placing FixedArray last and using (Vec, u32) removes mid-enum insertion churn and matches the PR intent.
399-409: Ohayo sensei! Discriminants & shapes are in sync across cratesAll the updates landed correctly—ByteArray=4, Enum=5, FixedArray=6—across size, serialize, and deserialize for both Layout and Ty. FixedArray now consistently uses
<(Vec<Layout>, u32)>::cairo_*(or<(Vec<Ty>, u32)>for Ty) in:
- core/src/meta/{layout,introspect}.cairo (FixedArray:
(Span<…, u32))- crates/dojo/world/src/contracts/abigen/model.rs & world.rs
– size:+1on<(Vec<…, u32)>::cairo_serialized_size
– serialize: prefix with6usizethen<(Vec<…, u32)>::cairo_serialize
– deserialize: arm6usize→FixedArray(<(Vec<…, u32)>::cairo_deserialize…)No stale
Vec<(Layout, u32)>,Span<(Layout, u32)>, or old tuple shapes remain. LGTM!crates/dojo/world/src/contracts/abigen/world.rs (2)
3681-3689: Layout: FixedArray moved to end with tuple payload — aligned with coreOhayo sensei — Variant order and the new (Vec, u32) payload match the PR objective and core Cairo definitions.
3694-3703: Layout discriminants remapped (ByteArray=4, Enum=5, FixedArray=6) — wire format stableOhayo sensei — I ran the scans and confirmed:
- No
Vec<(Layout, u32)>usages remain incrates/dojo/world.- The Cairo
Layoutenum incrates/dojo/core/src/meta/layout.cairolists ByteArray as 4, Enum as 5, FixedArray as 6 with the correct(Span<Layout>, u32)payload.All clear to merge!
FixedArray support has been added recently but it has been added in the middle of
TyandLayoutenums.To avoid any issue in case of enum serializing, it is better to add this FixedArray variant at the end of these enums.
Then, to ease FixedArray processing by Torii, Layout and Ty now use
(Span<Layout>, u32)/(Span<Ty>, u32)instead ofSpan<(Layout, u32)>/Span<(Ty, u32)>.Summary by CodeRabbit