dojo 1.0.0-rc.0
#2595
dojo 1.0.0-rc.0
#2595
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Cargo.toml (1)
11-18: Ohayo! Nice workspace restructuring, but consider adding migration docs.The reorganization of crates under the
crates/dojo/prefix improves the project structure. However, this breaking change might impact downstream projects.Consider adding a migration guide in the README to help users update their dependencies to the new paths.
Also applies to: 41-41
📜 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 (1)
Cargo.toml(7 hunks)
🔇 Additional comments (4)
Cargo.toml (4)
57-57: Version bump to RC status - LGTM!
The version update to 1.0.0-rc.0 aligns with the PR objectives introducing the new Dojo framework version.
66-78: Dependencies aligned with new structure - LGTM!
The dependency paths have been consistently updated to match the new workspace structure.
131-152: Skipping cairo-lang-diagnostics version comment
159-160: New dependencies support CLI improvements - LGTM!
The addition of colored, colored_json, and spinoff aligns with the PR objectives about Sozo CLI improvements.
Also applies to: 203-203
Cargo.toml
Outdated
| #scarb = { git = "https://github.com/software-mansion/scarb", tag = "v2.8.4" } | ||
| #scarb-ui = { git = "https://github.com/software-mansion/scarb", tag = "v2.8.4" } | ||
| #scarb = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" } | ||
| #scarb-ui = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" } | ||
| scarb = { path = "/Users/glihm/swm/scarb/scarb" } | ||
| scarb-ui = { path = "/Users/glihm/swm/scarb/utils/scarb-ui" } |
There was a problem hiding this comment.
Please remove local scarb paths, sensei!
The scarb dependencies are using local paths that are specific to your development environment. This should be replaced with the git dependencies.
Apply this change:
-#scarb = { git = "https://github.com/software-mansion/scarb", tag = "v2.8.4" }
-#scarb-ui = { git = "https://github.com/software-mansion/scarb", tag = "v2.8.4" }
-#scarb = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" }
-#scarb-ui = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" }
-scarb = { path = "/Users/glihm/swm/scarb/scarb" }
-scarb-ui = { path = "/Users/glihm/swm/scarb/utils/scarb-ui" }
+scarb = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" }
+scarb-ui = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" }📝 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.
| #scarb = { git = "https://github.com/software-mansion/scarb", tag = "v2.8.4" } | |
| #scarb-ui = { git = "https://github.com/software-mansion/scarb", tag = "v2.8.4" } | |
| #scarb = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" } | |
| #scarb-ui = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" } | |
| scarb = { path = "/Users/glihm/swm/scarb/scarb" } | |
| scarb-ui = { path = "/Users/glihm/swm/scarb/utils/scarb-ui" } | |
| scarb = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" } | |
| scarb-ui = { git = "https://github.com/dojoengine/scarb", branch = "dojo-284" } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bin/sozo/tests/test_data/policies.json (1)
47-115: New system operations indicate enhanced runtime flexibility, sensei!The addition of namespace, event, and contract management methods under
0x234d3...aligns with the PR objective of supporting dynamic namespaces at runtime. However, these powerful system operations require strict access control.Consider implementing:
- Multi-signature requirements for critical operations
- Time-locks for upgrades
- Emergency pause mechanisms
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/ci.yml(3 hunks)bin/sozo/tests/test_data/policies.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.yml
🔇 Additional comments (3)
bin/sozo/tests/test_data/policies.json (3)
Line range hint 1-122: Verify contract addresses across environments.
The target addresses have been significantly updated. Please ensure these are the correct contract addresses for all deployment environments.
#!/bin/bash
# Description: Verify contract addresses across deployment configurations
# Check for address references in deployment configs
echo "Checking deployment configurations..."
rg -l "0x7bc340927668bc87eea10d95cb2dfe0fa10be12075fe8189f363643205c34d4"
echo "Checking for any old addresses still in use..."
rg "0x2d24481107b55ecd73c4d1b62f6bfe8c42a224447b71db7dcec2eab484d53cd"Line range hint 1-122: Ensure JSON structure maintains backward compatibility.
The removal of methods like update_player_name and addition of new system operations represents a significant change in the API surface.
✅ Verification successful
Let me gather more information about the usage patterns of these methods to better understand the impact.
The API changes maintain backward compatibility through new method names
Ohayo sensei! After analyzing the codebase, I can confirm that the old methods (update_player_name, update_player_items) have been replaced with more comprehensive methods (set_player_config, reset_player_config, set_player_server_profile) that provide better functionality. The changes represent an improvement in the API design rather than a breaking change, as:
- The new methods are properly documented in the manifest files
- Implementation files show clear usage patterns in
examples/spawn-and-move/src/actions.cairo - Test files demonstrate the new methods are being used consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to removed methods
echo "Checking for references to removed methods..."
rg "update_player_name|update_player_items|upgrade_state|set_differ_program_hash|set_merger_program_hash|set_facts_registry"
Length of output: 2667
Script:
#!/bin/bash
# Check if these methods are used in test files or actual implementation
echo "Checking test files..."
rg -g "**/*test*" "update_player_name|update_player_items" --context 2
echo -e "\nChecking implementation files..."
rg -g "!**/*test*" -g "!**/manifest*" "update_player_name|update_player_items" --context 2
echo -e "\nChecking if new methods replace old functionality..."
rg "set_player" --context 2
Length of output: 8199
3-31: Ohayo! Verify the consolidation of game mechanics under a single address.
The consolidation of all game-related methods under 0x7bc340... simplifies the architecture but requires careful access control verification.
✅ Verification successful
Ohayo! The game mechanics consolidation is properly secured, sensei!
The codebase shows robust access control implementation for the consolidated game mechanics under 0x7bc340...:
- All game actions (spawn, move, set_player_config, etc.) properly validate the caller using
get_caller_address() - Actions are scoped to the calling player's own data/state
- The world contract enforces additional permission checks through the Dojo framework
The consolidation pattern actually improves security by:
- Centralizing access control in a single contract
- Ensuring consistent caller validation across all game actions
- Leveraging Dojo's built-in permission system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the target contract's access control implementation
ast-grep --pattern 'fn $_(ref self) {
$$$
assert(get_caller_address() == $_, $$$);
$$$
}'
Length of output: 371
Script:
#!/bin/bash
# Let's check the actual contract implementation for access control
fd -e cairo -e rs -e sol . | xargs rg -l "0x7bc340927668bc87eea10d95cb2dfe0fa10be12075fe8189f363643205c34d4"
# Look for access control patterns in Cairo files
fd -e cairo . | xargs rg -B2 -A2 "get_caller_address|assert_only_owner|assert_only_admin"
# Check if there are any security-related tests
fd "test|spec" | xargs rg -l "unauthorized|permission|access.*control|caller"
Length of output: 31199
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Cargo.toml (1)
66-66: Update cainome dependency to use tagged versionUsing a specific commit hash (
rev = "5c2616c...") for cainome dependency might make it harder to track and upgrade. Consider using a tagged version instead.-cainome = { git = "https://github.com/cartridge-gg/cainome", rev = "5c2616c273faca7700d2ba565503fcefb5b9d720", features = [ "abigen-rs" ] } +cainome = { git = "https://github.com/cartridge-gg/cainome", tag = "v0.4.6", features = [ "abigen-rs" ] }
📜 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 (2)
Cargo.toml(7 hunks)bin/sozo/src/commands/build.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/sozo/src/commands/build.rs
🔇 Additional comments (5)
Cargo.toml (5)
11-18: Ohayo! Nice workspace restructuring, sensei!
The reorganization of crates under the dojo namespace and addition of sozo/scarbext improves the project structure and maintainability.
Also applies to: 41-41
131-152: Verify cairo-lang-diagnostics version
191-196: Clean up commented scarb dependencies
159-160: New dependencies added for improved user experience
The addition of colored, colored_json, and spinoff suggests improvements to CLI output formatting and progress indicators. Please ensure these are used consistently across all CLI components.
#!/bin/bash
# Check if the new dependencies are being used consistently
rg "colored::"
rg "colored_json::"
rg "spinoff::"Also applies to: 203-203
57-57: Version bump to 1.0.0-rc.0 - significant milestone!
This indicates the project is approaching a stable release. Please ensure all breaking changes are documented in the changelog.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/sozo/src/commands/inspect.rs(1 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/inspect.rs (1)
145-152: Previous review comment about black color usage is still applicable.
| pub fn run(self, config: &Config) -> Result<()> { | ||
| trace!(args = ?self); | ||
| let ws = scarb::ops::read_workspace(config.manifest_path(), config)?; | ||
|
|
||
| let InspectArgs { world, starknet, resource } = self; | ||
|
|
||
| config.tokio_handle().block_on(async { | ||
| let (world_diff, _, _) = | ||
| utils::get_world_diff_and_provider(starknet.clone(), world, &ws).await?; | ||
|
|
||
| if let Some(resource) = resource { | ||
| inspect_resource(&resource, &world_diff); | ||
| } else { | ||
| inspect_world(&world_diff); | ||
| } | ||
|
|
||
| Ok(()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and user feedback in the run method.
Ohayo, sensei! The run method could benefit from more informative error messages when operations fail.
Consider applying this improvement:
pub fn run(self, config: &Config) -> Result<()> {
trace!(args = ?self);
- let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;
+ let ws = scarb::ops::read_workspace(config.manifest_path(), config)
+ .map_err(|e| anyhow::anyhow!("Failed to read workspace: {}", e))?;
let InspectArgs { world, starknet, resource } = self;
config.tokio_handle().block_on(async {
let (world_diff, _, _) =
- utils::get_world_diff_and_provider(starknet.clone(), world, &ws).await?;
+ utils::get_world_diff_and_provider(starknet.clone(), world, &ws)
+ .await
+ .map_err(|e| anyhow::anyhow!("Failed to get world diff: {}", e))?;📝 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.
| pub fn run(self, config: &Config) -> Result<()> { | |
| trace!(args = ?self); | |
| let ws = scarb::ops::read_workspace(config.manifest_path(), config)?; | |
| let InspectArgs { world, starknet, resource } = self; | |
| config.tokio_handle().block_on(async { | |
| let (world_diff, _, _) = | |
| utils::get_world_diff_and_provider(starknet.clone(), world, &ws).await?; | |
| if let Some(resource) = resource { | |
| inspect_resource(&resource, &world_diff); | |
| } else { | |
| inspect_world(&world_diff); | |
| } | |
| Ok(()) | |
| }) | |
| } | |
| pub fn run(self, config: &Config) -> Result<()> { | |
| trace!(args = ?self); | |
| let ws = scarb::ops::read_workspace(config.manifest_path(), config) | |
| .map_err(|e| anyhow::anyhow!("Failed to read workspace: {}", e))?; | |
| let InspectArgs { world, starknet, resource } = self; | |
| config.tokio_handle().block_on(async { | |
| let (world_diff, _, _) = | |
| utils::get_world_diff_and_provider(starknet.clone(), world, &ws) | |
| .await | |
| .map_err(|e| anyhow::anyhow!("Failed to get world diff: {}", e))?; | |
| if let Some(resource) = resource { | |
| inspect_resource(&resource, &world_diff); | |
| } else { | |
| inspect_world(&world_diff); | |
| } | |
| Ok(()) | |
| }) | |
| } |
bin/sozo/src/commands/inspect.rs
Outdated
| fn print_table<T>(data: T, title: &str) | ||
| where | ||
| T: IntoIterator + Clone, | ||
| <T as IntoIterator>::Item: Tabled, | ||
| { | ||
| if data.clone().into_iter().count() == 0 { | ||
| return; | ||
| } | ||
|
|
||
| let mut table = Table::new(data); | ||
| table.with(Style::modern()); | ||
|
|
||
| println!("{title}"); | ||
| println!("{table}\n"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize table printing performance.
Ohayo, sensei! The print_table function clones the data twice unnecessarily.
Consider this optimization:
-fn print_table<T>(data: T, title: &str)
+fn print_table<T>(data: &T, title: &str)
where
- T: IntoIterator + Clone,
+ T: IntoIterator + ?Sized,
<T as IntoIterator>::Item: Tabled,
{
- if data.clone().into_iter().count() == 0 {
+ let mut iter = data.into_iter();
+ if iter.next().is_none() {
return;
}
- let mut table = Table::new(data);
+ let mut table = Table::new(std::iter::once(iter.next().unwrap()).chain(iter));
table.with(Style::modern());Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
bin/sozo/src/commands/options/transaction.rs (1)
84-84: Consider adding documentation about timeout behavior, sensei.While the implementation is correct, it would be helpful to document the default timeout behavior when the value is None.
Add documentation above the From implementation:
/// Converts TransactionOptions to TxnConfig /// /// # Timeout Behavior /// - When timeout is None: Uses the default timeout from the underlying transaction provider /// - When timeout is Some(ms): Waits for the specified duration in milliseconds impl From<TransactionOptions> for TxnConfig {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/sozo/src/commands/inspect.rs(1 hunks)bin/sozo/src/commands/options/transaction.rs(2 hunks)
🔇 Additional comments (2)
bin/sozo/src/commands/options/transaction.rs (2)
70-70: LGTM! Clean timeout propagation.
The timeout value is correctly propagated to the TxnAction::Send variant.
Line range hint 44-84: Verify timeout usage across the codebase.
Since this is part of the Sozo rewrite, let's ensure the timeout parameter is properly handled by all consumers.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about how the timeout parameter is used across the codebase:
Timeout parameter is properly integrated and handled
The timeout parameter is correctly propagated through the transaction handling chain:
- In
TransactionOptions, it's properly passed toTxnAction::Send - The timeout is then transferred to
TxnConfigthrough theFromimplementation - The timeout is utilized in the transaction waiter implementation for controlling wait duration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TxnConfig usage to verify timeout handling
echo "Searching for TxnConfig usage patterns..."
rg -A 5 "TxnConfig"
# Search for transaction wait implementations
echo "Searching for transaction wait implementations..."
rg -A 5 "fn wait|async fn wait"
Length of output: 41113
|
|
||
| #[arg(long)] | ||
| #[arg(help = "The timeout in milliseconds for the transaction wait.")] | ||
| #[arg(value_name = "TIMEOUT-MS")] | ||
| #[arg(global = true)] | ||
| pub timeout: Option<u64>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo! Consider adding timeout validation, sensei.
While the timeout implementation is clean, it might be helpful to add validation for reasonable timeout values to prevent extremely short or long timeouts that could affect transaction processing.
Consider adding validation in the to_txn_action method:
pub fn to_txn_action(&self, simulate: bool, estimate_only: bool) -> Result<TxnAction> {
+ if let Some(timeout) = self.timeout {
+ if timeout < 1000 {
+ bail!("Timeout must be at least 1000ms");
+ }
+ if timeout > 3600000 { // 1 hour
+ bail!("Timeout cannot exceed 1 hour");
+ }
+ }
match (estimate_only, simulate) {Committable suggestion was skipped due to low confidence.
| ResourceStatus::Updated => write!(f, "{}", "Updated".yellow()), | ||
| ResourceStatus::Synced => write!(f, "{}", "Synced".green()), | ||
| ResourceStatus::DirtyLocalPerms => write!(f, "{}", "Dirty local perms".yellow()), | ||
| ResourceStatus::MigrationSkipped => write!(f, "{}", "Migration skipped".black()), |
There was a problem hiding this comment.
Use a more visible color for MigrationSkipped status.
Ohayo, sensei! Using .black() for the MigrationSkipped status may render it invisible on dark terminals. Consider using a more visible color like .cyan() or .magenta().
- ResourceStatus::MigrationSkipped => write!(f, "{}", "Migration skipped".black()),
+ ResourceStatus::MigrationSkipped => write!(f, "{}", "Migration skipped".cyan()),📝 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.
| ResourceStatus::MigrationSkipped => write!(f, "{}", "Migration skipped".black()), | |
| ResourceStatus::MigrationSkipped => write!(f, "{}", "Migration skipped".cyan()), |
| _ => value.white(), | ||
| }; | ||
|
|
||
| println!("{}: {}", key.black(), colored_values); |
There was a problem hiding this comment.
Use a more visible color for TOML keys.
Ohayo, sensei! Using .black() for TOML keys may render them invisible on dark terminals. Consider using a more visible color like .bright_black() or .cyan().
- println!("{}: {}", key.black(), colored_values);
+ println!("{}: {}", key.bright_black(), colored_values);📝 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.
| println!("{}: {}", key.black(), colored_values); | |
| println!("{}: {}", key.bright_black(), colored_values); |
| ResourceType::StarknetContract => { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Handle StarknetContract case gracefully.
Ohayo, sensei! The todo!() macro will cause a runtime panic if a Starknet contract is encountered. Consider providing a meaningful error message or implementing the necessary handling.
- ResourceType::StarknetContract => {
- todo!()
- }
+ ResourceType::StarknetContract => {
+ let status = ResourceStatus::Created; // or appropriate status
+ ResourceInspect::Contract(ContractInspect {
+ tag: resource.tag(),
+ status,
+ is_initialized: false,
+ address: format!("{:#066x}", resource.address()),
+ current_class_hash: format!("{:#066x}", resource.current_class_hash()),
+ selector: format!("{:#066x}", resource.dojo_selector()),
+ })
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
bin/sozo/src/commands/migrate.rs (1)
44-45: Remove commented spinner frames.Ohayo, sensei! Consider removing the commented-out spinner frames to maintain clean code.
let frames = spinner!(["⛩️ ", "🎃", "👻", "🧟", "💀"], 500); -// let frames = spinner!(["⛩️ ", "🥷 ", "🗡️ "], 500);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
bin/sozo/src/commands/migrate.rs(2 hunks)
🔇 Additional comments (4)
bin/sozo/src/commands/migrate.rs (4)
Line range hint 1-29: LGTM! Clean and well-structured command arguments.
Ohayo! The imports are well-organized and the MigrateArgs struct effectively uses clap's derive macros with flattened command options, making it maintainable and extensible.
74-80: The Box::leak memory leak issue is still present.
As noted in the previous review, using Box::leak can lead to memory leaks. The suggestion to use a reference instead of leaking memory is still valid.
53-54: Verify error handling for world diff operation.
The world diff operation is a critical part of the migration process. Let's verify that the error handling is comprehensive.
✅ Verification successful
Let me check the error handling implementation in more detail.
Ohayo sensei! After reviewing the implementation, I can now provide the final response:
Error handling for world diff operation is comprehensive and well-implemented
The get_world_diff_and_account function includes robust error handling with:
- Proper error context using
anyhowfor all critical operations - Account deployment verification with clear error messages
- RPC version compatibility checks
- Chain ID validation with context
- World address validation with helpful warning messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns around world diff operations
rg -A 5 "get_world_diff_and_account" .
Length of output: 1272
Script:
#!/bin/bash
# Check the implementation of get_world_diff_and_account
cat ./bin/sozo/src/utils.rs
Length of output: 8335
69-70: Verify manifest writing permissions.
While the code includes error handling for manifest writing, it's important to ensure the workspace has proper write permissions before attempting the operation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
bin/sozo/src/commands/migrate.rs (3)
48-49: Remove commented-out code.Ohayo sensei! The alternative spinner frames are commented out and should be removed to maintain clean code.
- let frames = spinner!(["⛩️ ", "🎃", "👻", "🧟", "💀"], 500); - // let frames = spinner!(["⛩️ ", "🥷 ", "🗡️ "], 500); + let frames = spinner!(["⛩️ ", "🎃", "👻", "🧟", "💀"], 500);
106-107: Enhance error message for chain_id parsing.Ohayo sensei! Consider making the error message more specific to help with debugging:
- parse_cairo_short_string(&chain_id).with_context(|| "Cannot parse chain_id as string")?; + parse_cairo_short_string(&chain_id).with_context(|| format!("Failed to parse chain_id '{}' as Cairo short string", chain_id))?;
38-90: Consider extracting migration UI logic.Ohayo sensei! The
runmethod is handling both migration logic and UI feedback. Consider extracting the spinner and banner logic into a separate UI manager struct to improve separation of concerns and make the code more maintainable.This would also make it easier to:
- Test the migration logic independently
- Support different UI implementations (e.g., CLI, GUI, or headless mode)
- Reuse the UI components in other commands
bin/sozo/src/commands/inspect.rs (1)
18-29: Add documentation for public types.Ohayo, sensei! The public types like
InspectArgs,ResourceStatus, andResourceInspectlack documentation. As this is a command-line tool, proper documentation would help users understand the functionality better.Add documentation comments for public types. For example:
/// Arguments for the inspect command. /// /// This struct contains all the options that can be passed to the `sozo inspect` command. #[derive(Debug, Args)] pub struct InspectArgs { // ... } /// Represents the current status of a resource. /// /// This enum is used to indicate whether a resource is newly created, /// updated, synced, or has dirty permissions. #[derive(Debug, Serialize)] enum ResourceStatus { // ... } /// Represents the inspection result for different types of resources. /// /// This enum contains detailed information about each type of resource /// that can be inspected in the Dojo framework. #[derive(Debug, Tabled, Serialize)] enum ResourceInspect { // ... }Also applies to: 53-60, 74-80
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
bin/sozo/src/commands/inspect.rs(1 hunks)bin/sozo/src/commands/migrate.rs(2 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/migrate.rs (1)
87-87: Memory leak risk: Avoid Box::leak.
Ohayo sensei! The previous review comment about the Box::leak memory leak is still valid. Let's fix this to prevent memory leaks:
- spinner.stop_and_persist(symbol, Box::leak(end_text.into_boxed_str()));
+ spinner.stop_and_persist(symbol, &end_text);| let status = if world_diff.profile_config.is_skipped(&resource.tag()) { | ||
| ResourceStatus::MigrationSkipped | ||
| } else { | ||
| status | ||
| }; | ||
|
|
||
| ResourceInspect::Namespace(NamespaceInspect { | ||
| name: resource.name(), | ||
| status, | ||
| selector: format!("{:#066x}", resource.dojo_selector()), | ||
| }) | ||
| } | ||
| ResourceType::Contract => { | ||
| let (is_initialized, contract_address, status) = match resource { | ||
| ResourceDiff::Created(_) => ( | ||
| false, | ||
| world_diff.get_contract_address(resource.dojo_selector()).unwrap(), | ||
| ResourceStatus::Created, | ||
| ), | ||
| ResourceDiff::Updated(_, remote) => ( | ||
| remote.as_contract_or_panic().is_initialized, | ||
| remote.address(), | ||
| ResourceStatus::Updated, | ||
| ), | ||
| ResourceDiff::Synced(_, remote) => ( | ||
| remote.as_contract_or_panic().is_initialized, | ||
| remote.address(), | ||
| if has_dirty_perms { | ||
| ResourceStatus::DirtyLocalPerms | ||
| } else { | ||
| ResourceStatus::Synced | ||
| }, | ||
| ), | ||
| }; | ||
|
|
||
| let status = if world_diff.profile_config.is_skipped(&resource.tag()) { | ||
| ResourceStatus::MigrationSkipped | ||
| } else { | ||
| status | ||
| }; | ||
|
|
||
| ResourceInspect::Contract(ContractInspect { | ||
| tag: resource.tag(), | ||
| status, | ||
| is_initialized, | ||
| address: format!("{:#066x}", contract_address), | ||
| current_class_hash: format!("{:#066x}", resource.current_class_hash()), | ||
| selector: format!("{:#066x}", resource.dojo_selector()), | ||
| }) | ||
| } | ||
| ResourceType::Model => { | ||
| let status = match resource { | ||
| ResourceDiff::Created(_) => ResourceStatus::Created, | ||
| ResourceDiff::Updated(_, _) => ResourceStatus::Updated, | ||
| ResourceDiff::Synced(_, _) => { | ||
| if has_dirty_perms { | ||
| ResourceStatus::DirtyLocalPerms | ||
| } else { | ||
| ResourceStatus::Synced | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| let status = if world_diff.profile_config.is_skipped(&resource.tag()) { | ||
| ResourceStatus::MigrationSkipped | ||
| } else { | ||
| status | ||
| }; | ||
|
|
||
| ResourceInspect::Model(ModelInspect { | ||
| tag: resource.tag(), | ||
| status, | ||
| selector: format!("{:#066x}", resource.dojo_selector()), | ||
| }) | ||
| } | ||
| ResourceType::Event => { | ||
| let status = match resource { | ||
| ResourceDiff::Created(_) => ResourceStatus::Created, | ||
| ResourceDiff::Updated(_, _) => ResourceStatus::Updated, | ||
| ResourceDiff::Synced(_, _) => { | ||
| if has_dirty_perms { | ||
| ResourceStatus::DirtyLocalPerms | ||
| } else { | ||
| ResourceStatus::Synced | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| let status = if world_diff.profile_config.is_skipped(&resource.tag()) { | ||
| ResourceStatus::MigrationSkipped | ||
| } else { | ||
| status | ||
| }; | ||
|
|
||
| ResourceInspect::Event(EventInspect { | ||
| tag: resource.tag(), | ||
| status, | ||
| selector: format!("{:#066x}", resource.dojo_selector()), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common status determination logic into a helper function.
Ohayo, sensei! The status determination logic is repeated across different resource types. This repetition makes the code harder to maintain and increases the risk of inconsistencies.
Consider extracting the common logic into a helper function:
fn determine_resource_status(
resource: &ResourceDiff,
has_dirty_perms: bool,
is_skipped: bool,
) -> ResourceStatus {
if is_skipped {
return ResourceStatus::MigrationSkipped;
}
match resource {
ResourceDiff::Created(_) => ResourceStatus::Created,
ResourceDiff::Updated(_, _) => ResourceStatus::Updated,
ResourceDiff::Synced(_, _) => {
if has_dirty_perms {
ResourceStatus::DirtyLocalPerms
} else {
ResourceStatus::Synced
}
}
}
}Then use it in each match arm:
let status = determine_resource_status(
resource,
has_dirty_perms,
world_diff.profile_config.is_skipped(&resource.tag())
);
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
bin/sozo/src/commands/migrate.rs (2)
48-49: Remove commented-out spinner frames.Ohayo! The commented spinner frames configuration should be removed to maintain clean code.
let frames = spinner!(["⛩️ ", "🎃", "👻", "🧟", "💀"], 500); -// let frames = spinner!(["⛩️ ", "🥷 ", "🗡️ "], 500);
105-107: Enhance error message for chain ID parsing.Ohayo, sensei! Consider providing more context in the error message to help users understand and resolve the issue.
- parse_cairo_short_string(&chain_id).with_context(|| "💀 Cannot parse chain_id as string")?; + parse_cairo_short_string(&chain_id).with_context(|| format!("💀 Cannot parse chain_id '{}' as a valid Cairo short string", chain_id))?;bin/sozo/src/commands/mod.rs (2)
35-46: Ohayo! The new command structure looks solid!The boxed arguments for commands will help with memory efficiency when dealing with large command structures. The new
Inspectcommand aligns with the PR objective of allowing users to check the world state.Using
Box<T>for large command structures is a good practice as it keeps the enum size constant and moves the variable-sized data to the heap.
Line range hint
84-120: Ohayo! The version compatibility check is thorough but could be more robust.The version check logic is good but could benefit from some improvements:
- It only checks git dependencies with explicit tags
- The error messages are duplicated
Consider this more concise approach:
pub fn check_package_dojo_version(ws: &Workspace<'_>, package: &Package) -> anyhow::Result<()> { if let Some(dojo_dep) = package.manifest.summary.dependencies.iter().find(|dep| dep.name.as_str() == "dojo") { let dojo_version = env!("CARGO_PKG_VERSION"); let dojo_dep_str = dojo_dep.to_string(); - // Only in case of git dependency with an explicit tag, we check if the tag is the same as - // the current version. - if dojo_dep_str.contains("git+") - && dojo_dep_str.contains("tag=v") - && !dojo_dep_str.contains(dojo_version) - { - if let Ok(cp) = ws.current_package() { - let path = - if cp.id == package.id { package.manifest_path() } else { ws.manifest_path() }; + if should_check_version(&dojo_dep_str) && !dojo_dep_str.contains(dojo_version) { + let path = get_manifest_path(ws, package); + anyhow::bail!( + "Found dojo-core version mismatch: expected {}. Please verify your dojo dependency in {}", + dojo_version, + path + ); + } + } + Ok(()) +} - anyhow::bail!( - "Found dojo-core version mismatch: expected {}. Please verify your dojo \ - dependency in {}", - dojo_version, - path - ) - } else { - // Virtual workspace. - anyhow::bail!( - "Found dojo-core version mismatch: expected {}. Please verify your dojo \ - dependency in {}", - dojo_version, - ws.manifest_path() - ) - } - } +fn should_check_version(dep_str: &str) -> bool { + dep_str.contains("git+") && dep_str.contains("tag=v") +} + +fn get_manifest_path(ws: &Workspace<'_>, package: &Package) -> &Path { + if let Ok(cp) = ws.current_package() { + if cp.id == package.id { + package.manifest_path() + } else { + ws.manifest_path() + } + } else { + ws.manifest_path() } - Ok(()) }
📜 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 (4)
bin/sozo/src/commands/hash.rs(1 hunks)bin/sozo/src/commands/inspect.rs(1 hunks)bin/sozo/src/commands/migrate.rs(2 hunks)bin/sozo/src/commands/mod.rs(2 hunks)
🔇 Additional comments (8)
bin/sozo/src/commands/migrate.rs (3)
Line range hint 1-37: LGTM! Clean command structure implementation.
Ohayo, sensei! The command structure is well-organized with flattened options, making it more maintainable and easier to use.
87-87: Box::leak usage in spinner text
The intentional memory leak through Box::leak has already been identified in previous reviews.
60-71: Implementation aligns with PR objectives.
Ohayo! The migration implementation properly fetches data from the chain and handles the world diff calculation robustly, which aligns perfectly with the PR's goal of making migration processes more robust and chain-dependent.
Let's verify the chain data fetching implementation:
✅ Verification successful
Let me verify the chain data fetching implementation further by checking the WorldDiff implementation:
Let me check one more aspect to verify the chain data fetching implementation:
Chain data fetching implementation verified successfully
Ohayo sensei! After reviewing the implementation, I can confirm that the migration process properly fetches data from the chain through:
- Direct provider interactions for chain ID and spec version verification
- WorldDiff creation using
new_from_chainwhich fetches world state directly from the chain - Robust error handling with proper context in the utils and migration modules
The implementation perfectly aligns with making migration processes more robust by fetching all data directly from the chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct chain data access patterns
rg -A 5 'provider\.' bin/sozo/src/
Length of output: 2838
Script:
#!/bin/bash
# Search for WorldDiff implementation
ast-grep --pattern 'impl WorldDiff {
$$$
async fn new_from_chain($$$) {
$$$
}
$$$
}'
Length of output: 98
Script:
#!/bin/bash
# Search for any chain data fetching in WorldDiff
rg -A 10 'WorldDiff::new_from_chain'
Length of output: 2203
Script:
#!/bin/bash
# Search for WorldDiff struct and its implementation
rg -A 10 'struct WorldDiff'
# Also check the actual implementation file
fd WorldDiff.rs -x cat
Length of output: 989
bin/sozo/src/commands/mod.rs (3)
6-6: LGTM! Clean import organization, sensei!
The new imports align perfectly with the PR objectives, particularly for the new inspect and hash commands.
Also applies to: 14-14, 23-24
55-59: 🛠️ Refactor suggestion
Consider implementing Display using a more maintainable approach, sensei.
Consider using a match-free approach to reduce maintenance overhead:
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- match self {
- Commands::Build(_) => write!(f, "Build"),
- Commands::Clean(_) => write!(f, "Clean"),
- Commands::Execute(_) => write!(f, "Execute"),
- Commands::Inspect(_) => write!(f, "Inspect"),
- Commands::Migrate(_) => write!(f, "Migrate"),
- Commands::Call(_) => write!(f, "Call"),
- Commands::Test(_) => write!(f, "Test"),
- Commands::Hash(_) => write!(f, "Hash"),
- }
- }
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ // Automatically derives the command name from the variant
+ write!(f, "{}", std::mem::discriminant(self).type_name().split("::").last().unwrap())
+ }Likely invalid or redundant comment.
76-81: Command execution looks clean, but let's verify the new commands, sensei!
The implementation looks good, but we should verify that all new commands (Inspect, Clean, Hash) have proper error handling.
✅ Verification successful
Let me explain: The previous script failed because we were looking in the wrong location. Since we're analyzing bin/sozo/src/commands/mod.rs, the command implementations are likely to be in the same directory or its subdirectories. Let's first find these files and then check their error handling patterns.
All new commands have proper error handling, sensei! ✨
The verification shows that all new commands (Inspect, Clean, and Hash) properly implement error handling:
inspect.rs: UsesResult<()>with proper workspace error propagationclean.rs: UsesResult<()>with workspace error handlinghash.rs: UsesResult<Vec<String>>with explicit error handling for empty input
All commands follow the same robust error handling pattern as existing commands, using the ? operator for error propagation and anyhow::Result type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling in new command implementations
echo "Checking error handling in new commands..."
rg -A 5 "fn run.*Result" inspect.rs clean.rs hash.rs
Length of output: 297
Script:
#!/bin/bash
# Let's find the command implementation files first
echo "Finding command implementation files..."
fd -e rs -t f "inspect|clean|hash" src/
# Then check their error handling
echo -e "\nChecking error handling in command implementations..."
rg -A 5 "fn run.*Result" $(fd -e rs -t f "inspect|clean|hash" src/)
Length of output: 10820
bin/sozo/src/commands/hash.rs (2)
44-44: Ohayo! The hash computation changes look solid, sensei! 🎋
The addition of array-based Poseidon hash computation alongside the existing single value hash provides more comprehensive hash information. The clear labeling in the print statements helps distinguish between the different hash types.
Also applies to: 47-48
44-50: 🛠️ Refactor suggestion
Consider including the array hash in the return value, sensei! 🍜
While the array hash is computed and displayed, it's not included in the returned vector. For consistency and completeness, consider updating the return value to include all computed hashes.
- return Ok(vec![poseidon.to_string(), snkeccak.to_string()]);
+ return Ok(vec![poseidon.to_string(), poseidon_array.to_string(), snkeccak.to_string()]);Note: This change would require updating the corresponding test cases.
Likely invalid or redundant comment.
| struct GranteeDisplay { | ||
| #[tabled(rename = "Tag")] | ||
| tag: String, | ||
| #[tabled(rename = "Contract Address")] | ||
| address: String, | ||
| #[tabled(rename = "Source")] | ||
| source: GranteeSource, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add address validation in GranteeDisplay
Ohayo, sensei! The GranteeDisplay struct accepts any string as an address without validation. Consider adding validation to ensure the address is a valid Starknet address.
#[derive(Debug, Tabled)]
struct GranteeDisplay {
#[tabled(rename = "Tag")]
tag: String,
#[tabled(rename = "Contract Address")]
+ #[validate(custom = "validate_starknet_address")]
address: String,
#[tabled(rename = "Source")]
source: GranteeSource,
}
+fn validate_starknet_address(address: &str) -> Result<(), ValidationError> {
+ if !address.starts_with("0x") || address.len() != 66 {
+ return Err(ValidationError::new("Invalid Starknet address format"));
+ }
+ Ok(())
+}Committable suggestion was skipped due to low confidence.
| pub fn halloween() -> Theme { | ||
| let mut style = Theme::default(); | ||
| style.set_borders_vertical('💀'); | ||
| style.set_borders_left('💀'); | ||
| style.set_borders_right('💀'); | ||
|
|
||
| style.set_borders_corner_top_left('🎃'); | ||
|
|
||
| style | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider terminal compatibility for emoji characters
Ohayo, sensei! The halloween theme uses emoji characters (💀, 🎃) which might not render correctly in all terminal environments. Consider providing a fallback theme using ASCII characters.
pub fn halloween() -> Theme {
let mut style = Theme::default();
- style.set_borders_vertical('💀');
- style.set_borders_left('💀');
- style.set_borders_right('💀');
- style.set_borders_corner_top_left('🎃');
+ // Try emoji first, fall back to ASCII if terminal doesn't support it
+ let (vertical, corner) = if supports_emoji() {
+ ('💀', '🎃')
+ } else {
+ ('|', '+')
+ };
+ style.set_borders_vertical(vertical);
+ style.set_borders_left(vertical);
+ style.set_borders_right(vertical);
+ style.set_borders_corner_top_left(corner);
style
}
+fn supports_emoji() -> bool {
+ // Implementation to detect terminal emoji support
+ std::env::var("TERM").map(|term| term.contains("256color")).unwrap_or(false)
+}Committable suggestion was skipped due to low confidence.
| for pdiff in writers.only_local() { | ||
| writers_disp.push(GranteeDisplay { | ||
| tag: pdiff.tag.unwrap_or("external".to_string()), | ||
| address: format!("{:#066x}", pdiff.address), | ||
| source: GranteeSource::Local, | ||
| }); | ||
| } | ||
|
|
||
| for pdiff in writers.only_remote() { | ||
| writers_disp.push(GranteeDisplay { | ||
| tag: pdiff.tag.unwrap_or("external".to_string()), | ||
| address: format!("{:#066x}", pdiff.address), | ||
| source: GranteeSource::Remote, | ||
| }); | ||
| } | ||
|
|
||
| for pdiff in writers.synced() { | ||
| writers_disp.push(GranteeDisplay { | ||
| tag: pdiff.tag.unwrap_or("external".to_string()), | ||
| address: format!("{:#066x}", pdiff.address), | ||
| source: GranteeSource::Synced, | ||
| }); | ||
| } | ||
|
|
||
| let owners = world_diff.get_owners(resource_diff.dojo_selector()); | ||
| let mut owners_disp = vec![]; | ||
|
|
||
| for pdiff in owners.only_local() { | ||
| owners_disp.push(GranteeDisplay { | ||
| tag: pdiff.tag.unwrap_or("external".to_string()), | ||
| address: format!("{:#066x}", pdiff.address), | ||
| source: GranteeSource::Local, | ||
| }); | ||
| } | ||
|
|
||
| for pdiff in owners.only_remote() { | ||
| owners_disp.push(GranteeDisplay { | ||
| tag: pdiff.tag.unwrap_or("external".to_string()), | ||
| address: format!("{:#066x}", pdiff.address), | ||
| source: GranteeSource::Remote, | ||
| }); | ||
| } | ||
|
|
||
| for pdiff in owners.synced() { | ||
| owners_disp.push(GranteeDisplay { | ||
| tag: pdiff.tag.unwrap_or("external".to_string()), | ||
| address: format!("{:#066x}", pdiff.address), | ||
| source: GranteeSource::Synced, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common permission handling logic
Ohayo, sensei! The code for handling writers and owners permissions contains significant duplication. Let's extract this into a reusable function.
+fn create_grantee_displays(pdiffs: impl Iterator<Item = PermissionDiff>, source: GranteeSource) -> Vec<GranteeDisplay> {
+ pdiffs
+ .map(|pdiff| GranteeDisplay {
+ tag: pdiff.tag.unwrap_or("external".to_string()),
+ address: format!("{:#066x}", pdiff.address),
+ source,
+ })
+ .collect()
+}
// Replace the duplicate code blocks with:
-for pdiff in writers.only_local() {
- writers_disp.push(GranteeDisplay {
- tag: pdiff.tag.unwrap_or("external".to_string()),
- address: format!("{:#066x}", pdiff.address),
- source: GranteeSource::Local,
- });
-}
+writers_disp.extend(create_grantee_displays(
+ writers.only_local(),
+ GranteeSource::Local
+));📝 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.
| for pdiff in writers.only_local() { | |
| writers_disp.push(GranteeDisplay { | |
| tag: pdiff.tag.unwrap_or("external".to_string()), | |
| address: format!("{:#066x}", pdiff.address), | |
| source: GranteeSource::Local, | |
| }); | |
| } | |
| for pdiff in writers.only_remote() { | |
| writers_disp.push(GranteeDisplay { | |
| tag: pdiff.tag.unwrap_or("external".to_string()), | |
| address: format!("{:#066x}", pdiff.address), | |
| source: GranteeSource::Remote, | |
| }); | |
| } | |
| for pdiff in writers.synced() { | |
| writers_disp.push(GranteeDisplay { | |
| tag: pdiff.tag.unwrap_or("external".to_string()), | |
| address: format!("{:#066x}", pdiff.address), | |
| source: GranteeSource::Synced, | |
| }); | |
| } | |
| let owners = world_diff.get_owners(resource_diff.dojo_selector()); | |
| let mut owners_disp = vec![]; | |
| for pdiff in owners.only_local() { | |
| owners_disp.push(GranteeDisplay { | |
| tag: pdiff.tag.unwrap_or("external".to_string()), | |
| address: format!("{:#066x}", pdiff.address), | |
| source: GranteeSource::Local, | |
| }); | |
| } | |
| for pdiff in owners.only_remote() { | |
| owners_disp.push(GranteeDisplay { | |
| tag: pdiff.tag.unwrap_or("external".to_string()), | |
| address: format!("{:#066x}", pdiff.address), | |
| source: GranteeSource::Remote, | |
| }); | |
| } | |
| for pdiff in owners.synced() { | |
| owners_disp.push(GranteeDisplay { | |
| tag: pdiff.tag.unwrap_or("external".to_string()), | |
| address: format!("{:#066x}", pdiff.address), | |
| source: GranteeSource::Synced, | |
| }); | |
| } | |
| fn create_grantee_displays(pdiffs: impl Iterator<Item = PermissionDiff>, source: GranteeSource) -> Vec<GranteeDisplay> { | |
| pdiffs | |
| .map(|pdiff| GranteeDisplay { | |
| tag: pdiff.tag.unwrap_or("external".to_string()), | |
| address: format!("{:#066x}", pdiff.address), | |
| source, | |
| }) | |
| .collect() | |
| } | |
| writers_disp.extend(create_grantee_displays( | |
| writers.only_local(), | |
| GranteeSource::Local | |
| )); | |
| writers_disp.extend(create_grantee_displays( | |
| writers.only_remote(), | |
| GranteeSource::Remote | |
| )); | |
| writers_disp.extend(create_grantee_displays( | |
| writers.synced(), | |
| GranteeSource::Synced | |
| )); | |
| let owners = world_diff.get_owners(resource_diff.dojo_selector()); | |
| let mut owners_disp = vec![]; | |
| owners_disp.extend(create_grantee_displays( | |
| owners.only_local(), | |
| GranteeSource::Local | |
| )); | |
| owners_disp.extend(create_grantee_displays( | |
| owners.only_remote(), | |
| GranteeSource::Remote | |
| )); | |
| owners_disp.extend(create_grantee_displays( | |
| owners.synced(), | |
| GranteeSource::Synced | |
| )); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
bin/sozo/src/commands/migrate.rs (2)
48-49: Remove commented-out code.The alternative spinner frames are commented out and should be removed if not needed.
- let frames = spinner!(["⛩️ ", "🎃", "👻", "🧟", "💀"], 500); - // let frames = spinner!(["⛩️ ", "🥷 ", "🗡️ "], 500); + let frames = spinner!(["⛩️ ", "🎃", "👻", "🧟", "💀"], 500);
94-119: LGTM! Clean banner implementation with room for improvement.Ohayo sensei! The banner implementation provides excellent user feedback. One minor suggestion would be to make the chain_id parsing error more specific:
- .with_context(|| "💀 Cannot parse chain_id as string")?; + .with_context(|| format!("💀 Cannot parse chain_id '{}' as string", chain_id))?;
📜 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 (1)
bin/sozo/src/commands/migrate.rs(2 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/migrate.rs (1)
Line range hint 1-37: LGTM! Well-structured imports and command arguments.
Ohayo, sensei! The imports are well-organized, and the MigrateArgs struct effectively uses command flattening to provide a clean interface for migration operations.
| ) | ||
| .await | ||
| }) | ||
| .map(|_| ()), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub async fn setup_env<'a>( | ||
| ws: &'a Workspace<'a>, | ||
| account: AccountOptions, | ||
| starknet: StarknetOptions, | ||
| world: WorldOptions, | ||
| name: &str, | ||
| env: Option<&'a Environment>, | ||
| ) -> Result<(Option<Felt>, SozoAccount<JsonRpcClient<HttpTransport>>, String)> { | ||
| trace!("Setting up environment."); | ||
| let ui = ws.config().ui(); | ||
|
|
||
| let world_address = world.address(env).ok(); | ||
| trace!(?world_address); | ||
| let frames = spinner!(["⛩️ ", "🎃", "👻", "🧟", "💀"], 500); | ||
| // let frames = spinner!(["⛩️ ", "🥷 ", "🗡️ "], 500); | ||
|
|
||
| let (account, rpc_url) = { | ||
| let provider = starknet.provider(env)?; | ||
| trace!(?provider, "Provider initialized."); | ||
| config.tokio_handle().block_on(async { | ||
| print_banner(&ws, &starknet).await?; | ||
|
|
||
| let spec_version = provider.spec_version().await?; | ||
| trace!(spec_version); | ||
| let mut spinner = | ||
| MigrationUi::Spinner(Spinner::new(frames, "Evaluating world diff...", None)); | ||
|
|
||
| if !is_compatible_version(&spec_version, RPC_SPEC_VERSION)? { | ||
| return Err(anyhow!( | ||
| "Unsupported Starknet RPC version: {}, expected {}.", | ||
| spec_version, | ||
| RPC_SPEC_VERSION | ||
| )); | ||
| } | ||
| let mut txn_config: TxnConfig = self.transaction.into(); | ||
| txn_config.wait = true; | ||
|
|
||
| let rpc_url = starknet.url(env)?; | ||
| trace!(?rpc_url); | ||
| let (world_diff, account, rpc_url) = | ||
| utils::get_world_diff_and_account(account, starknet, world, &ws).await?; | ||
|
|
||
| let chain_id = provider.chain_id().await?; | ||
| let chain_id = parse_cairo_short_string(&chain_id) | ||
| .with_context(|| "Cannot parse chain_id as string")?; | ||
| trace!(chain_id); | ||
| let world_address = world_diff.world_info.address; | ||
|
|
||
| let account = { | ||
| // This is mainly for controller account for creating policies. | ||
| let world_address_or_name = world_address | ||
| .map(WorldAddressOrName::Address) | ||
| .unwrap_or(WorldAddressOrName::Name(name.to_string())); | ||
| let migration = Migration::new( | ||
| world_diff, | ||
| WorldContract::new(world_address, &account), | ||
| txn_config, | ||
| ws.load_profile_config()?, | ||
| rpc_url, | ||
| ); | ||
|
|
||
| account.account(provider, world_address_or_name, &starknet, env, ws.config()).await? | ||
| }; | ||
| let MigrationResult { manifest, has_changes } = | ||
| migration.migrate(&mut spinner).await.context("💀 Migration failed.")?; | ||
|
|
||
| let address = account.address(); | ||
| spinner.update_text("Writing manifest..."); | ||
| ws.write_manifest_profile(manifest).context("🪦 Failed to write manifest.")?; | ||
|
|
||
| ui.print(format!("\nMigration account: {address:#x}")); | ||
| let colored_address = format!("{:#066x}", world_address).green(); | ||
|
|
||
| ui.print(format!("\nWorld name: {name}")); | ||
| let (symbol, end_text) = if has_changes { | ||
| ("⛩️ ", format!("Migration successful with world at address {}", colored_address)) | ||
| } else { | ||
| ("🎃", format!("No changes for world at address {:#066x}", world_address)) | ||
| }; | ||
|
|
||
| ui.print(format!("\nChain ID: {chain_id}\n")); | ||
| spinner.stop_and_persist(symbol, Box::leak(end_text.into_boxed_str())); | ||
|
|
||
| match account.provider().get_class_hash_at(BlockId::Tag(BlockTag::Pending), address).await { | ||
| Ok(_) => Ok((account, rpc_url)), | ||
| Err(ProviderError::StarknetError(StarknetError::ContractNotFound)) => { | ||
| Err(anyhow!("Account with address {:#x} doesn't exist.", account.address())) | ||
| } | ||
| Err(e) => Err(e.into()), | ||
| } | ||
| Ok(()) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider defining custom error types.
The error handling uses context with emoji, which is fun but could be more structured. Consider defining custom error types for better error handling and documentation:
#[derive(Debug, thiserror::Error)]
pub enum MigrationError {
#[error("Migration failed: {0}")]
ExecutionError(String),
#[error("Failed to write manifest: {0}")]
ManifestError(String),
}This would allow for more specific error handling and better error messages.
|
|
||
| ui.print(format!("\nChain ID: {chain_id}\n")); | ||
| spinner.stop_and_persist(symbol, Box::leak(end_text.into_boxed_str())); |
There was a problem hiding this comment.
Avoid memory leak from Box::leak.
Ohayo sensei! The Box::leak usage intentionally leaks memory. Consider using a reference instead:
- spinner.stop_and_persist(symbol, Box::leak(end_text.into_boxed_str()));
+ spinner.stop_and_persist(symbol, &end_text);📝 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.
| spinner.stop_and_persist(symbol, Box::leak(end_text.into_boxed_str())); | |
| spinner.stop_and_persist(symbol, &end_text); |
This new version comes with several changes:
Core changes
ModelStorageAPI, simpler, cleaner, more extensible.basecontract.Sozo
sozo inspectcommand to check world's state.TODOs
Summary by CodeRabbit
New Features
scarbtool to2.8.4.Bug Fixes
Documentation
Chores