feat(sozo): selector from tag command on sozo #2282
Conversation
WalkthroughOhayo, sensei! The recent updates significantly enhance the command handling of the application by introducing both Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Commands
participant Selector
participant Hash
User->>CLI: Executes Selector command with tag
CLI->>Commands: Match Commands::Selector
Commands->>Selector: Invoke run method with SelectorArgs
Selector->>Config: Process tag and compute selector
Selector->>User: Display computed selector
User->>CLI: Executes Hash command with input
CLI->>Commands: Match Commands::Hash
Commands->>Hash: Invoke run method with HashArgs
Hash->>Config: Process input and compute hashes
Hash->>User: Display computed hashes
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Additional comments not posted (6)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
bin/sozo/src/commands/selector.rs
Outdated
| impl SelectorArgs { | ||
| pub fn run(self, config: &Config) -> Result<()> { | ||
| trace!(args = ?self); | ||
|
|
||
| println!("{:#x}", compute_selector_from_tag(&self.tag)); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Consider adding error handling for robustness.
The run method currently assumes that compute_selector_from_tag will always succeed. It would be wise to handle potential errors to prevent unexpected crashes.
pub fn run(self, config: &Config) -> Result<()> {
trace!(args = ?self);
match compute_selector_from_tag(&self.tag) {
Ok(selector) => println!("{:#x}", selector),
Err(e) => eprintln!("Error computing selector: {}", e),
}
Ok(())
}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (4)
bin/sozo/src/commands/mod.rs (2)
17-17: Ohayo, sensei! Add missing module documentation.The new
hashmodule is introduced without any documentation. Consider adding a brief comment to explain its purpose.// Module for handling hash-related commands pub(crate) mod hash;
72-73: Ohayo, sensei! TheHashcommand description needs revision.The current description "Select a model" does not accurately reflect the functionality of the
Hashcommand. TheHashcommand is designed to hash input values, as indicated by theHashArgsstruct and itsrunmethod. Consider updating the description to better align with its purpose.
- File:
bin/sozo/src/commands/mod.rs- Line: 72
Please update the description to accurately reflect the command's functionality.
Analysis chain
Ensure consistency in command descriptions.
The description for the
Hashcommand is "Select a model," which might not accurately reflect its functionality. Ensure it aligns with the actual purpose of the command.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the purpose of the `Hash` command. # Test: Search for the command usage in documentation or comments. Expect: Consistent descriptions. rg --type markdown --type rust -A 2 "Hash"Length of output: 361719
Script:
#!/bin/bash # Search for the definition and usage of `HashArgs` to understand the `Hash` command's purpose. ast-grep --lang rust --pattern 'struct HashArgs { $$$ }'Length of output: 58
Script:
#!/bin/bash # Search for comments or documentation related to `HashArgs` for additional context on the `Hash` command. rg --type rust -A 3 "HashArgs"Length of output: 2766
bin/sozo/src/commands/hash.rs (2)
1-7: Ohayo, sensei! Consider organizing imports.The imports are comprehensive, but consider grouping them by functionality or origin for better readability.
// External crates use anyhow::Result; use clap::Args; use tracing::trace; // Starknet related use starknet::core::types::Felt; use starknet::core::utils::{get_selector_from_name, starknet_keccak}; use starknet_crypto::{poseidon_hash_many, poseidon_hash_single}; // Dojo specific use dojo_world::contracts::naming::compute_selector_from_tag;
65-70: Ohayo, sensei! Improve error messages forfelt_from_str.The error messages could be more descriptive to help users understand the input format requirements.
if s.starts_with("0x") { return Ok(Felt::from_hex(s).map_err(|_| anyhow::anyhow!("Invalid hex format for Felt"))?); } Ok(Felt::from_dec_str(s).map_err(|_| anyhow::anyhow!("Invalid decimal format for Felt"))?)
| impl HashArgs { | ||
| pub fn run(self) -> Result<Vec<String>> { | ||
| trace!(args = ?self); | ||
|
|
||
| if self.input.is_empty() { | ||
| return Err(anyhow::anyhow!("Input is empty")); | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Validate input handling.
The input validation checks for an empty string but consider trimming whitespace before this check to avoid false negatives.
if self.input.trim().is_empty() {
return Err(anyhow::anyhow!("Input is empty"));
}| if !self.input.contains(',') { | ||
| let felt = felt_from_str(&self.input)?; | ||
| let poseidon = format!("{:#066x}", poseidon_hash_single(felt)); | ||
| let snkeccak = format!("{:#066x}", starknet_keccak(&felt.to_bytes_le())); | ||
|
|
||
| println!("Poseidon: {}", poseidon); | ||
| println!("SnKeccak: {}", snkeccak); | ||
|
|
||
| return Ok(vec![poseidon.to_string(), snkeccak.to_string()]); | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Handle potential errors gracefully.
The felt_from_str function is used within a map and could panic if the input is invalid. Consider handling this gracefully.
.map(|s| felt_from_str(s.trim()).unwrap_or_else(|_| panic!("Invalid felt value")))
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2282 +/- ##
==========================================
+ Coverage 66.88% 66.96% +0.07%
==========================================
Files 342 343 +1
Lines 45149 45258 +109
==========================================
+ Hits 30198 30305 +107
- Misses 14951 14953 +2 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Bug Fixes