feat(sozo): split hash command into 'hash compute' and 'hash find' #2892
Conversation
WalkthroughOhayo, sensei! The pull request refines the hash command in the Sozo CLI by transforming the Changes
Sequence DiagramsequenceDiagram
participant User
participant HashCommand
participant Config
User->>HashCommand: Execute with command
alt Compute Hash
HashCommand->>HashCommand: compute(input)
HashCommand-->>User: Return hash result
else Find Hash
HashCommand->>Config: Retrieve namespaces/resources
HashCommand->>HashCommand: find(hash, namespaces, resources)
HashCommand-->>User: Return matching results
end
🪧 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: 0
🧹 Nitpick comments (4)
bin/sozo/src/commands/hash.rs (4)
32-32: Ohayo! Add description for Find command, sensei!The Find command is missing its about string which should describe its functionality.
- #[command(about = "")] + #[command(about = "Search for a hash among namespaces and resource names/tags")]
65-68: Enhance error message clarity, sensei!The error message could be more descriptive about the 32-character limit for Starknet selectors.
- return Err(anyhow::anyhow!("Input is too long for a starknet selector")); + return Err(anyhow::anyhow!( + "Input exceeds the 32-character limit for a Starknet selector" + ));
98-104: Consider performance optimization and error handling improvements, sensei!The find method could benefit from:
- Early validation of input parameters
- Capacity hints for collections to avoid reallocations
pub fn find( &self, config: &Config, hash: &String, namespaces: Option<Vec<String>>, resources: Option<Vec<String>>, ) -> Result<()> { + // Early validation of input parameters + if namespaces.as_ref().map_or(false, |n| n.is_empty()) + && resources.as_ref().map_or(false, |r| r.is_empty()) + { + return Err(anyhow::anyhow!( + "At least one namespace or resource must be provided" + )); + }
112-147: Optimize collection initialization, sensei!Consider providing capacity hints for HashSet to avoid reallocations.
- let mut ns_from_config = HashSet::new(); + let mut ns_from_config = HashSet::with_capacity(10); // Adjust capacity based on expected size
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/sozo/src/commands/hash.rs(3 hunks)bin/sozo/src/commands/mod.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
bin/sozo/src/commands/mod.rs (1)
115-115: Ohayo! LGTM, sensei!The modification to pass the config to the hash command's run method is necessary to support the new find subcommand's functionality.
bin/sozo/src/commands/hash.rs (1)
Line range hint
223-281: Add tests for find functionality, sensei!While the compute functionality is well-tested, there are no tests for the new find functionality. Consider adding tests for:
- Finding namespace matches
- Finding resource name matches
- Finding tag matches
- Invalid hash handling
- Empty configuration scenarios
Would you like me to help generate these test cases?
remybar
force-pushed
the
sozo-hash_command
branch
from
80a9963 to
9bbff84
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
bin/sozo/src/commands/hash.rs (1)
Line range hint
226-283: Let's enhance test coverage, sensei!While the existing tests are good, we're missing tests for the new
findcommand functionality.Consider adding these test cases:
#[test] fn test_hash_find_namespace() { let config = Config::default(); let hash = "0x123"; // Use an actual computed namespace hash let args = HashArgs { command: HashCommand::Find { hash: hash.to_string(), namespaces: Some(vec!["test_namespace".to_string()]), resources: None, } }; let result = args.find(&config, &hash.to_string(), None, None); assert!(result.is_ok()); } #[test] fn test_hash_find_with_config() { let config = Config::default(); let args = HashArgs { command: HashCommand::Find { hash: "0x123".to_string(), namespaces: None, resources: None, } }; let result = args.find(&config, &"0x123".to_string(), None, None); assert!(result.is_ok()); }
🧹 Nitpick comments (3)
bin/sozo/src/commands/hash.rs (3)
23-49: Well-structured command interface, sensei!The HashCommand enum is well-designed with clear documentation. Consider adding examples in the help messages to make it even more user-friendly.
#[command(about = "Compute the hash of the provided input.")] Compute { - #[arg(help = "Input to hash. It can be a comma separated list of inputs or a single \ - input. The single input can be a dojo tag or a felt.")] + #[arg(help = "Input to hash. It can be a comma separated list of inputs or a single \ + input. The single input can be a dojo tag or a felt. \ + Examples: 'dojo_examples-actions', '0x1,0x2,0x3'")]
Line range hint
52-97: Consider enhancing error handling and readability, sensei!The compute method handles multiple input types well, but could benefit from:
- More descriptive error messages
- Extraction of the hash computation logic into separate functions
pub fn compute(&self, input: &str) -> Result<()> { if input.is_empty() { - return Err(anyhow::anyhow!("Input is empty")); + return Err(anyhow::anyhow!("Input cannot be empty. Please provide a value to hash.")); } + + self.compute_impl(input) + } + + fn compute_impl(&self, input: &str) -> Result<()> { if input.contains('-') { return self.compute_tag(input); }
100-209: Ohayo! Let's improve the find method's organization, sensei!The find method is comprehensive but could be more maintainable if split into smaller functions.
Consider extracting the namespace and resource collection logic into separate functions:
+ fn collect_namespaces_from_config(&self, profile_config: &ProfileConfig, manifest: &Option<Manifest>) -> Vec<String> { + let mut ns_from_config = HashSet::new(); + ns_from_config.insert(profile_config.namespace.default); + // ... rest of namespace collection logic + Vec::from_iter(ns_from_config) + } + + fn collect_resources_from_config(&self, profile_config: &ProfileConfig, manifest: &Option<Manifest>) -> Vec<String> { + let mut res_from_config = HashSet::new(); + // ... rest of resource collection logic + Vec::from_iter(res_from_config) + }Also, consider adding error handling for cases where no matches are found:
// --- find the hash --- + let mut matches_found = false; // could be a namespace hash for ns in &namespaces { if hash == compute_bytearray_hash(ns) { println!("Namespace found: {ns}"); + matches_found = true; } } + + if !matches_found { + println!("No matches found for hash: {:#066x}", hash); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/sozo/src/commands/hash.rs(3 hunks)bin/sozo/src/commands/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/sozo/src/commands/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
bin/sozo/src/commands/hash.rs (2)
1-20: Ohayo sensei! Clean command structure implementation!The command structure follows clap's best practices for subcommands, and the imports are well-organized.
212-221: Clean command dispatch implementation!The run method is well-structured with proper tracing.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2892 +/- ##
==========================================
+ Coverage 55.77% 55.82% +0.05%
==========================================
Files 446 449 +3
Lines 57794 57824 +30
==========================================
+ Hits 32234 32283 +49
+ Misses 25560 25541 -19 ☔ View full report in Codecov by Sentry. |
glihm
changed the title
There was a problem hiding this comment.
Thanks @remybar for the work here.
- With namespaces, doesn't seem to work.
cargo run --bin sozo -r -- --manifest-path ./examples/simple/Scarb.toml hash find 0x02988854b91401f98206dff94028e22dce27feb5fbfdc2843bbe299db9660d49
This must find the ns namespace.
2. It's a bit weird at first if the command doesn't find anything, no output shown. Wondering if it would be better having "No resources matches the provided hash.", and return error instead of Ok.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
bin/sozo/src/commands/hash.rs (4)
Line range hint
51-101: Consider extracting hash computation logic into separate functions, sensei!The compute implementation handles multiple input types well, but could be more modular. Consider extracting the different hash computation paths (dojo tags, selectors, single felts, multiple felts) into separate private functions for better maintainability.
Example structure:
impl HashArgs { fn compute_dojo_tag(&self, input: &str) -> Result<()> { let selector = format!("{:#066x}", compute_selector_from_tag(input)); println!("Dojo selector from tag: {}", selector); Ok(()) } fn compute_selector(&self, input: &str) -> Result<()> { // ... selector computation logic } fn compute_single_felt(&self, input: &str) -> Result<()> { // ... single felt computation logic } fn compute_multiple_felts(&self, input: &str) -> Result<()> { // ... multiple felts computation logic } }
117-152: Extract namespace collection logic into a separate function, sensei!The namespace collection logic is complex and could be more maintainable if extracted into a dedicated function.
Example:
impl HashArgs { fn collect_namespaces( profile_config: &ProfileConfig, manifest: &Option<Manifest>, ) -> Vec<String> { let mut ns_from_config = HashSet::new(); // ... namespace collection logic ... Vec::from_iter(ns_from_config) } }
154-181: Extract resource collection logic into a separate function, sensei!Similar to the namespace collection, the resource collection logic would be more maintainable as a separate function.
189-214: Consider performance optimization for hash searching, sensei!The nested loops for searching through namespaces and resources could be performance-intensive with large datasets. Consider:
- Using parallel iterators with
rayonfor larger datasets- Pre-computing hashes in a HashSet for O(1) lookups
Example optimization:
use rayon::prelude::*; // Pre-compute hashes let namespace_hashes: HashSet<_> = namespaces .iter() .map(|ns| (compute_bytearray_hash(ns), ns)) .collect(); // Parallel search let matches: Vec<_> = namespaces .par_iter() .flat_map(|ns| { resources.par_iter().filter_map(move |res| { let tag = get_tag(ns, res); if hash == compute_selector_from_tag(&tag) { Some(format!("Resource tag found: {tag}")) } else { None } }) }) .collect();🧰 Tools
🪛 GitHub Actions: ci
[error] 213-213: Code formatting error: Multi-line if-else statement was reformatted to a single line, violating Rust formatting standards. Run 'cargo fmt' to fix formatting.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/hash.rs(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
bin/sozo/src/commands/hash.rs
[error] 213-213: Code formatting error: Multi-line if-else statement was reformatted to a single line, violating Rust formatting standards. Run 'cargo fmt' to fix formatting.
🔇 Additional comments (3)
bin/sozo/src/commands/hash.rs (3)
18-49: Ohayo sensei! The command structure looks excellent!The separation into
ComputeandFindsubcommands is well-designed with clear documentation and helpful command-line arguments.
223-232: Clean and efficient command delegation, sensei!The
runmethod effectively delegates to the appropriate subcommand with good tracing support.
Line range hint
235-294: Add tests for the new find functionality, sensei!While the compute functionality is well-tested, there are no tests for the new find functionality. Consider adding tests for:
- Finding namespace matches
- Finding resource name matches
- Finding tag matches
- No matches found case
- Invalid hash input
Example test structure:
#[test] fn test_find_namespace_match() { let hash = compute_bytearray_hash("test_namespace"); let args = HashArgs { command: HashCommand::Find { hash: format!("{:#x}", hash), namespaces: Some(vec!["test_namespace".to_string()]), resources: None, }, }; let result = args.find(&mock_config(), &hash.to_string(), None, None); assert!(result.is_ok()); }✅ Verification successful
Ohayo! The review is spot-on, sensei! No find tests found!
After thorough verification of the codebase, I can confirm that while the
HashCommand::Findfunctionality exists inbin/sozo/src/commands/hash.rs, there are no tests covering this feature. The suggested test structure in the review comment correctly matches the implementation's parameters (hash, namespaces, resources).🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for find-related tests ast-grep --pattern 'fn test_$_find$_($_) { $$$ }' # Search for find implementation ast-grep --pattern 'fn find($_) { $$$ }' # Search for test files containing find tests rg -l "test.*find" --type rustLength of output: 160
Script:
#!/bin/bash # Check the content of the found test file cat crates/torii/indexer/src/test.rs # Search for HashCommand::Find in tests rg "HashCommand::Find" --type rust # Search for find implementation in HashArgs ast-grep --pattern 'impl HashArgs { $$$ fn find($_) { $$$ } $$$ }'Length of output: 18755
Script:
#!/bin/bash # Get the implementation context around HashCommand::Find rg -A 10 "HashCommand::Find" --type rustLength of output: 593
| println!("Resource tag found: {tag}"); | ||
| hash_found = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix formatting issue, sensei!
The pipeline indicates a formatting error on this line. Please run cargo fmt to fix the formatting.
🧰 Tools
🪛 GitHub Actions: ci
[error] 213-213: Code formatting error: Multi-line if-else statement was reformatted to a single line, violating Rust formatting standards. Run 'cargo fmt' to fix formatting.
Yes, this is because
Yes, good point 👍 |
Thanks for the good catch, I've missed this computation, and yeah we must definitely add the Let's keep both for the computation of a hash, since it's always useful to have it around. But when searching we only use the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
bin/sozo/src/commands/hash.rs (3)
32-34: Ohayo sensei! Consider enhancing the help message for the Find command.The help message could be more explicit about the default behavior when
--namespacesand--resourcesare not provided. Consider adding: "If not provided, values will be read from both manifest and profile configurations."
117-152: Consider extracting namespace collection logic, sensei!The namespace collection logic is quite lengthy and could be moved to a separate method for better maintainability.
+ fn collect_namespaces_from_config( + profile_config: &ProfileConfig, + manifest: &Option<Manifest>, + ) -> Vec<String> { + let mut ns_from_config = HashSet::new(); + + // ... existing collection logic ... + + Vec::from_iter(ns_from_config) + }
189-214: Optimize hash matching loops, sensei!Consider using
HashSetfor the input hash and performing a single pass over the data to check all possible matches. This would improve performance for large datasets.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/hash.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: docs
- GitHub Check: ensure-wasm
🔇 Additional comments (1)
bin/sozo/src/commands/hash.rs (1)
Line range hint
232-290: Add tests for the Find command functionality, sensei!The test suite lacks coverage for the
findcommand. Consider adding tests for:
- Finding namespace matches
- Finding resource name matches
- Finding tag matches
- No matches found case
- Invalid hash input
Would you like me to help generate these test cases?
| let inputs: Vec<_> = input | ||
| .split(',') | ||
| .map(|s| felt_from_str(s.trim()).expect("Invalid felt value")) | ||
| .map(|s| Felt::from_str(s.trim()).expect("Invalid felt value")) | ||
| .collect(); |
There was a problem hiding this comment.
Enhance error handling for felt parsing, sensei!
The expect call on felt parsing could panic. Consider using map_err to provide a more graceful error handling:
- .map(|s| Felt::from_str(s.trim()).expect("Invalid felt value"))
+ .map(|s| Felt::from_str(s.trim())
+ .map_err(|e| anyhow::anyhow!("Invalid felt value '{}': {}", s.trim(), e)))
+ .collect::<Result<Vec<_>>>()?;📝 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.
| let inputs: Vec<_> = input | |
| .split(',') | |
| .map(|s| felt_from_str(s.trim()).expect("Invalid felt value")) | |
| .map(|s| Felt::from_str(s.trim()).expect("Invalid felt value")) | |
| .collect(); | |
| let inputs: Vec<_> = input | |
| .split(',') | |
| .map(|s| Felt::from_str(s.trim()) | |
| .map_err(|e| anyhow::anyhow!("Invalid felt value '{}': {}", s.trim(), e))) | |
| .collect::<Result<Vec<_>>>()?; |
Description
This PR splits the existing
sozo hashcommand into 2 subcommands:sozo hash computewhich does the same thing than the actualsozo hashcommand,sozo hash findwhich:--namespacesand--resourcesoptions,Related issue
#2850
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
Refactor