Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sozo): fix profile detection and UI rework #2606

Merged
merged 3 commits into from
Nov 1, 2024

Conversation

glihm
Copy link
Collaborator

@glihm glihm commented Nov 1, 2024

Description

Reworked the UI to be more usable, and aware of the profile. Which was causing the RPC URL not being taken correctly from the profile to display the banner.

The MigrationUi has also be reworked with a bit more information about the migration and ensures the keystone prompt is not hidden by the spinner.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a user interface for migration processes, enhancing user feedback with a spinner during execution.
    • Added a new configuration profile for the Sepolia environment.
  • Improvements

    • Enhanced error handling and logging for migration operations.
    • Updated user prompts for better clarity during password entry.
  • New Modules

    • Added a migration_ui module to manage the spinner UI for migration tasks.
  • Configuration Changes

    • Added a new TOML configuration file for a simple world setup in a blockchain environment.
    • Introduced a structured manifest for the blockchain project detailing architecture and interactions.

@glihm glihm marked this pull request as ready for review November 1, 2024 17:17
Copy link

coderabbitai bot commented Nov 1, 2024

Walkthrough

Ohayo, sensei! This pull request introduces significant enhancements to the migration and execution processes within the codebase. Key changes include the integration of a new MigrationUi component to improve user feedback during migration operations and command executions. The run methods in both ExecuteArgs and MigrateArgs have been updated to utilize this UI for better interaction. Additionally, various utility functions have been modified to accommodate the new UI, enhancing error handling and logging throughout the migration processes.

Changes

File Path Change Summary
bin/sozo/src/commands/execute.rs Added MigrationUi dependency; modified run method to include a spinner for user feedback during command execution.
bin/sozo/src/commands/migrate.rs Updated import statements for MigrationUi; simplified spinner initialization; enhanced error handling; modified run method and print_banner for improved configuration loading.
bin/sozo/src/commands/options/account/mod.rs Added trace log for chain ID in std_account method; removed previous signer log.
bin/sozo/src/utils.rs Updated get_world_diff_and_account to accept ui: &mut MigrationUi; modified return type to reflect new account representation; improved UI interaction during account verification.
crates/dojo/utils/src/keystore.rs Changed password prompt message from "Enter password: " to "Enter the keystore password: ".
crates/dojo/world/src/local/artifact_to_local.rs Added logging statement in from_directory method to log profile_config and directory being loaded.
crates/sozo/ops/src/lib.rs Introduced new public module migration_ui.
crates/sozo/ops/src/migrate/mod.rs Redefined MigrationUi usage; updated methods to accept ui: &mut MigrationUi for UI updates during migration operations.
crates/sozo/ops/src/migration_ui.rs Added MigrationUi struct with methods for managing spinner UI during migration processes.
examples/simple/Scarb.toml Added new profile section [profile.sepolia] for Sepolia environment configuration.
examples/simple/dojo_sepolia.toml Introduced new TOML configuration for a simple world setup, defining various sections for environment, namespaces, and migration rules.
examples/simple/manifest_sepolia.json New file defining a structured representation of a blockchain environment, detailing world, ABI, and contract definitions.
xtask/generate-test-db/src/main.rs Modified imports for MigrationUi; updated instantiation of MigrationUi in migration functions to use MigrationUi::new("").with_silent().

Possibly related PRs


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

🧹 Outside diff range and nitpick comments (10)
crates/sozo/ops/src/lib.rs (1)

5-5: Consider adding module documentation, sensei!

Adding module-level documentation would help other developers understand the purpose and usage of this UI component. For example:

+/// Provides user interface components and utilities for migration operations.
+/// This module handles the display of migration progress, status updates,
+/// and interactive elements during world migrations.
pub mod migration_ui;
examples/simple/dojo_sepolia.toml (3)

1-4: Ohayo! Consider adding more descriptive world metadata, sensei!

The world description "Simple world." is quite generic. Consider adding more specific details about the world's purpose and functionality to help other developers understand its intended use case.

