Event fields as key #2280
Event fields as key #2280
Conversation
WalkthroughOhayo, sensei! The recent updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant EventLog
User->>Contract: Call register_namespace(namespace)
Contract->>Contract: Process namespace
Contract->>EventLog: Log event with namespace
EventLog-->>Contract: Event logged
Contract-->>User: Confirm registration
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (2)
Files selected for processing (12)
Files skipped from review as they are similar to previous changes (3)
Additional comments not posted (20)
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
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
|
remybar
force-pushed
the
event_fields_as_key
branch
from
cf09955
to
6232b7b
Compare
Ok ! I've just reported an issue to Starkware about the |
remybar
force-pushed
the
event_fields_as_key
branch
4 times, most recently
from
2fde18f
to
3ee44ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
crates/dojo-world/src/contracts/abi/world.rs (1)
746-746: Ohayo, sensei! Possible ABI Consistency Issue DetectedThe change from
"data"to"key"is consistently applied inworld.rs, but not inexecutor.rsormodel.rs. This could lead to inconsistencies in ABI generation and contract interactions. Please review these files to ensure consistent application of the"kind": "key"change.
world.rs: Consistent application found.executor.rs: No occurrences found.model.rs: No occurrences found.Analysis chain
Ohayo, sensei! Confirm ABI consistency with key changes.
The transition from
"data"to"key"in theabigen!macro suggests these fields are now primary references. Verify that ABI generation and contract interactions are consistent with this change to avoid integration issues.Also applies to: 763-763, 773-773, 778-778, 790-790, 819-819, 836-836, 853-853, 858-858, 917-917, 922-922, 939-939, 944-944, 961-961, 966-966, 971-971, 988-988, 993-993, 1005-1005, 1010-1010, 1027-1027, 1032-1032
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm ABI consistency with `kind` changes from "data" to "key". # Test: Search for `abigen!` macro usage to ensure ABI consistency. rg --type rust 'abigen!\('Length of output: 469
Script:
#!/bin/bash # Description: Check the usage of `abigen!` macro in specific files to verify the change from "data" to "key". # Search for `abigen!` macro usage in `world.rs`. rg --type rust 'abigen!\(' crates/dojo-world/src/contracts/abi/world.rs -A 10 # Search for `abigen!` macro usage in `executor.rs`. rg --type rust 'abigen!\(' crates/dojo-world/src/contracts/abi/executor.rs -A 10 # Search for `abigen!` macro usage in `model.rs`. rg --type rust 'abigen!\(' crates/dojo-world/src/contracts/abi/model.rs -A 10Length of output: 849
Script:
#!/bin/bash # Description: Verify if the "kind": "key" change is consistently applied in ABI definitions. # Search for "kind": "key" in `world.rs`. rg --type rust '"kind": "key"' crates/dojo-world/src/contracts/abi/world.rs # Search for "kind": "key" in `executor.rs`. rg --type rust '"kind": "key"' crates/dojo-world/src/contracts/abi/executor.rs # Search for "kind": "key" in `model.rs`. rg --type rust '"kind": "key"' crates/dojo-world/src/contracts/abi/model.rsLength of output: 720
remybar
force-pushed
the
event_fields_as_key
branch
from
3ee44ce
to
02a33d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
examples/spawn-and-move/manifests/release/base/abis/dojo-world.json (1)
813-813: Ohayo, sensei! Inconsistent usage ofresourceas a key.The
resourcefield is marked as a"key"in some files but remains as"data"in others. Please ensure consistent usage across the codebase to avoid potential issues with resource identification.
- Check
examples/game-lib/armory/manifests/dev/base/abis/dojo-world.jsonfor"kind": "data".- Verify other instances where
resourceis used as"data"to ensure consistency.Analysis chain
Ohayo, sensei! Verify the
resourcefield usage.The change from
"data"to"key"for theresourcefield is approved. Ensure that this field is consistently used as a unique identifier throughout the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `resource` as a key. # Test: Search for the `resource` field usage. Expect: Consistent use as a key. rg --type json '"resource"' -A 3Length of output: 26757
remybar
force-pushed
the
event_fields_as_key
branch
from
02a33d8
to
94843af
Compare
Larkooo
force-pushed
the
event_fields_as_key
branch
from
94843af
to
2aa07db
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2280 +/- ##
==========================================
- Coverage 67.86% 67.86% -0.01%
==========================================
Files 359 359
Lines 46959 47003 +44
==========================================
+ Hits 31869 31898 +29
- Misses 15090 15105 +15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
the event messages are not working anymore.
@Larkooo @lambda-0x as we have the event messages processors being a catch all, it will run on every message as we only check the length of the keys.
But as now, most of events have keys, and they contains ByteArray, the length is not longer a good filter.
We end up with this processor being run on all StoreSetRecord etc...
Some solutions we may have:
- Blacklist all the world events, which ensure we only process
emit!generated events. But this is too bad for Torii performances. - When using
emit!, we already happen the system's address, we may also happen aselector!("SystemEvent")before the address. Like so, the event message processor can directly check the-2position in the array. - We could wrap the system event into a world's event to make the first key (the event selector) the actual
SystemEvent. - Something else?
|
was just thinking about this yesterday! since we already have ability to add custom key in the event the current behaviour of trying it for every event is slow in performance. i think 2 and 3 makes sense to me, preferably 3 if possible so we have to just first key for all types of event. |
Yeah I think blacklisting is not a sustainable solution, so 2 and 3 make the most sense to me. I think it just depends on if its going to be a breaking change or not. I remember when I was working on the event messages we had to follow the same event structure as a normal event for it to also work for the emit!. We need to take that into consideration and also what happens if we index older event messages (old structure) and newer event messages. Seems like we cannot have both |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (9)
examples/spawn-and-move/manifests/release/base/contracts/dojo_examples-actions-40b6994c.toml (1)
2-3: Ohayo, sensei! Missing ABI file and incorrect paths detected.The ABI file for the contract
dojo_examples-actions-40b6994cis missing, and the paths specified in the manifest files are incorrect. This could lead to deployment or integration issues.
- Ensure the ABI file exists and is correctly tracked in the repository.
- Verify and update the paths in the manifest files to point to the correct ABI file location.
Analysis chain
Ohayo, sensei! New class hash detected. Let's verify its implications.
The
class_hashandoriginal_class_hashhave been updated to the same value. This suggests a fresh deployment or significant update to the contract.To ensure this change doesn't introduce unexpected behavior, please run the following script:
This script will help us confirm that the new class hash is consistently used and that any related deployment scripts or ABIs have been updated accordingly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new class hash and its usage # Test 1: Check if the new class hash exists in other files echo "Checking for occurrences of the new class hash:" rg --type toml "0x270c8ffb9f92d3711fd67d9842f7519c663ddb17f15ddafd08bfc2aeb7d86d9" # Test 2: Check for any deployment scripts or logs referencing this contract echo "Checking for deployment scripts or logs:" rg --type yaml --type sh "dojo_examples-actions-40b6994c" # Test 3: Verify if there are any changes to the contract's ABI echo "Checking for changes in the contract's ABI:" git diff HEAD~1 manifests/release/base/abis/contracts/dojo_examples-actions-40b6994c.jsonLength of output: 1785
Script:
#!/bin/bash # Description: Further investigate the ABI file and other references # Test 1: Check if the ABI file exists in the repository echo "Checking for the existence of the ABI file:" fd 'dojo_examples-actions-40b6994c.json' manifests/release/base/abis/contracts # Test 2: Search for any other references to the contract in the repository echo "Searching for other references to the contract:" rg 'dojo_examples-actions-40b6994c'Length of output: 1441
crates/torii/core/src/processors/event_message.rs (4)
24-24: Sugoi change toevent_key, sensei!Returning
SYSTEM_EVENT_SELECTORinstead of an empty string is a great improvement. It provides a more meaningful event key representation.Consider using
SYSTEM_EVENT_SELECTOR.into()instead of.to_string()for better performance, as it avoids an unnecessary allocation:- SYSTEM_EVENT_SELECTOR.to_string() + SYSTEM_EVENT_SELECTOR.into()
29-31: Arigatou for the clear comments, sensei!The updated comments in the
validatefunction greatly improve the understanding of the expected event key structure. Well done!Consider adding a brief explanation of what
SYSTEM_EVENT_SELECTORrepresents for developers who might not be familiar with it:- // 1: SYSTEM_EVENT_SELECTOR + // 1: SYSTEM_EVENT_SELECTOR (identifies system-level events)
51-58: Subarashii error handling, sensei!The addition of a warning log when the model is not found is a great improvement. It enhances visibility into potential issues during event processing.
Consider including the
event_idin the warning log for easier tracing:warn!( target: LOG_TARGET, model = %event.keys[MODEL_INDEX], + event_id = %event_id, "System event model not found." );
67-71: Kakkoii update tokeys_and_unpacked, sensei!Skipping the first two keys aligns well with the updated event structure expectations. Good job on maintaining consistency!
Consider extracting the index calculations into named constants for better readability:
+ const EVENT_SELECTOR_INDEX: usize = 1; + const KEYS_START_INDEX: usize = MODEL_INDEX + 1; + const SYSTEM_KEY_INDEX: usize = event.keys.len() - 1; - let mut keys_and_unpacked = - [event.keys[MODEL_INDEX + 1..event.keys.len() - 1].to_vec(), event.data.clone()] - .concat(); + let mut keys_and_unpacked = [ + event.keys[KEYS_START_INDEX..SYSTEM_KEY_INDEX].to_vec(), + event.data.clone() + ].concat();This change would make the code more self-documenting and easier to maintain.
crates/torii/core/src/processors/metadata_update.rs (2)
38-39: Ohayo, sensei! The validation looks tighter now.The change to require exactly two keys aligns well with the expected structure. Nice work!
Consider updating the error log message to reflect this specific requirement:
- "Invalid event keys." + "Invalid event keys. Expected exactly 2 keys (selector and resource)."This will provide more precise information for debugging.
Line range hint
1-168: Ohayo, sensei! Overall, these changes look solid.The modifications to the
MetadataUpdateProcessorreflect a shift in how event data is structured and processed. The validation is now more precise, and the event data extraction has been adjusted accordingly. These changes seem to be part of a broader effort to standardize event handling across the system.As we move forward with these changes, consider the following:
- Ensure that all other event processors in the system are updated to match this new structure.
- Update any documentation or comments in other parts of the codebase that describe the event structure.
- If there are any external systems or services that interact with these events, make sure they're aware of the changes in the event structure.
These steps will help maintain consistency and prevent potential issues down the line. Keep up the great work, sensei!
examples/spawn-and-move/manifests/dev/deployment/manifest.json (2)
Line range hint
1-2348: Ohayo, sensei! Let's wrap up this massive upgrade!This manifest update represents a significant overhaul of the system:
- Multiple contract upgrades, including the world contract
- Extensive changes to event structures, with many fields changing from "data" to "key"
- New deployment addresses and transaction hashes
These changes will likely impact event indexing, querying, and overall system behavior.
Consider the following:
- Ensure all event listeners and data processors are updated to handle the new event structures
- Update any external systems or UIs that rely on the old event formats
- Thoroughly test the system to ensure all upgrades are compatible and functioning as expected
- Update documentation to reflect these changes, especially regarding event handling and contract addresses
Great work on this upgrade, sensei! Just make sure everything is properly tested and documented.
1667-1669: Ohayo, sensei! It seems that the contract upgrade involving theclass_hash0x4590a27e4ec7366358ba5f60323777f301435ebbdd113ab02c54b947717530dis not explicitly documented in the manifest files. The entries are present across multiple files, but there are no accompanying comments or documentation explaining the upgrade process. You might want to ensure that this upgrade is documented elsewhere or consider adding some comments for clarity.Analysis chain
Another contract upgrade, sensei!
The
address,class_hash, andoriginal_class_hashfor another contract have been updated. This suggests multiple contracts are being upgraded in this deployment.Let's check if this upgrade is properly documented and handled:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the new class hash rg "0x4590a27e4ec7366358ba5f60323777f301435ebbdd113ab02c54b947717530d"Length of output: 1386
Script:
#!/bin/bash # Search for documentation or comments related to the class hash rg "0x4590a27e4ec7366358ba5f60323777f301435ebbdd113ab02c54b947717530d" -A 5Length of output: 3623
|
@Larkooo should be ok, will let you make your checks and let's move forward if all good. |
| @@ -1,5 +1,7 @@ | |||
| use starknet::core::types::Felt; | |||
|
|
|||
| pub const SYSTEM_EVENT_SELECTOR: &str = "SystemEvent__"; | |||
There was a problem hiding this comment.
Just curious as to why we have that for the event messages? Can't we just do something similar to StoreSetRecord? Like EventMessage and define a wrapper struct?
There was a problem hiding this comment.
Hum, we could do something like:
pub struct EventMessage {
#[keys]
pub keys: Span<felt252>,
pub values: Span<felt252>,
}The problem with that is in the keys we will have the span length. But using a manual selector, we're not injecting the length of the span into the keys, which may break the indexing and query by keys.
| // and dont include last key as its the system key | ||
| let mut keys_and_unpacked = | ||
| [event.keys[1..event.keys.len() - 1].to_vec(), event.data.clone()].concat(); | ||
| [event.keys[MODEL_INDEX + 1..event.keys.len() - 1].to_vec(), event.data.clone()] |
There was a problem hiding this comment.
Maybe we should move the event.keys[] to a variable of its own
| warn!( | ||
| target: LOG_TARGET, | ||
| model = %event.keys[MODEL_INDEX], | ||
| "System event model not found." |
There was a problem hiding this comment.
I feel like it might be confusing referring to event messages as SystemEvents and EventMessages. Should we settle on one unique event name?
There was a problem hiding this comment.
Ideally as we mentioned during a call, we will actually only have event messages. Which should remove the confusion.
We could switch to Event message and keep this nomenclature yeah. Then I'll complete this PR by restricting events to only be available in Dojo and not using Starknet event, wdyt?
Closes #2228.
Depends on dojoengine/dojo-core#12.
TODO:
dojo-coreshould be removed as it has been moved todojo-corerepo.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
Summary by CodeRabbit
New Features
Eventenum for streamlined event handling in tests, enhancing type safety and modularity.Bug Fixes
Chores