refactor(sozo): migration logic to be more robust and testable (#1848)

* refactor: move `load_world_manifest` to utils * add class_hash suffix * refactor: clean up MigrateArgs * fix: enforce seed matches the provided world_address * refactor: print chain_id inside `setup_env` instead of passing it around * move functions to migrate.rs * refactor: some function names * refactor!: don't mutate `MigrationStrategy` use the `MigrationOutput` instead * move update_manifests_and_abi function to migration * refactor: make `name/seed` non optional * remove duplicate test file * add test for seed issue * fix clippy lints * fix formatting * make suggested improvements * fix err string in test * fix formatting --------- Co-authored-by: glihm <[email protected]>
dojoengine · Apr 24, 2024 · 0ac81cf · 0ac81cf
1 parent a573198
commit 0ac81cf
Show file tree

Hide file tree

Showing 19 changed files with 1,173 additions and 1,307 deletions.
diff --git a/bin/sozo/src/commands/clean.rs b/bin/sozo/src/commands/clean.rs
@@ -81,10 +81,9 @@ mod tests {
             migration::migrate(
                 &ws,
                 None,
-                "chain_id".to_string(),
                 runner.endpoint(),
                 &runner.account(0),
-                Some("dojo_examples".to_string()),
+                "dojo_examples",
                 true,
                 TxnConfig::default(),
             )

diff --git a/bin/sozo/src/commands/dev.rs b/bin/sozo/src/commands/dev.rs
@@ -18,7 +18,7 @@ use notify_debouncer_mini::notify::RecursiveMode;
 use notify_debouncer_mini::{new_debouncer, DebouncedEvent, DebouncedEventKind};
 use scarb::compiler::CompilationUnit;
 use scarb::core::{Config, Workspace};
-use sozo_ops::migration::{self, prepare_migration};
+use sozo_ops::migration;
 use starknet::accounts::SingleOwnerAccount;
 use starknet::core::types::FieldElement;
 use starknet::providers::Provider;
@@ -52,9 +52,9 @@ pub struct DevArgs {
 
 impl DevArgs {
     pub fn run(self, config: &Config) -> Result<()> {
-        let env_metadata = if config.manifest_path().exists() {
-            let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;
+        let ws = scarb::ops::read_workspace(config.manifest_path(), config)?;
 
+        let env_metadata = if config.manifest_path().exists() {
             dojo_metadata_from_workspace(&ws).env().cloned()
         } else {
             None
@@ -68,11 +68,13 @@ impl DevArgs {
             config.manifest_path().parent().unwrap().as_std_path(),
             RecursiveMode::Recursive,
         )?;
-        let name = self.name.clone();
+
+        let name = self.name.unwrap_or_else(|| ws.root_package().unwrap().id.name.to_string());
+
         let mut previous_manifest: Option<DeploymentManifest> = Option::None;
         let result = build(&mut context);
 
-        let Some((mut world_address, account, _, _)) = context
+        let Some((mut world_address, account, _)) = context
             .ws
             .config()
             .tokio_handle()
@@ -81,7 +83,7 @@ impl DevArgs {
                 self.account,
                 self.starknet,
                 self.world,
-                name.as_ref(),
+                &name,
                 env_metadata.as_ref(),
             ))
             .ok()
@@ -92,7 +94,7 @@ impl DevArgs {
         match context.ws.config().tokio_handle().block_on(migrate(
             world_address,
             &account,
-            name.clone(),
+            &name,
             &context.ws,
             previous_manifest.clone(),
         )) {
@@ -127,7 +129,7 @@ impl DevArgs {
                 match context.ws.config().tokio_handle().block_on(migrate(
                     world_address,
                     &account,
-                    name.clone(),
+                    &name,
                     &context.ws,
                     previous_manifest.clone(),
                 )) {
@@ -215,7 +217,7 @@ fn build(context: &mut DevContext<'_>) -> Result<()> {
 async fn migrate<P, S>(
     mut world_address: Option<FieldElement>,
     account: &SingleOwnerAccount<P, S>,
-    name: Option<String>,
+    name: &str,
     ws: &Workspace<'_>,
     previous_manifest: Option<DeploymentManifest>,
 ) -> Result<(DeploymentManifest, Option<FieldElement>)>
@@ -245,7 +247,7 @@ where
     }
 
     let ui = ws.config().ui();
-    let mut strategy = prepare_migration(&target_dir, diff, name, world_address, &ui)?;
+    let mut strategy = migration::prepare_migration(&target_dir, diff, name, world_address, &ui)?;
 
     match migration::apply_diff(ws, account, TxnConfig::default(), &mut strategy).await {
         Ok(migration_output) => {

diff --git a/bin/sozo/src/commands/migrate.rs b/bin/sozo/src/commands/migrate.rs
@@ -22,44 +22,30 @@ use super::options::world::WorldOptions;
 pub struct MigrateArgs {
     #[command(subcommand)]
     pub command: MigrateCommand,
+
+    #[arg(long, global = true)]
+    #[arg(help = "Name of the World.")]
+    #[arg(long_help = "Name of the World. It's hash will be used as a salt when deploying the \
+                       contract to avoid address conflicts. If not provided root package's name \
+                       will be used.")]
+    name: Option<String>,
+
+    #[command(flatten)]
+    world: WorldOptions,
+
+    #[command(flatten)]
+    starknet: StarknetOptions,
+
+    #[command(flatten)]
+    account: AccountOptions,
 }
 
 #[derive(Debug, Subcommand)]
 pub enum MigrateCommand {
     #[command(about = "Plan the migration and output the manifests.")]
-    Plan {
-        #[arg(long)]
-        #[arg(help = "Name of the World.")]
-        #[arg(long_help = "Name of the World. It's hash will be used as a salt when deploying \
-                           the contract to avoid address conflicts.")]
-        name: Option<String>,
-
-        #[command(flatten)]
-        world: WorldOptions,
-
-        #[command(flatten)]
-        starknet: StarknetOptions,
-
-        #[command(flatten)]
-        account: AccountOptions,
-    },
+    Plan,
     #[command(about = "Apply the migration on-chain.")]
     Apply {
-        #[arg(long)]
-        #[arg(help = "Name of the World.")]
-        #[arg(long_help = "Name of the World. It's hash will be used as a salt when deploying \
-                           the contract to avoid address conflicts.")]
-        name: Option<String>,
-
-        #[command(flatten)]
-        world: WorldOptions,
-
-        #[command(flatten)]
-        starknet: StarknetOptions,
-
-        #[command(flatten)]
-        account: AccountOptions,
-
         #[command(flatten)]
         transaction: TransactionOptions,
     },
@@ -80,71 +66,35 @@ impl MigrateArgs {
             return Err(anyhow!("Build project using `sozo build` first"));
         }
 
+        let MigrateArgs { name, world, starknet, account, .. } = self;
+
+        let name = name.unwrap_or_else(|| {
+            ws.root_package().expect("Root package to be present").id.name.to_string()
+        });
+
+        let (world_address, account, rpc_url) = config.tokio_handle().block_on(async {
+            setup_env(&ws, account, starknet, world, &name, env_metadata.as_ref()).await
+        })?;
+
         match self.command {
-            MigrateCommand::Plan { mut name, world, starknet, account } => {
-                if name.is_none() {
-                    if let Some(root_package) = ws.root_package() {
-                        name = Some(root_package.id.name.to_string())
-                    }
-                };
-
-                config.tokio_handle().block_on(async {
-                    let (world_address, account, chain_id, rpc_url) = setup_env(
-                        &ws,
-                        account,
-                        starknet,
-                        world,
-                        name.as_ref(),
-                        env_metadata.as_ref(),
-                    )
-                    .await?;
-
-                    migration::migrate(
-                        &ws,
-                        world_address,
-                        chain_id,
-                        rpc_url,
-                        &account,
-                        name,
-                        true,
-                        TxnConfig::default(),
-                    )
-                    .await
-                })
-            }
-            MigrateCommand::Apply { mut name, world, starknet, account, transaction } => {
+            MigrateCommand::Plan => config.tokio_handle().block_on(async {
+                migration::migrate(
+                    &ws,
+                    world_address,
+                    rpc_url,
+                    &account,
+                    &name,
+                    true,
+                    TxnConfig::default(),
+                )
+                .await
+            }),
+            MigrateCommand::Apply { transaction } => config.tokio_handle().block_on(async {
                 let txn_config: TxnConfig = transaction.into();
 
-                if name.is_none() {
-                    if let Some(root_package) = ws.root_package() {
-                        name = Some(root_package.id.name.to_string())
-                    }
-                };
-
-                config.tokio_handle().block_on(async {
-                    let (world_address, account, chain_id, rpc_url) = setup_env(
-                        &ws,
-                        account,
-                        starknet,
-                        world,
-                        name.as_ref(),
-                        env_metadata.as_ref(),
-                    )
-                    .await?;
-
-                    migration::migrate(
-                        &ws,
-                        world_address,
-                        chain_id,
-                        rpc_url,
-                        &account,
-                        name,
-                        false,
-                        txn_config,
-                    )
+                migration::migrate(&ws, world_address, rpc_url, &account, &name, false, txn_config)
                     .await
-                })
-            }
+            }),
         }
     }
 }
@@ -154,19 +104,18 @@ pub async fn setup_env<'a>(
     account: AccountOptions,
     starknet: StarknetOptions,
     world: WorldOptions,
-    name: Option<&'a String>,
+    name: &str,
     env: Option<&'a Environment>,
 ) -> Result<(
     Option<FieldElement>,
     SingleOwnerAccount<JsonRpcClient<HttpTransport>, LocalWallet>,
     String,
-    String,
 )> {
     let ui = ws.config().ui();
 
     let world_address = world.address(env).ok();
 
-    let (account, chain_id, rpc_url) = {
+    let (account, rpc_url) = {
         let provider = starknet.provider(env)?;
 
         let spec_version = provider.spec_version().await?;
@@ -191,12 +140,13 @@ pub async fn setup_env<'a>(
         let address = account.address();
 
         ui.print(format!("\nMigration account: {address:#x}"));
-        if let Some(name) = name {
-            ui.print(format!("\nWorld name: {name}\n"));
-        }
+
+        ui.print(format!("\nWorld name: {name}"));
+
+        ui.print(format!("\nChain ID: {chain_id}\n"));
 
         match account.provider().get_class_hash_at(BlockId::Tag(BlockTag::Pending), address).await {
-            Ok(_) => Ok((account, chain_id, rpc_url)),
+            Ok(_) => Ok((account, rpc_url)),
             Err(ProviderError::StarknetError(StarknetError::ContractNotFound)) => {
                 Err(anyhow!("Account with address {:#x} doesn't exist.", account.address()))
             }
@@ -205,5 +155,5 @@ pub async fn setup_env<'a>(
     }
     .with_context(|| "Problem initializing account for migration.")?;
 
-    Ok((world_address, account, chain_id, rpc_url.to_string()))
+    Ok((world_address, account, rpc_url.to_string()))
 }
diff --git a/bin/sozo/tests/register_test.rs b/bin/sozo/tests/register_test.rs
@@ -20,23 +20,23 @@ async fn reregister_models() {
 
     let base_dir = "../../examples/spawn-and-move";
     let target_dir = format!("{}/target/dev", base_dir);
-    let mut migration = prepare_migration(base_dir.into(), target_dir.into()).unwrap();
+    let migration = prepare_migration(base_dir.into(), target_dir.into()).unwrap();
 
     let sequencer =
         TestSequencer::start(SequencerConfig::default(), get_default_test_starknet_config()).await;
 
     let mut account = sequencer.account();
     account.set_block_id(BlockId::Tag(BlockTag::Pending));
 
-    execute_strategy(&ws, &mut migration, &account, TxnConfig::default()).await.unwrap();
+    execute_strategy(&ws, &migration, &account, TxnConfig::default()).await.unwrap();
     let world_address = &format!("0x{:x}", &migration.world_address().unwrap());
     let account_address = &format!("0x{:x}", account.address());
     let private_key = &format!("0x{:x}", sequencer.raw_account().private_key);
     let rpc_url = &sequencer.url().to_string();
 
     let moves_model =
         migration.models.iter().find(|m| m.diff.name == "dojo_examples::models::moves").unwrap();
-    let moves_model_class_hash = &format!("0x{:x}", moves_model.diff.local);
+    let moves_model_class_hash = &format!("0x{:x}", moves_model.diff.local_class_hash);
     let args_vec = [
         "register",
         "model",

diff --git a/crates/dojo-test-utils/src/migration.rs b/crates/dojo-test-utils/src/migration.rs
@@ -4,6 +4,8 @@ use dojo_lang::compiler::{BASE_DIR, MANIFESTS_DIR};
 use dojo_world::manifest::BaseManifest;
 use dojo_world::migration::strategy::{prepare_for_migration, MigrationStrategy};
 use dojo_world::migration::world::WorldDiff;
+use katana_primitives::FieldElement;
+use starknet::core::utils::cairo_short_string_to_felt;
 use starknet::macros::felt;
 
 pub fn prepare_migration(
@@ -20,5 +22,25 @@ pub fn prepare_migration(
 
     let world = WorldDiff::compute(manifest, None);
 
-    prepare_for_migration(None, Some(felt!("0x12345")), &target_dir, world)
+    prepare_for_migration(None, felt!("0x12345"), &target_dir, world)
+}
+
+pub fn prepare_migration_with_world_and_seed(
+    manifest_dir: Utf8PathBuf,
+    target_dir: Utf8PathBuf,
+    world_address: Option<FieldElement>,
+    seed: &str,
+) -> Result<MigrationStrategy> {
+    // In testing, profile name is always dev.
+    let profile_name = "dev";
+
+    let manifest = BaseManifest::load_from_path(
+        &manifest_dir.join(MANIFESTS_DIR).join(profile_name).join(BASE_DIR),
+    )
+    .unwrap();
+
+    let world = WorldDiff::compute(manifest, None);
+
+    let seed = cairo_short_string_to_felt(seed).unwrap();
+    prepare_for_migration(world_address, seed, &target_dir, world)
 }
diff --git a/crates/dojo-world/src/contracts/world_test.rs b/crates/dojo-world/src/contracts/world_test.rs
@@ -47,7 +47,7 @@ pub async fn deploy_world(
 
     let strategy = prepare_for_migration(
         None,
-        Some(FieldElement::from_hex_be("0x12345").unwrap()),
+        FieldElement::from_hex_be("0x12345").unwrap(),
         target_dir,
         world,
     )

diff --git a/crates/dojo-world/src/manifest/types.rs b/crates/dojo-world/src/manifest/types.rs
@@ -128,7 +128,7 @@ pub struct WorldContract {
     #[serde_as(as = "Option<UfeHex>")]
     pub transaction_hash: Option<FieldElement>,
     pub block_number: Option<u64>,
-    pub seed: Option<String>,
+    pub seed: String,
     pub metadata: Option<WorldMetadata>,
 }