[world]
-description = "Simple world."
+description = "Example world configuration for Sepolia testnet demonstrating basic Dojo setup and namespace management."
name = "simple"
seed = "simple2"

17-19: Consider documenting the initialization arguments

The hexadecimal value 0xfffe is used for both namespaces but its purpose isn't clear.

[init_call_args]
+# Initialize components with maximum allowed value (0xfffe)
"ns-c1" = ["0xfffe"]
"ns2-c1" = ["0xfffe"]

28-30: Document the migration skip reason

The skip_contracts entry includes ns-c3, but it's not clear why this contract should be skipped during migration.

[migration]
order_inits = ["ns-c2", "ns-c1"]
+# Skip ns-c3 as it's handled by a separate deployment process
skip_contracts = ["ns-c3"]
crates/sozo/ops/src/migration_ui.rs (1)

1-80: Overall implementation looks solid, sensei!

The MigrationUi struct provides a clean interface for managing spinner-based progress indication with proper silent mode support. The code is well-documented and follows Rust best practices.

Consider these architectural improvements:

  1. Add a configuration struct to make the UI more customizable
  2. Consider implementing Drop trait to ensure proper cleanup
  3. Add events/hooks for better integration with logging systems
bin/sozo/src/commands/migrate.rs (1)

Line range hint 51-84: Consider adding error handling for spinner state.

Ohayo sensei! While the error handling is generally good, there's a potential edge case where the spinner might remain active if an error occurs between spinner updates. Consider wrapping the spinner operations in a Drop implementation or using a guard pattern.

Here's a suggested approach:

struct SpinnerGuard<'a> {
    spinner: &'a mut MigrationUi,
}

impl<'a> Drop for SpinnerGuard<'a> {
    fn drop(&mut self) {
        // Ensure spinner is stopped even if an error occurs
        self.spinner.stop();
    }
}

// Usage in the run method:
let guard = SpinnerGuard { spinner: &mut spinner };
// ... rest of the code ...
// Explicitly drop the guard on success
std::mem::drop(guard);
spinner.stop_and_persist_boxed(symbol, end_text);
xtask/generate-test-db/src/main.rs (1)

59-59: Consider documenting the empty title parameter, sensei!

While using silent mode is perfect for test utilities, the empty string passed to new("") could use a brief comment explaining why no title is needed in this context.

-    .migrate(&mut MigrationUi::new("").with_silent())
+    // Empty title used for test DB generation since output is silenced
+    .migrate(&mut MigrationUi::new("").with_silent())
crates/dojo/world/src/local/artifact_to_local.rs (2)

24-28: Ohayo sensei! The trace logging looks good but could be even more helpful!

The structured logging is a nice addition for observability. However, we could make it even more helpful by including the count of resources being loaded.

Consider enhancing the trace logging like this:

         trace!(
             ?profile_config,
             directory = %dir.as_ref().to_string_lossy(),
+            resources_count = fs::read_dir(&dir)?.count(),
             "Loading world from directory."
         );

24-28: Ohayo sensei! Great addition to support observability!

The trace logging aligns perfectly with the PR's UI rework objectives. This structured logging will be valuable for:

  • Debugging profile detection issues
  • Understanding world loading state in the new UI
  • Monitoring resource loading in production

Consider adding similar trace logging to other critical paths in the migration process to maintain consistent observability.

examples/simple/manifest_sepolia.json (1)

2-6: Consider adding documentation for the world configuration.

