-
Notifications
You must be signed in to change notification settings - Fork 195
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
use katana_rpc_api::starknet::RPC_SPEC_VERSION; | ||
use scarb::core::{TomlManifest, Workspace}; | ||
use semver::Version; | ||
use sozo_ops::migration_ui::MigrationUi; | ||
use sozo_scarbext::WorkspaceExt; | ||
use starknet::accounts::{Account, ConnectedAccount}; | ||
use starknet::core::types::Felt; | ||
|
@@ -144,19 +145,25 @@ | |
starknet: StarknetOptions, | ||
world: WorldOptions, | ||
ws: &Workspace<'_>, | ||
ui: &mut MigrationUi, | ||
) -> Result<(WorldDiff, SozoAccount<JsonRpcClient<HttpTransport>>, String)> { | ||
let profile_config = ws.load_profile_config()?; | ||
let env = profile_config.env.as_ref(); | ||
|
||
let (world_diff, provider, rpc_url) = | ||
get_world_diff_and_provider(starknet.clone(), world, ws).await?; | ||
|
||
// Ensures we don't interfere with the spinner if a password must be prompted. | ||
ui.stop(); | ||
|
||
Comment on lines
+156
to
+158
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider enhancing error handling for UI operations, sensei! While the UI integration improves user feedback, consider handling potential UI operation failures: - ui.stop();
+ if let Err(e) = ui.stop() {
+ tracing::warn!("Failed to stop UI spinner: {}", e);
+ }
let account = {
account
.account(provider, world_diff.world_info.address, &starknet, env, &world_diff)
.await?
};
- ui.restart("Verifying account...");
+ if let Err(e) = ui.restart("Verifying account...") {
+ tracing::warn!("Failed to restart UI with status: {}", e);
+ } This ensures the main flow continues even if UI operations fail. Also applies to: 165-166 |
||
let account = { | ||
account | ||
.account(provider, world_diff.world_info.address, &starknet, env, &world_diff) | ||
.await? | ||
}; | ||
|
||
ui.restart("Verifying account..."); | ||
|
||
if !dojo_utils::is_deployed(account.address(), &account.provider()).await? { | ||
return Err(anyhow!("Account with address {:#x} doesn't exist.", account.address())); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
pub mod account; | ||
pub mod migrate; | ||
pub mod migration_ui; | ||
|
||
#[cfg(test)] | ||
pub mod tests; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ | |
//! initialization of contracts can mutate resources. | ||
|
||
use std::collections::HashMap; | ||
use std::fmt; | ||
use std::str::FromStr; | ||
|
||
use cainome::cairo_serde::{ByteArray, ClassHash, ContractAddress}; | ||
|
@@ -30,14 +29,15 @@ | |
use dojo_world::local::ResourceLocal; | ||
use dojo_world::remote::ResourceRemote; | ||
use dojo_world::{utils, ResourceType}; | ||
use spinoff::Spinner; | ||
use starknet::accounts::{ConnectedAccount, SingleOwnerAccount}; | ||
use starknet::core::types::{Call, FlattenedSierraClass}; | ||
use starknet::providers::AnyProvider; | ||
use starknet::signers::LocalWallet; | ||
use starknet_crypto::Felt; | ||
use tracing::trace; | ||
|
||
use crate::migration_ui::MigrationUi; | ||
|
||
pub mod error; | ||
pub use error::MigrationError; | ||
|
||
|
@@ -61,36 +61,6 @@ | |
pub manifest: Manifest, | ||
} | ||
|
||
pub enum MigrationUi { | ||
Spinner(Spinner), | ||
None, | ||
} | ||
|
||
impl fmt::Debug for MigrationUi { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
match self { | ||
Self::Spinner(_) => write!(f, "Spinner"), | ||
Self::None => write!(f, "None"), | ||
} | ||
} | ||
} | ||
|
||
impl MigrationUi { | ||
pub fn update_text(&mut self, text: &'static str) { | ||
match self { | ||
Self::Spinner(s) => s.update_text(text), | ||
Self::None => (), | ||
} | ||
} | ||
|
||
pub fn stop_and_persist(&mut self, symbol: &'static str, text: &'static str) { | ||
match self { | ||
Self::Spinner(s) => s.stop_and_persist(symbol, text), | ||
Self::None => (), | ||
} | ||
} | ||
} | ||
|
||
impl<A> Migration<A> | ||
where | ||
A: ConnectedAccount + Sync + Send, | ||
|
@@ -113,23 +83,17 @@ | |
/// spinner. | ||
pub async fn migrate( | ||
&self, | ||
spinner: &mut MigrationUi, | ||
ui: &mut MigrationUi, | ||
) -> Result<MigrationResult, MigrationError<A::SignError>> { | ||
spinner.update_text("Deploying world..."); | ||
ui.update_text("Deploying world..."); | ||
Comment on lines
85
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider enhancing error state handling in UI updates. While the UI updates provide good feedback during successful operations, consider adding error state handling to provide clear feedback when operations fail. This would improve the user experience during error scenarios. pub async fn migrate(
&self,
ui: &mut MigrationUi,
) -> Result<MigrationResult, MigrationError<A::SignError>> {
ui.update_text("Deploying world...");
- let world_has_changed = self.ensure_world().await?;
+ let world_has_changed = match self.ensure_world().await {
+ Ok(changed) => {
+ ui.update_text("World deployment successful!");
+ changed
+ }
+ Err(e) => {
+ ui.update_text(&format!("World deployment failed: {}", e));
+ return Err(e);
+ }
+ }; Also applies to: 91-96 |
||
let world_has_changed = self.ensure_world().await?; | ||
|
||
let resources_have_changed = if !self.diff.is_synced() { | ||
spinner.update_text("Syncing resources..."); | ||
self.sync_resources().await? | ||
} else { | ||
false | ||
}; | ||
let resources_have_changed = | ||
if !self.diff.is_synced() { self.sync_resources(ui).await? } else { false }; | ||
|
||
spinner.update_text("Syncing permissions..."); | ||
let permissions_have_changed = self.sync_permissions().await?; | ||
let permissions_have_changed = self.sync_permissions(ui).await?; | ||
|
||
spinner.update_text("Initializing contracts..."); | ||
let contracts_have_changed = self.initialize_contracts().await?; | ||
let contracts_have_changed = self.initialize_contracts(ui).await?; | ||
|
||
Ok(MigrationResult { | ||
has_changes: world_has_changed | ||
|
@@ -152,7 +116,12 @@ | |
/// found in the [`ProfileConfig`]. | ||
/// | ||
/// Returns true if at least one contract has been initialized, false otherwise. | ||
async fn initialize_contracts(&self) -> Result<bool, MigrationError<A::SignError>> { | ||
async fn initialize_contracts( | ||
&self, | ||
ui: &mut MigrationUi, | ||
) -> Result<bool, MigrationError<A::SignError>> { | ||
ui.update_text("Initializing contracts..."); | ||
|
||
let mut invoker = Invoker::new(&self.world.account, self.txn_config); | ||
|
||
let init_call_args = if let Some(init_call_args) = &self.profile_config.init_call_args { | ||
|
@@ -235,10 +204,21 @@ | |
|
||
let has_changed = !invoker.calls.is_empty(); | ||
|
||
if self.do_multicall() { | ||
invoker.multicall().await?; | ||
} else { | ||
invoker.invoke_all_sequentially().await?; | ||
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?; | ||
} | ||
Comment on lines
+207
to
+221
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?;
+ }
}
|
||
} | ||
|
||
Ok(has_changed) | ||
|
@@ -257,7 +237,12 @@ | |
/// overlay resource, which can contain also writers. | ||
/// | ||
/// Returns true if at least one permission has changed, false otherwise. | ||
async fn sync_permissions(&self) -> Result<bool, MigrationError<A::SignError>> { | ||
async fn sync_permissions( | ||
&self, | ||
ui: &mut MigrationUi, | ||
) -> Result<bool, MigrationError<A::SignError>> { | ||
ui.update_text("Syncing permissions..."); | ||
|
||
let mut invoker = Invoker::new(&self.world.account, self.txn_config); | ||
|
||
// Only takes the local permissions that are not already set onchain to apply them. | ||
|
@@ -296,8 +281,14 @@ | |
let has_changed = !invoker.calls.is_empty(); | ||
|
||
if self.do_multicall() { | ||
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); | ||
|
||
Comment on lines
+284
to
+291
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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);
|
||
invoker.invoke_all_sequentially().await?; | ||
} | ||
|
||
|
@@ -307,13 +298,19 @@ | |
/// Syncs the resources by declaring the classes and registering/upgrading the resources. | ||
/// | ||
/// Returns true if at least one resource has changed, false otherwise. | ||
async fn sync_resources(&self) -> Result<bool, MigrationError<A::SignError>> { | ||
async fn sync_resources( | ||
&self, | ||
ui: &mut MigrationUi, | ||
) -> Result<bool, MigrationError<A::SignError>> { | ||
ui.update_text("Syncing resources..."); | ||
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
// Collects the calls and classes to be declared to sync the resources. | ||
for resource in self.diff.resources.values() { | ||
|
@@ -327,16 +324,19 @@ | |
self.contracts_calls_classes(resource).await?; | ||
invoker.extend_calls(contract_calls); | ||
classes.extend(contract_classes); | ||
n_resources += 1; | ||
} | ||
ResourceType::Model => { | ||
let (model_calls, model_classes) = self.models_calls_classes(resource).await?; | ||
invoker.extend_calls(model_calls); | ||
classes.extend(model_classes); | ||
n_resources += 1; | ||
} | ||
ResourceType::Event => { | ||
let (event_calls, event_classes) = self.events_calls_classes(resource).await?; | ||
invoker.extend_calls(event_calls); | ||
classes.extend(event_classes); | ||
n_resources += 1; | ||
} | ||
_ => continue, | ||
} | ||
|
@@ -350,11 +350,16 @@ | |
// Since migrator account from `self.world.account` is under the [`ConnectedAccount`] trait, | ||
// we can group it with the predeployed accounts which are concrete types. | ||
let accounts = self.get_accounts().await; | ||
let n_classes = classes.len(); | ||
|
||
if accounts.is_empty() { | ||
trace!("Declaring classes with migrator account."); | ||
let mut declarer = Declarer::new(&self.world.account, self.txn_config); | ||
declarer.extend_classes(classes.into_iter().collect()); | ||
|
||
let ui_text = format!("Declaring {} classes...", n_classes); | ||
ui.update_text_boxed(ui_text); | ||
|
||
declarer.declare_all().await?; | ||
} else { | ||
trace!("Declaring classes with {} accounts.", accounts.len()); | ||
|
@@ -368,6 +373,10 @@ | |
declarers[declarer_idx].add_class(casm_class_hash, class); | ||
} | ||
|
||
let ui_text = | ||
format!("Declaring {} classes with {} accounts...", n_classes, declarers.len()); | ||
ui.update_text_boxed(ui_text); | ||
|
||
let declarers_futures = | ||
futures::future::join_all(declarers.into_iter().map(|d| d.declare_all())).await; | ||
|
||
|
@@ -388,8 +397,14 @@ | |
} | ||
|
||
if self.do_multicall() { | ||
let ui_text = format!("Registering {} resources...", n_resources); | ||
ui.update_text_boxed(ui_text); | ||
|
||
invoker.multicall().await?; | ||
} else { | ||
let ui_text = format!("Registering {} resources (sequentially)...", n_resources); | ||
ui.update_text_boxed(ui_text); | ||
|
||
invoker.invoke_all_sequentially().await?; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.
📝 Committable suggestion