feat(sozo): Add --output-manifest option to sozo inspect
#2952
Conversation
WalkthroughOhayo, sensei! The changes introduce new capabilities to the Changes
Assessment against linked issues
Sequence DiagramsequenceDiagram
participant User
participant InspectCommand
participant WorldDiff
participant ManifestGenerator
User->>InspectCommand: sozo inspect --output-manifest
InspectCommand->>WorldDiff: Check world status
alt World is deployed and synced
WorldDiff->>ManifestGenerator: Generate Manifest
ManifestGenerator-->>User: Output JSON Manifest
else World not synced
alt Force flag is true
WorldDiff->>ManifestGenerator: Generate Manifest with Warning
ManifestGenerator-->>User: Output JSON Manifest
else Force flag is false
WorldDiff-->>User: Return Synchronization Error
end
end
Sensei, the implementation looks solid and addresses the key requirements for on-demand manifest generation! 🍵🥋 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (3)
bin/sozo/src/commands/inspect.rs (3)
24-35: Ohayo! Consider adding a short form for the--output-manifestflag.The CLI arguments are well-structured, but for consistency with
-f, consider adding a short form (e.g.,-o) for the--output-manifestflag.#[arg( long = "output-manifest", + short = 'o', help = "Outputs the generated manifest to stdout in JSON format." )]
49-63: Maintain naming consistency, sensei!The field
output_manifestis renamed toexport_manifestduring destructuring. Let's keep the naming consistent to avoid confusion.-let InspectArgs { world, starknet, resource, output_manifest: export_manifest, force } = +let InspectArgs { world, starknet, resource, output_manifest, force } = self; config.tokio_handle().block_on(async { let (world_diff, _, _) = utils::get_world_diff_and_provider(starknet.clone(), world, &ws).await?; - if export_manifest { + if output_manifest {
514-536: Add documentation for the generate_manifest function.Consider adding documentation comments to explain:
- The purpose of the function
- The meaning of the
forceparameter- The format of the generated manifest
+/// Generates a JSON manifest of the world's current state. +/// +/// # Arguments +/// * `world_diff` - The world diff containing the current state +/// * `force` - If true, generates the manifest even if the world is not synced +/// +/// # Returns +/// * `Ok(())` if the manifest was generated successfully +/// * `Err` if the world is not deployed or not synced (unless forced) fn generate_manifest(world_diff: &WorldDiff, force: bool) -> Result<()> {Also, consider extracting error messages as constants for reusability:
+const ERR_WORLD_NOT_DEPLOYED: &str = "No world is deployed at address {}!"; +const ERR_WORLD_NOT_SYNCED: &str = "The world is not up to date. Use --force to generate the manifest anyway."; +const WARN_FORCED_GENERATION: &str = "Warning: The world is not up to date, but generation is forced. The generated manifest might not represent the remote world."; fn generate_manifest(world_diff: &WorldDiff, force: bool) -> Result<()> { if world_diff.world_info.status == WorldStatus::NotDeployed { let formatted_address = format!("{:#066x}", world_diff.world_info.address).green(); - return Err(anyhow!("No world is deployed at address {}!", formatted_address)); + return Err(anyhow!(ERR_WORLD_NOT_DEPLOYED, formatted_address)); } // Check if there is some contracts that are not up to date. if !world_diff.is_synced() { if !force { - return Err(anyhow!( - "The world is not up to date. Use --force to generate the manifest anyway." - )); + return Err(anyhow!(ERR_WORLD_NOT_SYNCED)); } else { - eprintln!("Warning: The world is not up to date, but generation is forced. The generated manifest might not represent the remote world."); + eprintln!("{}", WARN_FORCED_GENERATION); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/inspect.rs(4 hunks)
🔇 Additional comments (1)
bin/sozo/src/commands/inspect.rs (1)
513-514: Address the TODO comment, sensei!Consider moving this function to a more appropriate module, perhaps alongside other manifest-related functionality.
Let's check for other manifest-related code:
✅ Verification successful
Ohayo! Move generate_manifest to manifest.rs, sensei!
The
generate_manifestfunction should be moved tocrates/dojo/world/src/diff/manifest.rsas this module:
- Is the central place for manifest-related functionality
- Already handles WorldDiff to Manifest conversion
- Contains related manifest generation code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for manifest-related code rg -l "Manifest" --type rustLength of output: 430
Script:
#!/bin/bash # Check the content of manifest.rs to verify it's the right context cat crates/dojo/world/src/diff/manifest.rsLength of output: 7815
knownasred
force-pushed
the
feat/output-manifest
branch
from
cb7aacb to
971f8ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bin/sozo/src/commands/inspect.rs (3)
24-36: Ohayo! Consider enhancing the CLI argument documentation.The help text for the
--forceflag could better explain the potential implications of forcing manifest generation when the world is not up to date. Also, consider if-omight conflict with other common CLI patterns (e.g.,--output).#[arg( long = "output-manifest", - short = 'o', help = "Outputs the generated manifest to stdout in JSON format." )] output_manifest: bool, #[arg( long = "force", short = 'f', - help = "Force the generation of the manifest, even if the world is not up to date." + help = "Force the generation of the manifest, even if the world is not up to date. Warning: The generated manifest might not accurately represent the remote world state." )] force: bool,
57-64: Ohayo sensei! Consider flattening the nested if-else block.The code is functionally correct, but readability could be improved by flattening the nested if-else structure.
- if output_manifest { - generate_manifest(&world_diff, force)?; - } else { - if let Some(resource) = resource { - inspect_resource(&resource, &world_diff)?; - } else { - inspect_world(&world_diff); - } - } + if output_manifest { + return generate_manifest(&world_diff, force); + } + + if let Some(resource) = resource { + inspect_resource(&resource, &world_diff)?; + } else { + inspect_world(&world_diff); + }
514-539: Ohayo! A few suggestions to enhance error handling and user experience.
- The error message for NotDeployed status shows the address in green, which might be confusing in an error context
- The warning message could be more visible
- Consider wrapping errors with context
if world_diff.world_info.status == WorldStatus::NotDeployed { - let formatted_address = format!("{:#066x}", world_diff.world_info.address).green(); + let formatted_address = format!("{:#066x}", world_diff.world_info.address).red(); return Err(anyhow!("No world is deployed at address {}!", formatted_address)); } // Check if there is some contracts that are not up to date. if !world_diff.is_synced() { if !force { - return Err(anyhow!( + return Err(anyhow!( "The world is not up to date. Use --force to generate the manifest anyway." - )); + ).context("Manifest generation failed")); } else { - eprintln!("Warning: The world is not up to date, but generation is forced. The generated manifest might not represent the remote world."); + eprintln!("{}", "WARNING: The world is not up to date, but generation is forced.".yellow()); + eprintln!("{}", "The generated manifest might not represent the remote world.".yellow()); } } let manifest = Manifest::new(&world_diff); - println!("{}", serde_json::to_string_pretty(&manifest)?); + println!("{}", serde_json::to_string_pretty(&manifest).context("Failed to serialize manifest")?);
knownasred
force-pushed
the
feat/output-manifest
branch
from
971f8ef to
1cd6701
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bin/sozo/src/commands/inspect.rs (3)
50-64: Ohayo! Let's improve the control flow, sensei!The logic is correct, but we can make it more readable by flattening the nested if-else structure.
Consider this refactor:
- if output_manifest { - generate_manifest(&world_diff, force)?; - } else { - if let Some(resource) = resource { - inspect_resource(&resource, &world_diff)?; - } else { - inspect_world(&world_diff); - } - } + if output_manifest { + return generate_manifest(&world_diff, force); + } + + if let Some(resource) = resource { + inspect_resource(&resource, &world_diff)?; + return Ok(()); + } + + inspect_world(&world_diff); + Ok(())
519-520: Ohayo! Let's adjust the error message color, sensei!Using green color for an address in an error message might be confusing. Consider using a neutral or error-indicating color.
- let formatted_address = format!("{:#066x}", world_diff.world_info.address).green(); + let formatted_address = format!("{:#066x}", world_diff.world_info.address).red();
526-528: Consider providing more specific error information, sensei!The error message could be more helpful by indicating which specific aspects of the world are not up to date.
- "The world is not up to date. Use --force to generate the manifest anyway." + "The world is not up to date (some contracts need deployment or initialization). Use --force to generate the manifest anyway."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/inspect.rs(4 hunks)
🔇 Additional comments (2)
bin/sozo/src/commands/inspect.rs (2)
24-36: Ohayo! The CLI args are well-structured, sensei!The new options follow CLI best practices with clear help messages and conventional short/long option naming.
514-539: Ohayo! Overall implementation looks solid, sensei!The new manifest generation feature is well-implemented with proper error handling and user feedback. It successfully addresses the PR objective of enabling frontend generation on the fly.
There was a problem hiding this comment.
Thanks for the contribution here @knownasred.
Reading this, we may go directly to the fully remote generated manifest, to avoid confusion and maybe errors in using a manifest into a client application.
This way, as you have mentioned, one with sozo installed and the world_address is able to reconstruct the manifest.
Do you need any pointers to go this way, or it's clear where you will have to modify code for that?
sozo inspect is the good place to have this --output-manifest flag. 👍
Description
Adds a
--output-manifestoption tosozo inspectthat will output the generated manifest to stdout.Useful for scripts and to generate the frontend on the fly, removing the requirement of committing the manifest to git
Related issue
Fixes #2878
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
Improvements