feat(sozo): add events command back and event better #2619
Conversation
WalkthroughOhayo, sensei! This pull request introduces several significant changes across multiple files, primarily focusing on enhancing event handling and command processing in the system. Key modifications include the restructuring of the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 6
🧹 Outside diff range and nitpick comments (10)
crates/dojo/world/src/diff/resource.rs (1)
112-118: Implementation follows consistent patterns, sensei!The new
abi()method:
- Maintains consistency with existing methods in the enum
- Properly delegates to the local resource for all variants
- Follows the same pattern as other accessor methods
This implementation aligns well with the single responsibility principle, delegating the actual ABI retrieval to the local resource while maintaining a consistent interface at the diff level.
crates/dojo/world/src/local/resource.rs (1)
103-111: Consider adding documentation for the new method.Ohayo sensei! The implementation looks solid and follows the established patterns in the codebase. However, it would be beneficial to add documentation comments explaining:
- The purpose of the method
- What the ABI entries represent
- Why certain resource types return an empty vector
Consider adding documentation like this:
+ /// Returns the ABI entries for the resource. + /// + /// # Returns + /// - For Contract, Model, and Event resources: The ABI entries from their class + /// - For other resource types: An empty vector pub fn abi(&self) -> Vec<AbiEntry> {bin/sozo/src/commands/mod.rs (1)
Line range hint
1-138: Consider adding integration tests for the Events command.Sensei, while the code structure looks good, it would be beneficial to have integration tests to verify the Events command behavior, especially since it's being added back with enhanced functionality.
Would you like me to help create integration tests for the Events command functionality?
bin/sozo/src/commands/model.rs (2)
75-80: Block parameter additions are well-structured, sensei!The optional block parameters are consistently implemented across commands with clear help messages. Consider adding examples to the help text to show the expected format of block numbers.
#[arg( - help = "Block number at which to retrieve the model layout (pending block by default)" + help = "Block number at which to retrieve the model layout (e.g., 123456, defaults to pending block)" )]Also applies to: 97-102, 120-123
165-179: Implementation looks solid, sensei! A few suggestions for enhancement.The block handling implementation is consistent and robust across all commands. However, for the Get command's output, consider structuring the response in a more readable format.
Consider enhancing the output formatting in the Get command:
- println!("{}", record); + println!("Block: {}", block_id); + println!("Record: {:#?}", record);This would provide more context and better formatting for the output.
Also applies to: 182-194, 200-218
bin/sozo/src/commands/events.rs (5)
6-6: Ohayo, sensei! Confirm the need forcolored::Colorize.The addition of
use colored::Colorize;suggests that colorized output is intended. Ensure this aligns with the application's requirements and that it doesn't cause issues in environments where color support is limited.
35-35: Set a meaningful default forchunk_size.The default value for
chunk_sizeis set to100. Consider whether this default optimally balances performance and resource utilization for typical use cases.
80-82: Improve error handling for event parsing.In the
matchstatement handlingWorldEvent::try_from(event), consider providing more detailed error messages or retry logic to handle transient parsing issues.
103-106: Ohayo, sensei! Provide guidance on using continuation tokens.When printing the continuation token, consider adding instructions for the user on how to use it to fetch subsequent pages of events.
301-318: Implement pretty printing for updated members.The TODO comment on line 302 suggests the intention to improve the output format. Implementing this enhancement will provide clearer information to the users.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
bin/sozo/src/commands/events.rs(3 hunks)bin/sozo/src/commands/mod.rs(4 hunks)bin/sozo/src/commands/model.rs(6 hunks)crates/dojo/world/src/diff/resource.rs(2 hunks)crates/dojo/world/src/local/resource.rs(2 hunks)crates/sozo/ops/src/events.rs(0 hunks)crates/sozo/ops/src/model.rs(5 hunks)
💤 Files with no reviewable changes (1)
- crates/sozo/ops/src/events.rs
🔇 Additional comments (14)
crates/dojo/world/src/diff/resource.rs (1)
3-3: Ohayo! Clean import addition, sensei!
The AbiEntry import is well-placed and properly grouped with other starknet-related imports.
crates/dojo/world/src/local/resource.rs (1)
2-2: LGTM! Clean import addition.
Ohayo! The addition of AbiEntry to the existing import is clean and focused.
bin/sozo/src/commands/mod.rs (3)
5-5: LGTM! Clean module organization.
Ohayo! The new events module is properly imported and declared, maintaining good code organization principles.
Also applies to: 13-13
57-58: LGTM! Well-documented command addition.
The Events command is added with a clear, descriptive about text that aligns with the PR objectives. The use of Box is appropriate for heap allocation of potentially large event data.
74-74: LGTM! Consistent formatting implementation.
The fmt implementation follows the established pattern for command string representation.
bin/sozo/src/commands/model.rs (2)
7-7: Ohayo! Import changes look good, sensei!
The addition of BlockId and BlockTag types is well-aligned with the new block-specific functionality being introduced.
173-179: Verify model operations integration.
Let's ensure that the model operations properly handle the block_id parameter.
Also applies to: 208-213
✅ Verification successful
Let me gather more information about how these model operations are implemented.
Block ID is properly handled in model operations, sensei! ✨
The verification shows that all model operations (model_layout, model_schema, and model_get) correctly handle the block_id parameter by:
- Accepting
block_id: BlockIdas a parameter - Setting it on the
WorldContractReaderviaworld_reader.set_block(block_id)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if model operations properly handle block_id parameter
# Expected: Find implementations of model operations showing block_id handling
# Search for model operation implementations
rg -A 5 "fn model_(layout|schema|get)" "crates/sozo/ops/src/model.rs"
Length of output: 468
Script:
#!/bin/bash
# Check the implementation details of model operations
rg -A 15 "pub async fn model_(layout|schema|get)" "crates/sozo/ops/src/model.rs"
# Also check for any block_id usage in the implementations
rg "block_id" "crates/sozo/ops/src/model.rs"
Length of output: 1622
crates/sozo/ops/src/model.rs (4)
47-57: Ohayo! The block_id parameter addition looks good, sensei!
The enhancement to model_layout function with the block_id parameter provides more flexibility in block management, aligning well with the PR objectives.
78-85: The model_schema changes are consistent, sensei!
The addition of block_id parameter maintains consistency with other model operations while enhancing block management capabilities.
104-105: Nice improvements to model_get, sensei!
The changes enhance the function in two ways:
- Added block_id parameter for consistent block management
- Enhanced return type to include formatted string representation, improving usability
Also applies to: 114-114, 120-120
516-521: Clean implementation of format_deep_record, sensei!
The function effectively combines keys and values while maintaining the existing formatting logic, providing a good foundation for the enhanced model_get return type.
bin/sozo/src/commands/events.rs (3)
58-59: Ohayo, sensei! Verify error propagation from get_world_diff_and_provider.
When calling utils::get_world_diff_and_provider, ensure that any errors are properly propagated and that the user receives clear feedback if the operation fails.
91-92: Avoid logging sensitive event information.
When logging errors in tracing::error!(?e, "Failed to process event: {:?}", ev);, ensure that no sensitive or private data from the event is included in the logs.
[security_issue]
370-373: Ohayo, sensei! Verify that block and transaction formatting is user-friendly.
The block and transaction identifiers are formatted for display. Ensure that they are displayed in a way that users can easily reference them.
| @@ -90,6 +95,7 @@ pub fn run(command: Commands, config: &Config) -> Result<()> { | |||
| Commands::Hash(args) => args.run().map(|_| ()), | |||
| Commands::Init(args) => args.run(config), | |||
| Commands::Model(args) => args.run(config), | |||
| Commands::Events(args) => args.run(config), | |||
There was a problem hiding this comment.
💡 Codebase verification
Error handling needs improvement in EventsArgs::run implementation
Ohayo sensei! After reviewing the implementation, there are some concerning patterns in the error handling:
- The main
runfunction properly propagates errors with?operator for critical operations - However, there are multiple unsafe
.unwrap()calls when accessing tags, which could panic:- Multiple instances of
tags.get(&e.selector).unwrap() tags.get(&e.resource).unwrap()- These should be handled gracefully with proper error propagation or provide meaningful error messages
- Multiple instances of
The code should be updated to handle missing tags safely, either by:
- Using
tags.get().ok_or_else(|| anyhow!("Missing tag for selector: {}", selector))? - Or providing default values with
tags.get().unwrap_or_default()
🔗 Analysis chain
Verify error propagation in the Events command.
The run implementation follows the pattern of other commands, but let's verify the error handling in the EventsArgs::run implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns in EventsArgs::run
# Look for proper error handling patterns in the events implementation
# Search for Result/Error types and error handling
ast-grep --pattern 'Result<$_>' events.rs
# Look for error propagation patterns
rg '(\?|\.unwrap\(\)|\.expect\(|panic!)' events.rs
Length of output: 203
Script:
#!/bin/bash
# First, let's find the correct path to the events implementation
fd -t f "events" --exec echo '{}'
# Then let's examine the implementation
ast-grep --pattern 'impl EventsArgs {
$$$
fn run($$$) -> Result<$_> {
$$$
}
$$$
}'
# Also search for any file containing EventsArgs struct and its run method
rg -A 5 "struct EventsArgs"
rg -A 10 "impl EventsArgs"
# Look for error handling patterns in files containing EventsArgs
fd -t f "events" --exec rg -l "EventsArgs" {} \; | xargs rg '(\?|\.unwrap\(\)|\.expect\(|panic!)'
Length of output: 3711
| let ws = scarb::ops::read_workspace(config.manifest_path(), config)?; | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling when loading the workspace.
In let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;, if the workspace fails to load, the error might not provide sufficient context. Enhance error messages to aid in troubleshooting.
Would you like assistance in improving the error handling for workspace loading?
| let to_block = self.to_block.map(BlockId::Number); | ||
| let keys = self | ||
| .events | ||
| .map(|e| vec![e.iter().map(|event| starknet_keccak(event.as_bytes())).collect()]); |
There was a problem hiding this comment.
Handle empty event lists gracefully.
When mapping over self.events, if the list is empty, the event filter may not behave as expected. Consider adding a check to handle empty event lists to prevent potential issues.
| .collect::<Vec<String>>() | ||
| .join(", "), | ||
| record | ||
| ), | ||
| ) | ||
| } | ||
| WorldEvent::StoreUpdateRecord(e) => { | ||
| let tag = tags.get(&e.selector).unwrap(); | ||
| // TODO: model value impl + print. | ||
| ( | ||
| format!("Store update record ({})", tag), | ||
| format!( | ||
| "Selector: {:#066x}\nEntity ID: {:#066x}\nValues: {}", | ||
| e.selector, | ||
| e.entity_id, | ||
| e.values | ||
| .iter() | ||
| .map(|v| format!("{:#066x}", v)) | ||
| .collect::<Vec<String>>() | ||
| .join(", "), | ||
| ), | ||
| ) | ||
| } | ||
| WorldEvent::StoreUpdateMember(e) => { | ||
| let tag = tags.get(&e.selector).unwrap(); | ||
| // TODO: pretty print of the value. | ||
| ( | ||
| format!("Store update member ({})", tag), | ||
| format!( | ||
| "Selector: {:#066x}\nEntity ID: {:#066x}\nMember selector: {:#066x}\nValues: \ | ||
| {}", | ||
| e.selector, | ||
| e.entity_id, | ||
| e.member_selector, | ||
| e.values | ||
| .iter() | ||
| .map(|v| format!("{:#066x}", v)) | ||
| .collect::<Vec<String>>() | ||
| .join(", "), | ||
| ), | ||
| ) | ||
| } | ||
| WorldEvent::StoreDelRecord(e) => { | ||
| let tag = tags.get(&e.selector).unwrap(); | ||
| ( | ||
| format!("Store del record ({})", tag), | ||
| format!("Selector: {:#066x}\nEntity ID: {:#066x}", e.selector, e.entity_id,), | ||
| ) | ||
| } | ||
| WorldEvent::EventEmitted(e) => { | ||
| let tag = tags.get(&e.selector).unwrap(); | ||
| let contract_tag = if let Some(selector) = | ||
| contract_selectors_from_address.get(&e.system_address.into()) | ||
| { | ||
| tags.get(selector).unwrap().to_string() | ||
| } else { | ||
| format!("{:#066x}", e.system_address.0) | ||
| }; | ||
|
|
||
| let (record, _, _) = model::model_get( | ||
| tag.clone(), | ||
| e.keys.clone(), | ||
| world_diff.world_info.address, | ||
| provider, | ||
| block_id, | ||
| ) | ||
| .await?; | ||
|
|
||
| ( | ||
| format!("Event emitted ({})", tag), | ||
| format!( | ||
| "Selector: {:#066x}\nContract: {}\nHistorical: {}\nKeys: {}\nValues: \ | ||
| {}\nData:\n{}", | ||
| e.selector, | ||
| contract_tag, | ||
| e.historical, | ||
| e.keys | ||
| .iter() | ||
| .map(|k| format!("{:#066x}", k)) | ||
| .collect::<Vec<String>>() | ||
| .join(", "), | ||
| e.values | ||
| .iter() | ||
| .map(|v| format!("{:#066x}", v)) | ||
| .collect::<Vec<String>>() | ||
| .join(", "), | ||
| record | ||
| ), | ||
| ) | ||
| } | ||
| _ => ("Unprocessed event".to_string(), format!("Event: {:?}", event)), | ||
| }; | ||
|
|
||
| let block_str = block_number.map(|n| n.to_string()).unwrap_or("pending".to_string()); | ||
| let ptr = format!("[block:{} / tx:{:#066x}]", block_str, transaction_hash).bright_black(); | ||
|
|
||
| println!("> {name} {ptr}\n{content}\n-----\n"); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor match_event for better readability and maintainability.
The match_event function is quite extensive, handling numerous event types in a single block. Breaking it down into smaller helper functions or modules could improve readability and make maintenance easier.
For example, you could extract each event type handling into its own function:
async fn handle_world_spawned_event(...) { ... }
async fn handle_world_upgraded_event(...) { ... }
// And so on for each event type.| let tag = tags.get(&e.selector).unwrap(); | ||
| let (record, _, _) = model::model_get( | ||
| tag.clone(), | ||
| e.keys.clone(), | ||
| world_diff.world_info.address, | ||
| provider, | ||
| block_id, | ||
| ) | ||
| .await?; | ||
|
|
||
| ( | ||
| format!("Store set record ({})", tag), | ||
| format!( | ||
| "Selector: {:#066x}\nEntity ID: {:#066x}\nKeys: {}\nValues: {}\nData:\n{}", | ||
| e.selector, | ||
| e.entity_id, | ||
| e.keys | ||
| .iter() | ||
| .map(|k| format!("{:#066x}", k)) | ||
| .collect::<Vec<String>>() | ||
| .join(", "), | ||
| e.values | ||
| .iter() | ||
| .map(|v| format!("{:#066x}", v)) | ||
| .collect::<Vec<String>>() | ||
| .join(", "), | ||
| record | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors in StoreSetRecord event processing.
In the StoreSetRecord event handling, ensure that the call to model::model_get handles errors appropriately, possibly with informative messages if the model retrieval fails.
| let tag = tags.get(&e.resource).unwrap(); | ||
| let grantee = | ||
| if let Some(selector) = contract_selectors_from_address.get(&e.contract.into()) { | ||
| tags.get(selector).unwrap().to_string() | ||
| } else { | ||
| format!("{:#066x}", e.contract.0) | ||
| }; | ||
|
|
||
| ( | ||
| "Writer updated".to_string(), | ||
| format!("Target resource: {}\nContract: {}\nValue: {}", tag, grantee, e.value), | ||
| ) | ||
| } | ||
| WorldEvent::OwnerUpdated(e) => { | ||
| let tag = tags.get(&e.resource).unwrap(); | ||
| let grantee = | ||
| if let Some(selector) = contract_selectors_from_address.get(&e.contract.into()) { | ||
| tags.get(selector).unwrap().to_string() | ||
| } else { | ||
| format!("{:#066x}", e.contract.0) | ||
| }; |
There was a problem hiding this comment.
Ohayo, sensei! Ensure safe unwrapping of options in WriterUpdated and OwnerUpdated events.
In lines where tags.get(&e.resource).unwrap() and tags.get(selector).unwrap() are called, consider handling the None case to prevent potential panics.
Apply this diff to handle possible None values:
-let tag = tags.get(&e.resource).unwrap();
+let tag = match tags.get(&e.resource) {
+ Some(t) => t,
+ None => {
+ tracing::warn!("Tag not found for resource: {:#066x}", e.resource);
+ return Ok(());
+ }
+};Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2619 +/- ##
==========================================
- Coverage 57.25% 56.90% -0.36%
==========================================
Files 396 397 +1
Lines 49162 49466 +304
==========================================
Hits 28148 28148
- Misses 21014 21318 +304 ☔ View full report in Codecov by Sentry. |
Now the
sozo eventscommand is able to resolve all tags and addresses for a very complete logbook without the need of spinning up a Torii.Summary by CodeRabbit
Release Notes
New Features
Eventscommand to inspect events emitted by the world.Layout,Schema, andGetcommands for model data retrieval.Bug Fixes
Chores
These updates enhance the overall user experience by providing more robust event handling and flexible data retrieval options.