Ohayo sensei! The world configuration would benefit from adding comments or documentation to explain:

  • The purpose of this world
  • The significance of the class hash and address
  • The meaning of the seed value "simple2"
 {
+  // World configuration for the Simple example on Sepolia network
   "world": {
+    // Core world contract class hash
     "class_hash": "0x139239a99d627697b19b9856beaef7896fc75375caf3d750dd76982a7afeb78",
+    // Deployed world contract address
     "address": "0x1f21e5883353346629ec313c950e998982c12411c1d86e12b97bf26540760c1",
+    // Unique identifier for this world instance
     "seed": "simple2",
     "name": "simple",
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 66b76e6 and 96977d9.

📒 Files selected for processing (13)
  • bin/sozo/src/commands/execute.rs (2 hunks)
  • bin/sozo/src/commands/migrate.rs (4 hunks)
  • bin/sozo/src/commands/options/account/mod.rs (1 hunks)
  • bin/sozo/src/utils.rs (2 hunks)
  • crates/dojo/utils/src/keystore.rs (1 hunks)
  • crates/dojo/world/src/local/artifact_to_local.rs (1 hunks)
  • crates/sozo/ops/src/lib.rs (1 hunks)
  • crates/sozo/ops/src/migrate/mod.rs (11 hunks)
  • crates/sozo/ops/src/migration_ui.rs (1 hunks)
  • examples/simple/Scarb.toml (1 hunks)
  • examples/simple/dojo_sepolia.toml (1 hunks)
  • examples/simple/manifest_sepolia.json (1 hunks)
  • xtask/generate-test-db/src/main.rs (3 hunks)
✅ Files skipped from review due to trivial changes (2)
  • crates/dojo/utils/src/keystore.rs
  • examples/simple/Scarb.toml
🔇 Additional comments (16)
crates/sozo/ops/src/lib.rs (1)

5-5: Ohayo! The new module addition looks good, sensei!

The migration_ui module is properly integrated into the library structure and aligns well with its intended usage in execute and migrate commands.

examples/simple/dojo_sepolia.toml (2)

13-15: LGTM! Clear namespace configuration

The namespace mappings are well-structured and clearly define the relationships between namespaces and their components.


21-26: Verify the writer permissions configuration

The writers configuration shows that ns-M can write to ns2-c1, but there's no clear indication of why this cross-namespace write permission is necessary.

Let's check for any dependencies between these namespaces:

✅ Verification successful

The previous search might have been too specific. Let's broaden our search to understand the namespace configuration patterns and any documentation that might explain these relationships.


Looking at the configuration files, I can see that this cross-namespace write permission is actually part of a consistent pattern. The mappings section in the configuration shows that both ns and ns2 namespaces are defined with similar components:

mappings = { "ns" = ["c1", "M"], "ns2" = ["c1", "M"] }

This indicates that these namespaces are designed to interact with each other, and the write permission from ns-M to ns2-c1 is intentional as part of the system's architecture.

Cross-namespace write permission is correctly configured

The write permission configuration is valid as both namespaces (ns and ns2) are part of the same system design, sharing similar component structures (c1 and M).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any references to these namespace interactions
rg -l "ns-M.*ns2-c1|ns2-c1.*ns-M" --type rust

Length of output: 47


Script:

#!/bin/bash
# Let's search for any references to these namespaces in the entire codebase
rg "ns2-c1|ns-M" --type toml -A 5 -B 5

# Also check for any configuration files that might explain namespace relationships
fd -e toml -x cat {}

Length of output: 78307

crates/sozo/ops/src/migration_ui.rs (2)

8-13: Ohayo sensei! The struct design looks clean and well-documented.

The fields are well-chosen for managing spinner state and silent mode functionality.


23-27: Verify emoji compatibility across different terminals.

While the emoji spinner frames (⛩️, 🎃, 👻, 🧟, 💀) add a playful touch, some terminals might not render them correctly.

Let's check if there are any terminal compatibility considerations in the codebase:

✅ Verification successful

Emoji spinner implementation is well-supported, no changes needed

The spinner implementation is using the spinoff crate which handles terminal compatibility, and the code includes proper silent mode fallback. The emojis used (⛩️, 🎃, 👻, 🧟, 💀) are basic Unicode characters that are widely supported in modern terminals. Additionally, the UI implementation gracefully handles cases where output needs to be silent.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any terminal-related configurations or checks
rg -i "term|terminal|console" --type rust

Length of output: 11056


Script:

#!/bin/bash
# Let's check if there are any terminal/console configurations or fallback mechanisms
ast-grep --pattern 'use console::$_'

# Also check for any terminal-specific configurations or checks
ast-grep --pattern 'if $_.supports_unicode() { $$ }'

# And look for any spinner-related configurations
rg -i "spinner|frame" --type rust -A 3

Length of output: 7379

bin/sozo/src/commands/migrate.rs (3)

7-8: LGTM! Clean import additions for migration UI.

Ohayo! The new imports for Migration, MigrationResult, and MigrationUi are well-structured and align perfectly with the UI rework objectives, sensei! 🎯


51-51: LGTM! Enhanced UI feedback implementation.

The new MigrationUi spinner provides better user feedback during the migration process, and the world diff evaluation is properly integrated with the UI updates.

Also applies to: 57-58


100-101: LGTM! Improved profile configuration handling.

The changes properly fix the profile detection by loading the configuration before obtaining the provider and RPC URL. This ensures the banner displays the correct environment settings, sensei! 🎯

bin/sozo/src/commands/execute.rs (2)

10-10: Ohayo sensei! Clean import addition.

The MigrationUi import is well-placed and aligns with the PR's UI improvement objectives.


83-90: ⚠️ Potential issue

Ohayo sensei! The UI implementation needs some refinements.

Several concerns with the current implementation:

  1. The spinner is initialized with an empty message "", which might not provide meaningful feedback
  2. Silent mode is hardcoded without user control
  3. The spinner isn't properly cleaned up after operations complete

Consider applying these improvements:

-            let mut spinner = MigrationUi::new("").with_silent();
+            let mut spinner = MigrationUi::new("Preparing execution...").with_silent(self.starknet.silent);

             let (world_diff, account, _) = utils::get_world_diff_and_account(
                 self.account,
                 self.starknet.clone(),
                 self.world,
                 &ws,
                 &mut spinner,
-            )
-            .await?;
+            )
+            .await?;
+            
+            // Ensure spinner is stopped before printing results
+            spinner.stop();

Let's verify if the silent mode is configurable:

xtask/generate-test-db/src/main.rs (1)

13-14: LGTM! Clean module organization, sensei!

The separation of MigrationUi into its own module improves code organization by isolating UI-related functionality.

bin/sozo/src/commands/options/account/mod.rs (1)

110-113: Ohayo! Nice addition of chain ID tracing, sensei! 🎯

The new trace logging for chain ID fetching improves observability and aligns well with the profile detection improvements.

bin/sozo/src/utils.rs (2)

12-12: Ohayo! The import looks good, sensei!

The new MigrationUi import aligns perfectly with the PR's UI rework objectives.


148-148: Ohayo! The new parameter addition looks good, sensei!

The ui: &mut MigrationUi parameter is well-placed and follows Rust idioms.

crates/sozo/ops/src/migrate/mod.rs (1)

39-40: LGTM! Clean import addition.

The MigrationUi module import is properly placed and follows Rust conventions.

examples/simple/manifest_sepolia.json (1)

1122-1930: Verify the empty systems arrays in contract configurations.

Ohayo sensei! I noticed that all contracts have empty systems arrays. This might indicate:

  1. Missing system implementations
  2. Incomplete contract configurations
  3. Intentional design choice

Please verify if this is intended or if system implementations should be added.

✅ Verification successful

Empty systems arrays are expected in manifest configuration

Ohayo sensei! After checking the codebase, I found that the empty systems arrays in the manifest are correct because:

  • The system implementations exist in examples/simple/src/lib.cairo as part of the MyInterface trait and its implementation
  • The systems are defined at the contract level through interfaces rather than as standalone system entries in the manifest
  • This is a valid pattern where systems are encapsulated within the contract's ABI (as seen in the manifest's MyInterface section) rather than listed separately in the systems array
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for system implementations in the codebase

