feat(sozo): rework UI - #3342
Conversation
356489d to
b5e8b63
Compare
|
ohayo sensei, WalkthroughAdds a new themable UI crate Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Term as Terminal
participant Main as sozo::main
participant Theme as terminal-colorsaurus
participant UI as SozoUi
participant Cmd as commands::run
participant Sub as Subcommand
participant U as utils::get_world_diff_and_provider
participant Prov as RPC
User->>Term: run `sozo <cmd>`
Term->>Main: parse args
Main->>Theme: detect theme
Theme-->>Main: Light/Dark or error
Main->>UI: SozoUi::new(theme, verbosity)
Main->>Cmd: run(command, metadata, &UI)
Cmd->>Sub: args.run(metadata, &UI)
Sub->>U: get_world_diff_and_provider(..., &UI)
U->>Prov: RPC / health_check
Prov-->>U: status/data
U-->>Sub: (WorldDiff, Provider, profile)
Sub->>UI: title/step/result/warn(...)
Sub-->>Cmd: Result
Cmd-->>Main: Result
Main->>UI: error_block(...) on failure
sequenceDiagram
autonumber
participant Mig as sozo_ops::Migration
participant UI as SozoUi
participant Depl as deploy_via_udc
participant Net as RPC / Chain
participant Meta as UploadService
UI->>Mig: migrate(&UI)
Mig->>UI: title "Evaluate project's state"
Mig->>Depl: deploy_via_udc(...)
Depl->>Net: send tx
Net-->>Depl: (address, tx_result/receipt)
Depl-->>Mig: (Felt, TransactionResult)
Mig->>UI: step/result with address/status
Mig->>Meta: upload_metadata(&UI, ...)
Meta-->>Mig: cid/ok
Mig->>UI: result "Metadata uploaded."
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
bin/sozo/src/utils.rs (2)
131-135: Fix RPC version check: allow stable versions newer than the RC.
Current logic may reject node versions like 0.9.0 or 0.9.x even though they’re ≥ 0.9.0-rc.2. Switch to a VersionReq minimum and clarify the error.Apply this diff:
- pub const RPC_SPEC_VERSION: &str = "0.9.0-rc.2"; + pub const MIN_RPC_SPEC_VERSION_REQ: &str = ">=0.9.0-rc.2"; @@ - if !is_compatible_version(&spec_version, RPC_SPEC_VERSION)? { - return Err(anyhow!( - "Unsupported Starknet RPC version: {spec_version}, expected {RPC_SPEC_VERSION}.", - )); + if !is_compatible_version(&spec_version, MIN_RPC_SPEC_VERSION_REQ)? { + return Err(anyhow!( + "Unsupported Starknet RPC version: {spec_version}, required {MIN_RPC_SPEC_VERSION_REQ}.", + )); } @@ -fn is_compatible_version(provided_version: &str, expected_version: &str) -> Result<bool> { - let provided_ver = Version::parse(provided_version) - .map_err(|e| anyhow!("Failed to parse provided version '{}': {}", provided_version, e))?; - let expected_ver = Version::parse(expected_version) - .map_err(|e| anyhow!("Failed to parse expected version '{}': {}", expected_version, e))?; - - // Specific backward compatibility rule: 0.6 is compatible with 0.7. - if (provided_ver.major == 0 && provided_ver.minor == 7) - && (expected_ver.major == 0 && expected_ver.minor == 6) - { - return Ok(true); - } - - let expected_ver_req = VersionReq::parse(expected_version).map_err(|e| { - anyhow!("Failed to parse expected version requirement '{}': {}", expected_version, e) - })?; - - Ok(expected_ver_req.matches(&provided_ver)) +fn is_compatible_version(provided_version: &str, expected_req: &str) -> Result<bool> { + let provided_ver = Version::parse(provided_version) + .map_err(|e| anyhow!("Failed to parse provided version '{}': {}", provided_version, e))?; + + // Optional special-case if a plain version was supplied as the "requirement". + if let Ok(expected_ver) = Version::parse(expected_req) { + if (provided_ver.major == 0 && provided_ver.minor == 7) + && (expected_ver.major == 0 && expected_ver.minor == 6) + { + return Ok(true); + } + } + + // Treat input as a semver requirement. If it isn't one, fall back to ">=<version>". + let expected_ver_req = VersionReq::parse(expected_req) + .or_else(|_| VersionReq::parse(&format!(">={}", expected_req))) + .map_err(|e| { + anyhow!("Failed to parse expected version requirement '{}': {}", expected_req, e) + })?; + + Ok(expected_ver_req.matches(&provided_ver)) }Also applies to: 35-36, 445-463
480-492: ohayo sensei — Fix inline &ui.subsection() callsites; pass a stable &SozoUi referenceInline uses of &ui.subsection() create a temporary that can’t be safely borrowed across an async .await — bind the subsection to a local and pass its reference.
- Affected: bin/sozo/src/commands/migrate.rs:59 — change to
let step_ui = ui.subsection();then pass&step_ui.bin/sozo/src/commands/init.rs (3)
121-148: Checkgit cloneexit status and avoidto_str().unwrap()on paths.Failures currently go unnoticed, and
unwrap()will panic on non‑UTF8 paths.- ui.print(format!("Cloning project template from {}...", url)); - Command::new("git") - .args([ - "clone", - "--branch", - &format!("v{}", version), - "--single-branch", - "--recursive", - url, - path.to_str().unwrap(), - ]) - .output()?; + ui.print(format!("Cloning project template from {}...", url)); + let out = Command::new("git") + .arg("clone") + .arg("--branch").arg(format!("v{}", version)) + .arg("--single-branch") + .arg("--recursive") + .arg(url) + .arg(path) // accepts AsRef<OsStr> + .output()?; + anyhow::ensure!( + out.status.success(), + "git clone failed: {}", + String::from_utf8_lossy(&out.stderr) + ); ... - ui.warn( - "Couldn't find template for your current sozo version. Getting the latest version - instead.", - ); - Command::new("git").args(["clone", "--recursive", url, path.to_str().unwrap()]).output()?; + ui.warn("Couldn't find template for your current sozo version. Getting the latest version instead."); + let out = Command::new("git") + .arg("clone") + .arg("--recursive") + .arg(url) + .arg(path) + .output()?; + anyhow::ensure!( + out.status.success(), + "git clone failed: {}", + String::from_utf8_lossy(&out.stderr) + );
111-119: Checkgit ls-remotestatus before parsing tags.If
gitfails (e.g., offline),.contains()will just return false and pick “latest,” hiding the real error.- let output = Command::new("git").args(["ls-remote", "--tags", url]).output()?; - - let output_str = String::from_utf8(output.stdout)?; + let output = Command::new("git").args(["ls-remote", "--tags", url]).output()?; + anyhow::ensure!(output.status.success(), "git ls-remote failed: {}", String::from_utf8_lossy(&output.stderr)); + let output_str = String::from_utf8(output.stdout)?;
150-167: Check allgitcommands’ exit status inmodify_git_history.If any
gitinvocation fails, the code proceeds silently.- let git_output = Command::new("git").args(["rev-parse", "--short", "HEAD"]).output()?.stdout; - let commit_hash = String::from_utf8(git_output)?; + let head = Command::new("git").args(["rev-parse", "--short", "HEAD"]).output()?; + anyhow::ensure!(head.status.success(), "git rev-parse failed: {}", String::from_utf8_lossy(&head.stderr)); + let commit_hash = String::from_utf8(head.stdout)?; ... - Command::new("git").arg("init").output()?; - Command::new("git").args(["add", "--all"]).output()?; + anyhow::ensure!(Command::new("git").arg("init").status()?.success(), "git init failed"); + anyhow::ensure!(Command::new("git").args(["add", "--all"]).status()?.success(), "git add failed"); ... - Command::new("git").args(["commit", "-m", &commit_msg]).output()?; + anyhow::ensure!(Command::new("git").args(["commit", "-m", &commit_msg]).status()?.success(), "git commit failed");crates/sozo/ops/src/migrate/mod.rs (2)
347-401: Avoid syncing when there’s nothing to change.Same empty‑call risk applies here; also improves logs by stating there’s nothing to do.
Apply:
- let has_changed = !invoker.calls.is_empty(); - - if self.do_multicall() { - ui.step(format!("Sync {} permissions", invoker.calls.len())); - invoker.multicall().await?; - } else { - ui.print(format!("Sync {} permissions (sequentially)", invoker.calls.len())); - invoker.invoke_all_sequentially().await?; - } - - ui.result("Permissions synced."); - Ok(has_changed) + let has_changed = !invoker.calls.is_empty(); + if !has_changed { + ui.result("No permissions to sync."); + return Ok(false); + } + if self.do_multicall() { + ui.step(format!("Sync {} permissions", invoker.calls.len())); + invoker.multicall().await?; + } else { + ui.step(format!("Sync {} permissions (sequentially)", invoker.calls.len())); + invoker.invoke_all_sequentially().await?; + } + ui.result("Permissions synced."); + Ok(true)
565-606: Only register resources when there are calls to execute.Protects against empty multicall and clarifies output.
Apply:
- if self.do_multicall() { + if n_resources == 0 && invoker.calls.is_empty() && deploy_calls.is_empty() { + ui.result("No resources to register."); + } else if self.do_multicall() { ui.step(format!("Register {} resources", n_resources)); invoker.extend_calls(deploy_calls.values().cloned().collect()); let txs_results = invoker.multicall().await?; ... - } else { + } else { ui.step(format!("Register {} resources (sequentially)", n_resources)); invoker.invoke_all_sequentially().await?; ... } - ui.result("Resources registered."); + if n_resources > 0 { + ui.result("Resources registered."); + }
🧹 Nitpick comments (37)
bin/sozo/src/commands/version.rs (3)
10-10: Plan deprecation path before removal (confirm behavior overlap).Ohayo sensei — if this subcommand is removed, will
sozo --versionalso surface the Scarb version, or only Sozo’s? If not equivalent, consider a deprecation cycle: hide the subcommand, print a deprecation notice, then drop in the next release.Apply this optional deprecation notice if you keep it for one cycle:
impl VersionArgs { pub fn run(&self, scarb_metadata: &Metadata) -> Result<()> { + eprintln!("Deprecated: use `sozo --version`. This subcommand will be removed in a future release.");
19-21: Propagate child process failures and avoid UTF‑8 panics.Check
status.success()and print stderr on failure; preferfrom_utf8_lossyfor robustness.Apply:
- let output = Command::new(app).args(["--version"]).output()?; - println!("{}", String::from_utf8(output.stdout)?); + let output = Command::new(app).arg("--version").output()?; + if !output.status.success() { + bail!("Failed to run Scarb: {}", String::from_utf8_lossy(&output.stderr)); + } + println!("{}", String::from_utf8_lossy(&output.stdout));
14-16: Tiny copy edit in user‑facing error.Pluralize “instruction” → “instructions.”
- bail!( - "Scarb not found. Find install instruction here: https://docs.swmansion.com/scarb" - ) + bail!( + "Scarb not found. See install instructions: https://docs.swmansion.com/scarb" + )crates/dojo/utils/src/provider.rs (2)
1-3: Ohayo, sensei — keep the friendly error while preserving the root cause via ContextThe new generic message improves UX but drops diagnostic context. Use
anyhow::Contextto keep the underlying error in the chain while showing the same user-facing text.use starknet::core::types::{BlockId, BlockTag}; use starknet::providers::Provider; use tracing::trace; +use anyhow::Context; @@ pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>( provider: P, ) -> anyhow::Result<(), anyhow::Error> { - match provider.get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)).await { - Ok(block) => { - trace!( - latest_block = ?block, - "Provider health check." - ); - Ok(()) - } - Err(_) => Err(anyhow::anyhow!("Unhealthy provider. Please check your configuration.")), - } + let block = provider + .get_block_with_tx_hashes(BlockId::Tag(BlockTag::Latest)) + .await + .context("Unhealthy provider. Please check your configuration.")?; + trace!(latest_block = ?block, "Provider health check."); + Ok(()) }Also applies to: 12-21
9-11: Ohayo, sensei — simplify the return typeIdiomatic anyhow: prefer
anyhow::Result<()>overanyhow::Result<(), anyhow::Error>.-pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>( - provider: P, -) -> anyhow::Result<(), anyhow::Error> { +pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>( + provider: P, +) -> anyhow::Result<()> {crates/dojo/utils/src/tx/error.rs (2)
61-67: Improve top‑level rendering by threading indentation depth.Passing an initial depth clarifies nesting downstream.
-fn display_tx_execution_error(error: &TransactionExecutionErrorData) -> String { - format!( - "Transaction error (index: {})\n{}", - error.transaction_index, - display_tx_execution_detail(&error.execution_error) - ) -} +fn display_tx_execution_error(error: &TransactionExecutionErrorData) -> String { + format!( + "Transaction error (index: {})\n{}", + error.transaction_index, + display_tx_execution_detail(&error.execution_error, 0) + ) +}
69-80: Hex‑format addresses, indent nested errors, and drop the unnecessary clone.Better UX and less allocation.
-fn display_tx_execution_detail(detail: &ContractExecutionError) -> String { - match detail { - ContractExecutionError::Message(msg) => format!("Message: {}", msg.clone()), - ContractExecutionError::Nested(nested) => { - format!( - "Error in contract at {}\n{}", - nested.contract_address, - display_tx_execution_detail(&nested.error) - ) - } - } -} +fn display_tx_execution_detail(detail: &ContractExecutionError, depth: usize) -> String { + let indent = " ".repeat(depth); + match detail { + ContractExecutionError::Message(msg) => format!("{indent}Message: {msg}"), + ContractExecutionError::Nested(nested) => { + format!( + "{indent}Error in contract at {:#066x}\n{}", + nested.contract_address, + display_tx_execution_detail(&nested.error, depth + 1) + ) + } + } +}crates/dojo/world/src/diff/resource.rs (1)
133-141: Consider implementing Display for ResourceDiff for consistency.WorldStatus and ResourceType already implement Display. Mirroring that here would let UI code format ResourceDiff values uniformly.
Example outside this hunk:
impl std::fmt::Display for ResourceDiff { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}", self.status()) } }crates/dojo/world/src/lib.rs (1)
33-44: Ergonomics: derive Eq/Clone/Copy on ResourceType.Handy for passing by value and using in sets/maps without allocations. Safe for a small C-like enum.
Outside this hunk:
#[derive(Debug, PartialEq, Eq, Clone, Copy)] pub enum ResourceType { // ... }crates/dojo/world/src/diff/mod.rs (1)
58-66: ohayo sensei — Centralize status labels to prevent driftMultiple crates hard-code the same status strings (e.g. "Synced", "Not Deployed", "To Upgrade"); centralize them (shared consts or a single Display impl consumed across crates) and add a snapshot/unit test to lock the exact strings.
See: crates/dojo/world/src/diff/mod.rs and bin/sozo/src/commands/inspect.rs (rg shows many "Synced" occurrences).examples/spawn-and-move/manifest_dev.json (1)
1482-1866: ohayo sensei — Bulk artifact refresh: addresses/class_hashes updated — add quick integrity checksAutomated verification failed with jq/bash errors; run the script below against examples/spawn-and-move/manifest_dev.json and paste the output.
- Checks: 1) hex shape of addresses/class_hashes (0x + lowercase hex) 2) unique tags across contracts/libraries/models/events/external_contracts 3) presence of each contract.systems entry as a function in its contract ABI
#!/bin/bash set -euo pipefail JSON=examples/spawn-and-move/manifest_dev.json if [ ! -f "$JSON" ]; then echo "ERROR: file not found: $JSON" >&2 exit 2 fi # 1) Collect hex-like fields jq -r ' ( (.contracts[]? | [.address?, .class_hash?])[], (.libraries[]? | [.class_hash?])[], (.models[]? | [.class_hash?])[], (.events[]? | [.class_hash?])[], (.external_contracts[]? | [.address?, .class_hash?])[] ) | .[] | select(. != null) ' "$JSON" | sort -u > /tmp/manifest_hex_vals.txt bad_hex=0 while IFS= read -r v; do [ -z "$v" ] && continue if ! printf '%s\n' "$v" | grep -qE '^0x[0-9a-f]+$'; then echo "BAD_HEX: $v" bad_hex=1 fi done < /tmp/manifest_hex_vals.txt if [ $bad_hex -eq 0 ]; then echo "OK: hex formatting"; else echo "FAIL: hex formatting"; fi # 2) Duplicate tags jq -r ' ( (.contracts[]?.tag), (.libraries[]?.tag), (.models[]?.tag), (.events[]?.tag), (.external_contracts[]?.tag) ) | select(. != null) ' "$JSON" | sort | uniq -d > /tmp/manifest_dup_tags.txt || true if [ -s /tmp/manifest_dup_tags.txt ]; then echo "DUP_TAGS:" cat /tmp/manifest_dup_tags.txt echo "FAIL: duplicate tags" else echo "OK: unique tags" fi # 3) Systems -> ABI functions presence jq -c ' [.contracts[]? as $c | { tag: $c.tag, missing: [ ($c.systems // [])[]? as $s | select( ( ($c.abi // []) | any( .type=="function" and .name==$s ) ) | not ) ] } ] | map(select(.missing | length > 0)) ' "$JSON" > /tmp/manifest_missing_systems.json if [ -s /tmp/manifest_missing_systems.json ] && [ "$(jq -r 'length' /tmp/manifest_missing_systems.json)" -gt 0 ]; then echo "MISSING_SYSTEMS:" jq -r '.[] | "\(.tag): \(.missing | join(", "))"' /tmp/manifest_missing_systems.json echo "FAIL: missing systems entrypoints" else echo "OK: systems entrypoints present" fiexamples/spawn-and-move/dojo_dev.toml (1)
72-72: ohayo sensei — Normalize world_address formatting to avoid brittle string comparesThey match numerically, but normalize the hex string across files to avoid fragile string-equality checks.
-world_address = "0x035bab41ae664e0303f0dedf06c068f118c0a5649bdc5c5c07d909446d9beb72" +world_address = "0x35bab41ae664e0303f0dedf06c068f118c0a5649bdc5c5c07d909446d9beb72"bin/sozo/Cargo.toml (1)
39-39: Consider housing terminal-colorsaurus inside sozo-ui.
Keeps theming concerns in one place and reduces CLI crate surface.bin/sozo/src/utils.rs (4)
368-371: Avoid printing credentials embedded in IPFS URLs.
Sanitize userinfo if present before printing.Apply this diff:
- local_ui.debug(local_ui.indent(2, format!("url: {}", ipfs_config.url))); + local_ui.debug(local_ui.indent(2, format!("url: {}", sanitize_url(&ipfs_config.url))));Add this helper (and
use url::Url;at top if needed):fn sanitize_url(raw: &str) -> String { if let Ok(mut u) = url::Url::parse(raw) { let _ = u.set_username(""); let _ = u.set_password(None); u.to_string() } else { raw.to_string() } }
397-409: Show selectors as 0x-prefixed 66‑wide hex for consistency.
Improves readability alongside other hex fields.Apply this diff:
- .map(|(selector, resource)| ResourceDetails { + .map(|(selector, resource)| ResourceDetails { resource_type: resource.resource_type(), tag: resource.tag(), status: resource.status(), - selector: selector.to_string(), + selector: format!("{:#066x}", selector), })
468-475: Minor nit: avoid embedded newline in “not found” string.
Prevents double line breaks in output.Apply this diff:
- } else { - "not found in your PATH\n".to_string() - }; + } else { + "not found in your PATH".to_string() + };
501-509: Accept “yes” as confirmation too.
Slight UX polish for prompt parsing.Apply this diff:
- Ok(input.trim().to_lowercase() == "y") + Ok(matches!(input.trim().to_lowercase().as_str(), "y" | "yes"))crates/sozo/ui/Cargo.toml (1)
6-7: ohayo sensei — Centralize theme/color handling in crates/sozo/uiRepo contains multiple direct color/theming deps; move terminal-theme detection + helper APIs into crates/sozo/ui and re-export them so downstream crates stop depending on color crates directly.
Detected direct deps (Cargo.toml):
- root Cargo.toml: colored = "2.0.0" (ln131), console = "0.15.7" (ln133)
- bin/sozo/Cargo.toml: colored.workspace = true (ln19)
- crates/sozo/ui/Cargo.toml: colored.workspace = true (ln7)
- crates/sozo/mcp/Cargo.toml: colored.workspace = true (ln22)
- crates/sozo/ops/Cargo.toml: colored.workspace = true (ln12)
- bin/cairo-bench/Cargo.toml: colored.workspace = true (ln14)
- crates/sozo/walnut/Cargo.toml: console.workspace = true (ln11)
Action: implement detection + re-export in crates/sozo/ui and remove direct colored/console deps from the listed crates.
crates/sozo/ui/src/lib.rs (5)
117-123: Title prints add an unconditional newline; consider avoiding double spacing.
title()always callsnew_line()before printing; frequent titles can create extra vertical whitespace. Consider gating the leading newline on previous-write state or de-duplicating consecutive blank lines. Happy to sketch a tiny “last_was_newline” tracker if you want.
125-131: Fix small typos in docs.
- “withthe” → “within”
- “erorr” → “error”
- /// Prints a step withthe current section. + /// Prints a step within the current section. ... - /// Prints a block of text surrounded by newlines with a specific erorr color. + /// Prints a block of text surrounded by newlines with a specific error color.Also applies to: 189-193
244-279: Safer, clearer color fallback for deep nesting.When
section_level >= section_colors.len(), you drop coloring entirely. Prefer clamping to the last configured color and prefix to keep consistent styling for deep sections.- fn get_title_prefix(&self) -> String { - if self.section_level < self.theme.title_prefixes.len() { - self.theme.title_prefixes[self.section_level].to_string() - } else { - "".to_string() - } - } + fn get_title_prefix(&self) -> String { + let idx = self.section_level.min(self.theme.title_prefixes.len().saturating_sub(1)); + self.theme.title_prefixes.get(idx).cloned().unwrap_or_default() + } ... - fn print_with_section_color(&self, text: String) { - let text = if self.section_level < self.theme.section_colors.len() { - let mut text = text.color(self.theme.section_colors[self.section_level]); + fn print_with_section_color(&self, text: String) { + let idx = self.section_level.min(self.theme.section_colors.len().saturating_sub(1)); + let text = if let Some(color) = self.theme.section_colors.get(idx).cloned() { + let mut text = text.color(color);
5-13: Verbosity gating relies on enum ordering; lock intent in code.
self.verbosity >= verbositydepends on variant order. Future reordering would silently change behavior. Consider mapping to an explicit numeric level or derivingOrdand adding a unit test asserting the intended order (Quiet < Normal < Verbose < Debug < Trace).Also applies to: 165-175, 196-208
47-76: Consider honoring NO_COLOR/CI/non‑TTY to disable colors.Many CLIs disable colors when not a TTY or when NO_COLOR is set. You can integrate
colored::control::set_override(false)behind a smallSozoUi::auto_color_from_env()helper.bin/sozo/src/commands/init.rs (2)
93-109: Validatesozo --versionexit status.Guard against non‑zero exit codes.
- let output = Command::new("sozo") + let output = Command::new("sozo") .arg("--version") .output() .context("Failed to execute `sozo --version` command")?; + anyhow::ensure!( + output.status.success(), + "Command `sozo --version` failed: {}", + String::from_utf8_lossy(&output.stderr) + );
139-143: Trim the warning string (avoid embedded newline/indent).The current multiline literal embeds a newline and spaces. Use a single‑line string for cleaner output.
- ui.warn( - "Couldn't find template for your current sozo version. Getting the latest version - instead.", - ); + ui.warn("Couldn't find template for your current sozo version. Getting the latest version instead.");crates/sozo/ops/src/migrate/error.rs (1)
41-42: New error variant is fine; consider a From impl for ergonomics.
DeployWorldError(anyhow::Error)mirrorsDeployExternalContractError. Addimpl From<anyhow::Error> for MigrationError<S>behind a helper if it’s a common path; otherwise LGTM.crates/sozo/walnut/src/debugger.rs (1)
40-42: Preferui.resultfor link-style statusUsing
ui.result(format!("Debug with Walnut: {url}"))aligns with other success outputs and improves visual consistency.- ui.print(format!("Debug transaction with Walnut: {url}")); + ui.result(format!("Debug with Walnut: {url}"));bin/sozo/src/commands/execute.rs (1)
171-173: Route results through SozoUi instead ofprintln!Keeps output theming/verbosity consistent.
- for r in &txs_results { - println!("{}", r); - } + for r in &txs_results { + ui.result(r.to_string()); + }bin/sozo/src/commands/call.rs (1)
107-109: Error message source can be misleadingWhen using the local manifest (no
--diff), saying “not found in the world diff” is inaccurate. Tailor the message based on source.- let contract_address = contract_address - .ok_or_else(|| anyhow!("Contract {descriptor} not found in the world diff."))?; + let contract_address = contract_address.ok_or_else(|| { + let source = if self.diff || local_manifest.is_none() { + "world diff" + } else { + "local manifest" + }; + anyhow!("Contract {descriptor} not found in the {source}.") + })?;bin/sozo/src/main.rs (1)
37-39: Error rendering via UI is consistent
ui.error_block(format!("{err:?}").trim())is fine; consider including a friendly hint only if it doesn’t leak internals.bin/sozo/src/commands/migrate.rs (2)
111-121: Avoid mixingcoloredstyling with SozoUi themingColoring the address with
colored::green()may clash with UI themes. Prefer UI-rendered styling (e.g.,ui.result(...)or a themed block).- let colored_address = format!("{:#066x}", world_address).green(); - - let 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.new_line(); - ui.block(end_text); - ui.new_line(); + let end_text = if has_changes { + format!("Migration successful with world at address {:#066x}", world_address) + } else { + format!("No changes for world at address {:#066x}", world_address) + }; + ui.new_line(); + ui.result(end_text); + ui.new_line();
135-160: Banner printing via UI is clean; minor debug detail
ui.debug(format!("Provider: {:?}", provider));is helpful; just ensure no sensitive data can leak in debug logs for shared terminals.crates/sozo/ops/src/tests/migration.rs (1)
73-76: Consider silencing UI output in tests to reduce noise/flakes.Using SozoUi::default() may emit styled output; if a silent/quiet verbosity exists, prefer it for CI stability.
Can you confirm if sozo_ui exposes a Silent/Quiet verbosity or builder helper we can use here?
bin/sozo/src/commands/auth.rs (2)
332-339: Route color decisions through SozoUi for consistent theming.Direct Colorize styling here may conflict with theme/TTY detection. Consider letting SozoUi own the color for these segments.
340-344: Interactive confirm should be UI/flag‑driven to support non‑TTY runs.Using utils::prompt_confirm may block in CI/non‑interactive shells. Add a non‑interactive flag (e.g., --yes) or expose a SozoUi confirm.
I can draft a follow‑up PR adding a --yes flag and plumbing it through clone_permissions; want me to?
bin/sozo/src/commands/mod.rs (1)
82-103: Fix display label for Walnut command.Display returns "WalnutVerify"; this should likely be "Walnut" for consistency with the variant/help text.
Apply:
- #[cfg(feature = "walnut")] - Commands::Walnut(_) => write!(f, "WalnutVerify"), + #[cfg(feature = "walnut")] + Commands::Walnut(_) => write!(f, "Walnut"),crates/sozo/ops/src/migrate/mod.rs (1)
420-462: Skip class declaration when n_classes == 0.Prevents unnecessary declarer setup and clearer logs.
Apply:
- let n_classes = classes.len(); + let n_classes = classes.len(); + if n_classes == 0 { + ui.trace("No classes to declare."); + return Ok(()); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (37)
Cargo.toml(3 hunks)bin/sozo/Cargo.toml(1 hunks)bin/sozo/src/args.rs(2 hunks)bin/sozo/src/commands/auth.rs(21 hunks)bin/sozo/src/commands/call.rs(5 hunks)bin/sozo/src/commands/events.rs(2 hunks)bin/sozo/src/commands/execute.rs(3 hunks)bin/sozo/src/commands/init.rs(3 hunks)bin/sozo/src/commands/inspect.rs(2 hunks)bin/sozo/src/commands/migrate.rs(7 hunks)bin/sozo/src/commands/mod.rs(2 hunks)bin/sozo/src/commands/model.rs(7 hunks)bin/sozo/src/commands/version.rs(1 hunks)bin/sozo/src/main.rs(1 hunks)bin/sozo/src/utils.rs(5 hunks)crates/dojo/utils/src/provider.rs(1 hunks)crates/dojo/utils/src/tx/deployer.rs(2 hunks)crates/dojo/utils/src/tx/error.rs(3 hunks)crates/dojo/world/src/diff/mod.rs(1 hunks)crates/dojo/world/src/diff/resource.rs(1 hunks)crates/dojo/world/src/lib.rs(1 hunks)crates/sozo/ops/Cargo.toml(1 hunks)crates/sozo/ops/src/lib.rs(0 hunks)crates/sozo/ops/src/migrate/error.rs(1 hunks)crates/sozo/ops/src/migrate/mod.rs(19 hunks)crates/sozo/ops/src/migration_ui.rs(0 hunks)crates/sozo/ops/src/tests/migration.rs(2 hunks)crates/sozo/ui/Cargo.toml(1 hunks)crates/sozo/ui/src/lib.rs(1 hunks)crates/sozo/walnut/Cargo.toml(1 hunks)crates/sozo/walnut/src/debugger.rs(3 hunks)crates/sozo/walnut/src/verification.rs(2 hunks)crates/sozo/walnut/src/walnut.rs(2 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)examples/spawn-and-move/manifest_dev.json(15 hunks)xtask/generate-test-db/Cargo.toml(1 hunks)xtask/generate-test-db/src/main.rs(2 hunks)
💤 Files with no reviewable changes (2)
- crates/sozo/ops/src/lib.rs
- crates/sozo/ops/src/migration_ui.rs
🧰 Additional context used
🧬 Code graph analysis (18)
bin/sozo/src/commands/events.rs (2)
bin/sozo/src/commands/mod.rs (1)
run(106-134)bin/sozo/src/utils.rs (2)
world_diff(398-407)get_world_diff_and_provider(98-153)
bin/sozo/src/commands/init.rs (4)
bin/sozo/src/commands/auth.rs (1)
run(141-212)bin/sozo/src/commands/call.rs (1)
run(53-152)bin/sozo/src/commands/execute.rs (1)
run(67-183)bin/sozo/src/commands/mod.rs (1)
run(106-134)
crates/sozo/walnut/src/walnut.rs (3)
bin/sozo/src/commands/auth.rs (1)
run(141-212)bin/sozo/src/commands/execute.rs (1)
run(67-183)bin/sozo/src/commands/mod.rs (1)
run(106-134)
crates/dojo/world/src/lib.rs (4)
crates/dojo/world/src/diff/mod.rs (1)
fmt(59-65)bin/sozo/src/commands/inspect.rs (2)
fmt(65-73)fmt(183-189)crates/dojo/world/src/services/ipfs_service.rs (1)
fmt(18-21)crates/dojo/world/src/uri.rs (1)
fmt(61-67)
crates/dojo/world/src/diff/mod.rs (2)
bin/sozo/src/commands/inspect.rs (2)
fmt(65-73)fmt(183-189)crates/dojo/world/src/lib.rs (1)
fmt(34-43)
xtask/generate-test-db/src/main.rs (1)
crates/sozo/ui/src/lib.rs (1)
default(87-89)
bin/sozo/src/commands/inspect.rs (2)
bin/sozo/src/commands/mod.rs (1)
run(106-134)bin/sozo/src/utils.rs (2)
world_diff(398-407)get_world_diff_and_provider(98-153)
bin/sozo/src/commands/call.rs (3)
bin/sozo/src/commands/auth.rs (1)
run(141-212)bin/sozo/src/commands/mod.rs (1)
run(106-134)bin/sozo/src/commands/print_env.rs (1)
run(24-59)
bin/sozo/src/main.rs (1)
crates/sozo/ui/src/lib.rs (5)
trace(173-175)default(87-89)light(48-61)dark(63-76)new(94-96)
bin/sozo/src/args.rs (1)
crates/sozo/ui/src/lib.rs (2)
error(156-163)verbose(165-167)
bin/sozo/src/commands/auth.rs (2)
bin/sozo/src/commands/mod.rs (1)
run(106-134)bin/sozo/src/utils.rs (3)
world_diff(398-407)get_world_diff_and_account(159-192)get_world_diff_and_provider(98-153)
bin/sozo/src/commands/migrate.rs (2)
crates/sozo/ui/src/lib.rs (2)
trace(173-175)new(94-96)crates/dojo/utils/src/provider.rs (1)
health_check_provider(9-22)
bin/sozo/src/commands/execute.rs (9)
bin/sozo/src/commands/auth.rs (1)
run(141-212)bin/sozo/src/commands/call.rs (1)
run(53-152)bin/sozo/src/commands/init.rs (1)
run(28-90)bin/sozo/src/commands/inspect.rs (1)
run(37-52)bin/sozo/src/commands/migrate.rs (1)
run(46-124)bin/sozo/src/commands/model.rs (1)
run(134-222)bin/sozo/src/commands/mod.rs (1)
run(106-134)bin/sozo/src/commands/events.rs (1)
run(60-156)crates/sozo/walnut/src/walnut.rs (1)
run(26-33)
bin/sozo/src/utils.rs (7)
crates/dojo/world/src/local/mod.rs (2)
deterministic_world_address(100-103)new(83-97)crates/dojo/world/src/config/environment.rs (3)
world_address(26-28)rpc_url(30-32)account_address(34-36)bin/sozo/src/commands/options/starknet.rs (2)
provider(32-50)url(55-67)bin/sozo/src/commands/options/account/mod.rs (2)
account(70-95)account_address(139-152)crates/dojo/utils/src/tx/deployer.rs (2)
is_deployed(107-122)new(34-36)crates/dojo/world/src/diff/resource.rs (3)
name(63-69)namespace(72-78)status(133-141)crates/sozo/ui/src/lib.rs (1)
new(94-96)
crates/sozo/ops/src/migrate/mod.rs (1)
crates/sozo/ui/src/lib.rs (1)
trace(173-175)
crates/sozo/ops/src/tests/migration.rs (1)
crates/sozo/ui/src/lib.rs (1)
default(87-89)
bin/sozo/src/commands/mod.rs (7)
bin/sozo/src/commands/auth.rs (1)
run(141-212)bin/sozo/src/commands/call.rs (1)
run(53-152)bin/sozo/src/commands/execute.rs (1)
run(67-183)bin/sozo/src/commands/inspect.rs (1)
run(37-52)bin/sozo/src/commands/migrate.rs (1)
run(46-124)bin/sozo/src/commands/model.rs (1)
run(134-222)bin/sozo/src/commands/events.rs (1)
run(60-156)
bin/sozo/src/commands/model.rs (2)
bin/sozo/src/commands/mod.rs (1)
run(106-134)bin/sozo/src/utils.rs (1)
get_world_diff_and_provider(98-153)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: clippy
🔇 Additional comments (53)
crates/dojo/utils/src/tx/error.rs (2)
3-3: Ohayo, sensei — importing ContractExecutionError is correct.Needed for the richer nested error formatting introduced below.
18-19: Dynamic Display via thiserror expression looks good; please confirm toolchain support.Using a function call inside #[error("{}", …)] relies on thiserror’s expression support. Sanity‑check it builds across our MSRV/CI matrix.
crates/dojo/utils/src/tx/deployer.rs (3)
95-98: Receipt tuple return looks good.
102-102: Final tuple return LGTM.
72-72: Ohayo sensei — breaking return-type change handled.
Call site crates/sozo/ops/src/migrate/mod.rs:1122 already destructureslet (world_address, res) = deployer.deploy_via_udc(...); ripgrep found no other usages. Optional: addReturns: (contract_address, TransactionResult)to the fn docs.crates/dojo/world/src/diff/mod.rs (1)
58-66: LGTM: Display mapping reads well and matches CLI phrasing.The strings ("Not Deployed", "To Upgrade", "Synced") align with sozo inspect outputs.
crates/dojo/world/src/lib.rs (2)
33-44: LGTM: Clear, human-friendly Display for ResourceType.
33-44: Ohayo sensei — Name polish: consider spacing "External Contract".Minor UX nit: Display currently emits "ExternalContract"; changing to "External Contract" is cosmetic but repo-wide references to 'ExternalContract' exist (e.g., bin/sozo/src/commands/inspect.rs and many files under crates/sozo and crates/dojo/world) — keep the token if UIs/parsers depend on it or update all consumers.
examples/spawn-and-move/manifest_dev.json (1)
3-4: ohayo sensei — world class_hash/address verified against dojo_dev.tomlmanifest_dev.json (.world.class_hash, .world.address) are valid hex and .world.address equals examples/spawn-and-move/dojo_dev.toml world_address (script output: OK: world fields look valid).
crates/sozo/ui/Cargo.toml (1)
1-7: Ohayo, sensei — manifest looks clean and workspace-aligned.
Inherits edition/version correctly and scopes deps via workspace. No blockers.crates/sozo/ops/Cargo.toml (2)
19-23: Ohayo, sensei — UI dependency threading looks consistent.
Adding sozo-ui and early placement of scarb-interop is fine.
8-29: ohayo sensei — Verify/removecoloredin crates/sozo/ops/Cargo.tomlSandbox ripgrep returned no matches in crates/sozo/ops; confirm whether SozoUi now centralizes output styling and remove
coloredfrom crates/sozo/ops if unused. Suggested local checks: rg -n --hidden --no-ignore --type=rust "colored::|.color(|.bold(|.red(|.green(|.yellow(|.blue(" crates/ && rg -n "\bcolored\b" Cargo.lockCargo.toml (2)
22-23: Ohayo, sensei — workspace wiring for sozo-ui is correct.
Member and workspace dependency entries look good.Also applies to: 84-85
170-173: Drop scarb-ui if fully migratedohayo sensei — automated verification failed in the sandbox; confirm no references remain and remove scarb-ui from Cargo.toml to trim the build graph and avoid drift. Verify locally with: git grep -n -E 'scarb[-]ui|scarb-ui|scarb_ui' || rg --hidden --no-ignore -n -S 'scarb[-]ui|scarb-ui|scarb_ui' --glob '!.git/' --glob '!target/' --glob '!/node_modules/'
bin/sozo/src/utils.rs (3)
76-87: Ohayo, sensei — world address mismatch warning UX is solid.
Great use of ui.warn_block and 66-wide hex formatting.
165-192: Nice step/result flow for world diff and account verification.
Clear UX with subsection scoping; error includes 66‑wide address. Looks good.
112-126: Ohayo sensei — Avoid temporary Arc wrap/unwrap around providerHave provider_utils accept &impl Provider (or &dyn Provider) so you can pass a reference and remove the Arc/try_unwrap churn to avoid accidental clone leaks.
health_check_provider was not found by the earlier search; verify signature and callsites. Run from repo root: rg -nP 'health_check_provider\s*(' -C2 and inspect bin/sozo/src/utils.rs (lines ~112–126).
bin/sozo/Cargo.toml (1)
34-40: Ohayo, sensei — single sozo-ui entry confirmed.
Found one occurrence at bin/sozo/Cargo.toml:34.crates/sozo/walnut/Cargo.toml (1)
18-18: Ohayo, sensei — dependency swap LGTM.Switch to
sozo-uialigns with the new UI stack. No concerns here.xtask/generate-test-db/Cargo.toml (1)
19-19: Ohayo, sensei — don't remove sozo-ui from the xtask workspace.xtask/generate-test-db/src/main.rs imports SozoUi (use sozo_ui::SozoUi; at line 15) and calls .migrate(&SozoUi::default()) at line 85, so the workspace entry is required.
Likely an incorrect or invalid review comment.
xtask/generate-test-db/src/main.rs (2)
15-15: LGTM! UI migration looks good, sensei.The change from
MigrationUitoSozoUialigns perfectly with the PR-wide UI rework.
85-85: LGTM! Default UI usage is appropriate here.Ohayo! Using
SozoUi::default()for the test database generation is the right choice. The default constructor provides dark theme with standard verbosity, which is suitable for this automated task.crates/sozo/walnut/src/walnut.rs (2)
4-4: LGTM! Import change aligns with UI migration.The import update from scarb_ui to sozo_ui is consistent with the broader UI rework across the codebase.
26-26: LGTM! Function signature properly updated, sensei.The signature change to accept
ui: &SozoUiis consistent with other command modules in the PR.bin/sozo/src/commands/events.rs (3)
12-12: LGTM! Import addition follows the pattern.Ohayo! The
SozoUiimport is properly placed with other sozo-related imports.
60-60: LGTM! Method signature updated correctly.The addition of
ui: &SozoUiparameter matches the pattern used across all other command modules.
64-65: LGTM! UI parameter properly threaded through.The
uiparameter is correctly passed toutils::get_world_diff_and_provider, maintaining consistency with the new API.crates/sozo/walnut/src/verification.rs (2)
11-11: LGTM! Import updated for UI migration.The change from scarb_ui to sozo_ui import is consistent with the overall UI rework.
31-31: LGTM! Function signature properly updated, sensei.Ohayo! The
walnut_verifyfunction now correctly acceptsui: &SozoUiinstead of the old UI type, maintaining API consistency across the walnut module.bin/sozo/src/commands/inspect.rs (3)
10-10: LGTM! Import added correctly.The
SozoUiimport follows the established pattern for the UI migration.
37-37: LGTM! Function signature updated appropriately.The addition of
ui: &SozoUiparameter aligns with other command modules' signatures.
43-43: LGTM! UI parameter correctly propagated, sensei.Ohayo! The
uiparameter is properly passed through toutils::get_world_diff_and_provider, maintaining the new API contract.bin/sozo/src/commands/model.rs (3)
8-8: LGTM! Import follows the pattern.The
SozoUiimport is correctly placed with other sozo-related imports.
134-134: LGTM! Method signature properly updated.The addition of
ui: &SozoUiparameter is consistent with the UI migration pattern across all command modules.
145-145: LGTM! UI parameter consistently threaded through all subcommands.Ohayo sensei! Excellent work ensuring that the
uiparameter is passed toutils::get_world_diff_and_providerin all five subcommand branches (ClassHash, ContractAddress, Layout, Schema, and Get). This maintains API consistency throughout the model command implementation.Also applies to: 155-155, 171-171, 188-188, 206-206
crates/sozo/walnut/src/debugger.rs (2)
30-33: API migration to SozoUi looks goodSwitching the parameter to &SozoUi is consistent with the repo-wide UI refactor.
47-49: Staticverifysignature change is consistentAccepting
&SozoUihere matches how other commands pass UI through async paths.bin/sozo/src/commands/execute.rs (2)
66-90: Plumbing&SozoUithrough Execute.run is correctImports, signature, and passing
uiinto helpers all look consistent with the new UI model.
175-180: Walnut debug call is correctGuarded by the feature flag and respects Noop internally. 👍
bin/sozo/src/args.rs (2)
43-55: Verify verbosity mapping matches intentComment says “-v verbose, -vv debug, -vvv trace”. Ensure clap_verbosity_flag’s defaults map to the desired SozoVerbosity in practice (default likely maps to Warn). Adjust if default ends up too chatty.
Run a quick manual check:
- sozo (no -v): expect Normal or less chatty UI
- sozo -v: expect Verbose
- sozo -vv: expect Debug
- sozo -vvv: expect Trace
61-68: Logging threshold logic is soundUsing
as_trace() >= DEBUGto toggle detailed filters is clean.bin/sozo/src/main.rs (1)
26-35: Theme detection and fallback look goodLight/Dark mapping with a dark fallback is reasonable.
bin/sozo/src/commands/migrate.rs (2)
46-66: UI-first flow is well-threadedPassing
&SozoUi, usingui.subsection(), and removing spinner coupling is a solid improvement.
96-102: Great warning UX for missing IPFS credsClear guidance and link provided through
ui.warn_block.crates/sozo/ops/src/tests/migration.rs (2)
15-15: Ohayo, sensei — SozoUi import swap looks clean.The new UI abstraction is correctly referenced.
79-80: Ohayo, sensei — upload_metadata UI threading matches new signature.The test aligns with the ops API change.
bin/sozo/src/commands/auth.rs (3)
430-446: Ohayo, sensei — nice hierarchical output with subsection + headers.Indentation via permissions_ui improves readability.
497-564: print_diff_permissions uses SozoUi indentation correctly.Logic and formatting look good; no functional concerns.
141-156: Ohayo, sensei — threading &SozoUi through Auth::run is the right call; verification incomplete.rg produced no output, so I can't confirm other call sites compile — re-run these and paste the results:
rg -nP --hidden -S '\b(Auth|AuthArgs)\s*::\s*run\s*\(' -C2 rg -nP --hidden -S 'SozoUi' -C2 rg -nP --hidden -S 'list_permissions\([^)]*SozoUi' -n rg -nP --hidden -S 'update_(writers|owners)\([^)]*SozoUi' -nbin/sozo/src/commands/mod.rs (2)
6-6: Ohayo, sensei — SozoUi import integration LGTM.
106-134: Ohayo, sensei — passing ui to subcommands is consistent across the board.Main dispatcher changes read well.
crates/sozo/ops/src/migrate/mod.rs (2)
94-101: Ohayo, sensei — top-level Migrate sectioning is clear.Title + subsection pattern improves UX traceability.
1096-1154: ohayo sensei — Incorrect: TransactionResult::Receipt doesn't exist; keep handling HashReceipt.TransactionResult is defined as Noop / Hash / HashReceipt (crates/dojo/utils/src/tx/mod.rs) and invoker/deployer return HashReceipt, so the match in crates/sozo/ops/src/migrate/mod.rs:1131 is correct; only add a new enum variant if you mean to support a different result shape.
Likely an incorrect or invalid review comment.
| ui.print(format!( | ||
| "[ {} ]", | ||
| output.iter().map(|o| format!("0x{:x}", o)).collect::<Vec<_>>().join(" ") | ||
| ); | ||
| output.iter().map(|o| format!("0x{:#066x}", o)).collect::<Vec<_>>().join(" "), | ||
| )); |
There was a problem hiding this comment.
Double “0x” prefix bug in output formatting
format!("0x{:#066x}", o) produces 0x0x…. Remove the extra literal.
- ui.print(format!(
- "[ {} ]",
- output.iter().map(|o| format!("0x{:#066x}", o)).collect::<Vec<_>>().join(" "),
- ));
+ ui.print(format!(
+ "[ {} ]",
+ output.iter().map(|o| format!("{:#066x}", o)).collect::<Vec<_>>().join(" "),
+ ));📝 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.
| ui.print(format!( | |
| "[ {} ]", | |
| output.iter().map(|o| format!("0x{:x}", o)).collect::<Vec<_>>().join(" ") | |
| ); | |
| output.iter().map(|o| format!("0x{:#066x}", o)).collect::<Vec<_>>().join(" "), | |
| )); | |
| ui.print(format!( | |
| "[ {} ]", | |
| output.iter().map(|o| format!("{:#066x}", o)).collect::<Vec<_>>().join(" "), | |
| )); |
🤖 Prompt for AI Agents
In bin/sozo/src/commands/call.rs around lines 131 to 134, the output formatting
uses format!("0x{:#066x}", o) which produces a double "0x" prefix; remove the
extra literal "0x" and rely on the "{:#066x}" specifier (i.e., replace
format!("0x{:#066x}", o) with format!("{:#066x}", o)) so each value is prefixed
once and retains the intended zero-padding.
| let address = format!("{:#066x}", inner.contract_address); | ||
| let selector = format!("0x{:#066x}", inner.selector); | ||
| let inner_error = format_execution_error(&inner.error); | ||
| format!("Error in contract at {address} when calling {selector}:\n {inner_error}",) | ||
| } |
There was a problem hiding this comment.
Double “0x” prefix bug in error selector formatting
Same issue when formatting the selector.
- let address = format!("{:#066x}", inner.contract_address);
- let selector = format!("0x{:#066x}", inner.selector);
+ let address = format!("{:#066x}", inner.contract_address);
+ let selector = format!("{:#066x}", inner.selector);📝 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 address = format!("{:#066x}", inner.contract_address); | |
| let selector = format!("0x{:#066x}", inner.selector); | |
| let inner_error = format_execution_error(&inner.error); | |
| format!("Error in contract at {address} when calling {selector}:\n {inner_error}",) | |
| } | |
| let address = format!("{:#066x}", inner.contract_address); | |
| let selector = format!("{:#066x}", inner.selector); | |
| let inner_error = format_execution_error(&inner.error); | |
| format!("Error in contract at {address} when calling {selector}:\n {inner_error}",) | |
| } |
🤖 Prompt for AI Agents
In bin/sozo/src/commands/call.rs around lines 159 to 163, the selector
formatting adds a duplicate "0x" prefix because the format specifier {:#066x}
already includes "0x"; remove the extra literal "0x" or change the specifier to
{:066x} and prepend a single "0x" so the selector prints with exactly one "0x"
prefix (do the same consistent change if there are other selector/address format
occurrences).
| if let Some(http_headers) = env.http_headers.as_ref() { | ||
| local_ui.debug(local_ui.indent(1, "http_headers:")); | ||
| for header in http_headers { | ||
| local_ui.debug( | ||
| local_ui.indent(2, format!("name: {}, value: {}", header.name, header.value)), | ||
| ); | ||
| } | ||
| } else { | ||
| local_ui.debug(local_ui.indent(1, "http_headers: None")); | ||
| } |
There was a problem hiding this comment.
Don’t print raw HTTP header values (possible secrets).
Mask header values before output to avoid leaking tokens/keys, even at verbose/debug levels.
Apply this diff:
- for header in http_headers {
- local_ui.debug(
- local_ui.indent(2, format!("name: {}, value: {}", header.name, header.value)),
- );
- }
+ for header in http_headers {
+ local_ui.debug(local_ui.indent(
+ 2,
+ format!("name: {}, value: {}", header.name, mask_secret(&header.value)),
+ ));
+ }Add this helper somewhere in this module:
fn mask_secret(value: &str) -> String {
let v = value.trim();
if v.is_empty() { return "".into(); }
// Preserve scheme (e.g., "Bearer", "Basic") if present, mask the rest.
if let Some(space) = v.find(' ') {
let (scheme, _) = v.split_at(space);
return format!("{} {}", scheme, "********");
}
// Generic tail-preserving mask.
let n = v.chars().count();
let keep = n.min(4);
let tail: String = v.chars().skip(n - keep).collect();
format!("{}{}", "*".repeat(n.saturating_sub(keep)), tail)
}🤖 Prompt for AI Agents
In bin/sozo/src/utils.rs around lines 356 to 365, the debug output prints raw
HTTP header values which may leak secrets; add the provided mask_secret helper
function into this module and update the debug loop to call
mask_secret(&header.value) (preserving scheme handling and empty-string
behavior) when formatting the value for output so that header values are masked
before logging.
| None => return Ok((Felt::ZERO, TransactionResult::Noop)), | ||
| }; |
There was a problem hiding this comment.
Don’t return Felt::ZERO as a sentinel; return the actual address.
Leaking a magic value invites bugs; you can compute the address here.
- None => return Ok((Felt::ZERO, TransactionResult::Noop)),
+ None => {
+ let contract_address =
+ get_contract_address(salt, class_hash, constructor_calldata, deployer_address);
+ return Ok((contract_address, TransactionResult::Noop));
+ }📝 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.
| None => return Ok((Felt::ZERO, TransactionResult::Noop)), | |
| }; | |
| None => { | |
| let contract_address = | |
| get_contract_address(salt, class_hash, constructor_calldata, deployer_address); | |
| return Ok((contract_address, TransactionResult::Noop)); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In crates/dojo/utils/src/tx/deployer.rs around lines 78-79, replace the sentinel
return of Felt::ZERO with the actual deployed contract address: compute the
address using the same deterministic formula used elsewhere in the codebase
(e.g. the helper that takes class_hash, constructor_calldata and salt / deployer
address — typically named something like compute_contract_address or
calculate_contract_address), return that Felt instead of ZERO, and add any
needed imports and error handling so the function returns Ok((computed_address,
TransactionResult::Noop)).
| pub fn status(&self) -> String { | ||
| let res = match self { | ||
| ResourceDiff::Created(_) => "Created", | ||
| ResourceDiff::Updated(_, _) => "Updated", | ||
| ResourceDiff::Synced(_, _) => "Synced", | ||
| }; | ||
|
|
||
| res.to_string() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Avoid allocation in status(); return a string slice.
No need to allocate a new String each call. Return &'static str directly.
- pub fn status(&self) -> String {
- let res = match self {
- ResourceDiff::Created(_) => "Created",
- ResourceDiff::Updated(_, _) => "Updated",
- ResourceDiff::Synced(_, _) => "Synced",
- };
-
- res.to_string()
- }
+ pub fn status(&self) -> &'static str {
+ match self {
+ ResourceDiff::Created(_) => "Created",
+ ResourceDiff::Updated(_, _) => "Updated",
+ ResourceDiff::Synced(_, _) => "Synced",
+ }
+ }If any call sites rely on an owned String, they can do .to_string() at the edge. Please grep usages to confirm:
🏁 Script executed:
#!/bin/bash
rg -nP --type=rust '\.status\(\)'Length of output: 328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pairs=(
"xtask/generate-test-db/src/main.rs:124"
"bin/sozo/src/utils.rs:404"
"crates/sozo/walnut/src/verification.rs:63"
"crates/dojo/world/abigen/src/main.rs:94"
"crates/dojo/world/abigen/src/main.rs:111"
)
for p in "${pairs[@]}"; do
file="${p%%:*}"
line="${p##*:}"
start=$(( line>6 ? line-6 : 1 ))
end=$(( line+6 ))
echo "== $file lines $start-$end =="
sed -n "${start},${end}p" "$file" || true
echo
doneLength of output: 2369
Ohayo sensei: Update call site for status() refactor
Change status: resource.status(), to status: resource.status().to_string() in bin/sozo/src/utils.rs:404—other .status() calls (Command::status(), HTTP Response.status()) remain unaffected.
🤖 Prompt for AI Agents
In crates/dojo/world/src/diff/resource.rs lines 133-141 the
ResourceDiff::status() method now returns a String; update the single call site
in bin/sozo/src/utils.rs at around line 404 by replacing status:
resource.status(), with status: resource.status().to_string() so the produced
value is owned String (leave other .status() calls untouched).
| if self.do_multicall() { | ||
| ui.update_text_boxed(format!("Uploading {} metadata...", invoker.calls.len())); | ||
| ui.step(format!("Upload {} metadata", invoker.calls.len())); | ||
| invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?; | ||
| } else { | ||
| ui.update_text_boxed(format!( | ||
| "Uploading {} metadata (sequentially)...", | ||
| invoker.calls.len() | ||
| )); | ||
| ui.step(format!("Upload {} metadata (sequentially)", invoker.calls.len())); | ||
| invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?; | ||
| } | ||
|
|
||
| ui.result("Metadata uploaded."); | ||
|
|
There was a problem hiding this comment.
Guard empty metadata calls before multicall.
If invoker.calls is empty, multicall/invoke_all_sequentially may be a no‑op or error depending on Invoker impl. Safer to bail early with a result message.
Apply:
- if self.do_multicall() {
- ui.step(format!("Upload {} metadata", invoker.calls.len()));
- invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?;
- } else {
- ui.step(format!("Upload {} metadata (sequentially)", invoker.calls.len()));
- invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?;
- }
+ if invoker.calls.is_empty() {
+ ui.result("No metadata changes detected.");
+ } else if self.do_multicall() {
+ ui.step(format!("Upload {} metadata", invoker.calls.len()));
+ invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?;
+ } else {
+ ui.step(format!("Upload {} metadata (sequentially)", invoker.calls.len()));
+ invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/sozo/ops/src/migrate/mod.rs around lines 174 to 183, guard against
empty invoker.calls before attempting multicall/invoke_all_sequentially: check
if invoker.calls.is_empty(), and if so call ui.result("Metadata uploaded.") and
return early (Ok(())) to avoid invoking a no-op or triggering
implementation-specific errors; otherwise proceed with the existing
do_multicall() branch and calls as before.
| if self.do_multicall() { | ||
| let ui_text = format!("Registering {} external contracts...", n_external_contracts); | ||
| ui.update_text_boxed(ui_text); | ||
| ui.step(format!("Register {} external contracts", n_external_contracts)); | ||
| invoker.multicall().await?; | ||
| } else { | ||
| let ui_text = format!( | ||
| "Registering {} external contracts (sequentially)...", | ||
| n_external_contracts | ||
| ); | ||
| ui.update_text_boxed(ui_text); | ||
| ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts)); | ||
| invoker.invoke_all_sequentially().await?; | ||
| } | ||
|
|
||
| if !not_upgradeable_contract_names.is_empty() { | ||
| let msg = format!( | ||
| ui.warn_block(format!( | ||
| "The following external contracts are NOT upgradeable as they don't export an \ | ||
| `upgrade(ClassHash)` function:\n{}", | ||
| not_upgradeable_contract_names.join("\n") | ||
| ); | ||
| println!(); | ||
| println!("{}", msg.as_str().bright_yellow()); | ||
| println!(); | ||
| )); | ||
| } | ||
|
|
||
| ui.result("External contracts registered."); | ||
|
|
There was a problem hiding this comment.
Also guard external‑contract registration on empty call list.
Same reasoning; avoids no‑op multicalls.
Apply:
- if self.do_multicall() {
- ui.step(format!("Register {} external contracts", n_external_contracts));
- invoker.multicall().await?;
- } else {
- ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts));
- invoker.invoke_all_sequentially().await?;
- }
+ if n_external_contracts == 0 {
+ ui.result("No external contracts to register.");
+ } else if self.do_multicall() {
+ ui.step(format!("Register {} external contracts", n_external_contracts));
+ invoker.multicall().await?;
+ } else {
+ ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts));
+ invoker.invoke_all_sequentially().await?;
+ }📝 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.
| if self.do_multicall() { | |
| let ui_text = format!("Registering {} external contracts...", n_external_contracts); | |
| ui.update_text_boxed(ui_text); | |
| ui.step(format!("Register {} external contracts", n_external_contracts)); | |
| invoker.multicall().await?; | |
| } else { | |
| let ui_text = format!( | |
| "Registering {} external contracts (sequentially)...", | |
| n_external_contracts | |
| ); | |
| ui.update_text_boxed(ui_text); | |
| ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts)); | |
| invoker.invoke_all_sequentially().await?; | |
| } | |
| if !not_upgradeable_contract_names.is_empty() { | |
| let msg = format!( | |
| ui.warn_block(format!( | |
| "The following external contracts are NOT upgradeable as they don't export an \ | |
| `upgrade(ClassHash)` function:\n{}", | |
| not_upgradeable_contract_names.join("\n") | |
| ); | |
| println!(); | |
| println!("{}", msg.as_str().bright_yellow()); | |
| println!(); | |
| )); | |
| } | |
| ui.result("External contracts registered."); | |
| if n_external_contracts == 0 { | |
| ui.result("No external contracts to register."); | |
| } else if self.do_multicall() { | |
| ui.step(format!("Register {} external contracts", n_external_contracts)); | |
| invoker.multicall().await?; | |
| } else { | |
| ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts)); | |
| invoker.invoke_all_sequentially().await?; | |
| } | |
| if !not_upgradeable_contract_names.is_empty() { | |
| ui.warn_block(format!( | |
| "The following external contracts are NOT upgradeable as they don't export an \ | |
| `upgrade(ClassHash)` function:\n{}", | |
| not_upgradeable_contract_names.join("\n") | |
| )); | |
| } | |
| ui.result("External contracts registered."); |
🤖 Prompt for AI Agents
In crates/sozo/ops/src/migrate/mod.rs around lines 625 to 642, the code always
attempts to register external contracts even when the call list is empty; wrap
the registration block in a guard that checks the call count (e.g.,
n_external_contracts > 0 or invoker.has_calls()) and only run ui.step and
invoker.multicall()/invoke_all_sequentially() when there are calls to perform;
if there are zero calls, skip the multicall/serial invocation and emit an
appropriate ui message/result or silently skip so you avoid performing no-op
multicalls.
| /// Prints a warning without taking the indentation into account. | ||
| pub fn warn<T: Message>(&self, text: T) { | ||
| self.do_print(text, SozoVerbosity::Quiet, self.theme.warning_color, false); | ||
| } | ||
|
|
||
| /// Prints an error without taking the indentation into account. | ||
| pub fn error<T: Message>(&self, text: T) { | ||
| self.do_print( | ||
| format!("error: {}", text.text().trim()), | ||
| SozoVerbosity::Quiet, | ||
| self.theme.error_color, | ||
| false, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei — route warnings/errors to stderr (don’t pollute stdout).
Currently warn, error, warn_block, and error_block write to stdout via println!, which breaks piping (e.g., sozo ... | jq) and conventional CLI UX. Errors and warnings should go to stderr.
Patch (minimal change, no internal API churn):
pub fn warn<T: Message>(&self, text: T) {
- self.do_print(text, SozoVerbosity::Quiet, self.theme.warning_color, false);
+ if self.verbosity >= SozoVerbosity::Quiet {
+ eprintln!("{}", text.text().color(self.theme.warning_color));
+ }
}
pub fn error<T: Message>(&self, text: T) {
- self.do_print(
- format!("error: {}", text.text().trim()),
- SozoVerbosity::Quiet,
- self.theme.error_color,
- false,
- );
+ if self.verbosity >= SozoVerbosity::Quiet {
+ eprintln!("{}", format!("error: {}", text.text().trim()).color(self.theme.error_color));
+ }
}And for blocks:
pub fn warn_block<T: Message>(&self, block: T) {
- self.new_line();
- self.do_block(block, SozoVerbosity::Normal, self.theme.warning_color, false);
- self.new_line();
+ if self.verbosity > SozoVerbosity::Quiet {
+ eprintln!();
+ for line in block.text().lines() {
+ eprintln!("{}", line.color(self.theme.warning_color));
+ }
+ eprintln!();
+ }
}
pub fn error_block<T: Message>(&self, block: T) {
- self.new_line();
- self.do_block(block, SozoVerbosity::Normal, self.theme.error_color, false);
- self.new_line();
+ if self.verbosity > SozoVerbosity::Quiet {
+ eprintln!();
+ for line in block.text().lines() {
+ eprintln!("{}", line.color(self.theme.error_color));
+ }
+ eprintln!();
+ }
}Also applies to: 182-194
🤖 Prompt for AI Agents
In crates/sozo/ui/src/lib.rs around lines 150-163 (and similarly 182-194), the
warn/error and their _block counterparts currently print to stdout; change these
to write to stderr so warnings/errors don't pollute stdout and break piping.
Replace the println!/print usage with stderr output (e.g., eprintln! or writing
to std::io::stderr()/writeln!) in those methods, keeping the same message
formatting, verbosity, color/theme handling, and boolean flags so no public API
changes are introduced.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
bin/sozo/tests/test_data/policies.json (3)
203-205: Normalize method naming style (camelCase vs snake_case).All methods are snake_case except Line 204 "deployContract". If consumers expect snake_case, this is an inconsistency; if ABI expects camelCase, then the rest may be inconsistent. Please confirm the intended convention and align.
If snake_case is intended, apply:
- "method": "deployContract" + "method": "deploy_contract"
206-206: Add a trailing newline at EOF.Tiny formatting nit for POSIX tools and diffs.
Apply:
-] +] +
1-206: Optional: generate this fixture programmatically from manifest/ABI.To avoid drift, consider a tiny generator that pulls method names and target addresses from the current dev manifest and emits this JSON.
I can scaffold a small Rust/JS script to emit policies from the manifest if you want.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
bin/sozo/tests/test_data/policies.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-28T23:35:04.367Z
Learnt from: glihm
PR: dojoengine/dojo#2691
File: bin/sozo/tests/test_data/policies.json:7-32
Timestamp: 2024-11-28T23:35:04.367Z
Learning: The file `bin/sozo/tests/test_data/policies.json` is a test file containing policies used by sessions to determine which methods can be called for which contract addresses. There is not much to verify or check in this file.
Applied to files:
bin/sozo/tests/test_data/policies.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: clippy
🔇 Additional comments (2)
bin/sozo/tests/test_data/policies.json (2)
1-206: ohayo sensei — test policy list updates look fine overallSyntactically valid JSON and structure matches prior schema expectations for this fixture.
1-206: ohayo sensei — sanity checks for bin/sozo/tests/test_data/policies.json failed to run here; run these locally
Sandbox produced bash errors so verification couldn't complete. Run the checks below and fix any DUP/BAD/MISSING results.jq -r '.[] | "(.target)|(.method)"' bin/sozo/tests/test_data/policies.json | sort | uniq -c | awk '$1>1{print "DUP", $0}'
jq -r '.[].target' bin/sozo/tests/test_data/policies.json | sort -u | grep -v -E '^0x[0-9a-f]{64}$' || echo "OK: addresses look valid"
for m in $(jq -r '.[].method' bin/sozo/tests/test_data/policies.json | sort -u); do
rg -n -S -g '!/target/' -g '!/node_modules/' -g '!/dist/' -e "\b$m\b" . || echo "MISSING: $m"
donefor a in $(jq -r '.[].target' bin/sozo/tests/test_data/policies.json | sort -u); do
rg -n -S -g '!/target/' -g '!/node_modules/' -g '!/dist/' -F "$a" . || echo "ADDR_NOT_FOUND: $a"
doneFix duplicates, invalid addresses, or add missing ABI/manifests if any checks report issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
crates/sozo/ops/src/migrate/mod.rs (2)
174-180: Guard empty metadata calls before multicall.When
invoker.callsis empty, attempting multicall or sequential invocation might result in unnecessary operations or implementation-specific errors.Apply this diff to handle empty calls gracefully:
+ if invoker.calls.is_empty() { + ui.result("No metadata changes detected."); + return Ok(()); + } + if self.do_multicall() { ui.step(format!("Upload {} metadata", invoker.calls.len())); invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?; } else { ui.step(format!("Upload {} metadata (sequentially)", invoker.calls.len())); invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?; }
691-697: Also guard external contract registration on empty call list.Similar to the metadata upload, we should check if there are actually external contracts to register before attempting multicall/sequential invocation.
Apply this diff:
+ if n_external_contracts == 0 { + return Ok(has_changed); + } + if self.do_multicall() { ui.step(format!("Register {} external contracts", n_external_contracts)); invoker.multicall().await?; } else { ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts)); invoker.invoke_all_sequentially().await?; }bin/sozo/src/utils.rs (1)
358-362: Don't print raw HTTP header values (possible secrets).HTTP headers may contain sensitive authentication tokens or API keys that shouldn't be logged even at debug/verbose levels.
Apply this diff to mask sensitive header values:
for header in http_headers { local_ui.debug( - local_ui.indent(2, format!("name: {}, value: {}", header.name, header.value)), + local_ui.indent(2, format!("name: {}, value: {}", header.name, mask_secret(&header.value))), ); }Add this helper function to the module:
fn mask_secret(value: &str) -> String { let v = value.trim(); if v.is_empty() { return "".into(); } // Preserve scheme (e.g., "Bearer", "Basic") if present, mask the rest. if let Some(space) = v.find(' ') { let (scheme, _) = v.split_at(space); return format!("{} {}", scheme, "********"); } // Generic tail-preserving mask. let n = v.chars().count(); let keep = n.min(4); let tail: String = v.chars().skip(n - keep).collect(); format!("{}{}", "*".repeat(n.saturating_sub(keep)), tail) }
🧹 Nitpick comments (4)
crates/dojo/utils/src/provider.rs (3)
20-22: Ohayo, sensei — keep the friendly message but retain root-cause for debuggingGood privacy win removing provider Debug from user output. However, dropping the source error makes troubleshooting harder. Capture and trace-log the original error while keeping the sanitized message.
Apply this diff:
- Err(_) => Err(anyhow::anyhow!( - "Unhealthy RPC provider. Please check your configuration and that the node is running." - )), + Err(e) => { + trace!(error = ?e, "Provider health check failed"); + Err(anyhow::Error::new(e).context( + "Unhealthy RPC provider. Please check your configuration and that the node is running.", + )) + },Add this import at the top of the file:
use anyhow::Context;Please confirm the UI shows only the top-level message by default and reveals the chain under verbose/debug.
9-11: Drop unused Debug bound on P
std::fmt::Debugis no longer needed after removing provider details from the error. Loosen constraints.-pub async fn health_check_provider<P: Provider + Sync + std::fmt::Debug + 'static>( +pub async fn health_check_provider<P: Provider + Sync + 'static>(
11-11: Use anyhow::Result<()> shorthandMinor ergonomics/readability tweak.
-) -> anyhow::Result<(), anyhow::Error> { +) -> anyhow::Result<()> {bin/sozo/src/utils.rs (1)
53-63: Consider documenting theResourceDetailsstruct fields.While this internal struct is straightforward, adding brief doc comments for each field would improve maintainability, especially for the
selectorfield which stores a hex-formatted value.+/// Internal struct for displaying resource details in table format #[derive(Tabled)] struct ResourceDetails { + /// Type of the resource (Contract, Model, Event, etc.) #[tabled(rename = "Resource Type")] resource_type: ResourceType, + /// Human-readable tag/identifier for the resource #[tabled(rename = "Tag")] tag: String, + /// Hex-formatted selector (0x prefixed) #[tabled(rename = "Selector")] selector: String, + /// Current sync status (Created, Updated, Synced) #[tabled(rename = "Status")] status: String, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bin/sozo/src/utils.rs(5 hunks)crates/dojo/utils/src/provider.rs(1 hunks)crates/dojo/utils/src/tx/mod.rs(1 hunks)crates/sozo/ops/src/migrate/mod.rs(28 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/sozo/ops/src/migrate/mod.rs (5)
crates/sozo/ui/src/lib.rs (2)
new(94-96)trace(173-175)crates/dojo/utils/src/tx/deployer.rs (1)
new(34-36)crates/dojo/utils/src/tx/declarer.rs (2)
new(52-54)declare(86-147)crates/dojo/utils/src/tx/waiter.rs (1)
new(100-115)crates/dojo/utils/src/tx/invoker.rs (1)
new(28-30)
bin/sozo/src/utils.rs (5)
crates/dojo/world/src/local/mod.rs (2)
deterministic_world_address(100-103)new(83-97)crates/dojo/utils/src/tx/deployer.rs (2)
is_deployed(107-122)new(34-36)bin/sozo/src/commands/options/world.rs (1)
address(33-44)crates/dojo/world/src/diff/resource.rs (1)
status(133-141)crates/sozo/ui/src/lib.rs (1)
new(94-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (8)
crates/dojo/utils/src/tx/mod.rs (1)
62-62: LGTM! Clone trait addition enables tuple return patterns.Ohayo sensei! The addition of
Cloneto theTransactionResultenum's derive attributes is well-aligned with the cross-file changes mentioned in the AI summary. This enables thedeploy_via_udcfunction to return a tuple(Felt, TransactionResult)while allowing theTransactionResultto be cloned when needed.bin/sozo/src/utils.rs (3)
78-86: Well-structured warning message, ohayo sensei!The warning message provides clear guidance about the address mismatch and actionable next steps. The 66-width hex formatting ensures consistent address display.
398-408: Clean resource table presentation!The use of
Table::newwithStyle::psql()provides a well-formatted output for resource details. Good choice for readability.
427-430: Good separation of concerns in world details display.The function nicely delegates to specialized display functions for profile and world diff details, maintaining clean code organization.
crates/sozo/ops/src/migrate/mod.rs (4)
258-295: Excellent contract initialization tracking, sensei!The
init_detailsHashMap effectively captures initialization arguments for verbose logging, providing clear visibility into contract initialization parameters.
1185-1200: Ohayo! Great error handling for world deployment.The code properly declares the world class before deployment and captures the transaction result with receipt. The error messaging clearly guides users when world address mismatches occur.
1287-1350: Clean UI helper functions for resource display!The
display_resources_to_syncfunction provides well-organized verbose output with proper indentation for different resource categories. The consistent structure makes it easy to understand what resources are being synced.
1361-1380: Well-structured permission display, sensei!The
display_permissionsfunction clearly separates writer and owner permissions with proper indentation, making it easy to understand the permission changes being applied.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/sozo/ops/src/migrate/mod.rs (2)
631-671: Fix deploy block-number capture in multicall path (can record wrong block).Currently you take the first tx’s block for all deploys; deploy calls are appended last, and multicall may split into multiple txs. This can assign an incorrect block to some deployments and break registration.
Apply:
- if self.do_multicall() { - ui.debug("Deploying resources with multicall"); - - invoker.extend_calls(deploy_calls.values().cloned().collect()); - - let txs_results = invoker.multicall().await?; - - display_transaction_results(&ui, &txs_results, "Register"); - - // If some external contracts have been deployed, we need to - // get the block number of the multicall tx. - // Since the multicall may be split into multiple transactions, we take the block number - // of the first transaction. - if !deploy_calls.is_empty() { - // TODO: @remybar, wondering if here we should - // also handle the case when it contains the receipt - // already, due to the tx configuration. - if let TransactionResult::Hash(tx_hash) = txs_results[0] { - let receipt = - TransactionWaiter::new(tx_hash, &self.world.account.provider()).await?; - let block_number = receipt.block.block_number(); - - deploy_block_numbers = - deploy_calls.keys().map(|name| (name.clone(), block_number)).collect(); - } - } - } else { + if self.do_multicall() { + ui.debug("Register non-deploy resources with multicall"); + + // Run non-deploy calls via multicall first. + let txs_results = invoker.multicall().await?; + display_transaction_results(&ui, &txs_results, "Register"); + + // Deploy external contracts sequentially to capture accurate block numbers. + for (name, call) in deploy_calls { + let tx = invoker.invoke(call).await?; + display_transaction_results(&ui, &vec![tx.clone()], "Deploy external"); + if let TransactionResult::Hash(tx_hash) = tx { + let receipt = + TransactionWaiter::new(tx_hash, &self.world.account.provider()).await?; + let block_number = receipt.block.block_number(); + deploy_block_numbers.insert(name, block_number); + } + } + } else { let txs_results = invoker.invoke_all_sequentially().await?; display_transaction_results(&ui, &txs_results, "Register");
1009-1016: Avoid panics: replace unwrap() with proper error propagation.ByteArray::from_string can fail; don’t panic inside ops library.
Apply:
- let name = ByteArray::from_string(&library.common.name).unwrap(); - let version = ByteArray::from_string(&library.version).unwrap(); + let name = ByteArray::from_string(&library.common.name)?; + let version = ByteArray::from_string(&library.version)?;
♻️ Duplicate comments (3)
crates/sozo/ops/src/migrate/mod.rs (3)
174-183: Guard empty metadata callset before invoking multicall/sequential.Avoid calling invoker when there are 0 calls; emit a friendly result and return early. This was flagged earlier; still applicable.
Apply:
- if self.do_multicall() { - ui.step(format!("Upload {} metadata", invoker.calls.len())); - invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?; - } else { - ui.step(format!("Upload {} metadata (sequentially)", invoker.calls.len())); - invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?; - } - - ui.result("Metadata uploaded."); + if invoker.calls.is_empty() { + ui.result("No metadata changes detected."); + return Ok(()); + } else if self.do_multicall() { + ui.step(format!("Upload {} metadata", invoker.calls.len())); + invoker.multicall().await.map_err(|e| anyhow!(e.to_string()))?; + } else { + ui.step(format!("Upload {} metadata (sequentially)", invoker.calls.len())); + invoker.invoke_all_sequentially().await.map_err(|e| anyhow!(e.to_string()))?; + } + + ui.result("Metadata uploaded.");
693-702: Guard external‑contract registration on empty call list.Skip multicall/sequential when n_external_contracts == 0; show a “no‑op” result. Previously flagged; still needed.
Apply:
- if self.do_multicall() { - ui.step(format!("Register {} external contracts", n_external_contracts)); - invoker.multicall().await?; - } else { - ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts)); - invoker.invoke_all_sequentially().await?; - } + if n_external_contracts == 0 { + ui.result("No external contracts to register."); + } else if self.do_multicall() { + ui.step(format!("Register {} external contracts", n_external_contracts)); + invoker.multicall().await?; + } else { + ui.step(format!("Register {} external contracts (sequentially)", n_external_contracts)); + invoker.invoke_all_sequentially().await?; + } - if n_external_contracts > 0 { + if n_external_contracts > 0 { ui.result("External contracts registered."); }Also applies to: 711-713
410-426: Short‑circuit permissions sync when there are no changes.Avoid invoking invoker with zero calls; print a concise message and return false.
Apply:
- ui.step(format!("Sync {} permissions", invoker.calls.len())); - display_permissions(&ui, &writers_perms, &owners_perms); - - let has_changed = !invoker.calls.is_empty(); + ui.step(format!("Sync {} permissions", invoker.calls.len())); + display_permissions(&ui, &writers_perms, &owners_perms); + + let has_changed = !invoker.calls.is_empty(); + if !has_changed { + ui.result("No permission changes detected."); + return Ok(false); + } - if self.do_multicall() { + if self.do_multicall() { let txs_results = invoker.multicall().await?; display_transaction_results(&ui, &txs_results, "Sync permissions"); } else {
🧹 Nitpick comments (3)
crates/sozo/ops/src/migrate/mod.rs (3)
1024-1025: Don’t panic on unsupported library updates; return a typed error.Replace panic!("libraries cannot be updated!") with a MigrationError variant (e.g., UnsupportedOperation/InvalidUpdate) so callers can handle it gracefully.
I can wire a concrete error variant in error::MigrationError and update call sites if you want.
441-447: Early exit when there are no classes to declare.Skip setting up declarers and printing steps when n_classes == 0.
Apply:
let n_classes = classes.len(); - let ui_text = format!("Declare {} classes", n_classes); - ui.step(ui_text); + if n_classes == 0 { + ui.result("No classes to declare."); + return Ok(()); + } + + ui.step(format!("Declare {} classes", n_classes));
1276-1289: Accept slices, not &Vec, in display_transaction_results.More idiomatic and flexible API; existing call sites continue to work.
Apply:
-fn display_transaction_results(ui: &SozoUi, txs_results: &Vec<TransactionResult>, action: &str) { +fn display_transaction_results(ui: &SozoUi, txs_results: &[TransactionResult], action: &str) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/sozo/ops/src/migrate/mod.rs(28 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/sozo/ops/src/migrate/mod.rs (4)
crates/sozo/ui/src/lib.rs (2)
new(94-96)trace(173-175)crates/dojo/utils/src/tx/deployer.rs (1)
new(34-36)crates/dojo/utils/src/tx/declarer.rs (2)
new(52-54)declare(86-147)crates/dojo/utils/src/tx/invoker.rs (1)
new(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: clippy
🔇 Additional comments (2)
crates/sozo/ops/src/migrate/mod.rs (2)
500-503: Nice touch: result only when there’s work.Emitting “Classes declared.” only when n_classes > 0 is clean.
1169-1256: World deploy/upgrade UI and checks look solid.Good UX flow, receipt gating, and address display. The mismatch error message is explicit.
2 participants
Description
Improve the way sozo inputs and outputs data to the user.
Related issues
#3324
Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Improvements
Chores