feat(sozo): add sozo dev command
#2664
Conversation
WalkthroughOhayo, sensei! This pull request introduces several changes to 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: 2
🧹 Outside diff range and nitpick comments (8)
bin/sozo/Cargo.toml (1)
32-32: Ohayo! Consider using workspace versioning for the notify dependency.The
notifydependency is pinned to a specific version while most other dependencies use workspace versioning. For consistency and easier maintenance, consider moving the version specification to the workspace manifest.-notify = "7.0.0" +notify.workspace = truebin/sozo/src/commands/build.rs (2)
Line range hint
12-37: Consider enhancing documentation for better developer experience, sensei!While the individual field documentation is good, it would be helpful to add a struct-level documentation comment explaining the overall purpose and typical usage patterns of
BuildArgs, especially in the context of the newdevcommand.Add this documentation above the struct:
+/// BuildArgs configures the contract build process and binding generation. +/// It supports multiple binding types (TypeScript, Unity) and can be used both +/// for one-time builds and continuous rebuilds in dev mode. #[derive(Debug, Clone, Args)] pub struct BuildArgs {
Line range hint
39-106: Consider enhancing error handling in the run method, sensei!The
runmethod could benefit from more specific error handling, especially around the tokio runtime creation and binding generation.Consider wrapping the runtime operations with more specific error handling:
- tokio::runtime::Runtime::new() - .unwrap() - .block_on(bindgen.generate(None)) - .expect("Error generating bindings"); + tokio::runtime::Runtime::new() + .map_err(|e| anyhow::anyhow!("Failed to create Tokio runtime: {}", e))? + .block_on(bindgen.generate(None)) + .map_err(|e| anyhow::anyhow!("Failed to generate bindings: {}", e))?;bin/sozo/src/commands/mod.rs (2)
42-44: Documentation could be more detailed, sensei! (｡◕‿◕｡)While the command documentation is clear, consider adding more details about:
- The watch behavior
- File types that trigger rebuilds
- Any performance implications
42-44: Architectural considerations for the dev command, sensei! 🎯Since this command involves file watching and rebuilding, consider:
- Implementing graceful shutdown handling for the file watcher
- Adding configurable debounce/throttle for file change events
- Providing a way to exclude certain paths/patterns from triggering rebuilds
bin/sozo/src/commands/dev.rs (3)
32-56: Ohayo, sensei! Consider grouping relatedDevArgsfields for clarity.The newly added fields in the
DevArgsstruct enhance functionality, but organizing them into logical groups (e.g., binding options, feature specifications) with documentation comments can improve readability and maintainability.
128-149: Consider optimizing the main loop to reduce CPU usage.The use of
thread::sleep(Duration::from_millis(300))in the loop can lead to unnecessary CPU wake-ups. Using a blocking receive or increasing the sleep duration can improve efficiency.Consider this modification:
- thread::sleep(Duration::from_millis(300)); + // Use blocking receive with a timeout + if rebuild_rx.recv_timeout(Duration::from_millis(500)).is_ok() { + last_event_time = Some(Instant::now()); + }
Line range hint
163-195: Ohayo, sensei! Simplify theprocess_eventmethod logic.Streamlining the event processing logic using functional programming patterns can enhance readability.
Refactored code:
- let paths = match event.kind { - EventKind::Modify(_) => event.paths, - EventKind::Remove(_) => event.paths, - EventKind::Create(_) => event.paths, - _ => vec![], - }; + let paths = event.paths; if paths.is_empty() { return false; } - let mut is_rebuild_needed = false; - for path in &paths { + let is_rebuild_needed = paths.iter().any(|path| { if let Some(filename) = path.file_name() { if filename == "Scarb.toml" { info!("Rebuild to include Scarb.toml changes."); - is_rebuild_needed = true; + true } else if let Some(extension) = path.extension() { if extension == "cairo" { let file = path.to_string_lossy().to_string(); info!(file, "Rebuild from Cairo file change."); - is_rebuild_needed = true; + true } else { - false } } else { - false + false } } else { - false + false } - } + }); return is_rebuild_needed;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
bin/sozo/Cargo.toml(1 hunks)bin/sozo/src/commands/build.rs(1 hunks)bin/sozo/src/commands/dev.rs(2 hunks)bin/sozo/src/commands/migrate.rs(1 hunks)bin/sozo/src/commands/mod.rs(5 hunks)
🔇 Additional comments (8)
bin/sozo/Cargo.toml (1)
32-32: Verify the dependency changes impact, sensei!
The changes look good but let's verify:
- The new
notifydependency aligns with the PR's objective for file monitoring - Making
sozo-walnutoptional is a good practice for modularity
Let's check for any potential impact:
Also applies to: 39-39
✅ Verification successful
Ohayo! Dependencies are properly aligned, sensei! ✨
The verification shows:
notifyis actively used indev.rsfor file watching functionality withPollWatchersozo-walnutis correctly configured as an optional dependency with proper feature flag setup:- Defined as
optional = truein dependencies - Has a dedicated
walnutfeature that enables bothsozo-walnutandsozo-ops/walnut - Included in default features for convenience
- Defined as
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the dependency usage and optional feature configuration
# Check if notify is used in the dev command implementation
rg -l "use.*notify"
# Verify the optional dependency is properly gated with feature flags
rg -l "#\[cfg\(feature.*walnut\)\]"
Length of output: 88
Script:
#!/bin/bash
# Let's check the actual usage of notify in dev.rs and look for walnut-related code
rg "notify" bin/sozo/src/commands/dev.rs -B 2 -A 2
# Check for any walnut-related imports or uses
rg "walnut" bin/sozo/src/commands/dev.rs
# Let's also check the Cargo.toml for walnut feature configuration
cat bin/sozo/Cargo.toml
Length of output: 2359
bin/sozo/src/commands/migrate.rs (2)
22-34: Ohayo! The struct changes look good, sensei! 🎋
The addition of Clone derive and public field visibility aligns well with the needs of the new sozo dev command. These changes enable better reusability of the migration configuration across different commands.
Line range hint 38-93: Verify performance impact of cloning, sensei! 🎯
While the implementation remains solid, let's ensure that cloning MigrateArgs won't impact performance in the dev command's hot path. The struct contains nested options that might be expensive to clone repeatedly.
Let's check the size of the involved types:
✅ Verification successful
No performance concerns with MigrateArgs cloning, sensei! 🚀
After analyzing the struct composition, the cloning overhead is minimal since MigrateArgs only contains:
- TransactionOptions: Simple enum (FeeToken) and numeric fields
- WorldOptions: Single Option
- StarknetOptions: Single Option
- AccountOptions: Simple Option fields
All nested types are lightweight and implement efficient Clone. The struct is used only once during migration, not in any hot paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the size and composition of involved types
ast-grep --pattern 'struct TransactionOptions {
$$$
}'
ast-grep --pattern 'struct WorldOptions {
$$$
}'
ast-grep --pattern 'struct StarknetOptions {
$$$
}'
ast-grep --pattern 'struct AccountOptions {
$$$
}'
Length of output: 222
Script:
#!/bin/bash
# Let's search for these structs in the entire codebase with more context
rg "struct.*Options" -A 10
# Also search for MigrateArgs to understand its composition
rg "struct MigrateArgs" -A 10
Length of output: 11976
bin/sozo/src/commands/build.rs (1)
12-12: Ohayo! The Clone derive addition looks good, sensei!
The addition of Clone trait is well-justified as it enables the new sozo dev command to reuse BuildArgs for dynamic binding regeneration.
bin/sozo/src/commands/mod.rs (3)
14-14: Ohayo! Module organization looks clean, sensei! ٩(◕‿◕｡)۶
The new dev module and its corresponding imports are well-organized and follow the established pattern.
Also applies to: 28-28
74-74: Command execution implementation is solid! (ﾉ◕ヮ◕)ﾉ:･ﾟ✧*
The Dev command integration into both Display and execution handling follows the established patterns perfectly.
Also applies to: 99-99
42-42: Verify the impact of Boxing BuildArgs
The change from BuildArgs to Box<BuildArgs> might affect existing code that uses the Build command.
Let's check for potential impacts:
bin/sozo/src/commands/dev.rs (1)
61-67:
Add error handling for the file watcher setup.
Currently, the code uses unwrap() when calling watcher.watch, which can cause a panic if an error occurs. It's advisable to handle this error gracefully to prevent unexpected crashes.
Apply this diff to handle potential errors:
- watcher.watch(watched_directory.as_std_path(), RecursiveMode::Recursive).unwrap();
+ watcher
+ .watch(watched_directory.as_std_path(), RecursiveMode::Recursive)
+ .unwrap_or_else(|e| {
+ error!(?e, "Failed to watch directory.");
+ std::process::exit(1);
+ });Likely invalid or redundant comment.
| let migrate_args = MigrateArgs { | ||
| world: self.world, | ||
| starknet: self.starknet, | ||
| account: self.account, | ||
| transaction: self.transaction, | ||
| }; | ||
|
|
||
| let _ = migrate_args.clone().run(config); |
There was a problem hiding this comment.
Ohayo, sensei! Ensure errors in build and migrate steps are handled.
The run methods for build_args and migrate_args discard errors using let _ =. Handling these errors will provide feedback if these processes fail, improving robustness.
Apply this diff to properly handle errors:
- let _ = build_args.clone().run(config);
- let _ = migrate_args.clone().run(config);
+ if let Err(e) = build_args.clone().run(config) {
+ error!(?e, "Build failed.");
+ }
+ if let Err(e) = migrate_args.clone().run(config) {
+ error!(?e, "Migration failed.");
+ }Committable suggestion skipped: line range outside the PR's diff.
| let e_handler = EventHandler; | ||
| let rebuild_tx_clone = rebuild_tx.clone(); | ||
|
|
||
| // Independent thread to handle file events and trigger rebuilds. | ||
| config.tokio_handle().spawn(async move { | ||
| loop { | ||
| match file_rx.recv() { | ||
| Ok(maybe_event) => match maybe_event { | ||
| Ok(event) => { | ||
| trace!(?event, "Event received."); | ||
|
|
||
| if e_handler.process_event(event) && rebuild_tx_clone.send(()).is_err() | ||
| { | ||
| break; | ||
| } | ||
| } | ||
| Err(error) => { | ||
| error!(?error, "Processing event."); | ||
| break; | ||
| } | ||
| }, | ||
| Err(error) => { | ||
| error!(?error, "Processing event."); | ||
| error!(?error, "Receiving event."); | ||
| break; | ||
| } | ||
| }, | ||
| Err(error) => { | ||
| error!(?error, "Receiving event."); | ||
| break; | ||
| } | ||
| }; | ||
|
|
||
| if is_rebuild_needed { | ||
| // Ignore the fails of those commands as the `run` function | ||
| // already logs the error. | ||
| let _ = BuildArgs::default().run(config); | ||
|
|
||
| let _ = MigrateArgs::new_apply( | ||
| self.world.clone(), | ||
| self.starknet.clone(), | ||
| self.account.clone(), | ||
| ) | ||
| .run(config); | ||
| } | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Thread and channel cleanup needed in dev command
Ohayo sensei! After analyzing the code, there are indeed concerns about thread and channel management:
- The tokio task spawned for event handling has no explicit cleanup mechanism
- The
PollWatcherinstance is not explicitly dropped or cleaned up - The channels (
file_tx/rxandrebuild_tx/rx) are not properly closed
To fix this, consider:
- Implementing a proper shutdown mechanism using a shutdown channel or flag
- Adding proper cleanup in a
Dropimplementation or explicit cleanup method - Using
tokio::select!to handle graceful shutdown of the spawned task
🔗 Analysis chain
Verify proper closure of threads and channels.
The spawned thread may outlive the run function scope, potentially causing issues during shutdown. Ensure threads and channels are properly managed to avoid panics or resource leaks.
Run the following script to check for proper thread and channel management:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all threads are joined and channels are closed appropriately.
# Expected: No threads left running unexpectedly; channels are properly closed.
# Check for thread joins
rg 'spawn' -A 5 | rg -q 'join\(\)' && echo "All threads are joined." || echo "Threads may not be properly joined."
# Check for channel closure
rg 'channel' -A 5 | rg -q 'drop\(' && echo "Channels are properly closed." || echo "Channels may not be properly closed."
Length of output: 233
Script:
#!/bin/bash
# Let's check the full context of the thread spawning and channel usage in dev.rs
rg -l "spawn|channel" bin/sozo/src/commands/dev.rs
# If found, let's see the full implementation
cat bin/sozo/src/commands/dev.rs
# Let's also check for any cleanup or drop implementations
ast-grep --pattern 'impl Drop for $_ { $$ }'
# Check for any shutdown or cleanup related functions
rg -A 5 "shutdown|cleanup|drop|close" bin/sozo/src/commands/dev.rs
Length of output: 6439
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2664 +/- ##
==========================================
- Coverage 57.48% 57.36% -0.13%
==========================================
Files 400 401 +1
Lines 50277 50387 +110
==========================================
+ Hits 28904 28906 +2
- Misses 21373 21481 +108 ☔ View full report in Codecov by Sentry. |
Add
sozo devcommand which now supports regenerating the bindings on the fly.Summary by CodeRabbit
Release Notes
New Features
Devcommand for enhanced development workflows, allowing automatic builds and migrations on file changes.DevArgs.MigrateArgs, improving accessibility.Improvements
BuildArgsandMigrateArgsfor improved usability.Bug Fixes
Buildcommand for better functionality.