refactor(lang): reduce events generated code #2662
Conversation
Remove redundecies
glihm
changed the title
WalkthroughOhayo, sensei! The changes in this pull request primarily involve renaming types and updating method signatures across several files in the Changes
Possibly related PRs
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (13)
crates/dojo/core/src/utils/key.cairo (1)
22-23: Excellent generic implementation, sensei!The new generic implementation with the Serde trait bound is a smart approach that:
- Reduces code generation by handling serialization generically
- Provides type safety through the Serde trait bound
- Maintains a clean separation of concerns between serialization and hashing
crates/dojo/core/src/model/model_value.cairo (2)
16-16: Ohayo! Update method documentation to reflect serialization changes, sensei!The method signatures have been updated to emphasize serialization, but the documentation still refers to the old terminology. Let's align them:
- /// Returns a span of values associated with the entity, every field of a model - /// that is not a key. + /// Returns a span of serialized values for 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 serialized representation. fn from_serialized(values: Span<felt252>) -> Option<V>;Also applies to: 18-18
Line range hint
1-53: Excellent architectural direction, sensei! 🏗️The shift towards explicit serialization handling:
- Makes the data flow more transparent
- Reduces cognitive overhead by clearly indicating when we're dealing with serialized data
- Aligns well with the PR's objective of reducing generated code
This change sets a good foundation for further optimizations in event handling.
crates/dojo/core/src/lib.cairo (1)
73-73: Ohayo! The serialization-focused reorganization looks good, sensei!The prioritization of
entity_id_from_serialized_keysaligns well with the codebase's transition to serialized data handling.Consider adding a doc comment to indicate that
entity_id_from_serialized_keysis the preferred method:+ /// Preferred method for generating entity IDs from serialized keys pub use key::{entity_id_from_serialized_keys, combine_key, entity_id_from_keys};crates/dojo/core/src/model/storage.cairo (2)
53-53: Consistent parameter naming in ModelValueStorage trait, sensei!The parameter renaming from
keytokeysin bothread_valueandwrite_valuemethods maintains consistency with the broader changes.Consider updating the method documentation to reflect that these methods handle serialized keys, aligning with the new naming convention.
Also applies to: 67-67
Line range hint
1-3: Consider addressing the TODO comment.The TODO comment about defining "the right interface for member accesses" might be relevant to these changes, especially given the transition to serialized data handling.
Would you like help defining the interface for member accesses or should we create an issue to track this TODO?
crates/dojo/core-cairo-test/src/tests/model/model.cairo (2)
86-86: Key handling updates look solid, sensei!The transition to using
keys()provides a more consistent approach to key management. Consider adding a brief comment explaining the key structure for future maintainers.+ // Read model value using composite keys (k1, k2) let mut model_value: FooValue = world.read_value(foo.keys());Also applies to: 95-95
166-171: Excellent addition of serialized key testing, sensei!The new test provides good coverage for serialized key functionality. Consider adding edge cases to verify behavior with empty or invalid serialized keys.
Consider adding these test cases:
#[test] fn test_model_ptr_from_serialized_keys_empty() { // Test handling of empty serialized keys } #[test] fn test_model_ptr_from_serialized_keys_invalid() { // Test handling of invalid serialized keys }crates/dojo/core/src/model/model.cairo (1)
39-41: Consider optimizing array allocation in serialization methods!The methods are well-named and clearly indicate their serialized nature. However, there might be room for optimization in the
from_serializedimplementation to avoid the intermediate array allocation.Consider using a more efficient approach:
fn from_serialized(keys: Span<felt252>, values: Span<felt252>) -> Option<M> { - let mut serialized: Array<felt252> = keys.into(); - serialized.append_span(values); - let mut span = serialized.span(); + // TODO: Implement a deserializer that can handle two spans directly + // without the need for intermediate array allocationcrates/dojo/core-cairo-test/src/tests/benchmarks.cairo (3)
252-252: Ohayo sensei! Consider fixing the struct name typo.While the serialization change is good, I noticed that "Quaterions" in the struct name is misspelled. It should be "Quaternions".
-struct PositionWithQuaterions { +struct PositionWithQuaternions {
300-300: Ohayo sensei! Consider addressing the TODO comment.While the serialization change is good, there's a TODO comment about adapting this test to the new layout system. Would you like assistance in updating the test to work with the new layout system?
381-381: Ohayo sensei! Another TODO needs attention.The serialization change looks good, but like the previous test, this one is also ignored and needs to be adapted to the new layout system. Would you like help updating both benchmark tests together?
crates/dojo/core/src/event/event.cairo (1)
12-14: Ohayo, sensei! Confirm the necessity of theEventDefinition<E>trait.Consider whether the separate
EventDefinition<E>trait is necessary or if its functionality can be integrated into theEvent<E>trait to simplify the codebase and reduce complexity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
crates/dojo/core-cairo-test/src/tests/benchmarks.cairo(9 hunks)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/component.cairo(2 hunks)crates/dojo/core/src/event/event.cairo(1 hunks)crates/dojo/core/src/event/interface.cairo(1 hunks)crates/dojo/core/src/lib.cairo(2 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 (36)
crates/dojo/core-cairo-test/src/tests/utils/key.cairo (2)
1-1: Ohayo sensei! Import statement correctly updated for serialization.
The change from entity_id_from_keys to entity_id_from_serialized_keys aligns with the codebase-wide transition to serialized data handling.
6-9: Test assertion properly updated to use serialized keys.
The test maintains its original validation logic while adopting the new serialized key approach. The comparison with poseidon_hash_span ensures that the entity ID generation remains consistent with the previous implementation.
Let's verify that all other test files have been updated consistently:
✅ Verification successful
Both entity_id_from_keys and entity_id_from_serialized_keys are being used correctly throughout the codebase
The search results show that both functions coexist intentionally:
entity_id_from_keysis a higher-level function that handles serialization of generic keysentity_id_from_serialized_keysis used directly when working with already serialized keys- The test assertion is correctly using
entity_id_from_serialized_keyssince it's working with an already serialized span of felt252 values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of the old entity_id_from_keys function
# Expect: No occurrences of the old function name
# Search for any remaining usage of the old function name
rg "entity_id_from_keys\(" --type cairo
Length of output: 71
Script:
#!/bin/bash
# Let's try a more generic search without file type restriction
rg "entity_id_from_keys" -A 2
# Also search for the new function to understand the migration
rg "entity_id_from_serialized_keys" -A 2
# Let's also check all cairo files using fd
fd "\.cairo$" -x cat {} \; | grep -A 2 "entity_id_from"
Length of output: 11630
crates/dojo/core/src/utils/key.cairo (1)
12-12: Ohayo! Clear and descriptive function renaming, sensei!
The renaming to entity_id_from_serialized_keys better reflects the function's purpose and maintains consistency with the serialization-focused changes across the codebase.
crates/dojo/core/src/event/component.cairo (2)
1-1: Ohayo! Clean import update looks good, sensei!
The import statement has been cleanly updated to use the new EventDef type name, maintaining consistency with the broader codebase changes.
28-30: Return type update is properly implemented!
The definition method's return type has been correctly updated to use EventDef, aligning with the type renaming changes across the framework.
crates/dojo/core/src/model/model_value.cairo (1)
30-35: Implementation looks clean, sensei! 🎯
The implementation effectively uses Serde for deserialization and maintains a clean, focused approach to handling serialized data. The removal of the entity_id parameter simplifies the interface while maintaining functionality.
crates/dojo/core/src/lib.cairo (1)
15-15: Ohayo sensei! Please clarify the dual export of EventDefinition and EventDef.
The export includes both EventDefinition and EventDef. If EventDef is intended to replace EventDefinition, we should consider:
- Marking
EventDefinitionas deprecated - Adding a migration guide for users
Let's check the usage patterns:
✅ Verification successful
Ohayo sensei! The dual export is actually correct and intentional.
From the codebase analysis, I can see that:
EventDefinitionis a trait (interface) defined inevent.cairoEventDefis a struct that represents the concrete event definition with name, layout, and schema- They serve different purposes and are not meant to replace each other
The exports are correctly structured as they represent two distinct types that work together in the event system:
EventDefinitiontrait for implementing event behaviorEventDefstruct for storing event metadata
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage patterns of both types
echo "=== EventDefinition usage ==="
rg "EventDefinition" -A 2
echo "=== EventDef usage ==="
rg "EventDef[^i]" -A 2
Length of output: 5405
crates/dojo/core/src/model/storage.cairo (2)
105-105: Parameter renaming in test trait matches production code.
The change maintains consistency with the production traits, which is excellent!
17-17: Ohayo! Verify the impact of parameter renaming across the codebase.
The renaming from key to keys looks good and aligns with the broader transition to serialized data handling.
✅ Verification successful
Ohayo! The parameter renaming is consistently implemented
The verification shows that all occurrences of read_model and read_models methods are using the new keys parameter name consistently across the codebase. The change appears in both the trait definition and its implementation in WorldStorage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to read_model have been updated to use the new parameter name
# and there are no lingering references to the old parameter name
# Search for any remaining references to the old parameter name in read_model calls
rg -p "read_model.*key:[^s]"
# Search for current usage to verify correct parameter naming
rg -p "read_model.*keys:"
Length of output: 473
crates/dojo/core-cairo-test/src/tests/model/model.cairo (4)
65-68: Clean transition to serialized deserialization, sensei!
The update to use from_serialized maintains consistency with the new serialization approach while preserving the test's validation logic.
74-74: Error handling remains robust with serialization changes!
The negative test case has been properly updated to use the new serialized interface while maintaining important error condition validation.
156-159: Clean function rename and key handling update!
The function name now accurately reflects its purpose, and the implementation properly uses the new key management approach.
57-58: Ohayo! The serialization change looks good, sensei!
The transition from values() to serialized_values() aligns well with the PR's objective of reducing generated code through consistent serialization handling.
Let's verify the consistency of this change across other models:
✅ Verification successful
Ohayo! The serialization change is consistent across the codebase, sensei!
The transition from values() to serialized_values() is part of a well-implemented pattern throughout the codebase. The search results show that:
- The new
serialized_values()method is consistently used in model implementations - No legacy
values()method calls remain in the codebase - The change aligns with the broader serialization architecture, with proper trait definitions and implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining non-serialized value() calls that might need updating
rg -g '*.cairo' 'fn values\(\)'
Length of output: 33
Script:
#!/bin/bash
# Let's try a broader search for both the old and new method names to understand the transition
rg -g '*.cairo' '(values|serialized_values)' -A 2
# Also search for trait definitions that might define these methods
ast-grep --pattern 'trait $_ {
$$$
fn values($$$) $$$
$$$
}'
# And search for implementations
ast-grep --pattern 'impl $_ {
$$$
fn values($$$) $$$
$$$
}'
Length of output: 59285
crates/dojo/core/src/model/model.cairo (4)
3-3: Ohayo sensei! The import changes look good!
The addition of entity_id_from_serialized_keys while maintaining entity_id_from_keys aligns well with the PR's objective of supporting both serialized and non-serialized operations.
33-37: The keys-related method changes are well-structured!
The distinction between keys<K>() for type-safe key parsing and serialized_keys() for raw serialized access provides a clean API design that supports both use cases.
Line range hint 77-143: Excellent implementation of the serialization changes, sensei!
The implementation maintains a clean separation between serialized and non-serialized paths while providing consistent behavior. The use of entity_id_from_serialized_keys in the entity_id method ensures proper integration with the serialization system.
62-64: The pointer-related method changes look solid!
The naming changes from singular to plural form (key → keys) better reflect that models can have multiple keys. The addition of ptr_from_serialized_keys maintains API consistency.
Let's verify the usage patterns:
✅ Verification successful
Ohayo! The pointer-related changes are perfectly aligned, sensei!
The verification shows that:
- All usages of
ptr_from_keysare consistent with the new plural naming - The new
ptr_from_serialized_keysmethod is properly implemented and tested - No lingering singular "key" references were found
- Test coverage exists for both methods in
core-cairo-test
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining singular "key" references that might need updating
rg -i "ptr_from_key[^s]"
Length of output: 26
Script:
#!/bin/bash
# Let's search for any usage of ptr_from_key with more context
rg -i "ptr_from_key" -A 2 -B 2
# Also search for any references to the new method
rg -i "ptr_from_keys" -A 2 -B 2
# And check for serialized keys usage
rg -i "ptr_from_serialized_keys" -A 2 -B 2
Length of output: 4808
examples/spawn-and-move/src/actions.cairo (2)
105-107: Ohayo! The entity ID generation update looks good, sensei! 🎌
The transition to entity_id_from_serialized_keys aligns well with the codebase-wide serialization strategy. The comment accurately documents the available methods for entity ID generation.
261-261: Test coverage maintained properly, sensei! ✨
The test code has been correctly updated to use entity_id_from_serialized_keys, maintaining consistency with the implementation changes.
crates/dojo/core-cairo-test/src/tests/benchmarks.cairo (2)
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 approach.
271-271: Ohayo sensei! The database operation change is perfect!
The update to use serialized_values() in the database set operation maintains consistency while preserving the gas benchmarking functionality.
crates/dojo/core/src/world/world_contract.cairo (5)
49-51: Ohayo sensei! Import changes look good.
The updated import statement correctly reflects the transition to serialized key handling.
333-339: Metadata function changes are well implemented, sensei!
The transition to serialized methods (entity_id_from_serialized_keys and from_serialized) maintains the original functionality while improving consistency in data handling.
1177-1177: Set entity changes look good, sensei!
The update to use entity_id_from_serialized_keys aligns with the new serialization pattern while preserving the existing functionality.
1217-1217: Delete entity changes are properly implemented!
The update to use entity_id_from_serialized_keys maintains consistency with the new serialization pattern.
1241-1241: Get entity changes are spot on!
The update to use entity_id_from_serialized_keys completes the consistent transition to serialized key handling across all entity operations.
crates/dojo/core/src/event/event.cairo (4)
2-3: Ohayo, sensei! Imports are correctly updated.
The additional imports of Introspect, Struct, and Ty are appropriate for handling introspection and schema definitions.
27-52: Ohayo, sensei! Implementation of EventImpl is comprehensive and well-structured.
The EventImpl provides necessary methods for event handling, including name retrieval, definition, layout, schema, and serialization of keys and values. The use of traits like ModelParser, Introspect, and Serde enhances modularity and reusability.
18-25: Ohayo, sensei! Update implementations to use serialized_keys and serialized_values.
The methods serialized_keys and serialized_values replace the previous keys and values methods. Ensure that all event implementations and their usages are updated accordingly.
Run the following script to identify any outdated method implementations:
#!/bin/bash
# Description: Find implementations of the old `keys` and `values` methods.
rg 'fn\s+(keys|values)\s*\(' --type cairo44-48: Ohayo, sensei! Verify serialization logic in serialized_keys and serialized_values.
Ensure that the serialization methods correctly handle all data types within the events, and consider edge cases where serialization might fail.
Run the following script to identify potential serialization issues:
crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (4)
2-7: Ohayo, sensei! The name function implementation looks solid.
The implementation of $type_name$Definition correctly provides the name function returning "$type_name$". This ensures that the event name is properly defined.
22-22: Ohayo, sensei! The event implementation alias is appropriately defined.
The alias pub impl $type_name$EventImpl = dojo::event::event::EventImpl<$type_name$>; ensures that $type_name$ correctly implements the necessary event traits.
10-15: Ohayo, sensei! Please verify placeholder substitutions in serialization methods.
The methods serialize_keys and serialize_values use placeholders $serialized_keys$ and $serialized_values$. It's important to ensure that these placeholders are correctly replaced during code generation to prevent any compilation issues.
Here's a script to check for any unreplaced placeholders in the codebase:
✅ Verification successful
Ohayo, sensei! The placeholders are part of the patch template design!
The placeholders in event.patch.cairo are intentional as this is a patch template file. The file is designed to be used as a template where these placeholders are substituted during code generation:
$type_name$: For the event type name$serialized_keys$: For key serialization logic$serialized_values$: For value serialization logic$unique_hash$: For a unique contract class hash
This is the expected pattern for a patch template file and not a compilation issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all placeholders have been correctly substituted.
# Test: Search for any occurrences of unreplaced placeholders. Expect: No matches.
rg '\$\w+\$'
Length of output: 228
Script:
#!/bin/bash
# Let's check the content of the patch files to understand the placeholder usage
cat crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo
Length of output: 1898
Line range hint 32-47: Ohayo, sensei! Verify the unique class hash substitution.
In the ensure_unique function, the _hash variable is set to $unique_hash$. Confirm that $unique_hash$ is replaced with the actual computed hash to guarantee the uniqueness of the generated contract.
Here's a script to ensure that $unique_hash$ has been properly substituted:
✅ Verification successful
Ohayo, sensei! The $unique_hash$ substitution is working as expected!
The presence of $unique_hash$ in the patch files is intentional - these are template files that get processed by the DojoEvent implementation. Looking at the code, we can see that the actual substitution happens in the from_struct method where it computes the unique hash and replaces the placeholder:
let unique_hash = compute_unique_hash(db, &event_name, false, &struct_ast.members(db).elements(db)).to_string();
// ... later used in interpolation
("unique_hash".to_string(), RewriteNode::Text(unique_hash))The $unique_hash$ in the patch files serves as a template placeholder that gets replaced with the actual computed hash during macro expansion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that $unique_hash$ has been replaced with the actual hash value.
# Test: Search for any occurrence of '$unique_hash$'. Expect: No matches.
rg '\$unique_hash\$'
Length of output: 228
Script:
#!/bin/bash
# Let's check the macro implementation to understand how $unique_hash$ is processed
ast-grep --pattern 'fn expand_event_macro' -A 50
Length of output: 50
Script:
#!/bin/bash
# Let's check the macro source files to understand the event macro implementation
fd "event.*rs" --exec cat {}
Length of output: 91255
crates/dojo/core/src/world/storage.cairo (2)
61-62: Ohayo, sensei!
Correct usage of serialized_keys and serialized_values in emit_event
The changes to use serialized keys and values in the emit_event method align with the updated serialization approach and enhance consistency.
124-125: Ohayo, sensei!
Appropriate serialization in write_model
Using Model::<M>::serialized_keys(model) and Model::<M>::serialized_values(model) ensures that models are correctly serialized when writing to storage.
| @@ -1,4 +1,4 @@ | |||
| #[starknet::interface] | |||
| pub trait IEvent<T> { | |||
| fn definition(self: @T) -> super::EventDefinition; | |||
| fn definition(self: @T) -> super::EventDef; | |||
There was a problem hiding this comment.
💡 Codebase verification
Ohayo sensei! The EventDef transition is incomplete!
The codebase still contains multiple references to EventDefinition that need to be updated to EventDef, particularly in:
crates/dojo/lang/src/plugin_test_data/event(multiple occurrences)crates/dojo/core/src/event/event.cairo(trait and impl definitions)crates/dojo/core/src/lib.cairo(exports)
This inconsistency could lead to confusion and potential issues. The change should be applied uniformly across all files.
🔗 Analysis chain
Ohayo sensei! The return type change looks good!
The change from EventDefinition to EventDef aligns well with the PR's objective of reducing generated code by simplifying type names.
Let's verify the consistency of this change across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that EventDefinition has been completely replaced with EventDef
# Test 1: Check for any remaining references to EventDefinition
echo "Checking for remaining EventDefinition references..."
rg "EventDefinition"
# Test 2: Verify the new EventDef usage
echo "Verifying EventDef usage..."
rg "EventDef"
Length of output: 4423
| 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 method usage.
While most changes correctly use the new serialized methods, there's an inconsistency in the ComplexModel entity operation:
- index: ModelIndex::Keys(complex_entity.keys()),
+ index: ModelIndex::Keys(complex_entity.serialized_keys()),Also applies to: 480-481, 491-491
| match Introspect::<E>::ty() { | ||
| Ty::Struct(s) => s, | ||
| _ => panic!("Event: invalid schema.") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Consider graceful error handling in the schema method.
Using panic! may halt the program unexpectedly. Consider returning a Result type to handle errors more gracefully and provide better error propagation.
Apply this diff to modify the schema method:
fn schema() -> Struct {
match Introspect::<E>::ty() {
Ty::Struct(s) => s,
- _ => panic!("Event: invalid schema.")
+ _ => {
+ // Return an error for invalid schema type
+ let err_msg = "Event: invalid schema.";
+ Err(err_msg)
+ }
}
}This change allows calling functions to handle the error appropriately.
Committable suggestion skipped: line range outside the PR's diff.
| pub struct EventDef { | ||
| pub name: ByteArray, | ||
| pub layout: Layout, | ||
| pub schema: Struct |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! The renaming needs attention in several files.
The search results show that EventDefinition is still being used in multiple places while EventDef has been introduced, creating an inconsistent state:
crates/dojo/core/src/lib.cairoexports bothEventDefinitionandEventDefcrates/dojo/core/src/event/event.cairouses both names:- Has the new
EventDefstruct - Still uses
EventDefinitiontrait and implementations
- Has the new
crates/dojo/lang/src/plugin_test_data/eventhas multiple references toEventDefinitioncrates/dojo/lang/src/attribute_macros/patches/event.patch.cairousesEventDefinition
The renaming appears to be incomplete and needs to be consistently applied across all these files.
🔗 Analysis chain
Ohayo, sensei! Ensure consistent renaming from EventDefinition to EventDef.
Please verify that all references to EventDefinition have been updated to EventDef throughout the codebase to prevent any compilation errors.
Run the following script to check for any remaining references:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to `EventDefinition`.
rg 'EventDefinition' --type cairo
Length of output: 63
Script:
#!/bin/bash
# Let's search for EventDefinition without file type restriction since cairo might not be recognized
rg "EventDefinition"
# Also search for EventDef to understand the context better
rg "EventDef"
# Let's also check all cairo files to see their presence
fd "\.cairo$"
Length of output: 9181
| fn read_value<K, +Drop<K>, +Serde<K>, +ModelValueKey<V, K>>(self: @WorldStorage, key: K) -> V { | ||
| Self::read_value_from_id(self, entity_id_from_key(@key)) | ||
| fn read_value<K, +Drop<K>, +Serde<K>, +ModelValueKey<V, K>>(self: @WorldStorage, keys: K) -> V { | ||
| Self::read_value_from_id(self, entity_id_from_keys(@keys)) |
There was a problem hiding this comment.
Ohayo, sensei!
Inconsistent use of entity_id_from_keys instead of entity_id_from_serialized_keys
In these lines, entity_id_from_keys is used with @keys as an argument, whereas elsewhere entity_id_from_serialized_keys is used with serialize_inline(@keys). For consistency and correctness, please replace entity_id_from_keys with entity_id_from_serialized_keys and ensure that the keys are appropriately serialized.
Apply the following diffs to fix the inconsistency:
@@ -226,226 @@
- Self::read_value_from_id(self, entity_id_from_keys(@keys))
+ Self::read_value_from_id(self, entity_id_from_serialized_keys(serialize_inline(@keys)))@@ -251,251 @@
- entity_ids.append(entity_id_from_keys(k));
+ entity_ids.append(entity_id_from_serialized_keys(serialize_inline(k)));@@ -300,300 @@
- ids.append(entity_id_from_keys(k));
+ ids.append(entity_id_from_serialized_keys(serialize_inline(k)));@@ -441,441 @@
- Self::write_value_from_id_test(ref self, dojo::utils::entity_id_from_keys(@keys), value);
+ Self::write_value_from_id_test(ref self, dojo::utils::entity_id_from_serialized_keys(serialize_inline(@keys)), value);@@ -449,449 @@
- ids.append(entity_id_from_keys(k));
+ ids.append(entity_id_from_serialized_keys(serialize_inline(k)));Also applies to: 251-251, 300-300, 441-441, 449-449
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
crates/dojo/core/src/event/event.cairo (1)
16-25: Ohayo, sensei! Good improvements to method naming.The renaming of
keys()andvalues()toserialized_keys()andserialized_values()makes the serialization aspect more explicit. This improves code clarity and maintainability.Consider documenting the serialization format and any constraints in the trait documentation to help implementers understand the requirements.
crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (2)
1-8: Ohayo! Clean and well-documented struct implementation, sensei!The EventValue struct is well-designed with clear documentation explaining its ABI-driven purpose. The minimal trait derivation (Drop, Serde) is appropriate for this use case.
Consider adding a code example in the documentation to illustrate how the event definition looks after code generation:
// EventValue on it's own does nothing since events are always emitted and // never read from the storage. However, it's required by the ABI to // ensure that the event definition contains both keys and values easily distinguishable. // Only derives strictly required traits. +// Example generated code: +// ``` +// struct PlayerMoveValue { +// position: Position, +// timestamp: u64 +// } +// ```
59-64: Enhance documentation for ensure_values method, sensei!While the implementation is correct, the documentation could be more detailed to explain the method's role in ABI diffing.
Consider expanding the documentation:
- // Outputs EventValue to allow a simple diff from the ABI compared to the - // event to retrieved the keys of an event. + // Outputs EventValue to enable ABI diffing capabilities. By comparing the ABI of + // this method with the event's ABI, we can extract the event's keys since + // EventValue only contains the non-key fields. This approach provides a + // reliable way to distinguish between keys and values in the event structure.crates/dojo/lang/src/attribute_macros/event.rs (1)
84-94: Ohayo sensei! Clean implementation of member filtering.The filter_map implementation efficiently separates non-key members while maintaining proper Rust field declaration syntax. Consider adding a brief comment explaining the purpose of this collection for future maintainers.
+ // Collect non-key members for value struct generation let members_values = members
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ 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 (4)
Cargo.toml(1 hunks)crates/dojo/core/src/event/event.cairo(1 hunks)crates/dojo/lang/src/attribute_macros/event.rs(3 hunks)crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo(2 hunks)
🔇 Additional comments (6)
crates/dojo/core/src/event/event.cairo (2)
12-14: Ohayo, sensei! Clean trait definition.
The new EventDefinition trait with generic parameter E provides a clean separation of concerns for event naming.
27-52: Ohayo, sensei! Solid implementation with comprehensive trait bounds.
The EventImpl provides a complete event handling solution with good integration of ModelParser, Serde, and Introspect. The use of utility function for selector computation ensures consistency.
crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo (2)
10-15: Clean and efficient implementation, sensei!
The EventDefinition trait implementation is concise and uses appropriate optimizations with inline(always).
17-28: Efficient serialization implementation!
The ModelParser implementation uses appropriate data structures (Array for building, Span for returning) and follows memory-efficient practices.
Let's verify that all event implementations follow this serialization pattern:
✅ Verification successful
This script will:
- Find all Cairo files
- Search for ModelParser implementations with context
- Help us verify the serialization pattern across all implementations
ModelParser implementation pattern is consistent across the codebase!
The search results show that the ModelParser implementation pattern is identical in both model.patch.cairo and event.patch.cairo, following the same efficient serialization approach:
- Using mutable arrays for building serialized data
- Returning Span for memory efficiency
- Consistent method signatures and structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any event serialization implementations that might not follow the same pattern
ast-grep --pattern 'impl $name$ModelParser of dojo::model::model::ModelParser<$_> {
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Let's try a different approach to find ModelParser implementations
# First, let's find all .cairo files that might contain event-related code
fd ".cairo$" | while read -r file; do
echo "=== $file ==="
rg "impl.*ModelParser.*dojo::model::model::ModelParser" "$file" -A 10
done
Length of output: 26085
crates/dojo/lang/src/attribute_macros/event.rs (2)
110-110: Clear and direct error messaging, sensei!
The updated error message provides better guidance by explicitly stating what's not allowed, improving the developer experience.
137-137: Verify patch template usage of members_values.
The addition to the interpolation map looks good, but let's ensure the patch template correctly utilizes this new value.
✅ Verification successful
Patch template correctly uses members_values interpolation
The verification confirms that members_values is properly used in the event patch template at the expected location for member value initialization. The interpolation map addition is valid and correctly utilized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the event patch template uses the members_values interpolation
# Test: Search for members_values usage in the patch template
cat crates/dojo/lang/src/attribute_macros/patches/event.patch.cairo | grep -A 5 'members_values'
Length of output: 248
| use dojo::meta::{Introspect, introspect::{Struct, Ty}}; | ||
| use dojo::model::model::ModelParser; | ||
|
|
||
| #[derive(Drop, Serde, Debug, PartialEq)] | ||
| pub struct EventDefinition { | ||
| pub struct EventDef { | ||
| pub name: ByteArray, | ||
| pub layout: Layout, | ||
| pub schema: Struct | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo, sensei! The EventDef renaming is inconsistent and needs attention.
The codebase shows mixed usage of both EventDef and EventDefinition:
EventDefis the new struct name inevent.cairoEventDefinitionis still used as a trait name and in test data- Both names are exported in
lib.cairo:pub use event::{Event, EventDefinition, EventDef}
This dual naming creates confusion and violates consistent naming conventions. Either revert to EventDefinition throughout or complete the renaming to EventDef consistently.
🔗 Analysis chain
Ohayo, sensei! Verify the EventDef renaming across the codebase.
The renaming from EventDefinition to EventDef needs to be consistently applied across all files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to EventDefinition
# and verify EventDef usage
echo "Checking for remaining EventDefinition references:"
rg "EventDefinition" --type cairo
echo -e "\nVerifying EventDef usage:"
rg "EventDef" --type cairo
Length of output: 298
Script:
#!/bin/bash
# Let's try without file type filtering since cairo isn't recognized
echo "Checking for remaining EventDefinition references:"
rg "EventDefinition"
echo -e "\nVerifying EventDef usage:"
rg "EventDef"
Length of output: 4421
Cargo.toml
Outdated
| #scarb = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" } | ||
| #scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" } | ||
| scarb = { path = "/Users/glihm/swm/scarb/scarb" } | ||
| scarb-ui = { path = "/Users/glihm/swm/scarb/utils/scarb-ui" } |
There was a problem hiding this comment.
Ohayo sensei! Please revert the hardcoded local paths.
The changes introduce hardcoded local paths that will break builds for other developers. These paths are specific to your local development environment and should not be committed.
Please revert these changes and restore the git dependencies:
-#scarb = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" }
-#scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" }
-scarb = { path = "/Users/glihm/swm/scarb/scarb" }
-scarb-ui = { path = "/Users/glihm/swm/scarb/utils/scarb-ui" }
+scarb = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" }
+scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" }If you need to use local paths for development:
- Use relative paths instead of absolute paths
- Add the changes to
.gitignore - Consider using a local override with
[patch.crates-io]in a separateCargo.toml.localfile
📝 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.
| #scarb = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" } | |
| #scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" } | |
| scarb = { path = "/Users/glihm/swm/scarb/scarb" } | |
| scarb-ui = { path = "/Users/glihm/swm/scarb/utils/scarb-ui" } | |
| scarb = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" } | |
| scarb-ui = { git = "https://github.com/dojoengine/scarb", rev = "a6d3b5b17b288502fe9cc63c96c0ae22fd175857" } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
scripts/rebuild_test_artifacts.sh (1)
33-36: Consider optimizing build performance, sensei!The sequential execution of multiple build commands might impact the script's performance. Consider these improvements:
-./target/release/sozo build --manifest-path examples/simple/Scarb.toml -./target/release/sozo build --manifest-path examples/spawn-and-move/Scarb.toml -./target/release/sozo build --manifest-path examples/spawn-and-move/Scarb.toml -P release -./target/release/sozo build --manifest-path crates/torii/types-test/Scarb.toml +# Run builds in parallel and capture any errors +build_projects() { + local -a pids=() + local -a cmds=( + "./target/release/sozo build --manifest-path examples/simple/Scarb.toml" + "./target/release/sozo build --manifest-path examples/spawn-and-move/Scarb.toml" + "./target/release/sozo build --manifest-path examples/spawn-and-move/Scarb.toml -P release" + "./target/release/sozo build --manifest-path crates/torii/types-test/Scarb.toml" + ) + + for cmd in "${cmds[@]}"; do + eval "$cmd" & + pids+=($!) + done + + # Wait for all builds and check for errors + for pid in "${pids[@]}"; do + if ! wait "$pid"; then + echo "Build process $pid failed" + exit 1 + fi + done +} + +build_projects
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ 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 (2)
Cargo.toml(1 hunks)scripts/rebuild_test_artifacts.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🔇 Additional comments (1)
scripts/rebuild_test_artifacts.sh (1)
33-36: Ohayo! The new build commands are logically placed, sensei!
The additional sozo build commands are correctly positioned before the database generation step, ensuring all necessary artifacts are available for testing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2662 +/- ##
==========================================
- Coverage 57.36% 57.34% -0.03%
==========================================
Files 401 401
Lines 50387 50395 +8
==========================================
- Hits 28905 28899 -6
- Misses 21482 21496 +14 ☔ 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
Release Notes
New Features
Bug Fixes
Documentation
Chores