refactor(types): serialize fixed array to object w size #3306
Conversation
|
ohayo sensei WalkthroughSerialization and deserialization for Ty::FixedSizeArray in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Ty
Caller->>Ty: to_json_value()
alt Ty::FixedSizeArray
Ty-->>Caller: {"elements": [...], "size": N}
else Other Ty variants
Ty-->>Caller: existing JSON format
end
Caller->>Ty: from_json_value(json)
alt json is {"elements": ..., "size": N}
Ty->>Ty: validate size matches
Ty-->>Caller: construct FixedSizeArray (per-element)
else json is legacy array
Ty-->>Caller: construct FixedSizeArray (legacy path)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
|
![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: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/dojo/types/src/schema.rs (1)
446-456: Ohayo sensei — inline the JSON return; keep it tidyMicro-nit: you can return the json! directly without the temporary binding. This was already raised in an earlier review; reiterating here for completeness.
Apply:
- let value = json!( - { - "elements": values?, - "size": size - } - ); - Ok(value) + Ok(json!({ + "elements": values?, + "size": size + }))Optional: consider asserting elements count vs declared size at serialization time to catch inconsistent states earlier (can be a debug_assert to avoid breaking prod paths).
🧹 Nitpick comments (1)
crates/dojo/types/src/schema.rs (1)
574-601: Ohayo sensei — add unit tests for new JSON shape and legacy fallbackTo lock this in and prevent regressions:
- FixedSizeArray.to_json_value outputs {"elements":[...], "size": N}.
- from_json_value accepts the object form (happy path) and rejects when elements.len() != size.
- from_json_value continues to accept the legacy bare array and enforces length == fixed size.
I can draft targeted tests in this module mirroring the existing style (serde_json round-trip for a small FixedSizeArray).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/dojo/types/src/schema.rs (1)
crates/dojo/world/src/contracts/model.rs (2)
values(270-270)values(281-281)
🪛 GitHub Actions: test
crates/dojo/types/src/schema.rs
[error] 598-598: Cargo clippy failed with exit code 101. Rust type errors in crates/dojo/types/src/schema.rs:598: mismatched types (usize vs u32) (E0308); cannot compare usize with u32 (E0277).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/dojo/types/src/schema.rs (1)
573-594: Ohayo sensei — enforce elements length and avoid potential panic when deserializing object formTwo robustness issues:
- Missing check that
"elements".len()equals the fixedsize(could leave the type in an inconsistent state).items[0]will panic if empty; useget(0).cloned().ok_or(...)to fail gracefully.Apply this diff:
(Ty::FixedSizeArray((items, size)), JsonValue::Object(obj)) => { if let (Some(JsonValue::Array(values)), Some(JsonValue::Number(expected_size))) = (obj.get("elements"), obj.get("size")) { if let Some(expected_size) = expected_size.as_u64() { if expected_size != *size as u64 { return Err(PrimitiveError::TypeMismatch); } - let template = items[0].clone(); + if values.len() as u64 != expected_size { + return Err(PrimitiveError::TypeMismatch); + } + let template = items.get(0).cloned().ok_or(PrimitiveError::TypeMismatch)?; items.clear(); for value in values { let mut item = template.clone(); item.from_json_value(value.clone())?; items.push(item); } } else { return Err(PrimitiveError::TypeMismatch); } } else { return Err(PrimitiveError::TypeMismatch); } }Optional migration tweak (if desired): allow omitting
"size"in the object and validate onlyelements.len() == *size as usize. I can provide a minimal diff if you want that behavior.
🧹 Nitpick comments (2)
crates/dojo/types/src/schema.rs (2)
445-455: Ohayo sensei — simplify JSON construction and assert invariant for FixedSizeArrayYou can avoid the temporary
valueand add a lightweight invariant check to catch inconsistent states early.Apply this diff:
Ty::FixedSizeArray((items, size)) => { - let values: Result<Vec<_>, _> = items.iter().map(|ty| ty.to_json_value()).collect(); - - let value = json!( - { - "elements": values?, - "size": size - } - ); - Ok(value) + debug_assert_eq!(items.len(), *size as usize); + let values: Result<Vec<_>, _> = items.iter().map(|ty| ty.to_json_value()).collect(); + Ok(json!({ + "elements": values?, + "size": size + })) }
1149-1183: Ohayo sensei — add tests for from_json_value (new object and legacy forms) and mismatch casesGreat coverage for to_json_value and felts-based deserialize. Please add JSON-deserialization tests to lock the new behavior and BC path, including mismatched sizes.
Suggested tests:
#[test] fn ty_from_json_fixed_size_array_object() { // template for elements let mut ty = Ty::FixedSizeArray((vec![Ty::Primitive(Primitive::Felt252(None))], 3)); let json = json!({"size": 3, "elements": ["0x1", "0x2", "0x3"]}); ty.from_json_value(json).expect("failed to from_json_value"); assert_matches!(&ty, Ty::FixedSizeArray((elements, size)) => { assert_eq!(*size, 3); assert_eq!(elements.len(), 3); }); } #[test] fn ty_from_json_fixed_size_array_legacy_array() { let mut ty = Ty::FixedSizeArray((vec![Ty::Primitive(Primitive::Felt252(None))], 2)); let json = json!(["0xA", "0xB"]); ty.from_json_value(json).expect("failed to from_json_value (legacy)"); assert_matches!(&ty, Ty::FixedSizeArray((elements, size)) => { assert_eq!(*size, 2); assert_eq!(elements.len(), 2); }); } #[test] fn ty_from_json_fixed_size_array_mismatched_sizes_error() { // Provided JSON size mismatches type size let mut ty = Ty::FixedSizeArray((vec![Ty::Primitive(Primitive::Felt252(None))], 3)); let json = json!({"size": 2, "elements": ["0x1", "0x2"]}); let res = ty.from_json_value(json); assert_matches!(res, Err(PrimitiveError::TypeMismatch)); }If you decide to allow omitting
"size"in the object form, I can add a test for that variant too.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/dojo/types/src/schema.rs(4 hunks)
🔇 Additional comments (1)
crates/dojo/types/src/schema.rs (1)
595-607: Ohayo sensei — legacy bare-array path looks solidThe backward-compat branch validates length (with proper usize/u32 cast) and clones from a template as expected. LGTM.
| Ty::FixedSizeArray((items_ty, size)) => { | ||
| let item_ty = &items_ty[0]; | ||
| for _ in 0..*size { | ||
| let mut cur_item_ty = item_ty.clone(); | ||
| cur_item_ty.deserialize(felts, legacy_storage)?; | ||
| debug_assert_eq!(items_ty.len(), *size as usize); | ||
| for elem in items_ty { | ||
| elem.deserialize(felts, legacy_storage)?; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei — deserialize now assumes per-element entries; make serialize symmetric to prevent data corruption
This path asserts items_ty.len() == size and deserializes each element individually. However, serialize for FixedSizeArray (Lines 186–191) still repeats items_ty[0] size times, which risks emitting duplicated felts when elements differ. Consider iterating each element instead to keep round-trip symmetry.
Proposed change for the serialize arm (outside this hunk, shown as plain Rust for clarity):
// In Ty::serialize -> serialize_inner match arm for Ty::FixedSizeArray
Ty::FixedSizeArray((items_ty, size)) => {
debug_assert_eq!(items_ty.len(), *size as usize);
for item_ty in items_ty {
serialize_inner(item_ty, felts)?;
}
}If you want, I can open a follow-up patch and add a unit test that exercises serialize→deserialize round-trips where element values differ (to catch this class of regressions).
🤖 Prompt for AI Agents
In crates/dojo/types/src/schema.rs around lines 283 to 287, the deserialize arm
for Ty::FixedSizeArray deserializes each element individually but serialize
currently repeats items_ty[0] size times; change the serialize match arm for
Ty::FixedSizeArray to iterate over items_ty and call serialize_inner on each
element (keeping the existing debug_assert_eq!(items_ty.len(), *size as usize))
so serialization mirrors deserialization and prevents duplicated felts when
elements differ.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / Compatibility