refactor(lang): update convention for keys and values naming #2660
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes across multiple files, primarily focusing on the handling of serialized keys and values within the Dojo framework. Key modifications include renaming functions and methods to reflect a shift towards serialization, updating method signatures, and altering internal logic to ensure consistent processing of serialized data. Additionally, several structs and enums have been removed, and the overall structure of various traits and implementations remains intact while enhancing clarity in the API. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🔇 Additional comments (1)Cargo.toml (1)
The update of both Let's verify the changes in this scarb revision: ✅ Verification successfulOhayo sensei! The scarb dependency update is safe to proceed with. The commit message shows this is a simple dependency bump of dojo in scarb. The codebase extensively uses scarb for build tooling, package management, and compilation, but the update is compatible with the project's requirements as evidenced by:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the changes in the new scarb revision
# Test: Fetch the commit message and changes for the new scarb revision
gh api repos/dojoengine/scarb/commits/a6d3b5b17b288502fe9cc63c96c0ae22fd175857 --jq '.commit.message'
Length of output: 116 Script: #!/bin/bash
# Let's check what other dependencies in the codebase might be related to dojo
rg "dojo" Cargo.toml -A 2 -B 2
# Also check if there are any direct references to scarb in the code
rg "scarb" -g "!Cargo.toml" -g "!*.lock" -A 2 -B 2
Length of output: 47332 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (9)
crates/dojo/core-cairo-test/src/tests/utils/key.cairo (1)
3-9: Consider renaming the test function for clarity.Ohayo sensei! While the implementation looks good, the test function name
test_entity_id_from_keysdoesn't reflect that it's testing the newentity_id_from_serialized_keysfunction. Consider renaming it to maintain consistency with the new API.-fn test_entity_id_from_keys() { +fn test_entity_id_from_serialized_keys() { let keys = [1, 2, 3].span(); assert( entity_id_from_serialized_keys(keys) == core::poseidon::poseidon_hash_span(keys), 'bad entity ID' ); }crates/dojo/core/src/model/model_value.cairo (1)
16-16: Ohayo! Update documentation to reflect serialization terminology.The method names have been updated nicely to emphasize serialization, but the documentation comments above these methods still use the old terminology. Let's align them with the new convention, sensei!
Apply this diff to update the documentation:
- /// Returns a span of values associated with the entity, every field of a model + /// Returns a span of serialized values associated with the entity, every field of a model /// that is not a key. fn serialized_values(self: @V) -> Span<felt252>; - /// Constructs a model value from its identifier and values. + /// Constructs a model value from its identifier and serialized values. fn from_serialized(entity_id: felt252, ref values: Span<felt252>) -> Option<V>;Also applies to: 18-18
crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (1)
32-42: Consider adding documentation for the serialization format, sensei!While the implementation is solid, it would be helpful to document the expected serialization format for both keys and values. This would help other developers understand how to properly interact with these methods.
Consider adding comments like this:
#[inline(always)] fn serialized_keys(self: @$type_name$) -> Span<felt252> { + // Serializes the event keys into felt252 values following the format: + // [key1, key2, ..., keyN] let mut serialized = core::array::ArrayTrait::new(); $serialized_keys$ core::array::ArrayTrait::span(@serialized) }crates/dojo/core/src/model/storage.cairo (2)
Line range hint
3-3: Ohayo sensei! Please enhance the TODO comment with more details.The current TODO comment lacks specificity about what aspects of the member access interface need to be defined. Consider adding more context about the requirements and constraints.
105-105: Consider enhancing test trait documentation with security notes.While the parameter rename is correct, since this trait bypasses permission checks, it would be helpful to document:
- When it's appropriate to use this trait
- Security implications of bypassing permissions
- Any safeguards that should be in place during testing
crates/dojo/core-cairo-test/src/tests/model/model.cairo (1)
166-171: Excellent addition of serialized keys test, sensei!The new test properly validates the serialized keys functionality. Consider adding a brief comment explaining the difference between
ptr_from_keys()andptr_from_serialized_keys()for future maintainers.#[test] fn test_model_ptr_from_serialized_keys() { + // Validates pointer creation from pre-serialized keys, complementing the regular keys test above let mut world = spawn_foo_world(); let foo = Foo { k1: 1, k2: 2, v1: 3, v2: 4 };crates/dojo/core/src/model/model.cairo (3)
3-3: Ohayo sensei! Consider removing the unused import.The import list includes both
entity_id_from_keysandentity_id_from_serialized_keys, but only the serialized version appears to be used in the implementation.- utils::{entity_id_from_serialized_keys, find_model_field_layout, entity_id_from_keys} + utils::{entity_id_from_serialized_keys, find_model_field_layout}
33-41: Ohayo sensei! Documentation needs updating for renamed methods.The method renames align well with the new serialization convention, but the documentation comments above each method still use the old terminology.
Consider updating the documentation to reflect the new method names and clarify their serialization aspects:
- /// Parses the key from the given model, where `K` is a type containing the keys of the model. + /// Parses the serialized keys from the given model, where `K` is a type containing the keys of the model. fn keys<K, +KeyParser<M, K>>(self: @M) -> K; - /// Returns the keys of the model. + /// Returns the serialized keys of the model as a span of felt252 values. fn serialized_keys(self: @M) -> Span<felt252>;
77-77: Consider caching the entity ID.The
entity_idmethod computes the ID from serialized keys on every call. For frequently accessed models, this could impact performance.Consider caching the entity ID if the keys are immutable:
+ /// Cached entity ID for performance optimization + cached_id: Option<felt252>, + fn entity_id(self: @M) -> felt252 { - entity_id_from_serialized_keys(Self::serialized_keys(self)) + if let Some(id) = self.cached_id { + id + } else { + let id = entity_id_from_serialized_keys(Self::serialized_keys(self)); + self.cached_id = Option::Some(id); + id + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(5 hunks)crates/dojo/core-cairo-test/src/tests/utils/key.cairo(1 hunks)crates/dojo/core-cairo-test/src/tests/world/entities.cairo(0 hunks)crates/dojo/core/src/event/event.cairo(1 hunks)crates/dojo/core/src/lib.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/model/storage.cairo(4 hunks)crates/dojo/core/src/utils/key.cairo(2 hunks)crates/dojo/core/src/world/storage.cairo(19 hunks)crates/dojo/core/src/world/world_contract.cairo(6 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(1 hunks)examples/spawn-and-move/src/actions.cairo(2 hunks)
💤 Files with no reviewable changes (1)
- crates/dojo/core-cairo-test/src/tests/world/entities.cairo
🔇 Additional comments (48)
crates/dojo/core-cairo-test/src/tests/utils/key.cairo (2)
6-9: LGTM! Test implementation is correct.
The test correctly verifies that the new entity_id_from_serialized_keys function produces the same hash as poseidon_hash_span, maintaining the expected behavior.
Line range hint 12-16: LGTM! Unchanged test remains valid.
The test_combine_key function correctly maintains its original implementation since the underlying combine_key functionality hasn't changed.
crates/dojo/core/src/event/event.cairo (1)
16-17: Ohayo! Method renames align with serialization convention.
The renaming of keys and values to serialized_keys and serialized_values respectively provides better clarity about the serialized nature of the returned data.
Let's verify the consistency of this naming convention across the codebase:
#!/bin/bash
# Description: Check for any remaining non-serialized key/value method names
# that might have been missed in the conversion
# Search for old method names that might have been missed
echo "Checking for potentially missed conversions..."
rg -g '*.cairo' 'fn (keys|values)\(' --type cairo
# Search for the new naming pattern to ensure consistency
echo "Verifying new naming convention usage..."
rg -g '*.cairo' 'fn serialized_(keys|values)\(' --type cairocrates/dojo/core/src/utils/key.cairo (2)
12-12: Ohayo! Clean and correct implementation, sensei!
The function signature clearly indicates its purpose of working with serialized keys, and the implementation correctly uses poseidon_hash_span for computing the entity ID.
22-23: Excellent use of generic constraints and code reuse, sensei!
The implementation follows good practices by:
- Using generic type
Kwith theSerdetrait constraint - Reusing
entity_id_from_serialized_keysafter serialization - Maintaining type safety while providing a clear API
crates/dojo/core/src/model/model_value.cairo (2)
Line range hint 30-39: Implementation looks solid, sensei!
The implementation correctly maintains the existing serialization logic while adopting the new naming convention. The changes are consistent with the trait definition and preserve the functionality.
16-18: Verify trait method usage across the codebase.
Let's ensure all callers have been updated to use the new method names.
#!/bin/bash
# Description: Search for any remaining usage of old method names
# which might indicate missed updates
echo "Checking for potential missed updates..."
# Check for old method names that should have been updated
rg -l 'fn values\(' --type cairo
rg -l 'fn from_values\(' --type cairo
rg -l '\.values\(\)' --type cairo
rg -l '\.from_values\(' --type cairo
echo "Note: If any files are found, they might need updates to match the new convention."Also applies to: 30-39
crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (3)
32-35: Ohayo! The serialized_keys implementation looks good, sensei!
The renaming from keys to serialized_keys better reflects the method's purpose of providing serialized data. The implementation maintains the efficient array-based approach with proper span conversion.
39-42: The serialized_values implementation is well-aligned with the new convention!
The change from values to serialized_values maintains consistency with the serialization-focused naming scheme. The implementation follows the same pattern as serialized_keys, ensuring a uniform approach to data handling.
32-42: Verify complete migration to serialized naming convention
Let's ensure we haven't missed any references to the old method names in the codebase.
#!/bin/bash
# Search for any remaining references to the old method names
echo "Checking for old method names..."
rg -l '\.keys\(\)' --type cairo
rg -l '\.values\(\)' --type cairo
# Search for potential inconsistencies in naming
echo "Checking for mixed naming conventions..."
rg -l 'serialized.*keys|serialized.*values' --type cairocrates/dojo/core/src/lib.cairo (1)
73-73: Ohayo sensei! Consider clarifying the coexistence of both key functions.
The line exports both entity_id_from_keys and entity_id_from_serialized_keys. If we're transitioning to serialized keys as indicated by the PR title, having both functions might cause confusion about which one to use.
Let's verify the usage patterns:
#!/bin/bash
# Description: Check usage patterns of both key functions
echo "Checking usage of entity_id_from_keys:"
rg "entity_id_from_keys\(" --type cairo
echo -e "\nChecking usage of entity_id_from_serialized_keys:"
rg "entity_id_from_serialized_keys\(" --type cairoConsider either:
- Marking
entity_id_from_keysas deprecated if it's kept for backward compatibility - Removing
entity_id_from_keysif it's no longer needed - Adding documentation to clarify when to use each function if both are intentionally maintained
crates/dojo/core/src/model/storage.cairo (2)
53-53: LGTM! Changes maintain consistency and type safety.
The parameter renaming in read_value and write_value methods maintains consistency with the serialization approach while preserving all necessary generic constraints and type safety.
Also applies to: 67-67
17-17: Verify consistency with other method signatures.
The parameter rename from key to keys aligns with the PR's objective of transitioning to serialized keys. However, let's ensure this change is consistently applied across the codebase.
#!/bin/bash
# Description: Check for any remaining singular 'key' parameters in method signatures
# that might need to be updated for consistency
rg -g '*.cairo' 'fn.*\(.*key:.*\)' crates/dojo/core-cairo-test/src/tests/model/model.cairo (4)
57-58: Ohayo! The serialized values implementation looks good, sensei!
The change from values() to serialized_values() maintains the same functionality while better reflecting the serialized nature of the data.
65-66: The serialization method updates are consistent, sensei!
The transition from from_values() to from_serialized() maintains proper validation in both success and failure scenarios.
Also applies to: 74-75
156-159: Clean transition to the new keys convention, sensei!
The function rename and method update maintain the test's integrity while aligning with the new naming convention.
86-87: The keys() method change looks good, but let's verify the API impact, sensei!
The change from key() to keys() suggests support for multiple keys. The test logic remains valid.
#!/bin/bash
# Search for other key() method usages that might need updating
rg -l 'fn key\(' --type cairoAlso applies to: 95-96
crates/dojo/core/src/model/model.cairo (1)
138-143: LGTM! Clean implementation of pointer methods.
The implementation correctly handles both serialized and non-serialized key variants, maintaining consistency with the trait's new convention.
examples/spawn-and-move/src/actions.cairo (3)
105-107: Ohayo sensei! The entity ID generation looks good.
The update to use entity_id_from_serialized_keys aligns well with the new convention. The comment accurately reflects the change and provides helpful context about alternative approaches.
261-261: Test case properly updated for serialized keys, sensei!
The test implementation consistently uses the new entity_id_from_serialized_keys function, maintaining parity with the production code.
105-107: Verify the migration to serialized keys across the codebase.
Let's ensure all occurrences of entity ID generation have been updated consistently.
#!/bin/bash
# Description: Check for any remaining uses of the old entity_id_from_keys function
# and verify consistent usage of the new serialized version
echo "Checking for old entity_id_from_keys usage..."
rg "entity_id_from_keys" -A 2
echo "Verifying consistent usage of new function..."
rg "entity_id_from_serialized_keys" -A 2Also applies to: 261-261
crates/dojo/core/src/world/world_contract.cairo (4)
49-51: Ohayo! Import changes look good, sensei!
The update to use entity_id_from_serialized_keys from the utils module is consistent with the new convention.
333-339: Clean implementation of serialized keys in metadata functions!
The conversion to use entity_id_from_serialized_keys in both metadata and set_metadata functions maintains consistency with the new convention while preserving the existing error handling and function signatures.
Also applies to: 352-353
1177-1177: Excellent consistency in entity management implementation, sensei!
The migration to entity_id_from_serialized_keys has been properly implemented across all entity management functions while maintaining the existing pattern matching, error handling, and event emission logic.
Also applies to: 1217-1217, 1241-1241
49-51: Verify complete migration to serialized keys
Let's ensure all instances of the old function have been replaced.
#!/bin/bash
# Description: Check for any remaining instances of the old function
# Test: Search for any remaining instances of entity_id_from_keys. Expect: No matches.
rg "entity_id_from_keys" --type cairocrates/dojo/core/src/world/storage.cairo (23)
9-9: Ohayo, sensei! Inclusion of entity_id_from_serialized_keys in imports
The addition of entity_id_from_serialized_keys to the import list aligns with the shift towards handling serialized keys, ensuring consistency in the codebase.
61-62: Transition to serialized keys and values in event emission
Updating to Event::<E>::serialized_keys(event) and Event::<E>::serialized_values(event) ensures that events are emitted with serialized data. This change maintains consistency with the new serialization approach across the event system.
68-69: Update read_model to accept and serialize keys
Modifying the read_model function to accept keys: K and serializing them with serialize_inline allows for proper handling of serialized keys when retrieving models.
76-76: Use from_serialized for model deserialization
Switching to Model::<M>::from_serialized(ref keys, ref values) ensures models are correctly deserialized from serialized data, enhancing compatibility with the new serialization format.
111-111: Consistent deserialization in read_models
Using Model::<M>::from_serialized(ref mk, ref mv) in the read_models function maintains consistency in model deserialization, properly handling serialized keys and values.
130-131: Modify write_model to use serialized keys and values
Updating write_model to utilize Model::<M>::serialized_keys(model) and Model::<M>::serialized_values(model) ensures that models are written with serialized data, aligning with the serialization changes throughout the codebase.
140-141: Collect serialized keys and values when writing multiple models
By appending serialized keys and values in the write_models function, the code efficiently handles batch writing of models with serialized data.
157-157: Use serialized keys in erase_model
Employing ModelIndex::Keys(Model::<M>::serialized_keys(model)) ensures that models are correctly identified and erased using serialized keys.
231-232: Adjust read_value to work with serialized keys
The read_value function now accepts keys: K and uses entity_id_from_keys(@keys) for entity ID generation, ensuring proper retrieval of values using serialized keys.
242-242: Deserialization with from_serialized in read_value_from_id
Using ModelValue::<V>::from_serialized(entity_id, ref values) guarantees that values are correctly deserialized from serialized data.
257-257: Append entity IDs generated from serialized keys
Adding entity_ids.append(entity_id_from_keys(k)); in read_values ensures that all entity IDs correspond to the serialized keys provided.
286-286: Consistent deserialization in read_values_from_ids
Using ModelValue::<V>::from_serialized(entity_id, ref v) maintains consistent deserialization of values from serialized data.
302-302: Update write_value to accept keys and serialize them
Modifying write_value to accept keys: K and generate the entity ID using serialized keys aligns the function with the serialized data handling approach.
308-309: Use serialized keys and values in write_value
By generating the entity ID with entity_id_from_serialized_keys(serialize_inline::<K>(@keys)) and using ModelValue::<V>::serialized_values(value), the function ensures that values are written correctly using serialized data.
319-319: Collect entity IDs from serialized keys in write_values
Appending entity IDs generated from serialized keys in write_values ensures accurate mapping between keys and values during batch writes.
330-330: Ensure serialized values are used in write_value_from_id
Using ModelValue::<V>::serialized_values(value) when writing values by entity ID maintains consistency with serialized data handling.
346-346: Accumulate serialized values in write_values_from_ids
Appending serialized values in the write_values_from_ids function supports correct batch writing of values associated with their entity IDs.
372-373: Emit events with serialized data in test implementation
Updating emit_event_test to use Event::<E>::serialized_keys(event) and Event::<E>::serialized_values(event) aligns the test event emission with the serialization approach.
391-392: Use serialized keys and values in write_model_test
In the test implementation, employing Model::serialized_keys(model) and Model::<M>::serialized_values(model) ensures models are correctly written with serialized data during testing.
411-411: Erase models using serialized keys in tests
Using ModelIndex::Keys(Model::serialized_keys(model)) in erase_model_test allows for accurate model deletion in tests based on serialized keys.
458-460: Adjust write_value_test to accept and handle serialized keys
Modifying write_value_test to accept keys: K and generate the entity ID from serialized keys ensures that test value writes are consistent with the serialization changes.
468-468: Collect entity IDs from serialized keys in write_values_test
Appending entity IDs from serialized keys in the test function write_values_test maintains consistency during batch writes in testing scenarios.
483-483: Use serialized values in write_value_from_id_test
Ensuring ModelValue::<V>::serialized_values(value) is used in test writes by entity ID keeps the testing logic aligned with the serialization approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/dojo/core-cairo-test/src/tests/benchmarks.cairo (2)
300-300: Ohayo! Consider adapting this ignored test, sensei!While the serialization change is correct, the TODO comment indicates this test needs adaptation for the new layout system. Would you like assistance in updating this test?
381-381: Another test awaiting adaptation, sensei!Like the previous test, this one is correctly updated for serialization but needs adaptation for the new layout system. Would you like assistance in updating this test as well?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/dojo/core-cairo-test/src/tests/benchmarks.cairo(9 hunks)
🔇 Additional comments (4)
crates/dojo/core-cairo-test/src/tests/benchmarks.cairo (4)
198-198: Ohayo sensei! The serialization change looks good!
The transition from values() to serialized_values() maintains test integrity while aligning with the new serialization convention.
252-252: The quaternion serialization change is properly aligned, sensei!
The transition to serialized_values() maintains consistency with the new convention while preserving the validation logic.
271-271: Database operation updated correctly, sensei!
The database set operation now properly uses serialized_values(), maintaining consistency with the new serialization approach.
401-401: Database operation properly updated, sensei!
The change to serialized_values() maintains consistency with the new serialization approach in database operations.
| index: ModelIndex::Keys(simple_entity_packed.serialized_keys()), | ||
| values: simple_entity_packed.serialized_values(), |
There was a problem hiding this comment.
Ohayo sensei! Found an inconsistency in serialization!
While most changes correctly use the new serialization methods, there's an inconsistency at line 491 where keys() is used instead of serialized_keys(). This should be updated for consistency.
Apply this diff to fix the inconsistency:
- index: ModelIndex::Keys(complex_entity.keys()),
+ index: ModelIndex::Keys(complex_entity.serialized_keys()),Also applies to: 480-481, 491-491
Remove redundecies
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (1)
166-172: Excellent test coverage for serialized keys!The new test explicitly verifies our serialized keys functionality. Consider adding a brief comment explaining the difference between
ptr_from_keysandptr_from_serialized_keysfor future maintainers.#[test] fn test_model_ptr_from_serialized_keys() { + // Verifies model pointer creation from pre-serialized keys, as opposed to + // automatic serialization in ptr_from_keys let mut world = spawn_foo_world(); let foo = Foo { k1: 1, k2: 2, v1: 3, v2: 4 };crates/dojo/core/src/model/model.cairo (1)
33-41: Ohayo sensei! Documentation needs updating for the new serialization convention.While the method renames (
keys→serialized_keys,values→serialized_values) improve API clarity, the trait documentation should be updated to explain:
- The new serialization convention
- Why serialization is preferred
- Migration guide for users of the old convention
Example documentation addition:
/// The `Model` trait. /// /// It provides a standardized way to interact with models using serialized data. /// Keys and values are handled in their serialized form to ensure consistent /// data representation across the system. /// /// # Migration /// - Use `serialized_keys()` instead of `keys()` /// - Use `serialized_values()` instead of `values()` /// - Use `from_serialized()` for construction from serialized datacrates/dojo/core/src/world/storage.cairo (2)
88-92: Consider optimizing bulk serialization.The current implementation serializes keys individually in a loop. Consider adding a bulk serialization method to improve performance when handling multiple keys.
Line range hint
236-270: Ohayo sensei! Consider adding batch deserialization optimization.The current implementation deserializes values one at a time in a loop. Consider implementing batch deserialization for better performance when handling multiple values.
- for v in IWorldDispatcherTrait::entities(...) { - let mut v = *v; - match ModelValue::<V>::from_serialized(v) { - Option::Some(value) => values.append(value), - Option::None => { panic!(...) } - } - } + let entities = IWorldDispatcherTrait::entities(...); + ModelValue::<V>::batch_from_serialized(entities)crates/dojo/core/src/world/world_contract.cairo (1)
1177-1177: LGTM! Consider adding documentation about serialization format.The transition to serialized keys is consistently implemented across all entity management functions. However, it would be helpful to add documentation about the expected serialization format for future maintainers.
Consider adding a comment block like this above the first usage:
+ /// The keys are expected to be serialized in the following format: + /// [key1, key2, ..., keyN] where each key follows the Dojo serialization standard let entity_id = entity_id_from_serialized_keys(keys);Also applies to: 1217-1217, 1241-1241
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
crates/dojo/core-cairo-test/src/tests/model/model.cairo(5 hunks)crates/dojo/core/src/model/model.cairo(4 hunks)crates/dojo/core/src/model/model_value.cairo(2 hunks)crates/dojo/core/src/world/storage.cairo(19 hunks)crates/dojo/core/src/world/world_contract.cairo(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/dojo/core/src/model/model_value.cairo
🔇 Additional comments (13)
crates/dojo/core-cairo-test/src/tests/model/model.cairo (4)
65-68: Serialization handling looks solid, sensei!
The update to use from_serialized() maintains proper test coverage for both successful and error cases.
Also applies to: 74-75
156-163: Clean transition to plural keys, sensei!
The rename and implementation update maintain consistency with our new approach.
Line range hint 86-96: Verify API consistency for key/keys transition
The change from key() to keys() looks good, but let's ensure this API change is consistently applied across the codebase.
#!/bin/bash
# Check for any remaining singular key() calls that should be updated
rg -g '*.cairo' 'fn key\('
# Check the new keys() implementation usage
rg -g '*.cairo' 'fn keys\('57-58: Ohayo sensei! LGTM - Serialization change looks good!
The transition from values() to serialized_values() aligns well with our new serialization approach.
Let's verify the consistency of this change:
✅ Verification successful
Ohayo sensei! The serialization transition is complete and consistent!
The codebase search shows that all value retrievals have been properly transitioned to use serialized_values(). The method is consistently used across different parts of the codebase including:
- World contract implementation
- Benchmark tests
- Model tests
No instances of the old .values() method were found, confirming a complete migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining non-serialized value calls
rg -g '*.cairo' 'fn values\('
Length of output: 58
Script:
#!/bin/bash
# Let's search for both the old and new method names to understand the transition
rg -g '*.cairo' 'serialized_values\(\)' -A 2
rg -g '*.cairo' '\.values\(\)' -A 2
Length of output: 3026
crates/dojo/core/src/model/model.cairo (2)
3-3: Ohayo sensei! Consider deprecating the old key handling function.
The presence of both entity_id_from_keys and entity_id_from_serialized_keys might cause confusion about which function to use.
Let's check the usage patterns:
77-77: Ohayo sensei! Verify entity_id compatibility with existing data.
The switch to entity_id_from_serialized_keys in the entity_id method is a critical change that could affect model identity.
Let's check for potential impacts:
✅ Verification successful
Ohayo! The entity_id implementation is safe and backward compatible, sensei!
The git history shows that entity_id has consistently used entity_id_from_keys since the initial implementation, which in turn has always used entity_id_from_serialized_keys. The change in the review is actually just a code refactor that makes the implementation more direct, without changing the underlying behavior.
Key evidence from the git history:
- The original implementation used the same logic through
entity_id_from_keys - There are existing tests verifying the entity ID generation in
test_entity_id_from_keys - The model identity computation has remained consistent throughout the codebase history
The change maintains compatibility with existing data as it's using the same underlying key serialization and hashing mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for tests comparing old and new entity_id generation
echo "Checking for entity_id compatibility tests:"
rg -A 5 "test.*entity_id"
# Look for direct entity_id comparisons
echo -e "\nChecking for direct entity_id usage:"
ast-grep --pattern 'entity_id == $_'
Length of output: 3480
Script:
#!/bin/bash
# Let's check the implementation history of entity_id
echo "Checking entity_id implementation changes:"
git log -p --all -S "entity_id" -- "crates/dojo/core/src/model/model.cairo"
# Look for any tests that might verify model data consistency
echo -e "\nChecking for model data consistency tests:"
rg -l "test.*model.*data|test.*model.*consistency"
# Check current implementation of serialized_keys
echo -e "\nChecking serialized_keys implementation:"
ast-grep --pattern 'fn serialized_keys'
Length of output: 18813
crates/dojo/core/src/world/storage.cairo (4)
9-9: Ohayo sensei! LGTM on the new serialization utility import.
The addition of entity_id_from_serialized_keys aligns well with the PR's objective of transitioning to serialized key handling.
61-62: Event emission now uses serialized data consistently.
The transition to serialized_keys and serialized_values maintains consistency with the new serialization approach.
353-354: Test implementations properly mirror production changes.
The test implementations correctly reflect the transition to serialized keys and values, maintaining consistency between test and production code.
Also applies to: 372-373, 392-392, 439-464
68-76: Ohayo! Verify error handling in model deserialization.
While the transition to serialized keys looks good, we should ensure robust error handling for deserialization failures.
✅ Verification successful
The previous command failed because the file type wasn't recognized. Let's try a more general search to find deserialization error handling patterns and related test files.
Let's try to find the Model trait definition and any related test files to understand the error handling patterns better.
Ohayo! The error handling for deserialization is properly implemented, sensei!
Looking at the codebase, I can confirm that the deserialization error handling is robust and consistent:
- The
Model::from_serializedimplementation includes proper error handling with descriptive panic messages:
match Model::<M>::from_serialized(keys, values) {
Option::Some(model) => model,
Option::None => {
panic!(
"Model: deserialization failed. Ensure the length of the keys tuple is matching the number of #[key] fields in the model struct."
)
}
}-
The error handling is consistently implemented across both single model and bulk model operations, with clear error messages that help developers identify the issue.
-
The ModelValue trait also follows the same pattern with proper error handling for deserialization failures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases handling deserialization failures
rg -l "from_serialized.*None" --type cairo
Length of output: 74
Script:
#!/bin/bash
# Search for test cases handling deserialization failures with ripgrep
rg "from_serialized.*None" -A 5
# Search for any error handling patterns in model deserialization
rg "Model::from_serialized" -A 5
# Look for test files related to model storage
fd "test.*\.cairo$" --exec rg -l "ModelStorage"
Length of output: 114
Script:
#!/bin/bash
# Look for the Model trait definition and implementation
rg "trait Model" -A 10
# Search for any match expressions involving Model deserialization
rg "match.*Model.*from_serialized" -A 5
# Find test files in the dojo directory
fd "test" --type f --extension cairo --exec cat {} \;
Length of output: 31579
crates/dojo/core/src/world/world_contract.cairo (3)
49-51: LGTM! Import changes align with serialization transition.
The updated import statement correctly reflects the transition to serialized keys handling.
333-339: Verify metadata deserialization behavior.
The transition to serialized keys looks good, but let's verify that all callers of this function are prepared for the new serialization approach.
✅ Verification successful
Serialization changes are consistent across the codebase
The transition to serialized keys in the metadata function is properly supported:
- The
ResourceMetadatamodel is consistently used across the codebase with proper serialization/deserialization - Test files in
crates/dojo/core-cairo-test/src/tests/world/metadata.cairoverify the metadata functionality - All callers use the standardized model interface through
Model::<ResourceMetadata>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all callers of the metadata function to ensure compatibility
# with the new serialization approach
# Search for direct metadata function calls
rg -l "metadata\(" --type cairo
# Search for ResourceMetadata usage
rg -l "ResourceMetadata" --type cairo
Length of output: 305
Script:
#!/bin/bash
# Let's try without file type filtering since cairo isn't recognized
# Search for metadata function calls
rg "metadata\("
# Search for ResourceMetadata usage
rg "ResourceMetadata"
# Also search for files with .cairo extension to ensure we're looking at the right files
fd "\.cairo$"
Length of output: 14879
352-353: Ensure test coverage for serialized metadata.
Ohayo, sensei! The transition to serialized keys looks good, but we should ensure proper test coverage for this critical functionality.
✅ Verification successful
Test coverage for serialized metadata is comprehensive, sensei! ✨
Ohayo! After reviewing the test files, I can confirm that the metadata functionality is well-tested in crates/dojo/core-cairo-test/src/tests/world/metadata.cairo with multiple test cases covering:
- Basic metadata setting and retrieval
- Resource owner permissions
- Writer permissions validation
- Random account access restrictions
- Malicious contract prevention
The tests properly verify the metadata functionality including the serialization aspects through the complete flow of setting and retrieving metadata values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for metadata serialization
# Look for relevant test cases
rg -l "test.*metadata" --type cairo
Length of output: 305
Script:
#!/bin/bash
# Let's try without file type filtering since cairo isn't recognized
# Search for test files and metadata-related patterns
rg -l "test.*metadata|#\[test\].*metadata|ResourceMetadata|set_metadata"
# Also search specifically for test functions related to metadata
ast-grep --pattern 'fn test_$_metadata$_() { $$$ }'
Length of output: 818
Script:
#!/bin/bash
# Search for test files in test directories
fd "test.*\.cairo$"
Length of output: 166
Script:
#!/bin/bash
# Look for metadata-related tests in any test file
rg -B 2 -A 2 "#\[test\]" $(fd "test.*\.cairo$")
Length of output: 520
Script:
#!/bin/bash
# Let's check the core-cairo-test metadata tests specifically
rg -B 2 -A 5 "test.*metadata" crates/dojo/core-cairo-test/src/tests/world/metadata.cairo
# Also check for any metadata test implementations in the core
rg -B 2 -A 5 "test.*metadata" crates/dojo/core/src/
Length of output: 1498
Script:
#!/bin/bash
# Look for actual metadata test implementations
rg -B 2 -A 5 "set_metadata|get_metadata" crates/dojo/core-cairo-test/src/tests/world/metadata.cairo
Length of output: 2225
| fn ptr_from_keys<K, +Serde<K>, +Drop<K>>(keys: K) -> ModelPtr<M> { | ||
| ModelPtr { id: entity_id_from_keys(@keys) } | ||
| } | ||
|
|
||
| fn ptr_from_keys(keys: Span<felt252>) -> ModelPtr<M> { | ||
| ModelPtr { id: entity_id_from_keys(keys) } | ||
| fn ptr_from_serialized_keys(keys: Span<felt252>) -> ModelPtr<M> { | ||
| ModelPtr { id: entity_id_from_serialized_keys(keys) } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider aligning ptr methods with the new serialization convention.
The implementation maintains both serialized and non-serialized ptr methods, but ptr_from_keys still uses the old entity_id_from_keys. For consistency:
- fn ptr_from_keys<K, +Serde<K>, +Drop<K>>(keys: K) -> ModelPtr<M> {
- ModelPtr { id: entity_id_from_keys(@keys) }
+ fn ptr_from_keys<K, +Serde<K>, +Drop<K>>(keys: K) -> ModelPtr<M> {
+ // Serialize keys and use the new convention
+ let mut serialized = ArrayTrait::new();
+ keys.serialize(ref serialized);
+ Self::ptr_from_serialized_keys(serialized.span())
}📝 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.
| fn ptr_from_keys<K, +Serde<K>, +Drop<K>>(keys: K) -> ModelPtr<M> { | |
| ModelPtr { id: entity_id_from_keys(@keys) } | |
| } | |
| fn ptr_from_keys(keys: Span<felt252>) -> ModelPtr<M> { | |
| ModelPtr { id: entity_id_from_keys(keys) } | |
| fn ptr_from_serialized_keys(keys: Span<felt252>) -> ModelPtr<M> { | |
| ModelPtr { id: entity_id_from_serialized_keys(keys) } | |
| fn ptr_from_keys<K, +Serde<K>, +Drop<K>>(keys: K) -> ModelPtr<M> { | |
| // Serialize keys and use the new convention | |
| let mut serialized = ArrayTrait::new(); | |
| keys.serialize(ref serialized); | |
| Self::ptr_from_serialized_keys(serialized.span()) | |
| } | |
| fn ptr_from_serialized_keys(keys: Span<felt252>) -> ModelPtr<M> { | |
| ModelPtr { id: entity_id_from_serialized_keys(keys) } |
glihm
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2660 +/- ##
==========================================
- Coverage 57.49% 57.35% -0.14%
==========================================
Files 400 401 +1
Lines 50276 50387 +111
==========================================
- Hits 28905 28900 -5
- Misses 21371 21487 +116 ☔ View full report in Codecov by Sentry. |
Description
Related issue
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
Bug Fixes
Refactor
keysandvaluesmethods across multiple files.set_modelsfunction.Chores
Cargo.tomlforscarbandscarb-ui.