fix(core): introduce DojoStore trait to handle some specific storage cases #3051
Conversation
remybar
force-pushed
the
fix-enum_options_storage
branch
3 times, most recently
from
a4a6f88 to
7026e34
Compare
WalkthroughOhayo sensei! This update introduces a comprehensive overhaul to Dojo's storage serialization and deserialization, adding the new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Model (Contract)
participant DojoStore
participant LegacyStore
participant CLI/SDK
User->>CLI/SDK: Query or update model data
CLI/SDK->>Model: Call use_legacy_storage()
Model-->>CLI/SDK: Returns true (legacy) or false (new)
CLI/SDK->>DojoStore: (if false) Serialize/deserialize using DojoStore
CLI/SDK->>LegacyStore: (if true) Serialize/deserialize using legacy logic
DojoStore-->>CLI/SDK: Data in new format
LegacyStore-->>CLI/SDK: Data in legacy format
CLI/SDK-->>User: Presents/updates formatted data
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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
🧹 Nitpick comments (14)
crates/dojo/lang/src/derive_macros/mod.rs (1)
24-24: Consider adding proper documentation for the new derive attribute.While the constant itself is clear, adding documentation about its purpose and usage would help users understand when and why to use the DojoLegacyStorage derive attribute.
pub const DOJO_LEGACY_STORAGE_DERIVE: &str = "DojoLegacyStorage"; + +/// The DojoLegacyStorage derive allows models to retain the current Serde-based storage behavior +/// instead of using the new DojoStore trait. This is useful for maintaining compatibility with +/// existing world storage during the transition period.crates/dojo/core/src/storage/layout.cairo (1)
192-192: Enhanced error message with variant detailsThe improved error message includes the variant value, which will make debugging much easier when an invalid variant is encountered.
crates/dojo/lang/src/lib.rs (1)
44-53: Excellent debugging function for DojoStore implementationOhayo! The new
debug_store_expandfunction looks good and follows the same pattern as the existingdebug_expandfunction. This will be helpful for debugging the DojoStore trait implementation when needed.A small suggestion: consider adding a parameter for the verbosity level to control the output format, which could help when debugging complex storage operations.
-pub fn debug_store_expand(element_name: &str, code: &str) { +pub fn debug_store_expand(element_name: &str, code: &str, verbosity: Option<u8>) { if std::env::var("DOJO_STORE_EXPAND").is_ok() { + let verbosity_level = verbosity.unwrap_or(1); + if verbosity_level > 1 { println!( "\n*> EXPAND {} <*\n>>>>>>>>>>>>>>>>>>>>>>>>>>>\n{}\n<<<<<<<<<<<<<<<<<<<<<<<<<<<\n", element_name, code ); + } else { + println!("*> EXPAND {}: {} bytes of code", element_name, code.len()); + } } }crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (1)
73-77: Default implementation for AnotherEnum required by DojoStoreOhayo, sensei! Similar to MyEnum, this change adds the Default trait to AnotherEnum and marks X as the default variant. This is necessary for the new DojoStore trait implementation.
Consider documenting in the enum definition why Default is required, to make it clearer for future developers.
#[derive(Introspect, Copy, Drop, Serde, Default)] +/// AnotherEnum implements Default to work with DojoStore, +/// which uses variant 0 to represent uninitialized values enum AnotherEnum { #[default] X: u8, }crates/dojo/lang/src/attribute_macros/model.rs (1)
79-80: Ohayo sensei, good use of separate vectors for deserialized values.
Separating tracked arrays can improve readability. If performance becomes a concern, consider using specialized data structures or in-place transformations.crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (2)
17-20: Ohayo sensei, collecting struct member aliases.
Storing member details in vectors is workable. If you run into ordering or duplication issues, you may consider a small dedicated struct or map for more robust handling.
55-59: Ohayo sensei, efficient string joins.
Combining the joined segments outside the loop is good for clarity. Watch out for accidental whitespace or commas that break the final structure.crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
151-153: Critical change for the DojoStore implementation, sensei!This modification to increment the variant index is the core change that addresses the PR objective - solving the problem where variant id 0 cannot be distinguished from an uninitialized variant. Now variants start from 1, which is a fundamental change to support the new storage mechanism.
This change ensures that when deserializing, a variant index of 0 can be properly interpreted as uninitialized, triggering the return of the default value. Excellent architectural decision!
crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo (2)
33-35: Essential addition of use_legacy_storage method, sensei!This method is crucial for the PR objective - it allows models to indicate whether they should use the legacy storage behavior or the new
DojoStoretrait. This enables backward compatibility via theDojoLegacyStoragederive attribute mentioned in the PR description.This toggle mechanism provides an elegant migration path for existing models while allowing new ones to benefit from the improved storage handling.
88-96: Excellent implementation of ValueDojoStore!This is a core part of the PR - implementing the
DojoStoretrait for model values with both serialization and deserialization support. The template correctly includes placeholders for the serialization and deserialization logic.The separation of serialization and model logic through this trait is a good design pattern that allows for specialized handling of storage edge cases like
Option<T>and custom enums.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (2)
31-197: Ohayo sensei: Consider a table-driven approach to reduce repetitive tests.
This test function checks many primitive types in the same pattern. Using a table-driven approach or macro-based testing could eliminate duplicated code and make maintenance easier.
192-192: Ohayo sensei: Fix mismatch in the error message.
The error message referencesi128instead ofByteArray. Here’s a suggested fix:- assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<i128> serialization failed"); + assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<ByteArray> serialization failed");crates/dojo/core/src/storage/dojo_store.cairo (2)
66-76: Ohayo sensei! Watch out for recursion in large arrays.
Recursively invokingserialize_array_helpercould cause excessive stack usage for large arrays. An iterative approach might be more robust if large arrays are expected.
78-86: Ohayo sensei! Consider iterating indeserialize_array_helper.
Similarly, deserializing large arrays with recursion can hit stack limits in extreme cases. Switching to an iterative method may improve reliability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockcrates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (48)
Cargo.toml(1 hunks)crates/benches/contracts/src/models/character.cairo(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/storage/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(1 hunks)crates/dojo/core/src/meta/layout.cairo(1 hunks)crates/dojo/core/src/model/component.cairo(1 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/interface.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/layout.cairo(8 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(0 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/mod.rs(0 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(3 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(1 hunks)crates/dojo/lang/src/derive_macros/mod.rs(1 hunks)crates/dojo/lang/src/lib.rs(2 hunks)crates/dojo/lang/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/abigen/model.rs(2 hunks)crates/dojo/world/src/contracts/model.rs(2 hunks)crates/sozo/ops/src/model.rs(8 hunks)crates/torii/sqlite/src/model.rs(1 hunks)crates/torii/types-test/src/models.cairo(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/src/models.cairo(1 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/lang/src/attribute_macros/mod.rs
- crates/dojo/lang/src/attribute_macros/element.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (141)
examples/spawn-and-move/dojo_dev.toml (1)
71-71:✅ Verification successful
Verify that the world address has been updated in all necessary places
Ohayo, sensei! The world address has been updated to a new value. This change appears to be related to the addition of the
DojoStoretrait for handlingOption<T>and custom enum storage.
🏁 Script executed:
#!/bin/bash # Ensure the world address is consistent across the codebase rg "0x3aacf8c2da6370f1ba7f331094dff29e209d40a98ac94f20c66c34128177799" --type tomlLength of output: 204
Ohayo, sensei!
I’ve verified that the updated world address:
world_address = "0x3aacf8c2da6370f1ba7f331094dff29e209d40a98ac94f20c66c34128177799"appears only in
examples/spawn-and-move/dojo_dev.tomlat line 71. No additional occurrences were found across the TOML files, so it looks like the update is correctly localized. This change aligns with the recent additions related to theDojoStoretrait.Keep up the great work!
crates/dojo/core/src/model/interface.cairo (1)
10-10: Clean interface addition for the new storage trait systemOhayo, sensei! This new
use_legacy_storagemethod is a good addition to theIModeltrait. It provides a clean way for models to specify whether they should use the legacy Serde-based storage or the newDojoStoretrait that properly handlesOption<T>and custom enums.The method signature follows the conventions of other methods in this trait, taking a reference to self and returning a boolean.
crates/dojo/core/src/model/definition.cairo (1)
23-23: Nice addition to model definition for storage behavior controlOhayo, sensei! The
use_legacy_storagemethod is an excellent addition to theModelDefinitiontrait. This static method allows models to declare at the type level whether they should use legacy storage or the newDojoStoretrait system.This pairs well with the similar method added to the
IModeltrait and enables theDojoLegacyStoragederive attribute mentioned in the PR objectives.crates/dojo/lang/Cargo.toml (1)
31-31:❓ Verification inconclusive
Added parser dependency for attribute processing
Ohayo, sensei! Adding the Cairo language parser as a development dependency makes sense for implementing the
DojoLegacyStoragederive attribute. This will allow the code to properly parse and process the attribute during the compilation phase.
🏁 Script executed:
#!/bin/bash # Check if the cairo-lang-parser is being used in the implementation of the DojoLegacyStorage attribute rg -A 3 -B 3 "cairo-lang-parser" --type rustLength of output: 44
Ohayo, sensei! The addition of the
cairo-lang-parser.workspace = trueline incrates/dojo/lang/Cargo.tomllooks like it should support the attribute processing forDojoLegacyStorage. However, our automated search—limited to Rust source files—didn't show any direct usage ofcairo-lang-parser. This may be because the dependency is only referenced in the TOML configuration or used indirectly during macro expansion.
- Please ensure that the derive attribute implementation indeed leverages the Cairo parser during compilation.
- Consider reviewing the macro expansion or testing the build output to confirm that the attribute processing behaves as expected.
- If necessary, update your search scope (or perform a manual check) to include TOML files or any indirect references.
crates/dojo/lang/src/derive_macros/introspect/utils.rs (1)
42-44: Ohayo! Clean, functional addition for Option type detection.The
is_optionfunction follows the same pattern as the existing type detection functions, making it a natural extension to the type introspection capabilities. This will be valuable for handling Option types differently during serialization/deserialization.crates/dojo/core/src/model/component.cairo (1)
40-42: Ohayo! Solid implementation of the storage strategy selector.This method nicely delegates to the Model implementation, creating a consistent interface for determining whether to use legacy storage. This allows components to transparently handle different storage approaches without knowing the implementation details.
crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo (1)
64-66: Ohayo sensei! Testing the non-legacy storage path with a hardcoded return.The implementation always returns
false, ensuring this test model uses the newDojoStoreapproach. This is appropriate for testing the new storage mechanism.Do you plan to add test cases that explicitly test both
trueandfalsereturn values to ensure both storage paths are covered? This would help verify the correctness of both legacy and new storage mechanisms.crates/dojo/core/src/lib.cairo (1)
71-72:❓ Verification inconclusive
Ohayo! Important module export for the new storage functionality.
This change exposes the new
DojoStoretrait andContractAddressDefaulttype, making them available for use throughout the codebase. This is essential for the implementation of the solution described in the PR objectives.
🏁 Script executed:
#!/bin/bash # Let's check if there are implementations of DojoStore for Option types # and custom enums in the codebase echo "Checking for DojoStore implementations for Option types:" rg -A 10 "impl DojoStore<Option<" crates/ echo -e "\nChecking for Default implementations for custom enums:" rg -A 5 "#\[derive\(.*Default.*\)\].*enum" crates/Length of output: 344
Ohayo sensei! Please verify the new storage exports manually.
The module export for the new storage functionality in
crates/dojo/core/src/lib.cairo(lines 71–72) appears to be correctly exposing theDojoStoretrait andContractAddressDefaulttype, aligning with the PR’s objectives. However, our automated search for implementations (such asDojoStoreimplementations forOptiontypes and default implementations for custom enums) returned no output—which may be due to the absence of such patterns or might require manual verification.
- File Checked:
crates/dojo/core/src/lib.cairo- Exported Items:
pub mod dojo_store;pub use dojo_store::{ContractAddressDefault, DojoStore};Please manually verify that the absence of additional related implementations is expected.
crates/dojo/core-cairo-test/src/tests/world/storage.cairo (1)
133-134: Ohayo! The test assertions now correctly verify the default behavior for uninitialized variants.The changes to these assertions reflect the core improvement introduced by the DojoStore trait, which solves the problem with variant id 0 being indistinguishable from uninitialized values. Now the test correctly verifies that:
EnumOne::Two(0)is returned as the default value for an uninitialized enumOption::Noneis returned for an uninitialized OptionThis aligns perfectly with the PR objectives, sensei!
crates/dojo/core-cairo-test/src/lib.cairo (2)
20-20: Ohayo! Great addition of the layout test module, sensei.This new test module will help ensure the layout functionality works correctly with the DojoStore trait implementations. The structure of your test organization is clean and logical.
33-33: Excellent addition of tests for the new DojoStore functionality!The new dojo_store test module is essential for verifying that the trait handles Option and custom enums correctly, especially regarding the variant id 0 issue mentioned in the PR objectives.
crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (1)
18-21: Ohayo, sensei! Special case handling for event serialization looks good.The new deserialize method correctly specifies that events always use Serde since they're never stored in world storage. This is a good distinction to make and aligns with the PR objectives of introducing DojoStore for model storage while allowing legacy behavior where appropriate.
crates/dojo/lang/src/derive_macros/mod.rs (1)
24-24: Perfect implementation of the DojoLegacyStorage derive attribute, sensei!This constant introduces the derive attribute mentioned in the PR objectives, allowing models to retain the current storage behavior while the new DojoStore trait becomes the default. This provides a smooth migration path for existing code.
crates/dojo/core/src/model/model_value.cairo (2)
7-8: Effective API addition for handling DojoStore serializationOhayo, sensei! This new method in the ModelValueParser trait is a well-structured approach for implementing custom deserialization logic. It provides the necessary abstraction layer to handle Option and enum variants differently from regular Serde deserialization.
36-37: Appropriate implementation switch from Serde to ModelValueParserThis change correctly implements the PR objectives by redirecting deserialization from Serde to the new ModelValueParser trait, enabling custom handling for enum variants with 0 indices.
examples/spawn-and-move/src/models.cairo (1)
3-5: Default trait addition properly addresses enum variant issueOhayo, sensei! This change correctly implements the #[default] attribute on the None variant, which addresses the core problem described in the PR objectives. Now, the Direction enum can properly distinguish between an uninitialized variant and an explicitly set None variant.
crates/dojo/core/src/world/world_contract.cairo (1)
1156-1163: Valid protection for fixed layout resourcesOhayo, sensei! This additional validation is critical for ensuring that resources with fixed layouts maintain consistent storage patterns during upgrades. The check prevents potential data corruption by rejecting layout changes that would break existing storage.
crates/dojo/core/src/storage/layout.cairo (6)
10-11: Good refactoring with MAX_VARIANT_INDEX constantOhayo, sensei! Extracting this value into a named constant improves code maintainability and readability by centralizing the maximum variant index value that was previously hardcoded in multiple locations.
179-179: Consistent application of MAX_VARIANT_INDEXThe assertion properly uses the new constant instead of a hardcoded value, improving consistency and maintainability.
304-304: Consistent application of MAX_VARIANT_INDEXThe assertion correctly uses the newly defined constant, ensuring consistent validation across the codebase.
314-314: Better error reporting for variant layout issuesIncluding the variant value in the error message is a helpful improvement for debugging.
468-468: Consistent boundary check with MAX_VARIANT_INDEXThe validation is properly using the new constant, maintaining consistency with other assertions.
477-486: Crucial handling of uninitialized variantsOhayo, sensei! This is the core implementation that addresses the PR objective. The special handling for variant 0 is essential for the DojoStore trait to properly distinguish between uninitialized variants and explicitly set variants. The comments clearly explain the rationale behind this behavior.
crates/torii/sqlite/src/model.rs (1)
117-120:❓ Verification inconclusive
Ohayo! New method adds needed legacy storage support.
The new
use_legacy_storagemethod with a placeholder implementation will help distinguish between models using the newDojoStoretrait and those using legacy storage behavior.However, sensei, there's a TODO comment that should be addressed. Let's check what other implementations of this method look like:
🏁 Script executed:
#!/bin/bash # Search for other implementations of use_legacy_storage rg "fn use_legacy_storage" --type cairo -A 5Length of output: 76
Ohayo sensei! It looks like the initial search command used the wrong file type flag, which is why you saw the “unrecognized file type: cairo” error. Let's re-run the search using the proper flag for Rust files to check for other implementations of
use_legacy_storage.Please run this updated script and manually verify the results. Once you’ve confirmed whether there are existing implementations that need to be aligned with or if the placeholder can remain until further work is done, feel free to address or remove the TODO comment accordingly.
🏁 Script executed:
#!/bin/bash # Search for other implementations of use_legacy_storage in Rust files rg "fn use_legacy_storage" -t rust -A 5Length of output: 2700
Ohayo sensei! The new
use_legacy_storagemethod incrates/torii/sqlite/src/model.rsis correctly added as a placeholder to differentiate legacy storage from models using the newDojoStoretrait. That said, I noticed the lingering TODO comment. Our search shows that similar methods exist in:
crates/dojo/world/src/contracts/abigen/model.rs– where the function returns anFCallrelated to legacy storage.crates/dojo/world/src/contracts/model.rs– where an async trait method is defined and implemented similarly.These examples imply that the legacy storage support might eventually need a more robust implementation. Please either implement the intended functionality in line with these examples or remove the TODO comment to avoid confusion.
crates/dojo/core-cairo-test/src/tests/world/event.cairo (2)
48-51: Excellent addition of Default trait and default variant, sensei!Adding the
Defaulttrait toMyEnumand markingXas the default variant solves the problem with custom enums where variant id0couldn't be distinguished from an uninitialized variant.
64-67: Well done adding Default to AnotherEnum too!Ohayo! This follows the same pattern as the previous enum, ensuring consistent behavior across all custom enums. The
#[default]attribute on variantXensures it will be used when initializing withDefault::default().crates/torii/types-test/src/models.cairo (1)
74-77: Perfect implementation of Default for Depth enum, sensei!Adding the
Defaulttrait and markingZeroas the default variant is an excellent choice for the Depth enum - the name "Zero" naturally suggests it should be the default value, which improves code readability.crates/dojo/world/src/contracts/model.rs (2)
61-61: Ohayo! Good trait addition for model reader interface.Adding the
use_legacy_storagemethod to theModelReadertrait ensures consistency across all implementations and supports the PR objective of handling different storage behaviors.
209-211: Clean implementation of use_legacy_storage method, sensei!The implementation properly delegates to the underlying model reader's method, maintaining consistent behavior throughout the codebase.
crates/dojo/core/src/meta/layout.cairo (2)
52-58: Well-documented function implementation!Ohayo, sensei! This is a clear and thorough docstring that explains the purpose of the function, the problem it's addressing, and the context around variant indexing changes from 0 to 1.
59-109: Well-structured implementation of the legacy layout builder!The implementation recursively processes different layout types correctly:
- For enums: Decrements the selector (variant index) by 1 to convert from new format (1-based) to legacy format (0-based)
- For structs: Preserves selectors but recursively processes child layouts
- For tuples and arrays: Recursively processes their layouts
- For other types: Returns them unchanged
This function enables backward compatibility while allowing the new storage system to work correctly with Option and custom enums.
crates/dojo/world/src/contracts/abigen/model.rs (2)
671-682: Clean implementation of legacy storage checker for ModelContract!Ohayo, sensei! This implementation follows the established pattern of the other methods in this class. It creates a function call to the
use_legacy_storageentry point and properly returns anFCallobject with a boolean result type.
800-811: Consistent implementation of legacy storage checker for ModelContractReader!This mirrors the implementation for ModelContract, ensuring consistent behavior across both contract interfaces. Good job maintaining code symmetry!
crates/dojo/core/src/meta/introspect.cairo (1)
599-603: Important variant indexing change to support new storage system!Ohayo, sensei! This change is a critical part of the DojoStore trait implementation. By changing the Option's variant indices from 0/1 to 1/2, we ensure we can distinguish between uninitialized variants (which would be 0) and actual variant values.
The selector for
Someis now 1 (previously 0), and the selector forNoneis now 2 (previously 1). This change works together with thebuild_legacy_layoutfunction to enable backward compatibility.crates/benches/contracts/src/models/character.cairo (2)
39-44: Added Default trait and default variant for Weapon enum!Ohayo, sensei! Adding the
Defaulttrait and specifying#[default]for theDualWieldvariant ensures we have a valid default value when reading an uninitialized Weapon. This works with the new DojoStore trait to properly deserialize enum values.
46-50: Added Default trait to Sword struct!Adding the
Defaulttrait to theSwordstruct ensures we can instantiate default values for its fields when needed. This complements the default implementation forWeaponand supports the new storage management system.crates/dojo/lang/src/lib.rs (1)
14-14: New public module added for utilitiesOhayo, sensei! The addition of a new public module
utilsseems to be related to the DojoStore implementation, which is at the heart of this PR. Make sure that this module is properly documented and contains all necessary utility functions for working with DojoStore.crates/dojo/lang/src/derive_macros/introspect/layout.rs (3)
10-11: Added utility function for Option type detectionOhayo, sensei! The addition of
is_optionto the imported utilities is necessary for the new Option handling in the DojoStore trait. This seems well-integrated with the existing utility functions.
380-387: Proper error handling for Option type in packed layoutsOhayo, sensei! This addition correctly handles the case when Option is used in a packed layout. Since Option types can't be packed (due to their variable size nature), it's important to provide a clear error message to developers.
The diagnostic message is clear and indicates exactly what the issue is, which is excellent.
78-81:❓ Verification inconclusive
Critical change for variant indexing to support DojoStore trait
Ohayo! This is a fundamental change that addresses the core issue mentioned in the PR objectives. By incrementing the variant index to start from 1 instead of 0, you enable the use of 0 to represent uninitialized variants, which solves the problem with Option and custom enums.
The comment explains the rationale clearly, which is excellent for future maintainers.
Let's verify that all custom enums affected by this change also implement Default trait:
🏁 Script executed:
#!/bin/bash # Find all enum definitions in the codebase and check if they have Default implementation echo "Checking for enums that might be affected by the variant index change..." # Find all enums rg --type cairo -A 2 "enum\s+\w+\s*\{" --glob "*.cairo" | grep -v "impl.*Default" echo "These enums might need to implement Default to work properly with DojoStore"Length of output: 429
Ohayo sensei! Variant indexing change confirmed as a critical design update.
The update—incrementing the variant index by 1 so that 0 can denote an uninitialized variant—is an essential fix for the DojoStore trait and addresses the Option and custom enum issues nicely. However, our automated search for enums in *.cairo files flagged an issue (“unrecognized file type: cairo”), which means we couldn’t conclusively verify that all custom enums have a Default implementation.
Action items:
- Revisit the enum scan: Adjust the RG search command (e.g., remove the
--type cairoflag or define a custom file type for Cairo) to properly locate and verify enums.- Manual review: Alternatively, manually check that custom enums in the codebase properly implement the Default trait to ensure compatibility with the DojoStore logic.
crates/dojo/core-cairo-test/src/tests/helpers/model.cairo (1)
58-62: Default implementation for MyEnum required by DojoStoreOhayo! This change is necessary to support the new DojoStore trait. By adding the Default trait and marking variant X as default, you're ensuring that MyEnum can be properly deserialized when encountering variant 0 (uninitialized).
This aligns perfectly with the PR objectives that mention custom enums need to implement Default.
crates/dojo/core/src/model/model.cairo (4)
53-55: Key addition of deserialize method to ModelParser traitOhayo, sensei! This new method is a crucial part of the DojoStore implementation. It allows for custom deserialization logic that handles Option and custom enums correctly, by returning Default::default() when encountering variant 0.
The method signature is clean and follows Cairo conventions.
84-87: Essential method to detect legacy storage usageOhayo! The
use_legacy_storagemethod is a key part of the backward compatibility strategy mentioned in the PR objectives. It allows models to specify whether they should use the legacy storage behavior or the new DojoStore behavior.This is well-documented and the method name clearly conveys its purpose.
154-157: Implementation of use_legacy_storage methodOhayo! This implementation delegates to the ModelDefinition trait, which is a clean approach that keeps the responsibility for determining the storage type with the model definition.
This method is crucial for the backward compatibility mentioned in the PR objectives, allowing models marked with DojoLegacyStorage to maintain their current behavior.
135-136:❓ Verification inconclusive
Updated deserialization logic to use ModelParser
Ohayo, sensei! This change is essential to make use of the new custom deserialization logic provided by ModelParser instead of the standard Serde deserialization.
This one-line change has significant implications, as it's the switch point between the old and new deserialization approaches.
Let's verify that all places that might use this deserialization are properly handled:
🏁 Script executed:
#!/bin/bash # Find places where deserialization might be used echo "Checking for places that might be affected by the deserialization change..." # Look for any references to deserialization rg "deserialize" --type cairo --glob "*.cairo" | grep -v "ModelParser" echo "These places might need to be updated to use ModelParser::deserialize instead of Serde::deserialize"Length of output: 468
Ohayo, sensei!
It looks like our original verification script didn’t quite do the trick—the use of--type cairoisn’t supported by ripgrep and returned the "unrecognized file type: cairo" error. This means we might not be seeing all references to deserialization across our Cairo files.To be sure all affected areas are handled, please update the verification command to use file globbing instead. For example, try running:
rg -g "*.cairo" "deserialize" | grep -v "ModelParser"This adjusted command will properly search through all
.cairofiles for any instances of"deserialize"that haven’t been updated to useModelParser::deserialize. If any such references are found, they’ll likely need to be updated accordingly.crates/dojo/lang/src/cairo_plugin.rs (2)
15-17: Add DOJO_LEGACY_STORAGE_DERIVE to import properlyOhayo, sensei! Good job adding the
DOJO_LEGACY_STORAGE_DERIVEto the imports. This is a necessary addition to support the new derive macro for legacy storage handling, enabling the distinction between models using the new DojoStore trait and those that need to maintain backward compatibility.
114-119: Well-organized derive macro declarationNice work updating the
declared_derivesmethod to include the newDOJO_LEGACY_STORAGE_DERIVE. This ensures the Cairo plugin recognizes and processes this derive macro correctly during compilation. The vector organization is clean and maintainable.crates/dojo/core-cairo-test/src/tests/model/model.cairo (6)
65-71: Well-structured enum with default implementationOhayo sensei! I like how you've implemented
MyEnumwith appropriate traits includingDefaultand marked theZvariant as#[default]. This is a key part of addressing the storage issue mentioned in the PR objectives, where the first variant (id 0) can't be distinguished from uninitialized values.
73-81: Good implementation of legacy storage modelThe
LegacyModelwith theDojoLegacyStorageattribute demonstrates the backward compatibility aspect of your solution. The structure appropriately uses both simple types and complex types like tuples,Option<T>, and custom enums that were problematic in the original storage system.
83-91: Clean DojoStore model implementationThis model correctly omits the
DojoLegacyStorageattribute, which means it will use the newDojoStoretrait by default. Using the same field structure asLegacyModelwill make it easy to compare the two approaches in testing.
101-102: Properly registered new models in namespace definitionGood job updating the namespace definition to include both new models. This ensures they're properly recognized as resources within the test environment.
375-459: Comprehensive test for legacy model behaviorExcellent test implementation! The test validates both the layout definition and serialization/deserialization process for the legacy model. Your assertions clearly show the expected behavior where variant IDs start at 0, which is the core issue being addressed in this PR.
462-542: Thorough testing of DojoStore model with adjusted variant IDsOhayo sensei! This test nicely demonstrates the key difference in the new storage system - the variant IDs start at 1 (not 0) in the serialized values, allowing proper distinction between initialized and uninitialized variants. The expected layout assertion shows the structural changes needed to support this approach.
crates/dojo/lang/src/attribute_macros/event.rs (2)
24-24: Import path reorganizationGood adjustment to import from
crate::utilsinstead ofsuper::element. This suggests a beneficial reorganization of utility functions to improve code structure.
66-72: Updated serialization function call with legacy flagOhayo sensei! You've correctly updated the
serialize_keys_and_valuescall to include the new boolean parameter (set totrue). This indicates that events will use the legacy storage behavior by default, which maintains backward compatibility while allowing the new storage system to be used where needed.crates/dojo/core-cairo-test/src/tests/world/model.cairo (2)
59-62: Added Default trait and default variant to MyEnumOhayo sensei! Good job adding the
Defaulttrait toMyEnumand marking theXvariant as#[default]. This ensures that any uninitializedMyEnumvalues will have a well-defined state, which is essential for the new storage system to work correctly.
75-78: Added Default implementation to AnotherEnumSmart addition of the
Defaulttrait toAnotherEnumwith theXvariant marked as#[default]. This consistent approach across all enums in the codebase ensures that the new storage system handles all enum types uniformly.Cargo.toml (1)
213-225:✅ Verification successful
Ohayo sensei, please verify the new Scarb references!
Switching to a different GitHub fork and commit can sometimes introduce unexpected code changes. It's great that you've commented out the old references. Ensure this commit remains stable in the long run.You can run this script to confirm that commit “89125e3” exists and is accessible in the new fork:
🏁 Script executed:
#!/bin/bash # Description: Verify the referenced commit from remybar/scarb gh api repos/remybar/scarb/commits/89125e3Length of output: 4447
Ohayo sensei, the new Scarb commit "89125e3" has been verified!
Based on the API output, the commit fromremybar/scarbis valid and accessible. The updated GitHub references inCargo.toml(lines 213-225) correctly replace the old fork, and the commented-out references provide a useful fallback. Everything looks stable and in order—please continue monitoring for any unexpected changes in future updates.crates/dojo/lang/src/attribute_macros/model.rs (7)
13-13: Ohayo sensei, confirming the new Terminal import usage.
TheTerminalimport might be handy for reading or parsing tokens. Double-check if any additional lints or macros are needed to handleTerminalAST interactions thoroughly.
20-23: Ohayo sensei, nice reorganization of derive macros!
Consolidating theseDOJO_*_DERIVEconstants in one place clarifies how they interrelate. This change helps avoid repeated macro checks.
67-69: Ohayo sensei, ensuring the legacy storage flag logic.
Addinguse_legacy_storagebased on theDOJO_LEGACY_STORAGE_DERIVEattribute is straightforward. Just be sure any fallback paths also behave consistently.
88-168: Ohayo sensei, the extended parsing logic for keys and values looks neat!
Your new approach systematically enforces that all#[key]fields come first, which is a valuable check. Keep an eye on error-handling if the struct design changes frequently.
218-230: Ohayo sensei, aligning model deserialization with the correct path.
Usingdojo::storage::DojoStorevs.core::serde::Serdeis a nice toggle for legacy or modern approaches. Consider adding thorough tests across both paths for unexpected edge cases.
231-239: Ohayo sensei, consistent approach withbuild_legacy_layout.
Applyingbuild_legacy_layoutis a simple way to maintain backward compatibility. Double-check that the derived layout is correct for nested types or multi-level references.
262-265: Ohayo sensei, final injection of theuse_legacy_storageparameter is straightforward.
This signals the rest of the patch that old storage semantics might apply. Ensure all derived expansions handle this flag thoughtfully.crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (5)
1-4: Ohayo sensei, new imports for Dojo store building.
Everything looks solid. Just make sure these nodes (likeOptionTypeClause) handle advanced syntax cases without issues.
5-9: Ohayo sensei, good use of specialized utility functions.
Breaking down serialization intoserialize_primitive_member_tyandserialize_tuple_member_tyfosters clarity. This approach should simplify future expansions.
10-16: Ohayo sensei, struct store builder signature.
Your function parameters neatly separate the AST from generic substitutions. This design choice eases code reuse and potential extension for advanced struct patterns.
21-53: Ohayo sensei, systematic loop for serializing/deserializing each member.
This approach is quite modular. Keep in mind that advanced nested types or generics might need more special handling if they appear in the future.
77-175: Ohayo sensei, your enum Dojo store logic covers variant indexing well!
Incrementing the variant index to distinguish uninitialized from the first variant is clever. Always confirm that the default variant actually matches index0in your design.crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo (4)
39-45: Ohayo! Good addition of the Default trait for the enum.The addition of the
Defaulttrait and marking theTwovariant as#[default]is excellent for the newDojoStoretrait implementation. This helps resolve the issue with variant id 0 by providing an explicit default variant.
101-103: Implementation aligns with DojoStore trait requirements, sensei!The
use_legacy_storagemethod correctly returnsfalseto indicate this model will use the newDojoStoretrait for serialization/deserialization rather than legacy storage.
136-140: Great addition of Default trait to Sword struct.This change enables the
Swordstruct to work properly with the newDojoStoretrait serialization mechanics by providing default values when needed.
186-191: Well implemented Default trait for Weapon enum!Properly adding the
Defaulttrait and markingDualWieldas the#[default]variant ensures this enum will work correctly with the new storage system when deserialized from an uninitialized state.crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (5)
5-8: Ohayo! Good implementation of Serde and Default trait for Base.Adding the
SerdeandDefaulttraits to theBasestruct ensures compatibility with the newDojoStoretrait's serialization requirements.
58-64: Default trait implementation for EnumNoData looks perfect, sensei!Adding the
Defaulttrait and markingOneas the#[default]variant ensures this enum will work correctly with the newDojoStoreserialization system.
122-126: Great addition of GenericEnum with Default support!The new
GenericEnum<T>withDefaultimplementation and explicit#[default]variant will work well with theDojoStoretrait's handling of generics.
170-190: Well-updated test for the renamed GenericStruct!The test has been properly updated to reflect the rename from
GenerictoGenericStructand verifies the correct layout and type information.
193-213: Excellent new test for GenericEnum, sensei!This new test thoroughly validates the introspection capabilities for the newly added
GenericEnum<T>type, ensuring it works correctly with the layout and serialization systems.crates/dojo/core-cairo-test/src/tests/helpers/event.cairo (3)
13-19: Ohayo! Good addition of Serde trait to FooBaseEvent.Adding the
Serdetrait toFooBaseEventensures it can be properly serialized and deserialized with the newDojoStoresystem.
66-70: Well done implementing Default for MyEnum, sensei!Adding the
Defaulttrait and markingXas the#[default]variant ensures this enum will work correctly with theDojoStoretrait's deserialization logic when handling uninitialized variant indexes.
81-85: Excellent Default implementation for AnotherEnum!Similar to
MyEnum, this addition of theDefaulttrait withXas the#[default]variant preparesAnotherEnumfor the new storage system.crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo (3)
29-30: Ohayo! Good modification to use model_layout variable.This change allows for more flexible layout handling, potentially supporting both the legacy layout and the new layout format depending on configuration.
59-61: Well implemented deserialize method for ModelParser!Adding a proper deserialization method that delegates to the
DojoStoretrait's implementation is essential for the new storage system to work with custom enums andOption<T>.
75-77: Great addition of deserialize for ModelValueParser, sensei!Similar to the
ModelParserimplementation, this provides the necessary deserialization capabilities for model values in the new storage system.crates/dojo/lang/src/derive_macros/introspect/mod.rs (11)
10-14: Ohayo sensei: Nice approach adding the new modules and imports
No issues spotted. This correctly introducesitertools,debug_store_expand, and thedojo_storemodule.
29-44: Ohayo sensei: Great use of generic type handling
Building generic types and impls for both introspection and Dojo store looks consistent. Just be sure to handle edge cases ifgen_typesis empty.
58-67: Ohayo sensei: Good job building the struct Dojo store
Leveragingdojo_store::build_struct_dojo_storeand logging withdebug_store_expandis a neat approach for debugging.
68-76: Ohayo sensei: Clean usage of generate_introspect
Passing the newly builtdojo_storetogenerate_introspectnicely centralizes final output.
87-102: Ohayo sensei: Similar robust design for enums
Reusing the same generic building logic for enum introspection and store generation keeps the code DRY.
130-137: Ohayo sensei: Using dojo_store for enum
Consistently constructing the enum Dojo store is good. EnsureDefaultis implemented where needed for custom enums.
139-149: Ohayo sensei: Great final introspection call
Similar to structs, you integrate the new store with the introspection function elegantly.
161-161: Ohayo sensei: Additional parameter for dojo_store
Includingdojo_storein the signature clarifies the data flow for final introspect generation.
191-191: Ohayo sensei: Properly mapping dojo_store in UnorderedHashMap
Integrating the store text into the interpolation is consistent with the existing pattern.
198-221: Ohayo sensei: build_generic_types function
Your function neatly filters and collects only type parameters, keeping the logic straightforward.
223-244: Ohayo sensei: build_generic_impls is well structured
Generating the partial impls for each generic type is a neat approach. Just ensure string join logic withItertoolscovers all generics properly.crates/dojo/core-cairo-test/src/tests/storage/layout.cairo (17)
11-52: Ohayo sensei: test_fixed_layout
Comprehensive checks for reading/writing defaults, offsets, and deletions. No issues found.
54-102: Ohayo sensei: test_tuple_layout
Nicely tests tuple handling with multiple fields, including offsets and deletions. Looks correct.
104-148: Ohayo sensei: test_byte_array_layout
Good coverage. Verifies reading/writing of byte arrays, offset mechanics, and delete steps thoroughly.
149-164: Ohayo sensei: test_read_byte_array_layout_invalid_array_length
Panic test is properly used to catch erroneous array length. Good negative testing coverage.
165-175: Ohayo sensei: test_byte_array_layout_bad_input_length
Again, correct usage of a panic test for invalid input length. Great validation.
176-186: Ohayo sensei: test_byte_array_layout_bad_array_length_value
Ensuring we catch out-of-range lengths fortifies reliability. Nicely done.
187-197: Ohayo sensei: test_byte_array_layout_bad_array_length
Checks for mismatched or incomplete data lengths, ensuring robust error handling.
198-240: Ohayo sensei: test_array_layout
Solid approach for verifying normal array writing, reading, offsets, and deletion.
242-259: Ohayo sensei: test_read_array_layout_bad_array_length
Proper negative test ensuring we handle an overflow in array length.
261-273: Ohayo sensei: test_array_layout_bad_values_length
Good that you confirm mismatched data length triggers an appropriate panic.
274-286: Ohayo sensei: test_array_layout_bad_array_length
Another negative path confirming large invalid length is properly rejected.
287-333: Ohayo sensei: test_struct_layout
Excellent coverage of struct read/write logic, default values, and offset usage.
335-382: Ohayo sensei: test_enum_layout
Thorough checks of variant writing, reading, and deletion. Nicely verifies default variant.
383-403: Ohayo sensei: test_read_enum_layout_invalid_variant_value
Panic test ensures we reject out-of-range variant values. Valid approach.
404-423: Ohayo sensei: test_read_enum_layout_unexisting_variant_value
Another negative scenario confirmed. Good coverage of invalid variants.
425-441: Ohayo sensei: test_enum_layout_invalid_variant_value
Reinforces checks for out-of-range variants when writing, ensuring robust error handling.
442-458: Ohayo sensei: test_enum_layout_unexisting_variant
Ensures that any attempt to write a non-existent variant triggers a panic. Implementation is thorough.crates/dojo/core-cairo-test/src/tests/meta/layout.cairo (7)
4-19: Ohayo sensei: test_build_legacy_layout_option
Verifies thatOptionlayout transforms correctly, mapping Some/None properly.
21-44: Ohayo sensei: test_build_legacy_layout_simple_enum
Checks re-indexing of variants from 1..4 down to 0..3. Handy test to ensure correct legacy offset.
46-71: Ohayo sensei: test_build_legacy_layout_tuple
Great demonstration of nestedOptionin a tuple. Confirms correct field-level transformations.
73-95: Ohayo sensei: test_build_legacy_layout_array
Ensures arrays ofOptionmap properly in the legacy layout, revalidating the internal enum structure.
97-129: Ohayo sensei: test_build_legacy_layout_struct
Covers struct fields withOption, verifying correct reindexing of Some/None.
131-194: Ohayo sensei: test_build_legacy_layout_nested_enum
Comprehensive check for multi-level nesting of arrays and tuples inside an enum.
196-260: Ohayo sensei: test_build_legacy_layout_nested_struct
Solid final verification for nested arrays, tuples, andOptionfields in a struct.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (3)
3-9: Ohayo sensei: Enum definition looks tidy and concise!
No obvious pitfalls or syntactical issues. The variant types are clearly defined.
199-218: Ohayo sensei: Dynamic arrays test looks great!
The logic cleanly validates both the successful deserialization of an array with elements and theOption::Nonescenario.
243-260: Ohayo sensei: Enum variant testing is thorough.
The checks for valid variant indices, default variant, and out-of-range behavior appear correct.crates/sozo/ops/src/model.rs (4)
69-75: Ohayo sensei: Verify edge cases in fallback logic for legacy storage.
Whenuse_legacy_storageis missing, defaulting totrueis convenient but be sure that any unexpected error does not mask genuine issues.
126-133: Ohayo sensei: Consistent fallback usage for missinguse_legacy_storage.
This mirrored approach to the fallback logic is coherent with the earlier block, ensuring consistent default behavior.
397-410: Ohayo sensei: Layout printing improvements look good.
Displayinglegacy storage : <bool>is clear and helps quickly identify models using the older storage mechanism.
440-584: Ohayo sensei: Addeduse_legacy_storageparameter across formatter functions is consistent.
These changes unify how legacy or new storage formats are applied to arrays, tuples, enums, and fields.crates/dojo/lang/src/utils.rs (9)
1-21: Ohayo sensei: Initial imports andget_serialization_pathare solid.
Deferring toDojoStoreorSerdebased on theuse_serdeflag is straightforward and well-organized.
22-46: Ohayo sensei: Double-check thecompute_unique_hashfor collisions.
While the logic is correct for combining name and member data, consider verifying potential collisions in large codebases or add extra domain separation if needed.
48-90: Ohayo sensei: Model key parsing is neat.
Good job enforcing that keys are declared before non-keys. This helps keep data structures consistent.
92-108: Ohayo sensei:serialize_keys_and_valuesmethodical approach is straightforward.
Partitioning key and non-key serialization clarifies the flow for distinct sets of fields.
110-126: Ohayo sensei:deserialize_keys_and_valuesmirrors serialization properly.
This symmetry ensures that key fields and value fields align correctly before reconstruction.
128-177: Ohayo sensei: Tuple member destructuring is well-handled.
The solution elegantly tracks nested tuples and produces consistent output representations.
179-231: Ohayo sensei:serialize_member_ty& associated functions are consistent.
Grouping primitive vs. tuple serialization logic is clear and maintainable.
233-283: Ohayo sensei:deserialize_member_ty& associated functions appear robust.
The fallback todeserialize_primitive_member_tyordeserialize_tuple_member_tyensures correct handling of nested and complex types.
285-349: Ohayo sensei: Tests comprehensively validate tuple destructuring.
These test cases adequately cover varied tuple shapes and data types, reinforcing correctness.crates/dojo/core/src/storage/dojo_store.cairo (5)
1-5: Ohayo sensei! Nice use of a zero address as the default.
This implementation aligns well with typical default usage for a contract address, returning a “zero” contract address.
7-20: Ohayo sensei! Consider clarifying error handling for serialization and deserialization.
Currently, this trait just defers to Serde. If a deserialization error occurs, the returnedOption<T>may not be sufficient to handle edge cases in a more robust manner. You may want to provide or reference a custom error approach to distinguish between uninitialized data and a genuine deserialization error.
22-39: Ohayo sensei! These direct trait implementations look solid.
They ensure consistent default behavior for each primitive and special type with minimal overhead.
40-64: Ohayo sensei! Verify the collision risk of treating 0 as uninitialized.
When the variant index is0, the code yieldsOption::Some(Default::default()), which may collide with a genuineSome(Default::default()). If that collision is acceptable for your use case, great, but it might be worth documenting or reconsidering the variant numbering.
89-102: Ohayo sensei! Ensure array length is valid for deserialization.
The length is read from afelt, which might be negative or invalid if data is corrupted. You may wish to add a check before proceeding withremaining - 1.Here is a possible safeguard:
fn deserialize_array_helper<T, +Serde<T>, +DojoStore<T>, +Drop<T>>( ref serialized: Span<felt252>, mut curr_output: Array<T>, remaining: felt252, ) -> Option<Array<T>> { + // Early check for invalid lengths + if remaining < 0 { + return Option::None; + } if remaining == 0 { return Option::Some(curr_output); } ... }
|
@remybar do we want to wait the proc macro or I can move forward reviewing this next week? |
No need to wait for proc macro as we don't know exactly when the new proc macro API will be officially available. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
217-238: Consider additional checks inbuild_generic_impls
Ohayo sensei, the code is good as is, but you might consider detecting if no generics are provided and skipping the base impls to avoid trailing commas.crates/dojo/lang/src/derive_macros/introspect/mod.rs (2)
192-215: Minor note onbuild_generic_types
If a user has constraints likeT: SomeTrait, it might require extra merges in your code. For advanced usage, consider capturing trait bounds within the introspection.
217-238:build_generic_implslogic
The approach of formatting generics inline is effective. However, watch out for potential confusion if multiple lines of generics are needed. A multiline approach might be more readable.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (1)
11-29: Structures and Enums for Complex Types
The synergy betweenS,SComplex, andEComplexis well-structured. The combination of nested tuples and options is tested robustly.You might consider splitting
SComplexinto smaller sub-structs for clarity, especially if more fields are added in the future.crates/dojo/core/src/storage/dojo_store.cairo (3)
1-6: Ohayo! ContractAddressDefault implementation might be temporary.The comment on line 1 suggests this implementation might not be necessary from Cairo 2.11. Consider adding a more descriptive comment about why this default implementation is needed now and when it can be removed in the future.
44-68: Clever handling of Option variants!This implementation directly addresses the PR objective of fixing the issue where variant id 0 can't be distinguished from an uninitialized variant. By using 1 for Some and 2 for None, you avoid conflicts with 0.
However, I notice that in the deserialize method, when encountering a 0 value, you return
Default::default(). It would be helpful to add a comment explaining this special case for future maintainers.fn deserialize(ref values: Span<felt252>) -> Option<Option<T>> { if let Option::Some(x) = values.pop_front() { return match *x { + // Handle case where value is 0 (uninitialized) by returning default 0 => Option::Some(Default::default()), 1 => Option::Some(DojoStore::<T>::deserialize(ref values)), 2 => Option::Some(Option::None), _ => Option::None, }; }
92-105: Good implementation for Array.This implementation properly handles arrays by first serializing the length and then each element. The use of the helper functions keeps the code DRY.
One suggestion: consider adding error handling for the case where the length value is negative or unreasonably large, which could happen if corrupted data is passed. This could prevent potential issues when deserializing.
fn deserialize(ref values: Span<felt252>) -> Option<Array<T>> { let length = *values.pop_front()?; + // Safety check to prevent excessive memory allocation attempts + if length < 0 || length > 1000000 { + return Option::None; + } let mut arr = array![]; deserialize_array_helper(ref values, arr, length) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
crates/dojo/lang/src/derive_macros/introspect/mod.rs (2)
crates/dojo/lang/src/lib.rs (1) (1)
debug_store_expand(46-53)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (2) (2)
build_struct_dojo_store(10-75)build_enum_dojo_store(77-175)
🔇 Additional comments (17)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (5)
5-8: Ohayo sensei! Good addition of Default derivation
This ensures there's a well-defined default variant for your struct, which is especially useful for the new DojoStore approach.
58-64: Ohayo sensei! Great usage of the #[default] attribute
Flagging the first variant inEnumNoDataas default aligns well with the shift from 0-based to 1-based indexing.
151-156: Watch out for variant indexing in_enum
Switching indexing to start from 1 is consistent with the approach to distinguish uninitialized (0). Just be sure it doesn't cause unexpected off-by-one logic in dependent code.
170-190: Your test forGenericStructlooks thorough
Theassert_eq!checks confirm that the size and layout are as expected. Nicely done, sensei!
192-212: Kudos on the newly addedtest_generic_enum_introspect
The test effectively validates the variant indexing starting from 1 and ensures that the default variant is correct.crates/dojo/lang/src/derive_macros/introspect/mod.rs (4)
10-10: Ohayo sensei! Theitertoolsimport
Bringing in itertools is a good call for more concise joining operations. Approved!
63-64:debug_store_expandusage
Nice debugging helper to reveal expansions for Dojo store. This can be helpful for troubleshooting compile-time expansions.
90-97: EnsuringDefaultis included for your enums
You includecore::traits::Default<{enum_name_with_generics}>in the additional impls. This is critical for bridging uninitialized states. Great addition, sensei.
124-132: Smart approach withbuild_enum_dojo_store
Constructing theDojoStoreimpl for an enum hinges on a well-definedDefault. This design choice ensures correct fallback forvariant=0.Could you confirm that all existing enums, including user-defined ones in other files, have implemented
Default?crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (4)
3-9: Ohayo sensei! Good usage of#[default]inenum E
Defining the default variant asAensures safe fallback if the variant is 0. No issues spotted.
55-221: Comprehensive tests for primitive DojoStore
This block thoroughly covers numeric types, bool, addresses, and byte arrays. It's an excellent reference for future expansions.
223-263: Kudos on dynamic array and option coverage
The test cases meticulously verify serialization and deserialization for arrays andOption<T>. Great job ensuring edge cases (like empty spans) are handled.
319-385: Generic struct & enum dojo store tests
Verifying forGenericStruct<u32>andGenericEnum<u32>is crucial. This ensures the trait expansions handle generics gracefully.crates/dojo/core/src/storage/dojo_store.cairo (4)
8-12: Looking good, sensei! Clean trait definition.The
DojoStoretrait is well-defined with clear method signatures for serialization and deserialization. This aligns perfectly with the PR objective of introducing a trait to handle storage edge cases.
14-24: Great default implementation using Serde.The default implementation nicely encapsulates Serde calls as mentioned in the PR objectives. This provides a clean fallback for types that don't need special handling.
26-42: Comprehensive primitive type implementations, nice work!You've covered all the necessary primitive types with the default Serde-based implementation. This ensures consistent behavior across the common data types.
70-90: Well-designed array helper functions, sensei!These recursive helper functions for serializing and deserializing arrays are clean and efficient. The early return in
deserialize_array_helperwhen remaining is 0 is a nice optimization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Cargo.toml (2)
214-216: Deprecating old scarb dependencies.
Ohayo sensei! The old scarb dependency lines that reference the dojoengine repository are now commented out as expected. Ensure you keep these comments for historical context or remove them if they’re no longer needed for clarity.
222-224: Local path alternative configuration.
Ohayo sensei! The commented-out local path configurations offer a neat alternative for local development setups. Ensure that these remain well-documented so that future contributors know when and how to activate this option if necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml(1 hunks)
🔇 Additional comments (1)
Cargo.toml (1)
218-221: Introducing new scarb dependencies.
Ohayo sensei! The new dependency definitions forscarb,scarb-metadata, andscarb-uinow point to the remybar fork with the commit hasha634d0103395efde82331336eb76c77eac937966. This update aligns with the PR objectives. Please verify that this commit hash corresponds to a stable, tested revision to avoid any surprises down the line.
remybar
force-pushed
the
fix-enum_options_storage
branch
from
da08431 to
f2e1a84
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/dojo/core/src/storage/dojo_store.cairo (1)
82-106: 🛠️ Refactor suggestionOhayo sensei! Guard against invalid array length values.
When deserializing arrays (lines 100-104), we rely on a length from the serialized data without validating if it’s negative or excessively large. Consider validating the length to avoid potential panics or out-of-bounds issues.
fn deserialize(ref values: Span<felt252>) -> Option<Array<T>> { let length = *values.pop_front()?; + // Basic check to avoid negative or unexpectedly large lengths + if length < 0 || length > 999_999 { + // Adjust threshold as needed + return Option::None; + } let mut arr = array![]; deserialize_array_helper(ref values, arr, length) }
🧹 Nitpick comments (9)
crates/dojo/lang/src/utils.rs (3)
110-126: Ohayo sensei! Check for potential missing/extra fields.
Thedeserialize_keys_and_valuesapproach nicely mirrors the serialization counterpart. However, consider verifying that the count of deserialized items matches the expected schema size.
128-177: Ohayo sensei! Tuple destructuring looks robust.
You handle nested tuples recursively, which is clean. The fallback tounimplemented!for unsupported expressions is acceptable for early development, but could be improved to provide descriptive errors or fallback logic.
258-283: Ohayo sensei! Consider unify vs. repeated calls.
The repeated pattern of callingdestructure_tuple_memberand then enumerating the items is consistent. It might be beneficial to unify certain aspects of tuple type handling across serialization and deserialization in a future refactor for maintainability.crates/dojo/lang/src/derive_macros/introspect/layout.rs (1)
380-387: Consider providing more specific error message for Option types.The error handling for Option types correctly prevents them from being packed, which is necessary since they need special handling with DojoStore.
However, the error message could be more specific about why Option types can't be packed, making debugging easier for developers.
- message: format!("{item_type} cannot be packed."), + message: format!("{item_type} cannot be packed as it requires special handling by DojoStore for variant management."),crates/dojo/lang/src/derive_macros/introspect/mod.rs (1)
63-73: Ohayo sensei: Usingdebug_store_expand
Debugging expansions can be immensely helpful when verifying macro outputs. Logging the generated store code for debugging is a practical technique—nice usage of environment toggling to keep logs optional.crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo (1)
85-87: Ohayo sensei:$model_type$DojoStoredocumentation
Your note clarifies how store implementations are derived for the main model struct. It's always good to keep references visible for future maintainers.crates/dojo/core/src/storage/dojo_store.cairo (3)
8-12: Ohayo sensei! Validate error handling strategy for serialization/deserialization.Currently, the trait does not return any error information if deserialization fails beyond returning
Option::None. Consider using a more robust error-reporting approach so that calling code can differentiate between a legitimate "no data" scenario and an error in data parsing.
44-68: Ohayo sensei! Handle requirement forT: Default.Lines 59-60 rely on
Default::default()if the first popped felt is0. IfTdoesn’t implementDefault, this might fail at compile time. You may wish to add compile-time checks or doc comments to reinforce thatTmust implementDefault, preventing confusion for end users.
70-81: Ohayo sensei! Consider iterative approach for array serialization.The recursive function
serialize_array_helperis elegant but might lead to deep recursion if the array is large. An iterative approach could improve stack usage, especially in resource-constrained or large-scale scenarios.fn serialize_array_helper<T, +DojoStore<T>, +Drop<T>>( mut input: Span<T>, ref output: Array<felt252>, ) { - match input.pop_front() { - Option::Some(value) => { - DojoStore::serialize(value, ref output); - serialize_array_helper(input, ref output); - }, - Option::None => {}, - } + loop { + let item = input.pop_front(); + match item { + Option::Some(value) => DojoStore::serialize(value, ref output), + Option::None => break, + }; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (48)
Cargo.toml(1 hunks)crates/benches/contracts/src/models/character.cairo(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/storage/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(1 hunks)crates/dojo/core/src/meta/layout.cairo(1 hunks)crates/dojo/core/src/model/component.cairo(1 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/interface.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/layout.cairo(8 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(0 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/mod.rs(0 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(3 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(1 hunks)crates/dojo/lang/src/derive_macros/mod.rs(1 hunks)crates/dojo/lang/src/lib.rs(2 hunks)crates/dojo/lang/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/abigen/model.rs(2 hunks)crates/dojo/world/src/contracts/model.rs(2 hunks)crates/sozo/ops/src/model.rs(8 hunks)crates/torii/sqlite/src/model.rs(1 hunks)crates/torii/types-test/src/models.cairo(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/src/models.cairo(1 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/lang/src/attribute_macros/mod.rs
- crates/dojo/lang/src/attribute_macros/element.rs
🚧 Files skipped from review as they are similar to previous changes (30)
- examples/spawn-and-move/src/models.cairo
- crates/dojo/core/src/model/definition.cairo
- crates/dojo/core/src/model/component.cairo
- crates/dojo/core/src/model/interface.cairo
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/lang/Cargo.toml
- crates/dojo/lang/src/derive_macros/mod.rs
- crates/dojo/lang/src/derive_macros/introspect/utils.rs
- crates/dojo/core/src/lib.cairo
- crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo
- crates/dojo/core/src/storage/layout.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core/src/world/world_contract.cairo
- crates/torii/sqlite/src/model.rs
- examples/spawn-and-move/dojo_dev.toml
- crates/dojo/world/src/contracts/abigen/model.rs
- crates/dojo/core/src/meta/introspect.cairo
- crates/dojo/core-cairo-test/src/tests/world/model.cairo
- crates/dojo/lang/src/attribute_macros/event.rs
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/lang/src/lib.rs
- crates/benches/contracts/src/models/character.cairo
- Cargo.toml
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
- crates/dojo/core-cairo-test/src/tests/world/event.cairo
- crates/dojo/lang/src/cairo_plugin.rs
- crates/torii/types-test/src/models.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- crates/sozo/ops/src/model.rs
🧰 Additional context used
🧬 Code Definitions (4)
crates/dojo/lang/src/derive_macros/introspect/layout.rs (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs (4) (4)
is_array(34-36)is_byte_array(30-32)is_option(42-44)is_tuple(38-40)
crates/dojo/world/src/contracts/model.rs (2)
crates/dojo/world/src/contracts/abigen/model.rs (2) (2)
use_legacy_storage(673-682)use_legacy_storage(802-811)crates/torii/sqlite/src/model.rs (1) (1)
use_legacy_storage(117-120)
crates/dojo/lang/src/derive_macros/introspect/mod.rs (2)
crates/dojo/lang/src/lib.rs (1) (1)
debug_store_expand(46-53)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (2) (2)
build_struct_dojo_store(10-75)build_enum_dojo_store(77-175)
crates/dojo/lang/src/utils.rs (1)
crates/dojo/lang/src/attribute_macros/event.rs (2) (2)
members(90-99)members(101-104)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (91)
crates/dojo/lang/src/utils.rs (10)
13-20: Ohayo sensei! Good use of conditional paths.
This function cleanly switches between Serde and DojoStore. The inline attribute is a nice touch for performance.
22-46: Ohayo sensei! Verify handling of variant collisions.
The logic for hashing element names and members is straightforward. However, consider whether a more explicit method to detect collisions (e.g., consistent logging or diagnostics) might be needed to warn users of conflicting hashes in large projects. Otherwise, this is well-structured.Would you like a script to scan the codebase for repeated calls to
compute_unique_hashon different sets of members?
48-90: Ohayo sensei! Enforce the “keys-before-values” rule carefully.
The function enforces that all key fields appear before non-key fields. This is great. Just be sure any later merging or code generation processes are consistent with this assumption (e.g., potential dynamic object creation).
92-108: Ohayo sensei! Validate key vs. values usage.
The approach for separating keys and values is well-organized. Ensure that no code path attempts to serialize members that are not recognized as keys or values (e.g., attributes other than "key").
179-192: Ohayo sensei! Consistent naming for “with_self” usage.
This function introduces awith_selfparameter that is well-explained. Ensure that the naming is consistently used in related helper functions for clarity.
194-205: Ohayo sensei! Straightforward approach for serializing primitives.
The usage of the chosen serialization path is concise. No obvious issues found.
207-231: Ohayo sensei! Named destructuring with central reuse.
You nicely reusedestructure_tuple_memberhere. Be mindful of performance if there are extremely large nested tuples—though this is likely not a concern for typical usage.
233-247: Ohayo sensei! Straightforward bridging from types to the correct path.
This function focuses well on bridging between tuple-based and primitive-based handling. The logic is consistent with the rest of the file.
249-256: Ohayo sensei! Return clarity of function usage.
The code to retrieve the appropriate path and deserialize is correct. Including the?operator for error propagation is a good practice for readability.
285-350: Ohayo sensei! Test coverage is impressive for tuple scenarios.
The collection of use cases for nested tuples is thorough. Confirm that other, more complex custom types are similarly tested.crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (1)
18-21: LGTM! Clean implementation for event deserialization.Ohayo, sensei! This implementation correctly adds deserialization support for events using Serde, which aligns with the intent that event data isn't stored in world storage (so it doesn't need the DojoStore trait handling). The comment clearly explains the reasoning.
crates/dojo/core/src/meta/layout.cairo (1)
52-110: Excellent implementation for supporting legacy storage layouts!Ohayo, sensei! This function does exactly what we need for maintaining backward compatibility with legacy Dojo models. The function correctly handles all layout types:
- For Enum layouts: Properly decrements the selector by 1 to match legacy numbering
- For Struct layouts: Preserves selectors while recursively processing nested layouts
- For Tuple/Array layouts: Correctly processes nested contents
The clear documentation explains the reasoning perfectly - legacy variants started from 0, while the new DojoStore system starts from 1 to distinguish uninitialized variants.
crates/dojo/world/src/contracts/model.rs (2)
61-61: Interface addition for legacy storage detection.Ohayo, sensei! This trait method addition aligns perfectly with the PR's goal of allowing models to opt in or out of the new DojoStore trait functionality.
209-211: Clean implementation of the legacy storage detection.This implementation correctly delegates to the model_reader's use_legacy_storage method. The pattern matches other methods in this class like layout(), packed_size(), etc.
crates/dojo/lang/src/derive_macros/introspect/layout.rs (2)
10-10: Good dependency addition.Ohayo! Adding the import for is_option is necessary to support the new Option type validation below.
78-80: Good change for the DojoStore variant handling.Ohayo, sensei! This change correctly increments the enum variant selectors to start from 1 instead of 0, allowing 0 to represent uninitialized variants in the new DojoStore system. The comment clearly explains the reasoning.
crates/dojo/core/src/model/model.cairo (4)
53-54: Ohayo! New method to deserialize model dataThe addition of
deserializeto theModelParsertrait is a solid design choice and aligns with the PR objectives. This centralizes model deserialization logic, which will be critical for handlingOption<T>and custom enums correctly.
84-86: Clean implementation of storage mode indicatorThe
use_legacy_storagemethod provides a clear way to determine whether a model uses legacy or new storage behavior. This boolean flag will be essential for maintaining backward compatibility while enabling the new storage system.
135-135: Delegating to the specialized parser, excellent!Updating
from_serializedto use the newModelParser::<M>::deserializemethod ensures consistent deserialization behavior throughout the codebase. This is a key part of implementing theDojoStoretrait functionality.
154-156: Delegation to ModelDefinition is the right approachThe implementation properly delegates to
ModelDefinition::<M>::use_legacy_storage(), maintaining separation of concerns and ensuring consistent behavior across the system.crates/dojo/core-cairo-test/src/tests/model/model.cairo (6)
65-71: Clean enum definition with Default implementationThe
MyEnumimplementation with a#[default]attribute on theZvariant is excellent, sensei! This addresses the core issue described in the PR objective where the first variant ID (0) cannot be distinguished from an uninitialized variant.
73-81: Well-structured LegacyModel with DojoLegacyStorage attributeThis model will use the legacy storage system as indicated by the
DojoLegacyStoragederive. It provides a solid example for testing how legacy models should be defined and handled.
83-91: DojoStoreModel properly configured for new storage systemBy omitting the
DojoLegacyStorageattribute, this model will use the new storage system with theDojoStoretrait. Good job creating a contrasting example to test both storage approaches!
101-102: Test models correctly registered in namespaceAdding the new models to the namespace definition ensures they'll be properly recognized during testing. This is essential for comprehensive test coverage.
375-460: Comprehensive test for legacy model implementationOhayo! This test thoroughly validates the legacy storage model behavior by:
- Verifying the expected layout for legacy storage
- Confirming the distinction between ModelDefinition layout and Introspect layout
- Testing serialization and deserialization with appropriate variant indexing
The test successfully verifies that variant IDs remain at 0, preserving backward compatibility.
462-543: Excellent verification of DojoStore model implementationThis test confirms the new storage system behavior by:
- Verifying the layout matches with indices starting from 1
- Testing serialization with incremented variant indices (1, 2, 3 instead of 0, 1, 2)
- Validating deserialization works with the new indexing
The test effectively demonstrates the solution to the original problem, where variant ID 0 can now be distinguished from an uninitialized variant.
crates/dojo/lang/src/attribute_macros/model.rs (6)
21-21: Adding DojoLegacyStorage derive constantIntroducing
DOJO_LEGACY_STORAGE_DERIVEas a constant maintains consistency with other derive attributes. Good practice, sensei!
67-69: Detecting legacy storage from derive attributesThis logic correctly identifies models using legacy storage by checking for the
DojoLegacyStoragederive attribute. This flag drives the subsequent code generation paths.
89-119: Improved member parsing with validationOhayo! The refactoring of model member parsing provides several benefits:
- Better validation of key members ordering
- Clear error diagnostics for incorrect ordering
- Logical structuring using the
parsing_keysflagThis maintains the important constraint that key members must come before non-key members, which is essential for correct model structure.
124-142: Storage-aware serialization and deserializationPassing the
use_legacy_storageflag to serialization and deserialization functions ensures the correct behavior is used based on the model's storage mode. This is a key part of implementing the dual storage approach.
225-239: Smart conditional code generation based on storage modeThe conditional generation of model deserialize path and layout is elegant:
- For legacy storage: Uses
core::serde::Serdeandbuild_legacy_layout- For new storage: Uses
dojo::storage::DojoStoreand direct introspection layoutThis approach maintains backward compatibility while enabling the new storage system.
264-265: Exposing legacy storage flag to generated codeIncluding the
use_legacy_storageboolean in the code interpolation ensures that the generated model implementation will have access to this critical information at runtime.crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (5)
5-11: Adding Serde and Default derives to existing typesOhayo, sensei! Adding
SerdeandDefaultto these structs and enums is an excellent enhancement. This ensures they can be properly serialized/deserialized and have default values, which is essential for the new storage system to work with them.Also applies to: 16-35, 40-53, 58-110
117-120: Renamed Generic to GenericStruct with Serde supportThe rename improves clarity and the addition of
Serdeensures the generic struct can work with the new storage system. Good improvement!
122-126: New GenericEnum with Default implementationAdding
GenericEnum<T>with theDefaulttrait provides a great test case for generic enum types in the storage system. The#[default]attribute on variantAensures it has a proper default value.
151-156: Critical change to variant indexingThis is a key change that directly addresses the PR objective! By incrementing variant indices to start from 1 instead of 0, you solve the problem of distinguishing an uninitialized variant (0) from the first actual variant.
170-212: Updated tests for generic typesThe tests have been properly updated to:
- Test the renamed
GenericStruct<T>- Add verification for the new
GenericEnum<T>- Confirm correct variant indexing in the layouts
These tests ensure that the generic types work correctly with the new storage system.
crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (4)
1-9: Ohayo sensei: Imports are clear and well-chosen
These imports from Cairo's syntax and the utility module directly support struct/enum AST processing. Overall, they're neatly organized and relevant to the Dojo store logic.
10-75: Ohayo sensei:build_struct_dojo_storelogic looks solid
This function cleanly differentiates tuple-type members from primitive members, generating appropriate serialization and deserialization code segments. The iteration over struct members is straightforward and well-structured. Good job ensuring each primitive vs. tuple scenario is handled distinctly.
77-145: Ohayo sensei:build_enum_dojo_storecovers variants comprehensively
Your approach to matching tuple variants, empty variants, and other data variants is well-organized. The code generation for each variant, including the variant index increment, is neatly encapsulated. This ensures each variant is distinct, satisfying the PR objective of handling uninitialized variant 0.
167-171: Ohayo sensei: Fallback to default & potential array underflow
This line implements0 => Option::Some(Default::<{name}<{generic_types}>>::default()). It ensures a safe fallback for uninitialized variants, matching the PR's stated approach. However, please remain cautious about potential array underflows ifvaluesis unexpectedly empty when popping the variant.crates/dojo/lang/src/derive_macros/introspect/mod.rs (8)
10-14: Ohayo sensei: Adding theitertoolsdependency
Pulling initertools::Itertoolsis a neat choice for combining strings and arrays elegantly. This keeps the code concise where you join multiple generics or trait bounds.
28-38: Ohayo sensei: Handling generics inhandle_introspect_struct
The logic that builds generic type lists and corresponding trait impl strings is well-organized. Combining+dojo::meta::introspect::Introspectensures we always have introspection on the struct’s generics.
55-61: Ohayo sensei: Generating the Dojo store
Callingbuild_struct_dojo_storehere aligns with your approach to unify introspection and store logic in one pass. This is a clean solution that makes the final generated code more powerful.
85-96: Ohayo sensei:handle_introspect_enumgenerics
Extending the base impls with bothDojoStoreandSerdeensures your complex enums can be reliably serialized. The additional default bound is fitting for fallback logic.
124-131: Ohayo sensei: Building the Dojo store for enums
build_enum_dojo_storeusage here is consistent withhandle_introspect_struct. This symmetrical design in your macros fosters clarity and maintainability.
133-143: Ohayo sensei: Debugging and final introspection
The repeated pattern of debug expansion followed by the core introspection generation keeps your code flow consistent. This makes the final expansions predictable.
155-186: Ohayo sensei: Merging Dojo store logic in introspect rewrite
Injecting$dojo_store$into the same rewrite node that handles size, layout, and type is a clever approach that keeps all codegen consistent. This single final output merges store logic with meta introspection.
192-237: Ohayo sensei:build_generic_types&build_generic_impls
These builder functions streamline the composable generation of trait bounds. LeveragingItertoolsfor transformations and joins is elegantly succinct. This sets a good foundation for scaling more advanced generics.crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo (5)
29-29: Ohayo sensei:$model_layout$reference
Replacing direct introspection calls with an injected$model_layout$variable provides greater flexibility to customize or override layout logic while still integrating with the rest of your macros.
32-35: Ohayo sensei: Introducinguse_legacy_storage()
This function toggle is in line with the PR’s approach to maintain pre-existing storage behavior. Marking legacy usage explicitly will help keep new and old usage patterns separate.
59-61: Ohayo sensei: Modeldeserializefor$model_type$
This new function delegates to$model_deserialize_path$cleanly, aligning with how you handle custom macros for model serialization. It's straightforward, which should simplify refactoring or extension later.
75-77: Ohayo sensei: Model valuedeserialize
Mirroring the same approach used in$model_type$, you maintain consistency in the deserialization path for the$model_type$Value. This consistency will ease debugging.
88-96: Ohayo sensei:$model_type$ValueDojoStore
Definingserializeanddeserializefor the$model_type$Valueexplicitly is a great complement to storing the model itself, ensuring that values can be processed separately. You handle the arrays for serialization thoroughly, including the final assembly into$deserialized_modelvalue$.crates/dojo/core-cairo-test/src/tests/storage/layout.cairo (17)
11-52: Ohayo sensei! Thorough coverage intest_fixed_layout.
The step-by-step writes, reads, and deletes confirm every stage works as intended. Consider extracting common logic for writing/reading default values into a helper if you find yourself duplicating these steps frequently.
54-102: Ohayo sensei! Great job withtest_tuple_layout.
The multi-section offset handling is clear, and the negative tests are well-defined by separate functions.
105-147: Ohayo sensei! Good approach intest_byte_array_layout.
Reading uninitialized data as[0,0,0]and verifying offsets is excellent for ensuring robust coverage.
150-164: Ohayo sensei!test_read_byte_array_layout_invalid_array_lengthis well-defined.
Verifying the panic path helps ensure misconfigured lengths don't slip through.
166-175: Ohayo sensei! Thetest_byte_array_layout_bad_input_lengthscenario is handled thoroughly.
Great usage of#[should_panic]for negative tests.
177-186: Ohayo sensei!test_byte_array_layout_bad_array_length_valueeffectively checks overflow conditions.
This helps confirm robust bounds checking.
188-197: Ohayo sensei!test_byte_array_layout_bad_array_lengthhandles invalid lengths gracefully.
Another excellent negative test to prevent inconsistent data.
199-241: Ohayo sensei!test_array_layoutshows consistent patterns for array reads and writes.
The offset-based approach is well-demonstrated and likely to be stable in production.
243-259: Ohayo sensei!test_read_array_layout_bad_array_lengthconfirms array length invariants under error conditions.
It’s good to see panic scenarios tested early.
261-273: Ohayo sensei!test_array_layout_bad_values_lengthis a neat negative test.
Short and clear. No issues spotted.
275-286: Ohayo sensei!test_array_layout_bad_array_lengthonce again ensures we handle invalid array lengths safely.
Consistent with other negative tests.
288-334: Ohayo sensei!test_struct_layoutis well-organized and verifies default reads, writes, and deletions.
All fields appear logically sound.
336-382: Ohayo sensei!test_enum_layoutthoroughly checks default variant handling and removal.
The structure is cohesive, and no concerns jump out.
384-403: Ohayo sensei!test_read_enum_layout_invalid_variant_valueproperly verifies variant bounds.
This ensures the system won't interpret out-of-range values.
405-424: Ohayo sensei!test_read_enum_layout_unexisting_variant_valueis another solid check.
Covers an important edge case for enumerations.
426-441: Ohayo sensei!test_enum_layout_invalid_variant_valueconfirms the panic scenario for out-of-range variants.
Well tested.
443-458: Ohayo sensei!test_enum_layout_unexisting_variantalso ensures out-of-bounds variant selection triggers a panic.
This final test completes the negative coverage.crates/dojo/core-cairo-test/src/tests/meta/layout.cairo (7)
4-19: Ohayo sensei!test_build_legacy_layout_optionis clear and spot-on.
MappingOption<T>to an enum layout for ‘Some’ and ‘None’ is well validated.
21-44: Ohayo sensei!test_build_legacy_layout_simple_enumelegantly checks variant index shifting.
Ensures that old variant1..4maps to new0..3. Good approach.
46-71: Ohayo sensei!test_build_legacy_layout_tupleconfirms nested layout transformations.
It’s great to see the Option nested inside the tuple.
73-95: Ohayo sensei!test_build_legacy_layout_arrayensures arrays ofOption<T>handle variant increments properly.
All array logic is validated nicely.
97-129: Ohayo sensei!test_build_legacy_layout_structthoroughly covers assembling fields withOption<T>.
No issues, excellent clarity in verifying the final struct layout.
131-194: Ohayo sensei!test_build_legacy_layout_nested_enumis solid.
You’re verifying multiple nesting levels with arrays and tuples in the same enum, which is very comprehensive.
196-260: Ohayo sensei!test_build_legacy_layout_nested_structis a robust test for multi-layered structures.
Handling arrays and tuples inside a struct is well validated.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (11)
55-221: Ohayo sensei!test_dojo_store_primitivesdemonstrates thorough coverage of numeric and address types.
All baseline types appear correctly serialized and deserialized. Nicely done.
223-242: Ohayo sensei!test_dojo_store_dynamic_arraysrobustly checks array serialization lengths, including edge cases forOption::None.
Well-structured approach to confirm correctness.
244-263: Ohayo sensei!test_dojo_store_optionelegantly handles Some/None states with distinct indexes.
This is consistent with the modifications for legacy or new variant indexing.
265-284: Ohayo sensei!test_dojo_store_enumsverifies each enum variant’s index offset.
Keen fallback scenario for out-of-range variants returningOption::None.
286-301: Ohayo sensei!test_dojo_store_structsis straightforward.
Ensures fields are serialized in order, with coverage for partial or empty inputs.
303-317: Ohayo sensei!test_dojo_store_tuplesuses nested tuples inE::C.
Neat usage to confirm multi-value arrangement.
319-334: Ohayo sensei!test_dojo_store_generic_structproves thatDojoStorecan handle generic fields.
No issues spotted.
336-351: Ohayo sensei!test_dojo_store_generic_enumdemonstrates consistent enum indexing for generic payloads.
The approach is polished.
355-369: Ohayo sensei!test_dojo_store_use_generic_structensures a struct with generics remains consistent during serialization.
Great extension of the coverage.
371-386: Ohayo sensei!test_dojo_store_use_generic_enumis also well-handled.
Confirms that references toGenericEnumbecome stable across transitions.
388-443: Ohayo sensei!test_mixthoroughly stress-tests nested structures with arrays and options.
This is an excellent integration test to verify everything holds together.crates/dojo/core/src/storage/dojo_store.cairo (2)
1-6: Ohayo sensei! Consider verifying necessity of the custom default implementation.While this impl safely provides a default for
ContractAddress, it might be redundant if newer versions of Cairo (e.g., ≥2.11) automatically derive defaults for this type.Would you like to confirm if the default implementation is still needed for your specific compiler version or if we can remove it to reduce maintenance overhead?
14-24: Ohayo sensei! Confirm Serde-based default’s coverage of all scenarios.This block relies on
Serdefor all types. If certain types require custom (de)serialization logic (e.g., additional checks or transformations), we might consider specialized trait implementations instead of the default.
remybar
force-pushed
the
fix-enum_options_storage
branch
from
d5f46fb to
d20c0e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
crates/dojo/core-cairo-test/src/tests/world/model.cairo (1)
75-77: Consider adding PartialEq trait to AnotherEnum for consistencyOhayo! I noticed that you've added both
DefaultandPartialEqtraits toMyEnum, but onlyDefaulttoAnotherEnum. For consistency across your enum types, consider addingPartialEqtoAnotherEnumas well, unless there's a specific reason not to include it, sensei.-#[derive(Introspect, Copy, Drop, Serde, Default)] +#[derive(Introspect, Copy, Drop, Serde, Default, PartialEq)] enum AnotherEnum { #[default] X: bool, }crates/dojo/lang/src/utils.rs (1)
285-349: Good test coverage for tuple destructuringOhayo, sensei! The tests thoroughly validate the
destructure_tuple_memberfunction with various tuple structures.Consider adding tests for the other key functions like serialization and deserialization to ensure their correctness across different scenarios, particularly for edge cases in the new DojoStore implementation.
crates/dojo/core-cairo-test/src/tests/storage/layout.cairo (1)
1-458: Consider extracting common test patterns.Ohayo sensei! While the tests are comprehensive and well-structured, there's significant repetition in the testing patterns. Consider extracting common testing sequences into helper functions to reduce code duplication and make the tests more maintainable.
+// Helper function for basic layout test pattern +fn test_layout_operations<T>( + layout_type: &str, + layout: T, + read_fn: fn(felt252, felt252, ref Array<felt252>, T), + write_fn: fn(felt252, felt252, Span<felt252>, ref usize, T), + delete_fn: fn(felt252, felt252, T), + test_values: Array<felt252> +) { + const MODEL_KEY: felt252 = 1; + const KEY: felt252 = 2; + + // Read uninitialized data + let mut read_data = array![]; + read_fn(MODEL_KEY, KEY, ref read_data, layout); + + // Then write and read back + // ... rest of common testing pattern +}crates/dojo/core-cairo-test/src/tests/meta/layout.cairo (1)
1-260: Consider adding explanatory comments about variant mapping.Ohayo sensei! While the tests are comprehensive, they would benefit from more explanatory comments about why variant indices are shifted (from 1-based to 0-based). This would make the tests more self-documenting and help future developers understand the reasoning behind the legacy layout transformations.
+// In the new DojoStore implementation, enum variants are numbered starting from 1, +// with 0 reserved for Default handling. However, in legacy storage, variants started +// from 0. The build_legacy_layout function transforms the layout accordingly.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (2)
216-220: Ohayo sensei, there's a mismatch in the error messages for ByteArray tests.
They currently mentioni128instead ofByteArray, which may cause confusion when reading test failures.Use this diff to correct the messages:
-assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<i128> serialization failed"); +assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<ByteArray> serialization failed"); -assert_eq!(res, Option::Some("hello"), "DojoStore<i128> deserialization failed"); +assert_eq!(res, Option::Some("hello"), "DojoStore<ByteArray> deserialization failed");
55-443: Ohayo sensei, consider DRY-ing up repeated serialization/deserialization code in tests.
Many test blocks follow a similar pattern. You could refactor by extracting a helper function or macro to eliminate duplication and make the file more maintainable, especially as new types are added in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (49)
Cargo.toml(1 hunks)crates/benches/contracts/src/models/character.cairo(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/storage/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core/src/event/component.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(1 hunks)crates/dojo/core/src/meta/layout.cairo(1 hunks)crates/dojo/core/src/model/component.cairo(1 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/interface.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/layout.cairo(8 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(0 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/mod.rs(0 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(3 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(1 hunks)crates/dojo/lang/src/derive_macros/mod.rs(1 hunks)crates/dojo/lang/src/lib.rs(2 hunks)crates/dojo/lang/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/abigen/model.rs(2 hunks)crates/dojo/world/src/contracts/model.rs(2 hunks)crates/sozo/ops/src/model.rs(8 hunks)crates/torii/sqlite/src/model.rs(1 hunks)crates/torii/types-test/src/models.cairo(1 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/src/models.cairo(1 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/lang/src/attribute_macros/mod.rs
- crates/dojo/lang/src/attribute_macros/element.rs
🚧 Files skipped from review as they are similar to previous changes (32)
- crates/dojo/core/src/model/definition.cairo
- crates/dojo/core/src/model/interface.cairo
- crates/dojo/lang/Cargo.toml
- crates/dojo/lang/src/derive_macros/mod.rs
- crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo
- crates/dojo/core/src/lib.cairo
- crates/torii/sqlite/src/model.rs
- crates/dojo/lang/src/derive_macros/introspect/utils.rs
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/core/src/model/component.cairo
- examples/spawn-and-move/src/models.cairo
- crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo
- crates/torii/types-test/src/models.cairo
- crates/dojo/core/src/event/component.cairo
- crates/dojo/lang/src/lib.rs
- crates/dojo/core/src/meta/introspect.cairo
- examples/spawn-and-move/dojo_dev.toml
- crates/dojo/core/src/storage/layout.cairo
- crates/benches/contracts/src/models/character.cairo
- crates/dojo/core-cairo-test/src/tests/world/event.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
- crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs
- crates/dojo/lang/src/cairo_plugin.rs
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- Cargo.toml
- crates/dojo/lang/src/attribute_macros/event.rs
- crates/sozo/ops/src/model.rs
- crates/dojo/core/src/storage/dojo_store.cairo
🧰 Additional context used
🧬 Code Definitions (4)
crates/dojo/lang/src/derive_macros/introspect/layout.rs (1)
crates/dojo/lang/src/derive_macros/introspect/utils.rs (6)
get_array_item_type(46-52)get_tuple_item_types(56-96)is_array(34-36)is_byte_array(30-32)is_option(42-44)is_tuple(38-40)
crates/dojo/world/src/contracts/model.rs (2)
crates/dojo/world/src/contracts/abigen/model.rs (2)
use_legacy_storage(673-682)use_legacy_storage(802-811)crates/torii/sqlite/src/model.rs (1)
use_legacy_storage(117-120)
crates/dojo/lang/src/derive_macros/introspect/mod.rs (2)
crates/dojo/lang/src/lib.rs (1)
debug_store_expand(46-53)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (2)
build_struct_dojo_store(10-75)build_enum_dojo_store(77-175)
crates/dojo/lang/src/utils.rs (1)
crates/dojo/lang/src/attribute_macros/event.rs (2)
members(90-99)members(101-104)
🔇 Additional comments (78)
crates/dojo/core-cairo-test/src/tests/world/model.cairo (1)
59-61: Ohayo! Good implementation of Default trait for enums.The addition of the
Defaulttrait and marking theXvariant as default is an excellent solution for the storage issue with custom enums. This will allow proper distinction between uninitialized variants and actual variant 0, which is crucial for theDojoStoretrait implementation you're introducing, sensei.crates/dojo/lang/src/utils.rs (13)
1-11: Ohayo! Clean imports with good organizationThe imports are well-organized, properly separating Cairo language, syntax, and Dojo-specific dependencies. The use of the poseidon hash for cryptographic purposes is appropriate for generating unique identifiers.
13-20: Well-designed serialization path selection function, sensei!This function cleanly implements the core mechanism that powers the PR's objective of supporting both Serde and DojoStore serialization paths. The inline attribute is appropriate for this simple but frequently called function.
22-46: Solid implementation of the hash computationThe unique hash computation is well-documented and correctly implemented. It uses poseidon_hash_many to create a deterministic identifier based on element properties and member information, which is essential for ensuring uniqueness in element contracts.
48-90: Nice enforcement of key member order, sensei!The member parsing logic correctly enforces that key members are defined before non-key members, with appropriate error handling. The function maintains a boolean flag for tracking the parsing state and provides helpful error messages.
92-108: Elegant separation of serialization concernsThis function effectively separates members into keys and values based on attributes, which aligns with the PR's objective of handling specific storage cases.
110-126: Clean parallel implementation for deserializationThe deserialization function mirrors the serialization approach, maintaining consistency in the codebase. Good job maintaining symmetry between these related functions.
128-177: Excellent tuple handling with clear documentationThe tuple destructuring function is well-documented with examples that make its purpose and behavior clear. The implementation correctly handles nested tuples and maintains the index count across recursion.
179-192: Good pattern matching for member type serializationThis function cleanly dispatches to the appropriate serialization method based on the member type.
194-205: Clear primitive type serializationThe function correctly handles the serialization path selection based on the
use_serdeflag, supporting the PR's objective of mixed storage approaches.
207-231: Thoughtful handling of tuple serializationThe function has a special case for empty tuples and properly destructures complex tuple structures. The code generates formatted serialization code that will work correctly for nested tuples.
233-247: Well-structured deserialization dispatchThis function appropriately dispatches between tuple and primitive deserialization methods.
249-256: Concise primitive type deserializationThe function correctly handles the deserialization path selection and generates appropriate code for primitive types.
258-283: Comprehensive tuple deserializationThe function handles empty tuples gracefully and correctly destructures complex tuple structures for deserialization.
crates/dojo/core/src/model/model_value.cairo (2)
7-8: Ohayo, sensei! New deserialize method added for greater model value flexibility.This new method enhances the
ModelValueParsertrait by providing custom deserialization capabilities, which is crucial for handling the difference between legacy and new storage models. It complements the existingserialize_valuesmethod, creating a more robust serialization/deserialization interface.
37-37: Implementation correctly updated to use new deserialize method!The change from
Serde::<V>::deserializetoModelValueParser::<V>::deserializealigns with the introduction of the DojoStore trait, enabling the special handling needed for Option and custom enums. This leverages the customized deserialization logic that addresses the variant identification issue.crates/dojo/core/src/meta/layout.cairo (1)
52-110: Ohayo! Excellent implementation of legacy support for variant indices.The
build_legacy_layoutfunction properly transforms the new storage layout (where variants start from 1) to the legacy format (where variants start from 0) by decrementing enum selectors. The recursive approach correctly handles nested layouts, ensuring complete transformation across complex data structures.The implementation properly handles different layout types:
- For
Enumlayouts: decrements selectors by 1- For
Structlayouts: preserves selectors- For
TupleandArraylayouts: recursively processes contained layouts- For other layouts: returns them unchanged
This enables seamless backward compatibility with existing Dojo models.
crates/dojo/world/src/contracts/abigen/model.rs (2)
671-682: Clean implementation of use_legacy_storage method for ModelContract!The method properly constructs a function call to the contract's
use_legacy_storageentry point, following the established pattern of other methods in this class. This enables checking whether a model uses legacy storage, which is crucial for the proper (de)serialization of models.
800-811: Consistent implementation for ModelContractReader, nice work sensei!This implementation mirrors the one in ModelContract, providing the same functionality for read-only contract interactions. The consistent approach across both classes ensures that legacy storage checking is available regardless of how the contracts are accessed.
crates/dojo/world/src/contracts/model.rs (2)
61-61: Good addition to the ModelReader trait interface!Adding the
use_legacy_storagemethod to the trait ensures that all implementations will provide this functionality, maintaining a consistent interface for model interactions regardless of the underlying storage model.
209-211: Clean implementation of use_legacy_storage in ModelRPCReader!The implementation correctly delegates to the model_reader's method and wraps the result in an Ok variant, following the established pattern of other methods in this class. This enables the ModelRPCReader to check legacy storage status without duplicating logic.
crates/dojo/lang/src/derive_macros/introspect/layout.rs (3)
10-10: The new import enhances type detection capabilities.The addition of
is_optionto the imports list aligns with the new functionality to properly handle Option types within the layout system.
78-80: Ohayo, variants now start from 1 instead of 0!This critical change ensures that variant 0 can be used as an uninitialized state, allowing the system to differentiate between default and explicitly set values. This addresses the core issue mentioned in the PR objectives.
380-386: Added proper error handling for packed Option types, nice work sensei!This new conditional branch prevents users from attempting to pack Option types, which would lead to runtime errors. The clear error message helps developers understand the limitation.
crates/dojo/core/src/model/model.cairo (4)
53-54: New deserialization method enhances the ModelParser trait.This addition provides a standardized way to convert raw data into model structs, supporting the new DojoStore trait functionality.
84-86: Added storage type detection capability, sugoi!This method allows the system to determine whether a model should use legacy or new storage mechanisms, providing the flexibility mentioned in the PR objectives.
135-135: Updated to use the new deserialization approach.The implementation now calls
ModelParser::<M>::deserializeinstead of directly using Serde, enabling the custom serialization logic needed for the DojoStore trait.
154-156: Implemented the storage type detection method.This implementation delegates to
ModelDefinition::<M>::use_legacy_storage(), ensuring consistent behavior across the model system.crates/dojo/core-cairo-test/src/tests/model/model.cairo (6)
65-71: Ohayo! New test enum with Default implementation.The enum properly implements the Default trait with Z as the default variant, satisfying the requirements for custom enums mentioned in the PR objectives.
73-81: LegacyModel with DojoLegacyStorage attribute for backwards compatibility.This model uses the
DojoLegacyStorageattribute to maintain compatibility with existing world storage as described in the PR objectives.
83-91: New DojoStoreModel using the enhanced storage system.This model will use the new DojoStore trait for serialization/deserialization, allowing comparison with the legacy approach.
101-102: Added new models to the namespace, nice work sensei!Both the legacy and new storage models are properly registered in the namespace definition, allowing them to be recognized as resources.
375-460: Comprehensive test for legacy model behavior.This test verifies that LegacyModel:
- Produces the expected layout with variant IDs starting from 0
- Correctly serializes/deserializes values using the legacy approach
- Maintains backward compatibility as intended
The detailed assertions help validate that the legacy storage behavior is preserved.
462-543: Thorough test for the new DojoStore model functionality.This test confirms that DojoStoreModel:
- Produces layouts with variant IDs starting from 1 (not 0)
- Correctly handles Option types and custom enums
- Properly serializes/deserializes with the new approach
The assertions verify that the serialized values include the incremented variant IDs as expected.
crates/dojo/lang/src/attribute_macros/model.rs (6)
21-21: Added DojoLegacyStorage derive attribute constant.This constant defines the attribute name used to identify models that should use legacy storage behavior.
67-69: Detect legacy storage flag from derive attributes.The code now checks for the presence of
DojoLegacyStorageamong the derive attributes to determine the storage approach.
89-119: Improved member parsing with key ordering validation, nice work sensei!This refactored approach:
- Processes struct members sequentially
- Verifies that key members are defined before non-key members
- Provides clear error messages when the ordering is incorrect
- Tracks the parsing state to enforce the structure
The validation ensures models follow best practices for key/value organization.
124-142: Serialization/deserialization now considers storage type.The serialization and deserialization code generation now passes the
use_legacy_storageflag to ensure the appropriate approach is used based on the model's attributes.
219-240: Storage-aware path selection and layout generation.The code now:
- Uses
Serdefor legacy models andDojoStorefor new models- Applies
build_legacy_layoutfor legacy models to ensure compatibility- Uses the introspected layout directly for new models
This conditional approach enables the seamless transition described in the PR objectives.
263-265: Added use_legacy_storage flag to the template context.This ensures the generated code can access the storage type information during model implementation.
crates/dojo/lang/src/derive_macros/introspect/mod.rs (12)
10-10: Ohayo! Good addition of Itertools crate.The
itertoolscrate is a nice addition here, sensei. UsingItertools::joinis much cleaner than manually building strings with loops.
12-14: New modules organization looks perfect!Nice structure with the additional debug helper and new
dojo_storemodule, sensei. This cleanly separates the DojoStore generation logic from the rest of the introspection code.
29-38: Much improved generic type handling!Ohayo! This is a significant improvement to the generic type handling, sensei. Splitting the types from implementations makes the code more focused and easier to maintain. The separation of concerns between
inspect_gen_implsanddojo_store_gen_implsis excellent!
55-61: Excellent DojoStore implementation for structs.The
dojo_store::build_struct_dojo_storecall properly generates the DojoStore trait implementation for structs, which aligns perfectly with the PR objective of introducing the DojoStore trait to handle special storage cases.
63-63: Smart debugging approach.Ohayo sensei! Using environment variables for conditional debug output is an excellent practice. This will help tremendously when troubleshooting implementation issues without cluttering normal output.
65-73: Updated introspect generation properly includes DojoStore.The updated call to
generate_introspectnow includes the DojoStore implementation, which is essential for the new functionality to work correctly.
85-97: Important enhancement for enum handling.Ohayo! The special handling for enums is particularly important, sensei. The additional requirements (Default trait) match what was described in the PR objectives for handling enum variants where the first variant ID is 0.
125-131: DojoStore implementation for enums looks good.Similar to the struct implementation, this properly generates the DojoStore trait for enums with the correct parameters.
133-133: Helpful debug output for enums.Same helpful debugging approach as for structs, which will make troubleshooting easier.
155-155: Clean integration of DojoStore into introspection output.The addition of the DojoStore parameter and its integration into the output template is clean and non-disruptive to the existing introspection functionality.
Also applies to: 176-176, 185-185
192-215: Good refactoring of the generic types function.The
build_generic_typesfunction has been streamlined to focus solely on extracting and returning the types, which makes it more reusable and clearer in its purpose.
217-237: Excellent new implementation builder function.Ohayo, sensei! This new
build_generic_implsfunction is well-designed. It handles both default implementations and additional implementations, which provides the flexibility needed for different trait requirements. The empty check at line 232 is an important edge case handling.crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo (6)
29-29: Good switch to using model layout variable.Ohayo! The change to use
$model_layout$instead of directly calling the introspect function allows for more flexibility in how layouts are determined, supporting the new legacy storage functionality.
32-36: Key addition for legacy storage support.This new function
use_legacy_storageis central to the PR's objective, sensei. It allows models to opt into using legacy storage behavior, which is exactly what was described in the PR summary.
59-61: Good deserialization implementation for models.Adding the
deserializemethod to theModelParserimplementation properly delegates to the appropriate deserializer path, providing a clean integration point for the DojoStore functionality.
75-77: Consistent deserialization for model values.Similar to the model parser, this provides deserialization for model values, ensuring consistency in the API.
85-87: Helpful comment about DojoStore implementation.Ohayo sensei! This comment clarifies how the DojoStore implementation for the model itself is handled, which is useful for understanding the overall implementation approach.
88-96: Well-structured DojoStore implementation for model values.The implementation of
ValueDojoStoreprovides both serialization and deserialization methods that align with the DojoStore trait requirements. This completes the storage handling for model values.crates/dojo/core-cairo-test/src/tests/storage/layout.cairo (11)
1-10: Good imports setup for the test file.Ohayo sensei! The imports are well-organized, bringing in all the necessary layout functions and constants needed for the tests.
11-52: Comprehensive fixed layout tests.This test function thoroughly verifies fixed layout functionality, covering initialization, reading/writing, offset handling, and cleanup. The assertions with descriptive messages are particularly helpful.
54-102: Well-structured tuple layout tests.Similar to the fixed layout tests, these tuple layout tests are comprehensive and well-structured, ensuring the functionality works as expected.
104-147: Thorough byte array layout tests.Ohayo! The byte array tests properly verify string-like data structures, sensei. Good job handling the specific encoding format with explicit values.
149-163: Good negative test for invalid array length.This test properly verifies that the system detects and handles invalid array lengths with appropriate error messages.
165-196: Comprehensive error condition tests for byte arrays.These tests verify different error conditions for byte arrays, ensuring robust error handling.
198-240: Well-designed array layout tests.The array layout tests cover all the necessary functionality including default values, reading/writing, and cleanup.
242-285: Good error handling tests for arrays.These tests verify that the array operations properly handle error conditions such as invalid lengths.
287-333: Thorough struct layout tests.Ohayo, sensei! These struct layout tests properly verify the functionality for structures with named fields.
335-381: Comprehensive enum layout tests.The enum layout tests properly verify variant handling, which is particularly important given the PR's focus on properly handling enum variants.
383-457: Important error handling tests for enums.These tests verify that the enum functions properly handle error conditions such as invalid variant values, which is critical for the functionality described in the PR.
crates/dojo/core-cairo-test/src/tests/meta/layout.cairo (8)
1-3: Good imports setup for legacy layout tests.Ohayo! The imports bring in the necessary components for testing the legacy layout functionality.
4-19: Important test for Option legacy layout.This test is particularly important given the PR objectives of handling
Option<T>storage cases. It verifies that the legacy layout is correctly generated with the variant indices adjusted as expected.
21-44: Critical enum layout regression test.Great test for simple enums, sensei! This verifies that the variant indices are properly adjusted in the legacy layout, which is central to the PR's objective.
46-71: Good test for tuple with nested Option.This test verifies that the legacy layout handling works correctly for tuples containing
Option<T>, ensuring nested structures are properly converted.
73-95: Well-designed array test with Option elements.Ohayo! This test verifies arrays containing Option elements, ensuring the legacy layout is correctly generated for container types.
97-129: Comprehensive struct layout test.This test covers structs with Option fields, verifying that legacy layout generation works correctly for complex data structures.
131-194: Excellent test for nested enum structures.This complex test verifies that deeply nested structures with enums, arrays, and tuples are correctly handled by the legacy layout generation.
196-260: Thorough test for nested struct structures.Similar to the nested enum test, this verifies that complex nested structures with structs are correctly handled.
crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (1)
3-9: Ohayo sensei, excellent use of default variants in your enum.
These definitions look consistent with the DojoStore approach, and the default variant appears well-suited for seamless initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Cargo.toml (2)
224-230: Dependency Source Reassignment: Validate the New RepositoryOhayo, sensei! You’ve successfully switched the
scarb,scarb-metadata, andscarb-uidependencies from the dojoengine repo to the remybar/scarb repository with commit6279a8e. Please verify that the new repo and commit provide all the required features and that they’re compatible with the rest of the codebase.
232-234: Local Path Dependencies: Enhance Developer GuidanceOhayo, sensei! The commented-out local path dependencies are a nice touch for local development troubleshooting. Consider adding a brief note or comment to inform contributors when they might switch to these local paths for testing or debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
Cargo.toml(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo (1)
5-5: Ohayo! New import added for ContractAddressDefaultThis import seems to support the DojoStore trait introduced in this PR, though it's not directly used within this file. Consider adding a comment to clarify its purpose or removing it if truly unused.
crates/dojo/lang/src/attribute_macros/model.rs (1)
67-68: Consider adding a documentation comment for use_legacy_storage.Adding a comment that explains the purpose and effect of this flag would improve code readability and maintenance.
+// Flag indicating whether this model should use legacy storage (Serde) +// instead of the new DojoStore trait let use_legacy_storage = derive_attr_names.contains(&DOJO_LEGACY_STORAGE_DERIVE.to_string());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
crates/dojo/lang/Scarb.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/benches/contracts/src/models/character.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(2 hunks)crates/dojo/core/src/event/component.cairo(1 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/model.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/benches/contracts/src/models/character.cairo
- crates/dojo/core/src/storage/dojo_store.cairo
🧰 Additional context used
🪛 GitHub Actions: ci
crates/dojo/lang/src/attribute_macros/model.rs
[error] 20-20: Rust formatting check failed. Please run 'rustfmt' to format the code.
[error] 141-141: Rust formatting check failed. Please run 'rustfmt' to format the code.
crates/dojo/core/src/event/component.cairo
[error] 27-27: Formatting check failed. Run 'scarb fmt' to fix code style issues in this file.
🔇 Additional comments (29)
crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo (5)
39-45: Nicely implemented Default for EnumOne, sensei!Adding the Default trait and explicitly marking the Two variant as #[default] aligns perfectly with the PR objectives. This ensures proper handling of enum variants when using the new DojoStore trait, addressing the issue where variant id 0 can't be distinguished from uninitialized state.
56-63: Excellent demonstration of DojoLegacyStorage attribute, sensei!This new struct provides a perfect test case for comparing the behavior between legacy storage and the new DojoStore trait. It contains both a custom enum (EnumOne) and an Option, which are exactly the types this PR aims to address.
110-112: Implementation of use_legacy_storage looks good!The default return value of
falseensures models will use the new DojoStore trait by default, which aligns with the PR objectives. This provides a clean way for models to opt-out of the new storage behavior when needed.
145-146: Default trait added to Sword struct - nice!Adding the Default trait to Sword ensures it can be properly handled by the new DojoStore trait. This is essential when Sword is used within other structures or enums.
195-198: Perfect implementation of Default for Weapon enum!Adding Default trait and marking DualWield as #[default] ensures proper serialization/deserialization behavior with the new DojoStore trait. This pattern is consistent with the EnumOne implementation earlier in the file.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (15)
5-5: Adding Default trait to Base - nice work sensei!This change supports the PR's objective of requiring each custom struct to implement the Default trait for proper integration with DojoStore.
40-40: Vec3 now has Default implementation - excellent!Adding the Default trait to this packed struct ensures it works correctly with the new DojoStore serialization system.
58-61: EnumNoData now properly implements Default with marked default variant!This change addresses the core issue in the PR by ensuring the enum has a Default implementation and explicitly marking the first variant as default, which solves the problem of distinguishing variant 0 from uninitialized.
66-69: Default implementation for EnumWithSameData looks good!Adding Default and marking the first variant explicitly helps solve the storage issue described in the PR objectives.
74-77: EnumWithSameTupleData Default implementation is correct!This follows the same pattern as the other enum modifications, properly implementing the Default trait and marking the first variant.
82-85: EnumWithVariousData Default implementation looks perfect!The Default trait and default variant are correctly implemented here, consistent with the other enum changes.
91-94: EnumPacked now has Default - aligned with PR objectives!Ensuring packed enums also implement Default is important for consistent behavior with the new DojoStore trait.
98-101: EnumInnerPacked Default implementation looks great sensei!This maintains consistency with the other enum implementations, ensuring proper behavior with DojoStore.
105-108: EnumInnerNotPacked Default implementation is correct!This completes the set of enum modifications to ensure they all work correctly with the new storage approach.
118-120: Renamed Generic to GenericStruct for clarity!This renaming improves code readability by differentiating between generic structs and enums.
122-126: Added GenericEnum with Default - excellent addition sensei!This new generic enum with Default implementation ensures that generics also work correctly with the DojoStore trait, addressing a potential gap in the implementation.
151-152: Critical change to enum variant indexing!This is a key implementation detail for the PR objectives - changing variant indexing to start from 1 instead of 0 allows distinguishing between uninitialized variants and the first variant.
155-156: Updated match cases to use the new variant indexing!This change ensures the enum handling logic matches the new variant indexing scheme where variants start from 1.
170-190: Updated test for renamed GenericStruct - looks great!The test has been properly updated to reflect the struct name change and ensures that generic structs continue to work as expected.
192-212: New test for GenericEnum provides good coverage!This test ensures that the new GenericEnum works correctly with the introspection system, maintaining proper test coverage for the codebase.
crates/dojo/lang/src/attribute_macros/model.rs (9)
13-13: Ohayo! New Terminal import added for improved AST handling.The addition of
Terminalfromcairo_lang_syntax::nodeenhances your AST manipulation capabilities, which is essential for the newDojoStoretrait implementation.
20-22: Nice work adding the DOJO_LEGACY_STORAGE_DERIVE constant, sensei!This constant enables the backward compatibility mechanism discussed in the PR objectives, allowing models to retain their current storage behavior when needed.
🧰 Tools
🪛 GitHub Actions: ci
[error] 20-20: Rust formatting check failed. Please run 'rustfmt' to format the code.
23-23: Well-organized utility imports for storage handling.The added utility functions properly encapsulate the serialization logic needed for the new
DojoStoretrait implementation.
61-69: Elegant implementation of legacy storage detection!This code correctly identifies whether a model should use legacy storage based on the presence of the
DojoLegacyStoragederive attribute.
79-80: Excellent addition of deserialized_values collection.This new collection enables proper handling of deserialization based on the storage strategy, which is key to resolving the issue with variant id 0.
86-117: Great enhancement to member parsing logic, sensei!The refactoring to use iterators and the addition of key-first ordering enforcement improves code organization and prevents potential errors. The diagnostics provide clear error messages when the ordering rule is violated.
122-128: Nice integration of legacy storage flag in serialization.The
use_legacy_storageparameter is now properly passed to the serialization function, ensuring consistency with the chosen storage approach.
214-232: Clean implementation of conditional model layout generation!This code elegantly handles the different paths for model layout based on whether legacy storage is used, supporting the PR's objective of maintaining compatibility with existing world storage.
240-257: Comprehensive template interpolation with new storage parameters.All necessary variables for the code template are now included, ensuring that the generated code correctly implements the chosen storage strategy.
remybar
force-pushed
the
fix-enum_options_storage
branch
from
e334d24 to
64fae38
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/dojo/lang/src/attribute_macros/model.rs (1)
13-13: Ohayo sensei! Unused import detected.The
Terminalimport fromcairo_lang_syntax::nodeappears to be unused in this file. Unless it's required by one of the functions called in this file, consider removing it to keep imports clean.-use cairo_lang_syntax::node::{Terminal, TypedStablePtr, TypedSyntaxNode}; +use cairo_lang_syntax::node::{TypedStablePtr, TypedSyntaxNode};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (3)
crates/dojo/lang/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (7)
crates/benches/contracts/src/models/character.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(6 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(2 hunks)crates/dojo/core/src/event/component.cairo(1 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/model.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core/src/event/component.cairo
- crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo
- crates/benches/contracts/src/models/character.cairo
- crates/dojo/core/src/storage/dojo_store.cairo
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (12)
crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo (6)
5-5: Ohayo! Good addition for supporting default values for ContractAddress types.Adding the import for
ContractAddressDefaultenables default value support for ContractAddress which aligns with the DojoStore trait's functionality to handle default values during deserialization.
39-45: Looking good, sensei! Nice default trait implementation for enums.The addition of the
Defaulttrait derivation forEnumOneand explicitly marking theTwovariant as#[default]is excellent. This supports the PR objective of providing proper default values for custom enums, which is necessary for the DojoStore serialization/deserialization mechanism to handle the variant index correctly.
56-63: Excellent strategy for backward compatibility, sensei!The introduction of
WithOptionAndEnumsLegacywith theDojoLegacyStorageattribute demonstrates how legacy storage behavior can be maintained while moving to the new serialization mechanism. This allows for testing both approaches and ensures smooth transition for existing code.
110-112: Perfectly implementing the new storage interface, sugoi!The
use_legacy_storagemethod returnsfalse, indicating this model will use the newDojoStoretrait rather than legacy serialization. This aligns with the PR objective of providing a mechanism to distinguish between legacy and new storage behaviors.
145-149: Default trait for structured types - nice work, sensei!Adding the
Defaulttrait derivation to theSwordstruct enables proper initialization of default values, which is necessary for the DojoStore serialization/deserialization mechanism.
195-200: Great default implementation for the Weapon enum!Similar to the changes for
EnumOne, adding theDefaulttrait toWeaponand explicitly markingDualWieldas the default variant supports the proper serialization/deserialization with the new DojoStore trait. This ensures there's a clear default when deserializing an uninitialized variant.crates/dojo/lang/src/attribute_macros/model.rs (6)
19-21: Great addition of legacy storage support, sensei!The inclusion of
DOJO_LEGACY_STORAGE_DERIVEconstant alongside other derive attributes is a good approach for implementing the legacy storage feature.
67-68: Clean implementation of the legacy storage detection, sensei!The code elegantly determines if legacy storage should be used by checking for the presence of the
DOJO_LEGACY_STORAGE_DERIVEattribute. This approach ensures backward compatibility while allowing newer code to use the improved storage mechanism.
86-117: Excellent implementation of member parsing with key ordering enforcement!Ohayo, sensei! Your approach of using an iterator over member AST nodes improves the code structure while enforcing the important rule that key members must be defined before non-key members. The error diagnostics for incorrect ordering are clear and helpful.
122-141: Well-implemented serialization with legacy storage support!The code correctly passes the
use_legacy_storageflag to serialization and deserialization functions, ensuring consistent behavior regardless of the storage strategy. This implementation aligns perfectly with the PR's objective of addressing special storage cases.
216-233: Clean implementation of conditional model layout, sensei!The conditional handling of model layout based on the legacy storage flag is well-structured. Using the
build_legacy_layoutfunction for legacy storage while directly using the introspection layout for the new approach is a clean solution.
254-257: Good addition to the interpolation parameters!Adding the
use_legacy_storageflag to the interpolation parameters makes it available in the generated code, ensuring consistency throughout the model implementation.
remybar
force-pushed
the
fix-enum_options_storage
branch
from
64fae38 to
ad3d846
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
crates/dojo/lang/src/derive_macros/introspect/mod.rs (1)
158-160: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid empty generic angle-brackets – causes invalid Cairo syntax
If the inspected struct/enum is not generic, the placeholders
$generics$and$generics_types$become empty strings, yielding code such as
impl FooIntrospect<> of …andFoo<>, which is illegal Cairo and aborts compilation.Consider guarding the brackets:
-impl $name$Introspect<$generics$> of dojo::meta::introspect::Introspect<$name$<$generics_types$>> { +impl $name$Introspect$( + if $generics$ == "" { "" } else { format!("<{generics}>") } +) of dojo::meta::introspect::Introspect< + $name$$( + if $generics_types$ == "" { "" } else { format!("<{generics_types}>") } + > +> {Or, more simply, build
impl_generics/ty_genericsstrings beforehand and interpolate only when non-empty.
Failing to fix this will break every non-generic model.
♻️ Duplicate comments (1)
crates/dojo/lang/src/utils.rs (1)
158-163: Graceful handling for unsupported tuple elements still missingThe
unimplemented!panic for unsupported expressions inside tuples remains unchanged from the previous review.
This will crash the macro expansion phase and halt compilation rather than emitting a diagnostic.Please downgrade to a diagnostic push (severity = Error) and generate placeholder code so other models can still compile.
🧹 Nitpick comments (4)
crates/dojo/lang/src/utils.rs (1)
14-20: Micro-optimisation: Reuse the serialization path string
get_serialization_pathallocates a newStringevery call.
As this helper is invoked for every member insideserialize_*/deserialize_*, the extra heap traffic adds up during macro expansion.A static
const SERDE_PATH: &str = "core::serde::Serde";and
const DOJO_PATH: &str = "dojo::storage::DojoStore";(or returning&'static str) eliminates the allocations with zero ergonomic cost.crates/dojo/core/src/storage/dojo_store.cairo (3)
1-6: Ohayo! ContractAddressDefault implementation might be temporary.The comment on line 1 suggests that this implementation might not be necessary in Cairo 2.11+. Consider adding a more specific TODO or a tracking issue reference to ensure this is revisited when Cairo 2.11 is released.
52-52: Variant ID for None is 2 instead of the conventional 0.In most Rust/Cairo implementations,
Nonetypically uses variant ID 0. Here it's using 2, which is intentional based on the PR description to avoid confusion with uninitialized values. This choice should be documented more clearly for future maintainers.- Option::None => { serialized.append(2); }, + // Using variant ID 2 for None (instead of conventional 0) + // to distinguish from uninitialized storage values + Option::None => { serialized.append(2); },
8-118: Ohayo, sensei! Consider adding documentation for custom enum handling.While the implementation for
Option<T>is provided, the PR description also mentions handling custom enums. Consider adding documentation about how custom enums are expected to be handled (possibly through derive macros or custom implementations).Also, it would be helpful to add examples in the documentation showing how to use
DojoStorefor custom types and how it integrates with theDojoLegacyStorageattribute.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
Cargo.toml(1 hunks)crates/benches/contracts/src/models/character.cairo(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/storage/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core/src/event/component.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(1 hunks)crates/dojo/core/src/meta/layout.cairo(1 hunks)crates/dojo/core/src/model/component.cairo(1 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/interface.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/layout.cairo(8 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(0 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/mod.rs(0 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(3 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(2 hunks)crates/dojo/lang/src/derive_macros/mod.rs(1 hunks)crates/dojo/lang/src/lib.rs(2 hunks)crates/dojo/lang/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/abigen/model.rs(2 hunks)crates/dojo/world/src/contracts/model.rs(2 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/lang/src/attribute_macros/mod.rs
- crates/dojo/lang/src/attribute_macros/element.rs
✅ Files skipped from review due to trivial changes (2)
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
🚧 Files skipped from review as they are similar to previous changes (32)
- crates/dojo/lang/Cargo.toml
- crates/dojo/core/src/model/component.cairo
- crates/dojo/core/src/model/interface.cairo
- crates/dojo/core/src/model/definition.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/core/src/meta/introspect.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core/src/meta/layout.cairo
- crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo
- crates/dojo/lang/src/lib.rs
- crates/dojo/lang/src/derive_macros/mod.rs
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/lang/src/derive_macros/introspect/utils.rs
- crates/dojo/core/src/event/component.cairo
- crates/dojo/core-cairo-test/src/tests/world/event.cairo
- crates/dojo/lang/src/attribute_macros/event.rs
- crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo
- crates/dojo/core/src/storage/layout.cairo
- crates/dojo/core/src/model/model.cairo
- crates/dojo/world/src/contracts/model.rs
- crates/dojo/lang/src/derive_macros/introspect/layout.rs
- crates/dojo/lang/src/cairo_plugin.rs
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- crates/dojo/world/src/contracts/abigen/model.rs
- crates/dojo/core-cairo-test/src/tests/world/model.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- Cargo.toml
- crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo
- crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo
- crates/dojo/core-cairo-test/src/tests/storage/layout.cairo
- crates/dojo/core-cairo-test/src/tests/meta/layout.cairo
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (2)
crates/dojo/lang/src/derive_macros/introspect/utils.rs (1)
clean_ty(66-68)crates/dojo/lang/src/utils.rs (5)
deserialize_primitive_member_ty(249-256)deserialize_tuple_member_ty(258-283)serialize_primitive_member_ty(194-205)serialize_tuple_member_ty(207-231)expr(143-165)
🔇 Additional comments (29)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (7)
75-80: Ohayo sensei! Good use of Default trait with properly marked variant.The
EnumWithCommentOnLastVariantenum correctly derivesDefaultwith the#[default]attribute on theXvariant. This is essential for the newDojoStoretrait to handle enums properly. The comment on the last variant is a nice edge case test for the parsing logic.
82-88: Well-structured test enum with nested option type!This enum includes an
Option<u32>variant which is perfect for testing nested enum serialization with the newDojoStoretrait. The#[default]attribute on theZvariant ensures proper default implementation.
90-99: Great test model with legacy storage annotation!The
LegacyModelappropriately derivesDojoLegacyStorageand includes a diverse set of field types (tuple, option, enum), making it an excellent test case for verifying legacy serialization behavior.
100-108: Perfect companion model for storage comparison testing!The
DojoStoreModelmirrorsLegacyModelbut without the legacy storage annotation, creating an ideal pair for direct comparison between the two storage implementations. This will help verify the behavior differences clearly.
118-119: Resources correctly registered for testing.The namespace definition is properly updated to include both new test models as resources, enabling them to be used in the world storage tests.
392-477: Comprehensive test for legacy model storage!This test thoroughly validates the legacy storage implementation, checking:
- The layout differences between
ModelDefinition::layout()andIntrospect::layout()- Serialization of keys and values with legacy variant indices (starting at 0)
- Successful deserialization back to the original model
The validation of layout differences is particularly important to ensure compatibility with existing storage.
479-559: Excellent test for new DojoStore model!This parallel test confirms the new storage implementation works correctly, verifying:
- The layout consistency between
ModelDefinition::layout()andIntrospect::layout()- Serialization with new variant indices (starting at 1 instead of 0)
- Successful round-trip deserialization
The test clearly demonstrates the key difference between storage formats: variant indices start at 1 in the new format, solving the uninitialized variant issue described in the PR objectives.
crates/dojo/lang/src/attribute_macros/model.rs (5)
61-69: Ohayo! Smart approach to early derive detection and flag setting.The code now extracts derive attributes early and sets a
use_legacy_storageflag based on the presence ofDojoLegacyStorage. This is a clean approach that allows the subsequent code to make storage-related decisions during code generation.
88-119: Better member ordering with clear error messages!This change enforces that key members must be defined before non-key members, providing clear error diagnostics when this constraint is violated. The approach using the
parsing_keysflag is elegant and doesn't halt parsing after the first error, allowing for multiple diagnostics to be collected.
124-143: Serialization code now handles both storage modes correctly.The code now passes the
use_legacy_storageflag to both serialization and deserialization helper functions, ensuring consistent handling across the model. The addition of deserialized value nodes is crucial for the round-trip serialization process.
219-240: Excellent conditional path selection for layout and deserialization!This is a key implementation that:
- Selects between
core::serde::Serdeanddojo::storage::DojoStorebased on the storage mode- For legacy storage, wraps the introspection layout with
build_legacy_layout- For new storage, uses the introspection layout directly
This allows seamless compatibility with both storage approaches while moving forward with the improved variant handling.
263-265: Storage flag properly passed to template.The
use_legacy_storageflag is correctly added to the interpolation parameters, ensuring the model patch template can generate the appropriate code based on the storage mode.crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs (2)
11-81: Ohayo sensei! Well-implemented struct serialization code generator!The
build_struct_dojo_storefunction generates a complete implementation of theDojoStoretrait for structs:
- Properly handles both primitive and tuple members with specialized code
- Generates serialization code using the appropriate helpers
- Constructs deserialization code that builds the struct from its members
- Supports generic types correctly
The implementation is clean and modular, leveraging the utility functions for serialization and deserialization.
83-185: Excellent enum serialization with variant offset handling!The
build_enum_dojo_storefunction addresses the core problem described in the PR:
- Increments variant index by 1 (line 96) to avoid using 0 as a variant index
- Adds special handling for variant 0 in deserialization to return
Default::default()(line 178)- Handles empty, tuple, and primitive variants with appropriate code generation
- Returns None for unknown variant indices for proper error handling
This implementation solves the issue with uninitialized enum variants by ensuring all serialized variants use indices starting at 1, with 0 reserved for uninitialized/default values.
crates/benches/contracts/src/models/character.cairo (4)
1-14: Ohayo sensei! Good model structure with proper key field.The
Charactermodel is well-structured with:
- Appropriate use of
#[dojo::model]attribute- Correct key field using
ContractAddress- Nice mix of primitive and complex nested types
This will make a good benchmark case for testing the storage system's handling of complex models.
16-37: Well-designed supporting structs for nested model testing.The
AbilitiesandStatsstructs provide good test cases for nested serialization within models. They have appropriate derives (Introspect,Copy,Drop) and contain a variety of field types to exercise the serialization system.
39-50: Excellent default implementation for enum and nested struct.Both
Weaponenum andSwordstruct correctly deriveDefault, which is essential for the newDojoStoretrait to handle uninitialized variants properly. The#[default]attribute on theDualWieldvariant ensures the correct default implementation.
52-67: Good additional model definitions for benchmark diversity.The
CaseandAliasmodels provide additional test cases with different structures:
- Both correctly use
#[dojo::model]with appropriate key fieldsCaseincludes a nestedSwordstruct, testing complex field serialization- Both use simple primitive fields alongside the complex ones
These will help ensure the benchmark covers diverse model patterns.
crates/dojo/lang/src/derive_macros/introspect/mod.rs (1)
218-237: Potential double comma / spacing in generic impl list
build_generic_implsproduces strings like"T, +Trait1<T>, +Trait2<T>"for eachTand then joins them with", ".
Whenadditional_implsare appended the result can become
"T, +Trait<T>, Extra, Extra2"→ notice the missingTbefore the extra items.Verify that resulting
where-like lists match your intended grammar or buildgen_implsfrom a single iterator of homogeneous entries instead of mixing two phases.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (2)
55-128: Ohayo sensei – impressive coverage!The primitive round-trip tests are exhaustive and clearly validate the
DojoStorecontract for each scalar type. No issues spotted here.
389-442: Complex nested structure test is 💯Great job stress-testing nested generics, options, arrays and enums in a single assertion. This will guard against a whole class of regressions.
crates/dojo/core/src/storage/dojo_store.cairo (8)
8-12: Looks great, sensei! Core trait design is clean and focused.The
DojoStoretrait provides a clear, minimal interface for serialization and deserialization of types for world storage. TheOptionreturn type fordeserializeproperly handles potential deserialization failures.
14-24: Nice implementation of the default DojoStore using Serde!This default implementation elegantly reuses existing
Serdeimplementations, which reduces duplication and maintains consistency with existing serialization logic.
26-42: All essential primitive and StarkNet types covered, good work sensei!The comprehensive list of implementations ensures that all commonly used types have proper
DojoStoresupport out of the box.
44-68: Key implementation addressing Option variant ID issue.This custom implementation for
Option<T>solves the core problem mentioned in the PR description by using variant IDs 1 and 2 instead of 0 and 1, allowing distinction between uninitialized (0) and actual variants.The deserialize method's handling of variant ID 0 returning
Default::default()matches the PR objectives perfectly.
59-59: Default handling for uninitialized variant IDs.This is the key part of the solution - when encountering variant ID 0 (uninitialized storage), it returns the default value instead of failing, which aligns perfectly with the PR objectives. Great implementation, sensei!
70-90: Well-structured array helper functions.These recursive helper functions provide clean, reusable logic for array serialization and deserialization. The recursion is properly terminated and handles error cases well.
92-105: Proper DojoStore implementation for arrays.This implementation correctly serializes the array length first, followed by each element, and handles deserialization failures appropriately with the
?operator.
107-118: DojoStore implementation for Span matches Array implementation.The Span implementation follows the same pattern as the Array implementation, which is good for consistency. The double-wrapping with Option on line 116 looks correct for handling potential deserialization failures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Cargo.toml (1)
161-163: Clean up commented Git dependenciesOhayo sensei, these commented-out Git entries for
scarb,scarb-metadata, andscarb-uiclutter the manifest and may confuse contributors. If they’re no longer needed, please remove them or add a note explaining their purpose.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (5)
Cargo.toml(1 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/dojo/lang/src/attribute_macros/event.rs
- crates/dojo/lang/src/attribute_macros/model.rs
🔇 Additional comments (24)
crates/dojo/lang/src/derive_macros/introspect/mod.rs (12)
10-12: Good additions for debugging and functionality!Ohayo sensei! The use of
Itertoolswill be helpful for joining collections, and the debug utility will aid in diagnostics during development.
14-14: New DojoStore module looks good!This module will encapsulate the DojoStore trait implementation logic, keeping the codebase well-organized.
27-35: Clean separation of generic type handling!The extraction of generic types and separate implementation constraints for introspection and DojoStore is well-structured. This will make the code more maintainable and easier to extend in the future.
52-61: Great modular approach to building DojoStore for structs!The separate function call to build DojoStore implementation keeps the code clean, and the debug output will be valuable during development.
62-70: Nice integration of DojoStore into introspect generation!The modified function signature ensures that both introspection and storage traits are generated cohesively.
82-94: Well-structured generic handling for enums!Ohayo sensei! I particularly like how you're handling the enum name with generics and the additional trait constraints for Serde and Default. This ensures that enums will properly implement the necessary traits for the DojoStore functionality.
122-131: Well-implemented DojoStore for enums!The enum DojoStore implementation follows the same clean pattern as for structs. The debug logging will help with troubleshooting during development.
132-140: Good integration of DojoStore for enums!The updated generate_introspect function call properly includes the DojoStore implementation in the generated code.
173-173: Clean template integration!Adding the DojoStore to the template with a simple placeholder ensures clean code generation.
182-182: Good interpolation of the DojoStore code!The RewriteNode map correctly includes the DojoStore implementation.
189-212: Clean refactoring of generic type handling!The extracted build_generic_types function is well-implemented and focused on a single responsibility.
214-234: Excellent helper function for building generic impl constraints!The build_generic_impls function is well-designed with a clean separation of concerns. It handles both base implementations and additional constraints elegantly.
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (12)
5-8: Addition of Default trait is appropriate!Ohayo sensei! Adding the Default trait to Base struct aligns with the new DojoStore requirements, allowing proper handling of uninitialized values.
58-64: Good addition of Default for EnumNoData and default variant!The #[default] attribute on the first variant ensures proper default value handling for the enum.
66-72: Similar pattern applied consistently for EnumWithSameData!Maintaining consistency with Default trait and default variant annotation.
74-80: Consistent pattern for EnumWithSameTupleData!The Default trait and default variant continue to be applied consistently.
82-88: Good handling for EnumWithVariousData!Default trait and default variant are properly applied.
91-96: EnumPacked with Default looks good!Consistent pattern application continues.
117-120: Good rename from Generic to GenericStruct!Ohayo sensei! The rename to GenericStruct makes the purpose clearer and aligns with the new GenericEnum type.
122-126: Nice addition of GenericEnum!Adding a generic enum with proper Default implementation complements the GenericStruct and provides better test coverage.
151-153: Critical change to enum variant indexing!The shift from zero-based to one-based indexing for enum variants is a key change that addresses the core issue in the PR. This reserves zero for uninitialized variants, allowing proper distinction in storage.
155-156: Updated variant usage in match pattern!The updated field creation correctly uses the new variant index.
170-190: Good update to the generic struct test!The renamed test accurately verifies the introspection of GenericStruct with comprehensive assertions for size, layout, and type.
192-212: Excellent new test for GenericEnum!Ohayo sensei! This new test comprehensively verifies the introspection functionality for GenericEnum, ensuring that the size, layout, and type introspection work correctly.
remybar
force-pushed
the
fix-enum_options_storage
branch
2 times, most recently
from
5786d1f to
ea46867
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/dojo/lang/src/derive_macros/introspect/mod.rs (1)
154-175:⚠️ Potential issueOhayo sensei – generated impls break for non-generic types
generate_introspecthard-codes angle-brackets:impl $name$Introspect<$generics$> of ... <$name$<$generics_types$>>When the inspected type has no generics,
$generics$and$generics_types$are empty strings, leaving dangling<>which is illegal Cairo syntax and fails compilation for every non-generic struct/enum.A minimal fix is to include the brackets inside the placeholders:
-"impl $name$Introspect<$generics$> of dojo::meta::introspect::Introspect<$name$<$generics_types$>>" +"impl $name$Introspect$generics$ of dojo::meta::introspect::Introspect<$name$$generics_types$>"And populate
let generics = if generic_impls.is_empty() { "".into() } else { format!("<{generic_impls}>") }; let generics_types = if generic_types.is_empty() { "".into() } else { format!("<{}>", generic_types.join(", ")) };before interpolation.
♻️ Duplicate comments (1)
crates/dojo/lang/src/utils.rs (1)
158-163: Ohayo sensei –unimplemented!will blow up at runtimeWe already discussed this in an earlier round: panicking on unsupported tuple expressions will abort macro expansion and confuse users. Please replace the
unimplemented!with a diagnostic + graceful fallback exactly as suggested before.
🧹 Nitpick comments (8)
crates/dojo/lang/src/utils.rs (3)
13-20: Ohayo sensei – avoid heap allocations inget_serialization_path()The function returns a newly-allocated
Stringevery time, yet it only ever returns one of two static literals. Returning&'static str(or even making the functionconst) removes the allocation and enables further compile-time optimisations.-#[inline(always)] -pub fn get_serialization_path(use_serde: bool) -> String { - if use_serde { - "core::serde::Serde".to_string() - } else { - "dojo::storage::DojoStore".to_string() - } -} +#[inline(always)] +pub const fn get_serialization_path(use_serde: bool) -> &'static str { + if use_serde { + "core::serde::Serde" + } else { + "dojo::storage::DojoStore" + } +}This is zero-cost and keeps the call-sites unchanged (string literals coerce to
&'static str).
167-173: Trailing comma logic may mis-place commas
commais empty only whenstart == 1, but this is also true for nested tuples that recursed withstart = 1. This can produce output like"(e1,e2,),"(note the comma after the closing)), which later string-parsers treat as an extra field. Consider deriving the delimiter fromelements.len()or appending,inside the join operation instead.
194-205: Uniform generic notation forserialize/deserialize
deserialize_primitive_member_tyemitsDojoStore::<T>::deserialize(...)whereas
serialize_primitive_member_tyemitsDojoStore::serialize(value, ...)For
Serdethe symmetric<T>is already required; forDojoStoreit is optional but keeping the two consistent prevents surprises and aligns with thedeserializeside:- format!( - "{path}::serialize({}{member_name}, ref serialized);\n", - if with_self { "self." } else { "" }, - ) + format!( + "{path}::<{member_ty}>::serialize({}{member_name}, ref serialized);\n", + if with_self { "self." } else { "" }, + member_ty = member_name // you already have the concrete type when `with_self` is false + )(You'll need to thread
member_tyinto the caller.)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
170-177: Ohayo sensei – preferassert_eq!for clearer failure messagesMost tests use
assert!(a == b). Switching toassert_eq!(a, b)prints both sides on failure and avoids redundantunwrap()calls:-let size = Introspect::<GenericStruct<u32>>::size(); -assert!(size.is_some()); -assert!(size.unwrap() == 1); +assert_eq!(Introspect::<GenericStruct<u32>>::size(), Option::Some(1));This makes the suite more readable and yields richer diagnostics when a test breaks.
crates/dojo/core-cairo-test/src/tests/storage/layout.cairo (2)
198-233: Consider extracting shared helpers for array-length driven casesThe three write/read/delete cycles here are almost identical to the earlier
fixed/tupletests (only the writer/reader pairs differ). A tiny helper such asfn roundtrip(write, read)would slash ~60 duplicated lines and keep future tests DRY.
242-260: Nit: two almost-identical panic messages – unify spelling to avoid brittle assertions
'invalid array length'vs'Invalid values length'(capital “I”). If the underlying implementation ever changes its exact wording, half the tests will fail while the other half pass, hiding regressions. Recommend asserting on a shared constant or at least keeping the wording consistent across tests.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (2)
212-220: Copy-paste typo in ByteArray assertionsThe failure messages still read “DojoStore …” although the test targets
ByteArray. This can mislead newcomers skimming CI logs.- assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<i128> serialization failed"); + assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<ByteArray> serialization failed"); @@ - assert_eq!(res, Option::Some("hello"), "DojoStore<i128> deserialization failed"); + assert_eq!(res, Option::Some("hello"), "DojoStore<ByteArray> deserialization failed");
436-441: Impressive stress test, but consider property-based fuzzing for long-term safetyThe deeply nested
EComplexround-trip is fantastic, yet still a single concrete sample. Leveragingproptest-style fuzzing (once available for Cairo) would give higher confidence that any arbitrary permutation of the same pattern survives serialise/deserialise cycles.
🛑 Comments failed to post (1)
crates/dojo/lang/src/derive_macros/introspect/mod.rs (1)
215-235: 🛠️ Refactor suggestion
Generic bound builder produces unparsable parameter list
build_generic_implscurrently returns a flat string like"T, +dojo::meta::introspect::Introspect<T>, +dojo::storage::DojoStore<T>, U, +…"which is inserted inside the
<>of the type name. This mixes parameter names (T) with bounds (+dojo::…) without the:separator Cairo expects, givingT, +Trait<T>instead ofT: Trait<T>.Consider emitting
"T: Trait1<T> + Trait2<T>"per param and then joining them:for g in gen_types { - format!("{g}, {bounds}") + format!("{g}: {}", bounds.join(" + ")) }where
boundsis the mapped list ofbase_impls. This will align with Cairo’s generic syntax and fix compile failures on every generic type.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In crates/dojo/lang/src/derive_macros/introspect/mod.rs around lines 215 to 235, the function build_generic_impls currently produces a string mixing generic parameter names and trait bounds without the required ':' separator, resulting in invalid syntax like "T, +Trait<T>". To fix this, modify the function to emit each generic parameter followed by a colon and its trait bounds joined with '+', for example "T: Trait1<T> + Trait2<T>", then join these per-parameter strings with commas. This will produce a valid generic parameter list for Cairo.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/dojo/lang/src/utils.rs (1)
158-163: Avoidunimplemented!panics – surface a diagnostic insteadOhayo sensei! Falling back to
unimplemented!here will explode at macro-expansion time and abort compilation, giving the user no actionable hint. We already raised this in an earlier round, so consider piping the unsupported expression intoPluginDiagnostic(similar to the tuple-key ordering check) and return a harmless placeholder tuple fragment instead.
🧹 Nitpick comments (4)
crates/dojo/lang/src/utils.rs (1)
13-20: Return a&'static strto skip heap allocation
get_serialization_path()rebuilds the same two strings every call. Converting it to return a&'static stravoids repeated allocations and simplifies call-sites:-pub fn get_serialization_path(use_serde: bool) -> String { - if use_serde { "core::serde::Serde".to_string() } - else { "dojo::storage::DojoStore".to_string() } +pub fn get_serialization_path(use_serde: bool) -> &'static str { + if use_serde { "core::serde::Serde" } else { "dojo::storage::DojoStore" } }Since the value is immediately interpolated into a
format!call, lifetimes stay trivial.crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
118-121: DeriveDefaultforGenericStructas well for symmetry
GenericEnumderivesDefault, but its sibling struct does not. AlthoughDojoStoredoes not strictly require it for structs, having both generic helpers implementDefaultoffers a consistent public surface and eases test authoring.crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (2)
214-220: Assertion message copy-paste typoOhayo sensei! The failure string still mentions
i128instead ofByteArray, which could mislead debugging:-assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<i128> serialization failed"); +assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<ByteArray> serialization failed"); ... -assert_eq!(res, Option::Some("hello"), "DojoStore<i128> deserialization failed"); +assert_eq!(res, Option::Some("hello"), "DojoStore<ByteArray> deserialization failed");
55-173: Consider table-driven tests to cut down boilerplateThe primitive round-trip assertions are excellent but very verbose. A small helper macro (e.g.
assert_dojo_roundtrip!(T, value, serialized...)) would slash ~120 lines, keep intent crystal clear, and make future additions painless.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (47)
Cargo.toml(1 hunks)crates/benches/contracts/src/models/character.cairo(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/storage/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core/src/event/component.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(1 hunks)crates/dojo/core/src/meta/layout.cairo(1 hunks)crates/dojo/core/src/model/component.cairo(1 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/interface.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/layout.cairo(8 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(0 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/mod.rs(0 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(3 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(2 hunks)crates/dojo/lang/src/derive_macros/mod.rs(1 hunks)crates/dojo/lang/src/lib.rs(2 hunks)crates/dojo/lang/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/abigen/model.rs(2 hunks)crates/dojo/world/src/contracts/model.rs(2 hunks)crates/sozo/ops/src/model.rs(8 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/src/models.cairo(1 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/lang/src/attribute_macros/mod.rs
- crates/dojo/lang/src/attribute_macros/element.rs
✅ Files skipped from review due to trivial changes (3)
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
- crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs
🚧 Files skipped from review as they are similar to previous changes (39)
- crates/dojo/core/src/model/component.cairo
- crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo
- crates/dojo/lang/Cargo.toml
- crates/dojo/core/src/model/definition.cairo
- crates/dojo/core/src/lib.cairo
- examples/spawn-and-move/src/models.cairo
- crates/dojo/core/src/meta/introspect.cairo
- crates/dojo/lang/src/derive_macros/mod.rs
- crates/dojo/core/src/model/interface.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/lang/src/lib.rs
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/core/src/meta/layout.cairo
- crates/dojo/world/src/contracts/model.rs
- crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo
- crates/dojo/lang/src/attribute_macros/event.rs
- crates/dojo/world/src/contracts/abigen/model.rs
- crates/dojo/core-cairo-test/src/tests/world/model.cairo
- crates/dojo/core/src/storage/layout.cairo
- crates/dojo/lang/src/cairo_plugin.rs
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- crates/dojo/core/src/event/component.cairo
- crates/dojo/lang/src/attribute_macros/model.rs
- crates/dojo/lang/src/derive_macros/introspect/layout.rs
- examples/spawn-and-move/dojo_dev.toml
- crates/dojo/core/src/model/model.cairo
- crates/dojo/lang/src/derive_macros/introspect/utils.rs
- crates/dojo/core-cairo-test/src/tests/world/event.cairo
- crates/dojo/core-cairo-test/src/tests/model/model.cairo
- Cargo.toml
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- crates/dojo/lang/src/derive_macros/introspect/mod.rs
- crates/benches/contracts/src/models/character.cairo
- crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo
- crates/dojo/core-cairo-test/src/tests/storage/layout.cairo
- crates/dojo/core-cairo-test/src/tests/meta/layout.cairo
- crates/sozo/ops/src/model.rs
- crates/dojo/core/src/storage/dojo_store.cairo
🔇 Additional comments (1)
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (1)
151-157: Confirm downstream code handles one-based variant selectorsThe new
_enumhelper now shiftsvarianttoi + 1, matching theDojoStoreconvention. Please double-check any runtime that still expects zero-based discriminants (legacy generators, Sozo CLI, on-chain deserialisers) to avoid mismatched layouts.
remybar
force-pushed
the
fix-enum_options_storage
branch
from
ea46867 to
ff6958f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/dojo/core-cairo-test/src/tests/meta/layout.cairo (1)
1-260: Consider extracting common test helpers to reduce duplicationThe test file has some repetitive layout construction patterns that could be extracted into helper functions to improve readability and maintainability.
+ /// Creates a legacy Option<T> enum layout with the given inner layout + fn create_legacy_option_layout(inner_layout: Layout) -> Layout { + Layout::Enum( + [ + dojo::meta::FieldLayout { // Some + selector: 0, layout: inner_layout, + }, + dojo::meta::FieldLayout { // None + selector: 1, layout: Layout::Fixed([].span()), + }, + ] + .span(), + ) + }This would allow simplifying test assertions like:
- let expected = Layout::Enum( - [ - dojo::meta::FieldLayout { // Some - selector: 0, layout: Introspect::<u32>::layout(), - }, - dojo::meta::FieldLayout { // None - selector: 1, layout: Layout::Fixed([].span()), - }, - ] - .span(), - ); + let expected = create_legacy_option_layout(Introspect::<u32>::layout());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.lockcrates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (47)
Cargo.toml(1 hunks)crates/benches/contracts/src/models/character.cairo(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/storage/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core/src/event/component.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(1 hunks)crates/dojo/core/src/meta/layout.cairo(1 hunks)crates/dojo/core/src/model/component.cairo(1 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/interface.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/layout.cairo(8 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(0 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/mod.rs(0 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(3 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(2 hunks)crates/dojo/lang/src/derive_macros/mod.rs(1 hunks)crates/dojo/lang/src/lib.rs(2 hunks)crates/dojo/lang/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/abigen/model.rs(2 hunks)crates/dojo/world/src/contracts/model.rs(2 hunks)crates/sozo/ops/src/model.rs(8 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/src/models.cairo(1 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/lang/src/attribute_macros/mod.rs
- crates/dojo/lang/src/attribute_macros/element.rs
✅ Files skipped from review due to trivial changes (1)
- crates/dojo/lang/src/utils.rs
🚧 Files skipped from review as they are similar to previous changes (41)
- crates/dojo/core/src/model/definition.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core/src/model/component.cairo
- crates/dojo/lang/src/derive_macros/mod.rs
- crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo
- examples/spawn-and-move/src/models.cairo
- crates/dojo/core/src/model/interface.cairo
- crates/dojo/lang/src/derive_macros/introspect/layout.rs
- crates/dojo/lang/Cargo.toml
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/lang/src/lib.rs
- crates/dojo/core/src/meta/introspect.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/lang/src/derive_macros/introspect/utils.rs
- crates/dojo/core/src/event/component.cairo
- crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo
- crates/dojo/lang/src/attribute_macros/event.rs
- crates/dojo/world/src/contracts/abigen/model.rs
- examples/spawn-and-move/dojo_dev.toml
- crates/dojo/core/src/storage/layout.cairo
- crates/dojo/core/src/meta/layout.cairo
- crates/dojo/world/src/contracts/model.rs
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/core-cairo-test/src/tests/world/model.cairo
- crates/dojo/core/src/model/model.cairo
- Cargo.toml
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- crates/dojo/lang/src/cairo_plugin.rs
- crates/dojo/core-cairo-test/src/tests/world/event.cairo
- crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs
- crates/benches/contracts/src/models/character.cairo
- crates/dojo/core-cairo-test/src/tests/model/model.cairo
- crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- crates/dojo/core-cairo-test/src/tests/storage/layout.cairo
- crates/dojo/lang/src/derive_macros/introspect/mod.rs
- crates/sozo/ops/src/model.rs
- crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo
- crates/dojo/core/src/storage/dojo_store.cairo
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (15)
crates/dojo/lang/src/attribute_macros/model.rs (7)
61-72: Ohayo! Restricting generic models is a good defensive measureValidating that models cannot be generic aligns perfectly with the PR objective since generic structs and enums aren't yet supported by the DojoStore trait's implementation. Clear error message with proper severity.
80-81: Smart detection of legacy storage flag, sensei!This flag detection cleanly implements the backward compatibility strategy mentioned in the PR objectives, allowing models to choose between legacy Serde-based storage and the new DojoStore trait.
101-132: Enforcing key ordering is a wise structural constraintThis validation ensures that all key members appear before non-key members, providing clear diagnostics without interrupting the processing flow. This maintains a consistent model structure that's easier to reason about.
138-142: Clean propagation of legacy storage flag to serialization/deserialization helpersThe code properly forwards the legacy storage flag to both serialization and deserialization helper functions, ensuring consistent behavior throughout the serialization stack.
Also applies to: 146-150, 151-155
238-242: Excellent conditional path selection based on storage modeThis cleanly selects the appropriate deserialization path - either the legacy Serde implementation or the new DojoStore trait - based on the legacy storage flag.
244-252: Nice implementation of conditional layout generationThe code smartly switches between standard layout introspection and legacy layout building based on the storage strategy. This maintains backward compatibility while allowing new models to use the improved storage semantics.
263-264: Well integrated interpolation values for new deserialization approachThe new interpolation parameters for deserialized values, modelvalue creation and legacy storage flag properly wire up the template with the conditional code paths.
Also applies to: 276-278
crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (4)
5-5: Ohayo sensei! Comprehensive Default trait implementation for storageThe addition of
Defaulttrait andSerdederivations to structs and enums is essential for supporting the new DojoStore trait requirements. Every enum also has a properly annotated#[default]variant, which ensures correct behavior during deserialization when handling uninitialized values.The new
GenericEnum<T>addition complements the renamedGenericStruct<T>(previously justGeneric<T>) for more consistent naming and better test coverage of generic types.Also applies to: 40-41, 58-59, 66-67, 74-75, 82-83, 91-92, 98-99, 105-106, 112-113, 117-118, 122-126
151-153: Critical change to enum variant indexingThis change addresses the core issue from the PR objectives - shifting enum variants from 0-based to 1-based indexing so that variant 0 can be used to represent uninitialized values. The comment clearly explains the reasoning behind this change.
170-190: Thorough tests for generic struct introspectionThese tests validate that the introspection system properly handles generic structs, checking size, layout, and type information. This ensures the infrastructure supports the generic types that will be needed in future PRs.
192-212: Comprehensive generic enum introspection testsSimilar to the generic struct tests, these validate that the introspection system correctly handles generic enums, testing size, layout and type information. The tests are well-structured and verify all necessary aspects of the new functionality.
crates/dojo/core-cairo-test/src/tests/meta/layout.cairo (4)
1-19: Ohayo! Clean Option legacy layout testThis test verifies that
build_legacy_layoutcorrectly transformsOption<u32>from the new 1-based indexing (variants at indexes 1 and 2) to the legacy 0-based indexing (variants at indexes 0 and 1). This is essential for backward compatibility.
21-44: Excellent simple enum conversion testThis test validates that the
build_legacy_layoutfunction correctly shifts enum variant selectors from 1-based indexing to 0-based indexing for legacy compatibility. The test case covers a simple enum with multiple variants, ensuring thorough coverage.
46-71: Comprehensive tests for compound types with Option elementsThese tests for tuples, arrays, and structs containing
Option<T>elements ensure that thebuild_legacy_layoutfunction correctly handles nested data structures. Each test verifies that options are properly converted to legacy enum layouts with 0-based indexing.Also applies to: 73-95, 97-129
131-194: Thorough tests for deeply nested data structuresThese tests validate that
build_legacy_layoutcorrectly handles complex nested structures combining enums, arrays, tuples, and options. The tests are comprehensive and ensure that the legacy conversion works correctly at all levels of nesting.The consistent test structure and detailed assertions make it easy to understand the expected behavior.
Also applies to: 196-260
remybar
force-pushed
the
fix-enum_options_storage
branch
from
ff6958f to
ed43a45
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/dojo/lang/src/utils.rs (1)
158-163:⚠️ Potential issueAvoid
unimplemented!– return a placeholder and emit diagnostics instead
The macro currently panics on any unsupported expression inside a tuple,
aborting the entire plugin and producing a compiler error that is hard to trace.
A graceful fallback—returning a dummy variable name and pushing a diagnostic—was
requested in a previous review but is still outstanding.
🧹 Nitpick comments (7)
crates/dojo/lang/src/utils.rs (1)
14-20: Prefer returning a&'static strto avoid repeated heap allocations
get_serialization_pathis invoked from every (de)serialization helper.
Returning an ownedStringallocates on every call, which is unnecessary because
the returned value is always one of two static literals.-#[inline(always)] -pub fn get_serialization_path(use_serde: bool) -> String { - if use_serde { - "core::serde::Serde".to_string() - } else { - "dojo::storage::DojoStore".to_string() - } +#[inline(always)] +pub const fn get_serialization_path(use_serde: bool) -> &'static str { + if use_serde { + "core::serde::Serde" + } else { + "dojo::storage::DojoStore" + } }This change removes the allocation, shortens generated code, and may shave a few
µs off macro expansion time.crates/dojo/core/src/model/model.cairo (1)
84-88: Document the semantics ofuse_legacy_storageGreat to see run-time introspection of storage strategy!
A short doc-comment explaining thattrue⇒ Serde layout,false⇒ DojoStore
layout, will make the API self-explanatory for downstream tooling.crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (2)
170-212: Great test coverage for the generic types!The tests thoroughly verify the introspection of generic types, including size, layout, and type information. This ensures that the introspection system can handle generics correctly.
Consider adding a comment in
test_generic_enum_introspectexplaining that the size of 2 comes from the enum discriminant (variant index) plus the u32 value.
272-283: Ohayo sensei! Minor style issue with double blank line.There's an extra blank line that could be removed for consistency with the rest of the file.
assert!(size.unwrap() == 4); } - #[test] fn test_size_of_enum_with_variant_data() {crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (3)
216-220: Fix comment in ByteArray test - incorrect type reference.The comments for ByteArray serialization and deserialization incorrectly refer to
i128instead ofByteArray.-assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<i128> serialization failed"); +assert_eq!(serialized, array![0, 0x68656c6c6f, 0x05], "DojoStore<ByteArray> serialization failed"); let mut values = [0, 0x68656c6c6f, 0x05].span(); let res = DojoStore::<ByteArray>::deserialize(ref values); -assert_eq!(res, Option::Some("hello"), "DojoStore<i128> deserialization failed"); +assert_eq!(res, Option::Some("hello"), "DojoStore<ByteArray> deserialization failed");
277-280: Add clarity about default value handling for variant 0.This test case demonstrates a crucial part of the PR's purpose - handling variant 0 as uninitialized storage. A comment would help future developers understand this special case.
let mut values = [0].span(); let res = DojoStore::<E>::deserialize(ref values); +// Variant 0 is treated as uninitialized storage and returns the default value assert_eq!(res, Option::Some(E::A(0)), "DojoStore<E> deserialization failed");
353-353: Minor style issue with double blank line.There's an extra blank line that could be removed for consistency with the rest of the file.
} - #[test]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.lockcrates/dojo/core-cairo-test/Scarb.lockis excluded by!**/*.lockexamples/simple/Scarb.lockis excluded by!**/*.lockexamples/spawn-and-move/Scarb.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (47)
Cargo.toml(1 hunks)crates/benches/contracts/src/models/character.cairo(1 hunks)crates/dojo/core-cairo-test/src/lib.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/helpers/event.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/helpers/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo(3 hunks)crates/dojo/core-cairo-test/src/tests/meta/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/model/model.cairo(4 hunks)crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/storage/layout.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/event.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/model.cairo(2 hunks)crates/dojo/core-cairo-test/src/tests/world/storage.cairo(1 hunks)crates/dojo/core/src/event/component.cairo(1 hunks)crates/dojo/core/src/lib.cairo(1 hunks)crates/dojo/core/src/meta/introspect.cairo(1 hunks)crates/dojo/core/src/meta/layout.cairo(1 hunks)crates/dojo/core/src/model/component.cairo(1 hunks)crates/dojo/core/src/model/definition.cairo(1 hunks)crates/dojo/core/src/model/interface.cairo(1 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/storage/dojo_store.cairo(1 hunks)crates/dojo/core/src/storage/layout.cairo(8 hunks)crates/dojo/core/src/world/world_contract.cairo(1 hunks)crates/dojo/lang/Cargo.toml(1 hunks)crates/dojo/lang/src/attribute_macros/element.rs(0 hunks)crates/dojo/lang/src/attribute_macros/event.rs(2 hunks)crates/dojo/lang/src/attribute_macros/mod.rs(0 hunks)crates/dojo/lang/src/attribute_macros/model.rs(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo(3 hunks)crates/dojo/lang/src/cairo_plugin.rs(2 hunks)crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs(1 hunks)crates/dojo/lang/src/derive_macros/introspect/layout.rs(3 hunks)crates/dojo/lang/src/derive_macros/introspect/mod.rs(9 hunks)crates/dojo/lang/src/derive_macros/introspect/utils.rs(2 hunks)crates/dojo/lang/src/derive_macros/mod.rs(1 hunks)crates/dojo/lang/src/lib.rs(2 hunks)crates/dojo/lang/src/utils.rs(1 hunks)crates/dojo/world/src/contracts/abigen/model.rs(2 hunks)crates/dojo/world/src/contracts/model.rs(2 hunks)crates/sozo/ops/src/model.rs(8 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/src/models.cairo(1 hunks)
💤 Files with no reviewable changes (2)
- crates/dojo/lang/src/attribute_macros/mod.rs
- crates/dojo/lang/src/attribute_macros/element.rs
🚧 Files skipped from review as they are similar to previous changes (40)
- crates/dojo/core/src/model/interface.cairo
- crates/dojo/core/src/model/definition.cairo
- crates/dojo/core/src/lib.cairo
- crates/dojo/core-cairo-test/src/tests/world/storage.cairo
- crates/dojo/core/src/model/component.cairo
- crates/dojo/core-cairo-test/src/tests/world/event.cairo
- crates/dojo/core/src/storage/layout.cairo
- examples/spawn-and-move/src/models.cairo
- crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo
- crates/dojo/core-cairo-test/src/tests/expanded/selector_attack.cairo
- crates/dojo/core/src/event/component.cairo
- crates/dojo/world/src/contracts/model.rs
- crates/dojo/core/src/world/world_contract.cairo
- crates/dojo/core/src/model/model_value.cairo
- crates/dojo/core-cairo-test/src/lib.cairo
- crates/dojo/core/src/meta/introspect.cairo
- crates/dojo/lang/Cargo.toml
- crates/dojo/lang/src/derive_macros/introspect/layout.rs
- crates/dojo/lang/src/lib.rs
- crates/dojo/core/src/meta/layout.cairo
- crates/dojo/world/src/contracts/abigen/model.rs
- examples/spawn-and-move/dojo_dev.toml
- crates/dojo/core-cairo-test/src/tests/world/model.cairo
- crates/dojo/core-cairo-test/src/tests/helpers/helpers.cairo
- crates/dojo/lang/src/derive_macros/introspect/utils.rs
- crates/dojo/lang/src/derive_macros/mod.rs
- crates/dojo/core-cairo-test/src/tests/helpers/model.cairo
- crates/dojo/lang/src/cairo_plugin.rs
- crates/dojo/lang/src/attribute_macros/model.rs
- Cargo.toml
- crates/dojo/lang/src/attribute_macros/patches/model.patch.cairo
- crates/dojo/lang/src/attribute_macros/event.rs
- crates/dojo/core-cairo-test/src/tests/helpers/event.cairo
- crates/benches/contracts/src/models/character.cairo
- crates/dojo/core-cairo-test/src/tests/meta/layout.cairo
- crates/dojo/core-cairo-test/src/tests/storage/layout.cairo
- crates/dojo/lang/src/derive_macros/introspect/dojo_store.rs
- crates/dojo/core-cairo-test/src/tests/model/model.cairo
- crates/sozo/ops/src/model.rs
- crates/dojo/core/src/storage/dojo_store.cairo
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (7)
crates/dojo/core/src/model/model.cairo (1)
53-55: Clarifydeserializecontract – expects keys + values or values only?
from_serializedconcatenateskeysandvaluesbefore passing the slice
toModelParser::deserialize. If an implementation ofModelParserassumes it
receives values only, subtle decoding bugs will surface.Please confirm in the derive macro that
deserializeis implemented with the
same concatenation logic, or split the parameters explicitly.crates/dojo/core-cairo-test/src/tests/meta/introspect.cairo (3)
5-110: Ohayo sensei! Good work implementing theDefaulttrait for all structs and enums!You've consistently added
#[derive(Default)]and#[default]annotations to ensure that all types have proper default values. This change is crucial for the newDojoStoretrait to handle uninitialized storage values correctly.
112-126: Nice implementation of generic types with proper trait derivations!I like how you've renamed
Generic<T>toGenericStruct<T>for clarity and added the newGenericEnum<T>type. Both derive the necessary traits for proper serialization and introspection.
140-157: Critical change to enum variant indexing - excellent implementation!The shift from zero-based to one-based indexing for enum variants (with the comment explaining why) is the key fix for the storage issue with enums and options.
// in the new Dojo storage layout, variants start from 1. let variant = i + 1;This ensures that variant ID 0 can be used to represent uninitialized storage, which solves the core problem mentioned in the PR description.
crates/dojo/core-cairo-test/src/tests/storage/dojo_store.cairo (3)
3-53: Well-structured test types with appropriate trait derivations!You've created a good variety of test types with different structures and complexities, all deriving the necessary traits for testing the
DojoStorefunctionality. The mix of primitive types, complex nested structures, and generics provides a comprehensive test suite.
248-252: Great testing of Option serialization with variant indices!Ohayo sensei! Your testing shows how the
DojoStoretrait representsOption::Somewith index 1 andOption::Nonewith index 2, implementing the variant shift mentioned in the PR objectives.
388-442: Excellent comprehensive test for complex nested types!The
test_mixfunction thoroughly tests serialization and deserialization of deeply nested data structures with multiple types. This is a great way to ensure theDojoStoretrait works correctly with real-world complex data.
| let mut parsing_keys = true; | ||
|
|
||
| members | ||
| .iter() | ||
| .map(|member_ast| { | ||
| let is_key = member_ast.has_attr(db, "key"); | ||
|
|
||
| let member = Member { | ||
| name: member_ast.name(db).text(db).to_string(), | ||
| ty: member_ast | ||
| .type_clause(db) | ||
| .ty(db) | ||
| .as_syntax_node() | ||
| .get_text(db) | ||
| .trim() | ||
| .to_string(), | ||
| key: is_key, | ||
| }; | ||
|
|
||
| // Make sure all keys are before values in the model. | ||
| if is_key && !parsing_keys { | ||
| diagnostics.push(PluginDiagnostic { | ||
| message: "Key members must be defined before non-key members.".into(), | ||
| stable_ptr: member_ast.name(db).stable_ptr().untyped(), | ||
| severity: Severity::Error, | ||
| }); | ||
| // Don't return here, since we don't want to stop processing the members after the | ||
| // first error to avoid diagnostics just because the field is | ||
| // missing. | ||
| } | ||
|
|
||
| parsing_keys &= is_key; | ||
|
|
||
| member | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Possible missing validation for duplicated #[key] fields
parse_members enforces key-before-value ordering, but it does not check:
- That at least one
#[key]exists. - That no two members are marked as the same key (e.g. duplicate names).
If an end-user mistakenly tags multiple fields with #[key] the generated
layout and entity_id computation become ambiguous.
Consider adding:
• A counter to ensure ≥ 1 and flag duplicates.
• Diagnostics with Severity::Error for violations.
🤖 Prompt for AI Agents (early access)
In crates/dojo/lang/src/utils.rs between lines 54 and 90, the parse_members function currently checks key-before-value ordering but does not validate that at least one member is marked as a key or that there are no duplicate key members. To fix this, add a counter or a set to track members marked with #[key], verify that at least one key exists after processing all members, and check for duplicates by comparing member names or identifiers. For any violations, push diagnostics with Severity::Error to inform the user about missing or duplicate key fields.
| fn build_generic_impls( | ||
| gen_types: &[String], | ||
| base_impls: &[String], | ||
| additional_impls: &[String], | ||
| ) -> String { | ||
| let mut gen_impls = gen_types | ||
| .iter() | ||
| .map(|g| format!("{g}, impl {g}Introspect: dojo::meta::introspect::Introspect<{g}>")) | ||
| .collect::<Vec<_>>() | ||
| .join(", "); | ||
| .map(|g| { | ||
| format!( | ||
| "{g}, {base_impls}", | ||
| base_impls = base_impls.iter().map(|i| format!("{i}<{g}>")).join(", ") | ||
| ) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| if !gen_types.is_empty() { | ||
| gen_impls.extend(additional_impls.to_vec()); | ||
| } | ||
|
|
||
| (generic_types, generic_impls) | ||
| gen_impls.join(", ") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Generated generic impl list duplicates type identifiers
build_generic_impls currently generates
"T, +DojoStore<T>, U, +DojoStore<U>" for <T, U>, mixing types and trait
bounds in a single comma-separated list. Cairo expects either:
• T, U in the type list, and
• +DojoStore<T>, +DojoStore<U> in the impl list,
but not interleaved.
The interleaving compiles only by accident and becomes unreadable with several
bounds.
Consider splitting return values:
struct GenericParts {
types: String, // "T, U"
impls: String, // "+DojoStore<T>, +DojoStore<U>"
}and interpolating them into the template separately.
| let gen_types = build_generic_types(db, enum_ast.generic_params(db)); | ||
| let gen_joined_types = gen_types.join(", "); | ||
|
|
||
| let enum_name_with_generics = format!("{enum_name}<{gen_joined_types}>"); | ||
|
|
There was a problem hiding this comment.
<> emitted for non-generic enums ⇒ invalid Cairo syntax
When gen_types.is_empty(), gen_joined_types is "", so
enum_name_with_generics becomes "MyEnum<>", which does not parse.
-let enum_name_with_generics = format!("{enum_name}<{gen_joined_types}>");
+let enum_name_with_generics = if gen_types.is_empty() {
+ enum_name.clone()
+} else {
+ format!("{enum_name}<{gen_joined_types}>")
+};Failing to guard here will break every non-generic enum/model processed by the
macro.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let gen_types = build_generic_types(db, enum_ast.generic_params(db)); | |
| let gen_joined_types = gen_types.join(", "); | |
| let enum_name_with_generics = format!("{enum_name}<{gen_joined_types}>"); | |
| let gen_types = build_generic_types(db, enum_ast.generic_params(db)); | |
| let gen_joined_types = gen_types.join(", "); | |
| let enum_name_with_generics = if gen_types.is_empty() { | |
| enum_name.clone() | |
| } else { | |
| format!("{enum_name}<{gen_joined_types}>") | |
| }; |
🤖 Prompt for AI Agents (early access)
In crates/dojo/lang/src/derive_macros/introspect/mod.rs around lines 82 to 86, the code always appends angle brackets to the enum name even when there are no generic types, resulting in invalid syntax like "MyEnum<>". To fix this, add a conditional check to only append "<{gen_joined_types}>" when gen_types is not empty; otherwise, use just the enum name without angle brackets.
|
Should target |
|
Closed because manually recreated for 1.6.0: #3219 |
Description
In the current Dojo version,
Option<T>and custom enums are not correctly stored in the world storage, because the first variant id is0and cannot be differenciated from an uninitialized variant.To fix that, this PR introduces the
DojoStoretrait.DojoStoretrait which, by default, encapsulatesSerdecalls.DojoStoretrait for custom enums andOption<T>handles the variant index incrementing and returnsDefault::default()as default value during deserialization if the variant index is 0. That means, each custom enums must implement theDefaulttrait.As these changes may break the world storage, we also introduce the
DojoLegacyStoragederive attribute. This attribute must be used on models for which we want to keep the current storage behavior. So, by default, model (de)serialization is done throughDojoStore. IfDojoLegacyStorageis set, model (de)serialization is done throughSerde.Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
DojoStore) for efficient data handling in world storage.Improvements
Bug Fixes
Tests
Chores