refactor(lang): reduce events generated code #2662
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,52 @@ | ||
| use dojo::meta::Layout; | ||
| use dojo::meta::{Introspect, introspect::{Struct, Ty}}; | ||
| use dojo::model::model::ModelParser; | ||
|
|
||
| #[derive(Drop, Serde, Debug, PartialEq)] | ||
| pub struct EventDef { | ||
| pub name: ByteArray, | ||
| pub layout: Layout, | ||
| pub schema: Struct | ||
| } | ||
|
Comment on lines
+2
to
10
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
This dual naming creates confusion and violates consistent naming conventions. Either revert to 🔗 Analysis chainOhayo, sensei! Verify the EventDef renaming across the codebase. The renaming from 🏁 Scripts executedThe 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 |
||
|
|
||
| pub trait EventDefinition<E> { | ||
| fn name() -> ByteArray; | ||
| } | ||
|
|
||
| pub trait Event<T> { | ||
| fn name() -> ByteArray; | ||
| fn definition() -> EventDef; | ||
| fn layout() -> Layout; | ||
| fn schema() -> Struct; | ||
| fn serialized_keys(self: @T) -> Span<felt252>; | ||
| fn serialized_values(self: @T) -> Span<felt252>; | ||
| /// Returns the selector of the event computed for the given namespace hash. | ||
| fn selector(namespace_hash: felt252) -> felt252; | ||
| } | ||
|
|
||
| pub impl EventImpl<E, +ModelParser<E>, +EventDefinition<E>, +Serde<E>, +Introspect<E>> of Event<E> { | ||
| fn name() -> ByteArray { | ||
| EventDefinition::<E>::name() | ||
| } | ||
| fn definition() -> EventDef { | ||
| EventDef { name: Self::name(), layout: Self::layout(), schema: Self::schema() } | ||
| } | ||
| fn layout() -> Layout { | ||
| Introspect::<E>::layout() | ||
| } | ||
| fn schema() -> Struct { | ||
| match Introspect::<E>::ty() { | ||
| Ty::Struct(s) => s, | ||
| _ => panic!("Event: invalid schema.") | ||
| } | ||
|
Comment on lines
+38
to
+41
There was a problem hiding this comment. 🛠️ Refactor suggestion Ohayo, sensei! Consider graceful error handling in the Using Apply this diff to modify the 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.
|
||
| } | ||
| fn serialized_keys(self: @E) -> Span<felt252> { | ||
| ModelParser::<E>::serialize_keys(self) | ||
| } | ||
| fn serialized_values(self: @E) -> Span<felt252> { | ||
| ModelParser::<E>::serialize_values(self) | ||
| } | ||
| fn selector(namespace_hash: felt252) -> felt252 { | ||
| dojo::utils::selector_from_namespace_and_name(namespace_hash, @Self::name()) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| #[starknet::interface] | ||
| pub trait IEvent<T> { | ||
| 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
This inconsistency could lead to confusion and potential issues. The change should be applied uniformly across all files. 🔗 Analysis chainOhayo sensei! The return type change looks good! The change from Let's verify the consistency of this change across the codebase: 🏁 Scripts executedThe 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 |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Ohayo, sensei! The renaming needs attention in several files.
The search results show that
EventDefinitionis still being used in multiple places whileEventDefhas been introduced, creating an inconsistent state:crates/dojo/core/src/lib.cairoexports bothEventDefinitionandEventDefcrates/dojo/core/src/event/event.cairouses both names:EventDefstructEventDefinitiontrait and implementationscrates/dojo/lang/src/plugin_test_data/eventhas multiple references toEventDefinitioncrates/dojo/lang/src/attribute_macros/patches/event.patch.cairousesEventDefinitionThe renaming appears to be incomplete and needs to be consistently applied across all these files.
🔗 Analysis chain
Ohayo, sensei! Ensure consistent renaming from
EventDefinitiontoEventDef.Please verify that all references to
EventDefinitionhave been updated toEventDefthroughout 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:
Length of output: 63
Script:
Length of output: 9181