From 70ad72826df84cef73368cb38abda3d4715548dd Mon Sep 17 00:00:00 2001 From: lambda-0x <0xlambda@protonmail.com> Date: Mon, 17 Jun 2024 19:21:40 +0530 Subject: [PATCH] fix(migrate): when `init_calldata` depends on contract that was already deployed (#2058) * fix(migrate): when `init_calldata` depends on contract that was already deployed * add tests --- crates/dojo-world/src/migration/strategy.rs | 51 ++++++--- crates/sozo/ops/src/tests/migration.rs | 109 +++++++++++++++++++- crates/torii/types-test/Scarb.lock | 2 +- 3 files changed, 144 insertions(+), 18 deletions(-) diff --git a/crates/dojo-world/src/migration/strategy.rs b/crates/dojo-world/src/migration/strategy.rs index 2a1ce96155..258b34ea68 100644 --- a/crates/dojo-world/src/migration/strategy.rs +++ b/crates/dojo-world/src/migration/strategy.rs @@ -13,6 +13,11 @@ use super::contract::{ContractDiff, ContractMigration}; use super::world::WorldDiff; use super::MigrationType; +#[derive(Debug, Clone)] +pub enum MigrationMetadata { + Contract(ContractDiff), +} + #[derive(Debug, Clone)] pub struct MigrationStrategy { pub world_address: Option, @@ -20,6 +25,7 @@ pub struct MigrationStrategy { pub base: Option, pub contracts: Vec, pub models: Vec, + pub metadata: HashMap, } #[derive(Debug)] @@ -61,23 +67,29 @@ impl MigrationStrategy { } pub fn resolve_variable(&mut self, world_address: FieldElement) -> Result<()> { - let contracts_clone = self.contracts.clone(); for contract in self.contracts.iter_mut() { for field in contract.diff.init_calldata.iter_mut() { if let Some(dependency) = field.strip_prefix("$contract_address:") { - let dependency_contract = - contracts_clone.iter().find(|c| c.diff.name == dependency).unwrap(); - let contract_address = get_contract_address( - generate_salt(&dependency_contract.diff.name), - dependency_contract.diff.base_class_hash, - &[], - world_address, - ); - *field = contract_address.to_string(); + let dependency_contract = self.metadata.get(dependency).unwrap(); + + match dependency_contract { + MigrationMetadata::Contract(c) => { + let contract_address = get_contract_address( + generate_salt(&c.name), + c.base_class_hash, + &[], + world_address, + ); + *field = contract_address.to_string(); + } + } } else if let Some(dependency) = field.strip_prefix("$class_hash:") { - let dependency_contract = - contracts_clone.iter().find(|c| c.diff.name == dependency).unwrap(); - *field = dependency_contract.diff.local_class_hash.to_string(); + let dependency_contract = self.metadata.get(dependency).unwrap(); + match dependency_contract { + MigrationMetadata::Contract(c) => { + *field = c.local_class_hash.to_string(); + } + } } } } @@ -94,6 +106,7 @@ pub fn prepare_for_migration( target_dir: &Utf8PathBuf, diff: WorldDiff, ) -> Result { + let mut metadata = HashMap::new(); let entries = fs::read_dir(target_dir).with_context(|| { format!( "Failed trying to read target directory ({target_dir})\nNOTE: build files are profile \ @@ -122,8 +135,12 @@ pub fn prepare_for_migration( // else we need to evaluate which contracts need to be migrated. let mut world = evaluate_contract_to_migrate(&diff.world, &artifact_paths, false)?; let base = evaluate_class_to_migrate(&diff.base, &artifact_paths, world.is_some())?; - let contracts = - evaluate_contracts_to_migrate(&diff.contracts, &artifact_paths, world.is_some())?; + let contracts = evaluate_contracts_to_migrate( + &diff.contracts, + &artifact_paths, + &mut metadata, + world.is_some(), + )?; let models = evaluate_models_to_migrate(&diff.models, &artifact_paths, world.is_some())?; // If world needs to be migrated, then we expect the `seed` to be provided. @@ -151,7 +168,7 @@ pub fn prepare_for_migration( world.contract_address = generated_world_address; } - Ok(MigrationStrategy { world_address, world, base, contracts, models }) + Ok(MigrationStrategy { world_address, world, base, contracts, models, metadata }) } fn evaluate_models_to_migrate( @@ -191,11 +208,13 @@ fn evaluate_class_to_migrate( fn evaluate_contracts_to_migrate( contracts: &[ContractDiff], artifact_paths: &HashMap, + metadata: &mut HashMap, world_contract_will_migrate: bool, ) -> Result> { let mut comps_to_migrate = vec![]; for c in contracts { + metadata.insert(c.name.clone(), MigrationMetadata::Contract(c.clone())); match c.remote_class_hash { Some(remote) if remote == c.local_class_hash && !world_contract_will_migrate => { continue; diff --git a/crates/sozo/ops/src/tests/migration.rs b/crates/sozo/ops/src/tests/migration.rs index 1c070dbd48..1cc6b0b3a8 100644 --- a/crates/sozo/ops/src/tests/migration.rs +++ b/crates/sozo/ops/src/tests/migration.rs @@ -12,7 +12,7 @@ use dojo_world::metadata::{ dojo_metadata_from_workspace, ArtifactMetadata, DojoMetadata, Uri, WorldMetadata, IPFS_CLIENT_URL, IPFS_PASSWORD, IPFS_USERNAME, }; -use dojo_world::migration::strategy::prepare_for_migration; +use dojo_world::migration::strategy::{prepare_for_migration, MigrationMetadata}; use dojo_world::migration::world::WorldDiff; use dojo_world::migration::TxnConfig; use futures::TryStreamExt; @@ -91,6 +91,113 @@ async fn migrate_with_small_fee_multiplier_will_fail() { ); } +#[tokio::test] +async fn metadata_calculated_properly() { + let config = setup::load_config(); + let ws = setup::setup_ws(&config); + + let base = config.manifest_path().parent().unwrap(); + let target_dir = format!("{}/target/dev", base); + + let profile_name = ws.current_profile().unwrap().to_string(); + + let mut manifest = BaseManifest::load_from_path( + &base.to_path_buf().join(MANIFESTS_DIR).join(profile_name).join(BASE_DIR), + ) + .unwrap(); + + let overlay_manifest = + OverlayManifest::load_from_path(&base.join(MANIFESTS_DIR).join("dev").join(OVERLAYS_DIR)) + .unwrap(); + + manifest.merge(overlay_manifest); + + let world = WorldDiff::compute(manifest, None); + + let migration = prepare_for_migration( + None, + felt!("0x12345"), + &Utf8Path::new(&target_dir).to_path_buf(), + world, + ) + .unwrap(); + + // verifies that key name and actual item name are same + for (key, value) in migration.metadata.iter() { + match value { + MigrationMetadata::Contract(c) => { + assert_eq!(key, &c.name); + } + } + } +} + +#[tokio::test] +async fn migration_with_correct_calldata_second_time_work_as_expected() { + let config = setup::load_config(); + let ws = setup::setup_ws(&config); + + let base = config.manifest_path().parent().unwrap(); + let target_dir = format!("{}/target/dev", base); + + let sequencer = KatanaRunner::new().expect("Failed to start runner."); + + let account = sequencer.account(0); + + let profile_name = ws.current_profile().unwrap().to_string(); + + let mut manifest = BaseManifest::load_from_path( + &base.to_path_buf().join(MANIFESTS_DIR).join(&profile_name).join(BASE_DIR), + ) + .unwrap(); + + let world = WorldDiff::compute(manifest.clone(), None); + + let migration = prepare_for_migration( + None, + felt!("0x12345"), + &Utf8Path::new(&target_dir).to_path_buf(), + world, + ) + .unwrap(); + + let migration_output = + execute_strategy(&ws, &migration, &account, TxnConfig::init_wait()).await.unwrap(); + + // first time others will fail due to calldata error + assert!(!migration_output.full); + + let world_address = migration_output.world_address; + + let remote_manifest = DeploymentManifest::load_from_remote(sequencer.provider(), world_address) + .await + .expect("Failed to load remote manifest"); + + let overlay = OverlayManifest::load_from_path( + &base.join(MANIFESTS_DIR).join(&profile_name).join(OVERLAYS_DIR), + ) + .expect("Failed to load overlay"); + + // adding correct calldata + manifest.merge(overlay); + + let mut world = WorldDiff::compute(manifest, Some(remote_manifest)); + world.update_order().expect("Failed to update order"); + + let mut migration = prepare_for_migration( + Some(world_address), + felt!("0x12345"), + &Utf8Path::new(&target_dir).to_path_buf(), + world, + ) + .unwrap(); + migration.resolve_variable(migration.world_address().unwrap()).expect("Failed to resolve"); + + let migration_output = + execute_strategy(&ws, &migration, &account, TxnConfig::init_wait()).await.unwrap(); + assert!(migration_output.full); +} + #[tokio::test] async fn migration_from_remote() { let config = setup::load_config(); diff --git a/crates/torii/types-test/Scarb.lock b/crates/torii/types-test/Scarb.lock index 0babbac9af..bf6a5a2123 100644 --- a/crates/torii/types-test/Scarb.lock +++ b/crates/torii/types-test/Scarb.lock @@ -15,7 +15,7 @@ source = "git+https://github.com/dojoengine/dojo?tag=v0.3.11#1e651b5d4d3b79b14a7 [[package]] name = "types_test" -version = "0.6.0" +version = "0.7.0" dependencies = [ "dojo", ]