# Search for system implementations
echo "Searching for system implementations..."
rg -g "!manifest*.json" "system_[1-4]" -A 5

Length of output: 2179

Comment on lines +6 to +11
[env]
rpc_url = "https://api.cartridge.gg/x/starknet/sepolia"
# Default account for katana with seed = 0
account_address = "0x4ba5ae775eb7da75f092b3b30b03bce15c3476337ef5f9e3cdf18db7a7534bd"
keystore_path = "/path/keystore"
#world_address = "0x077c0dc7c1aba7f8842aff393ce6aa71fa675b4ced1bc927f7fc971b6acd92fc"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security consideration: Sensitive configuration values should be externalized

The account address and keystore path should not be hardcoded in the configuration file, especially if this is an example file that might be committed to version control.

Consider using environment variables:

[env]
rpc_url = "https://api.cartridge.gg/x/starknet/sepolia"
-# Default account for katana with seed = 0
-account_address = "0x4ba5ae775eb7da75f092b3b30b03bce15c3476337ef5f9e3cdf18db7a7534bd"
-keystore_path = "/path/keystore"
+account_address = "${DOJO_ACCOUNT_ADDRESS}"
+keystore_path = "${DOJO_KEYSTORE_PATH}"
📝 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.

Suggested change
[env]
rpc_url = "https://api.cartridge.gg/x/starknet/sepolia"
# Default account for katana with seed = 0
account_address = "0x4ba5ae775eb7da75f092b3b30b03bce15c3476337ef5f9e3cdf18db7a7534bd"
keystore_path = "/path/keystore"
#world_address = "0x077c0dc7c1aba7f8842aff393ce6aa71fa675b4ced1bc927f7fc971b6acd92fc"
[env]
rpc_url = "https://api.cartridge.gg/x/starknet/sepolia"
account_address = "${DOJO_ACCOUNT_ADDRESS}"
keystore_path = "${DOJO_KEYSTORE_PATH}"
#world_address = "0x077c0dc7c1aba7f8842aff393ce6aa71fa675b4ced1bc927f7fc971b6acd92fc"

Comment on lines +44 to +47
/// Updates the text of the spinner with a boxed string.
pub fn update_text_boxed(&mut self, text: String) {
self.update_text(Box::leak(text.into_boxed_str()));
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Caution: Potential memory leak in boxed string methods.

The Box::leak calls in update_text_boxed and stop_and_persist_boxed deliberately leak memory by converting the boxed strings into static references. While this works, it could lead to memory growth over time if these methods are called frequently.

Consider alternatives:

  1. Use a string cache if you need to reuse strings
  2. Accept String by value and clone only when needed
  3. Use Cow<'static, str> for flexible ownership

Example implementation using Cow:

use std::borrow::Cow;

pub fn update_text_flexible(&mut self, text: impl Into<Cow<'static, str>>) {
    if self.silent {
        return;
    }
    let text = text.into();
    self.spinner.update_text(text.as_ref());
}

Also applies to: 49-52

Comment on lines +72 to +79
/// Restarts the spinner with the default frames if it has been stopped.
pub fn restart(&mut self, text: &'static str) {
if self.silent {
return;
}

self.spinner = Spinner::new(self.default_frames.clone(), text, None);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding error handling for edge cases.

The restart method creates a new spinner instance but doesn't handle potential failure cases. Consider:

  1. What happens if spinner creation fails?
  2. Should we handle the case where restart is called on a running spinner?

Here's a suggested implementation with error handling:

pub fn restart(&mut self, text: &'static str) -> Result<(), SpinnerError> {
    if self.silent {
        return Ok(());
    }

    match Spinner::new(self.default_frames.clone(), text, None) {
        Ok(new_spinner) => {
            self.spinner = new_spinner;
            Ok(())
        }
        Err(e) => {
            // Fallback to simple println if spinner creation fails
            println!("{}", text);
            Err(e)
        }
    }
}

@@ -55,7 +56,7 @@ async fn migrate_spawn_and_move(db_path: &Path) -> Result<Manifest> {
profile_config,
runner.url().to_string(),
)
.migrate(&mut MigrationUi::None)
.migrate(&mut MigrationUi::new("").with_silent())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Ohayo! Consider extracting common migration logic, sensei!

Both migration functions share similar patterns. We could reduce duplication by extracting the common migration logic into a helper function.

Here's a suggested approach:

async fn perform_migration(
    db_path: &Path,
    setup: CompilerTestSetup,
    target: &str,
    profile: Profile,
) -> Result<Manifest> {
    let cfg = KatanaRunnerConfig {
        db_dir: Some(db_path.to_path_buf()),
        n_accounts: 10,
        dev: true,
        ..Default::default()
    };
    let runner = KatanaRunner::new_with_config(cfg)?;
    
    let cfg = setup.build_test_config(target, profile);
    // ... rest of the common logic ...
    
    let result = Migration::new(/* ... */)
        .migrate(&mut MigrationUi::new("").with_silent())
        .await?;
    
    Ok(result.manifest)
}

This would simplify both migration functions to:

async fn migrate_spawn_and_move(db_path: &Path) -> Result<Manifest> {
    println!("migrate spawn_and_move");
    let setup = CompilerTestSetup::from_examples("crates/dojo/core", "examples/");
    perform_migration(db_path, setup, "spawn-and-move", Profile::DEV).await
}

Also applies to: 106-106

Comment on lines +110 to 113
trace!("Fetching chain id...");
let chain_id = provider.chain_id().await?;
trace!(?chain_id);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider enhancing error context for chain ID fetching.

The chain ID fetch could benefit from additional error context to help diagnose RPC-related issues.

-        let chain_id = provider.chain_id().await?;
+        let chain_id = provider.chain_id().await.with_context(|| "Failed to fetch chain ID from provider")?;
📝 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.

Suggested change
trace!("Fetching chain id...");
let chain_id = provider.chain_id().await?;
trace!(?chain_id);
trace!("Fetching chain id...");
let chain_id = provider.chain_id().await.with_context(|| "Failed to fetch chain ID from provider")?;
trace!(?chain_id);

let mut invoker = Invoker::new(&self.world.account, self.txn_config);

// Namespaces must be synced first, since contracts, models and events are namespaced.
self.namespaces_getcalls(&mut invoker).await?;

let mut classes: HashMap<Felt, FlattenedSierraClass> = HashMap::new();
let mut n_resources = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance resource type tracking in UI updates.

The current implementation tracks total resources, but providing a breakdown by resource type would offer more detailed progress information.

-let mut n_resources = 0;
+#[derive(Default)]
+struct ResourceCounts {
+    contracts: usize,
+    models: usize,
+    events: usize,
+}
+let mut counts = ResourceCounts::default();

 // In the resource type match blocks:
 match resource.resource_type() {
     ResourceType::Contract => {
         // ... existing code ...
-        n_resources += 1;
+        counts.contracts += 1;
     }
     ResourceType::Model => {
         // ... existing code ...
-        n_resources += 1;
+        counts.models += 1;
     }
     ResourceType::Event => {
         // ... existing code ...
-        n_resources += 1;
+        counts.events += 1;
     }
 }

-let ui_text = format!("Registering {} resources...", n_resources);
+let ui_text = format!(
+    "Registering resources: {} contracts, {} models, {} events",
+    counts.contracts, counts.models, counts.events
+);

Also applies to: 327-327, 333-333, 339-339, 400-407

Comment on lines +284 to +291
let ui_text = format!("Syncing {} permissions...", invoker.calls.len());
ui.update_text_boxed(ui_text);

invoker.multicall().await?;
} else {
let ui_text = format!("Syncing {} permissions (sequentially)...", invoker.calls.len());
ui.update_text_boxed(ui_text);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Differentiate between writer and owner permissions in UI updates.

The current UI feedback could be more informative by breaking down the permission types being synced.

-let ui_text = format!("Syncing {} permissions...", invoker.calls.len());
+let (writer_count, owner_count) = invoker.calls.iter().fold((0, 0), |(w, o), call| {
+    if call.selector.to_string().contains("grant_writer") {
+        (w + 1, o)
+    } else {
+        (w, o + 1)
+    }
+});
+let ui_text = format!("Syncing {} writer and {} owner permissions...", writer_count, owner_count);

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +207 to +221
if !ordered_init_calls.is_empty() {
if self.do_multicall() {
let ui_text = format!("Initializing {} contracts...", ordered_init_calls.len());
ui.update_text_boxed(ui_text);

invoker.multicall().await?;
} else {
let ui_text = format!(
"Initializing {} contracts (sequentially)...",
ordered_init_calls.len()
);
ui.update_text_boxed(ui_text);

invoker.invoke_all_sequentially().await?;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance progress tracking for sequential contract initialization.

While the current implementation provides good feedback, sequential execution could benefit from per-contract progress updates to give users better visibility into the initialization process.

 if !self.do_multicall() {
     let ui_text = format!(
         "Initializing {} contracts (sequentially)...",
         ordered_init_calls.len()
     );
     ui.update_text_boxed(ui_text);

-    invoker.invoke_all_sequentially().await?;
+    for (i, call) in invoker.calls.iter().enumerate() {
+        ui.update_text(&format!("Initializing contract {}/{}", i + 1, invoker.calls.len()));
+        invoker.invoke_single(call).await?;
+    }
 }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +1932 to +1943
"models": [
{
"members": [],
"class_hash": "0xb35ce9998d1524acfc8b0318aed7375b0d977b6362a2f7af23be2950aa96fd",
"tag": "M"
},
{
"members": [],
"class_hash": "0xb35ce9998d1524acfc8b0318aed7375b0d977b6362a2f7af23be2950aa96fd",
"tag": "M"
}
],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Review duplicate model configurations.

Ohayo sensei! There appear to be two identical model configurations:

  • Same class hash: 0xb35ce9998d1524acfc8b0318aed7375b0d977b6362a2f7af23be2950aa96fd
  • Both have empty members
  • Both tagged as "M"

This might be unintentional. Consider:

  1. Removing the duplicate if not needed
  2. Differentiating the models if they serve different purposes

Comment on lines +1944 to +1955
"events": [
{
"members": [],
"class_hash": "0x65aa33d998d733abc890ee36503fe1df8e7c01f2cf1a92b147bd424a1af56d7",
"tag": "E"
},
{
"members": [],
"class_hash": "0x58568a90180a44515609dbaf69bb0c1aa56f29e93688f4bfdab10268fe68ce1",
"tag": "EH"
}
]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider defining event members.

Ohayo sensei! Both events have empty member arrays:

  1. Event with tag "E" (class_hash: 0x65aa...)
  2. Event with tag "EH" (class_hash: 0x585...)

Empty events might limit their usefulness for:

  • Tracking state changes
  • Debugging
  • Event filtering

Consider defining appropriate members based on the events' purposes.

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 1.58730% with 124 lines in your changes missing coverage. Please review.

Project coverage is 55.02%. Comparing base (66b76e6) to head (96977d9).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/sozo/ops/src/migrate/mod.rs 0.00% 61 Missing ⚠️
crates/sozo/ops/src/migration_ui.rs 0.00% 46 Missing ⚠️
bin/sozo/src/commands/migrate.rs 0.00% 6 Missing ⚠️
bin/sozo/src/utils.rs 0.00% 4 Missing ⚠️
bin/sozo/src/commands/execute.rs 0.00% 2 Missing ⚠️
crates/dojo/world/src/local/artifact_to_local.rs 33.33% 2 Missing ⚠️
xtask/generate-test-db/src/main.rs 0.00% 2 Missing ⚠️
crates/dojo/utils/src/keystore.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2606      +/-   ##
==========================================
- Coverage   55.12%   55.02%   -0.11%     
==========================================
  Files         385      386       +1     
  Lines       47775    47860      +85     
==========================================
- Hits        26337    26334       -3     
- Misses      21438    21526      +88     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant