diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/Move.toml b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/Move.toml deleted file mode 100644 index f011a1a17e686..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/Move.toml +++ /dev/null @@ -1,10 +0,0 @@ -[package] -name = "package_upgrade_base" -version = "0.0.1" -edition = "2024.beta" - -[addresses] -base_addr = "0x0" - -[dependencies] -Sui = { local = "../../../../../../sui-framework/packages/sui-framework" } diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/base.move b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/base.move deleted file mode 100644 index 72f006e60cff4..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/base.move +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) Mysten Labs, Inc. -// SPDX-License-Identifier: Apache-2.0 - -module base_addr::base { - - public struct A { - f1: bool, - f2: T - } - - public fun return_0(): u64 { abort 42 } - - public fun plus_1(x: u64): u64 { x + 1 } - - public(package) fun friend_fun(x: u64): u64 { x } - - fun non_public_fun(y: bool): u64 { if (y) 0 else 1 } - - entry fun entry_fun() { } -} diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/friend_module.move b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/friend_module.move deleted file mode 100644 index 474ad93c69325..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/friend_module.move +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright (c) Mysten Labs, Inc. -// SPDX-License-Identifier: Apache-2.0 - -module base_addr::friend_module { - - public struct A { - field1: u64, - field2: T - } - - public fun friend_call(): u64 { base_addr::base::friend_fun(1) } - - public fun return_0(): u64 { 0 } - - public fun plus_1(x: u64): u64 { x + 1 } - - fun non_public_fun(y: bool): u64 { if (y) 0 else 1 } -} diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/new_friend_module.move b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/new_friend_module.move deleted file mode 100644 index 04d2694736316..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_friend_module/sources/new_friend_module.move +++ /dev/null @@ -1,6 +0,0 @@ -// Copyright (c) Mysten Labs, Inc. -// SPDX-License-Identifier: Apache-2.0 - -module base_addr::new_friend_module; - -public fun friend_call(): u64 { base_addr::base::friend_fun(1) } diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/Move.toml b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/Move.toml deleted file mode 100644 index f011a1a17e686..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/Move.toml +++ /dev/null @@ -1,10 +0,0 @@ -[package] -name = "package_upgrade_base" -version = "0.0.1" -edition = "2024.beta" - -[addresses] -base_addr = "0x0" - -[dependencies] -Sui = { local = "../../../../../../sui-framework/packages/sui-framework" } diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/base.move b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/base.move deleted file mode 100644 index 72f006e60cff4..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/base.move +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) Mysten Labs, Inc. -// SPDX-License-Identifier: Apache-2.0 - -module base_addr::base { - - public struct A { - f1: bool, - f2: T - } - - public fun return_0(): u64 { abort 42 } - - public fun plus_1(x: u64): u64 { x + 1 } - - public(package) fun friend_fun(x: u64): u64 { x } - - fun non_public_fun(y: bool): u64 { if (y) 0 else 1 } - - entry fun entry_fun() { } -} diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/friend_module.move b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/friend_module.move deleted file mode 100644 index 474ad93c69325..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/friend_module.move +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright (c) Mysten Labs, Inc. -// SPDX-License-Identifier: Apache-2.0 - -module base_addr::friend_module { - - public struct A { - field1: u64, - field2: T - } - - public fun friend_call(): u64 { base_addr::base::friend_fun(1) } - - public fun return_0(): u64 { 0 } - - public fun plus_1(x: u64): u64 { x + 1 } - - fun non_public_fun(y: bool): u64 { if (y) 0 else 1 } -} diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/new_module.move b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/new_module.move deleted file mode 100644 index 2a96fd527cd2a..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_new_module/sources/new_module.move +++ /dev/null @@ -1,4 +0,0 @@ -// Copyright (c) Mysten Labs, Inc. -// SPDX-License-Identifier: Apache-2.0 - -module base_addr::new_module; diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_remove_module/Move.toml b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_remove_module/Move.toml deleted file mode 100644 index f011a1a17e686..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_remove_module/Move.toml +++ /dev/null @@ -1,10 +0,0 @@ -[package] -name = "package_upgrade_base" -version = "0.0.1" -edition = "2024.beta" - -[addresses] -base_addr = "0x0" - -[dependencies] -Sui = { local = "../../../../../../sui-framework/packages/sui-framework" } diff --git a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_remove_module/sources/base.move b/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_remove_module/sources/base.move deleted file mode 100644 index 72f006e60cff4..0000000000000 --- a/crates/sui-core/src/unit_tests/data/move_upgrade/dep_only_upgrade_remove_module/sources/base.move +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) Mysten Labs, Inc. -// SPDX-License-Identifier: Apache-2.0 - -module base_addr::base { - - public struct A { - f1: bool, - f2: T - } - - public fun return_0(): u64 { abort 42 } - - public fun plus_1(x: u64): u64 { x + 1 } - - public(package) fun friend_fun(x: u64): u64 { x } - - fun non_public_fun(y: bool): u64 { if (y) 0 else 1 } - - entry fun entry_fun() { } -} diff --git a/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs b/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs index 4efa2d45e44b2..3e265224a9388 100644 --- a/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs +++ b/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs @@ -15,7 +15,12 @@ use sui_types::{ MOVE_STDLIB_PACKAGE_ID, SUI_FRAMEWORK_PACKAGE_ID, }; -use std::{collections::BTreeSet, path::PathBuf, str::FromStr, sync::Arc}; +use std::{ + collections::BTreeSet, + path::{Path, PathBuf}, + str::FromStr, + sync::Arc, +}; use sui_types::effects::{TransactionEffects, TransactionEffectsAPI}; use sui_types::error::{SuiError, UserInputError}; use sui_types::execution_config_utils::to_binary_config; @@ -48,11 +53,62 @@ macro_rules! move_call { } } +enum FileOverlay<'a> { + Remove(&'a str), + Add { + file_name: &'a str, + contents: &'a str, + }, +} + +fn build_upgrade_test_modules_with_overlay( + base_pkg: &str, + overlay: FileOverlay<'_>, +) -> (Vec, Vec>) { + // Root temp dirs under `move_upgrade` directory so that dependency paths remain correct. + let mut tmp_dir_root_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + tmp_dir_root_path.extend(["src", "unit_tests", "data", "move_upgrade"]); + + let tmp_dir = tempfile::TempDir::new_in(tmp_dir_root_path).unwrap(); + let tmp_dir_path = tmp_dir.path(); + + let mut copy_options = fs_extra::dir::CopyOptions::new(); + copy_options.copy_inside = true; + copy_options.content_only = true; + let source_dir = pkg_path_of(base_pkg); + fs_extra::dir::copy(source_dir, tmp_dir_path, ©_options).unwrap(); + + match overlay { + FileOverlay::Remove(file_name) => { + let file_path = tmp_dir_path.join(format!("sources/{}", file_name)); + std::fs::remove_file(file_path).unwrap(); + } + FileOverlay::Add { + file_name, + contents, + } => { + let new_file_path = tmp_dir_path.join(format!("sources/{}", file_name)); + std::fs::write(new_file_path, contents).unwrap(); + } + } + + build_pkg_at_path(tmp_dir_path) +} + fn build_upgrade_test_modules(test_dir: &str) -> (Vec, Vec>) { + let path = pkg_path_of(test_dir); + build_pkg_at_path(&path) +} + +fn pkg_path_of(pkg_name: &str) -> PathBuf { let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - path.extend(["src", "unit_tests", "data", "move_upgrade", test_dir]); + path.extend(["src", "unit_tests", "data", "move_upgrade", pkg_name]); + path +} + +fn build_pkg_at_path(path: &Path) -> (Vec, Vec>) { let with_unpublished_deps = false; - let package = BuildConfig::new_for_testing().build(&path).unwrap(); + let package = BuildConfig::new_for_testing().build(path).unwrap(); ( package.get_package_digest(with_unpublished_deps).to_vec(), package.get_package_bytes(with_unpublished_deps), @@ -466,8 +522,15 @@ async fn test_upgrade_package_add_new_module_in_dep_only_mode_pre_v68() { }); let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; - - let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_new_module"); + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let (digest, modules) = build_upgrade_test_modules_with_overlay( + base_pkg, + FileOverlay::Add { + file_name: "new_module.move", + contents: "module base_addr::new_module;", + }, + ); let effects = runner .upgrade( UpgradePolicy::DEP_ONLY, @@ -483,8 +546,15 @@ async fn test_upgrade_package_add_new_module_in_dep_only_mode_pre_v68() { #[tokio::test] async fn test_upgrade_package_add_new_module_in_dep_only_mode() { let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; - - let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_new_module"); + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let (digest, modules) = build_upgrade_test_modules_with_overlay( + base_pkg, + FileOverlay::Add { + file_name: "new_module.move", + contents: "module base_addr::new_module;", + }, + ); let effects = runner .upgrade( UpgradePolicy::DEP_ONLY, @@ -510,8 +580,18 @@ async fn test_upgrade_package_add_new_friend_module_in_dep_only_mode_pre_v68() { }); let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; - - let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_new_friend_module"); + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let (digest, modules) = build_upgrade_test_modules_with_overlay( + base_pkg, + FileOverlay::Add { + file_name: "new_friend_module.move", + contents: r#" +module base_addr::new_friend_module; +public fun friend_call(): u64 { base_addr::base::friend_fun(1) } + "#, + }, + ); let effects = runner .upgrade( UpgradePolicy::DEP_ONLY, @@ -532,8 +612,19 @@ async fn test_upgrade_package_add_new_friend_module_in_dep_only_mode_pre_v68() { #[tokio::test] async fn test_upgrade_package_add_new_friend_module_in_dep_only_mode() { let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let (digest, modules) = build_upgrade_test_modules_with_overlay( + base_pkg, + FileOverlay::Add { + file_name: "new_friend_module.move", + contents: r#" +module base_addr::new_friend_module; +public fun friend_call(): u64 { base_addr::base::friend_fun(1) } + "#, + }, + ); - let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_new_friend_module"); let effects = runner .upgrade( UpgradePolicy::DEP_ONLY, @@ -554,8 +645,12 @@ async fn test_upgrade_package_add_new_friend_module_in_dep_only_mode() { #[tokio::test] async fn test_upgrade_package_remove_module_in_dep_only_mode() { let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; - - let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_remove_module"); + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let (digest, modules) = build_upgrade_test_modules_with_overlay( + base_pkg, + FileOverlay::Remove("friend_module.move"), + ); let effects = runner .upgrade( UpgradePolicy::DEP_ONLY, @@ -688,18 +783,7 @@ async fn test_upgrade_package_additive_dep_only_mode() { #[tokio::test] async fn test_upgrade_package_dep_only_mode() { let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; - - let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade"); - let effects = runner - .upgrade( - UpgradePolicy::DEP_ONLY, - digest, - modules, - vec![SUI_FRAMEWORK_PACKAGE_ID, MOVE_STDLIB_PACKAGE_ID], - ) - .await; - - assert!(effects.status().is_ok(), "{:#?}", effects.status()); + assert_valid_dep_only_upgrade(&mut runner, "dep_only_upgrade").await; } #[tokio::test] @@ -1548,3 +1632,17 @@ async fn test_upgrade_more_than_max_packages_error() { } ); } + +async fn assert_valid_dep_only_upgrade(runner: &mut UpgradeStateRunner, package_name: &str) { + let (digest, modules) = build_upgrade_test_modules(package_name); + let effects = runner + .upgrade( + UpgradePolicy::DEP_ONLY, + digest, + modules, + vec![SUI_FRAMEWORK_PACKAGE_ID, MOVE_STDLIB_PACKAGE_ID], + ) + .await; + + assert!(effects.status().is_ok(), "{:#?}", effects.status()); +} diff --git a/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs b/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs index 7f8d9b9f67b6e..2b72db7222021 100644 --- a/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs +++ b/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs @@ -640,7 +640,7 @@ mod checked { )]) } - fn check_compatibility<'a>( + fn check_compatibility( context: &ExecutionContext, existing_package: &MovePackage, upgrading_modules: &[CompiledModule], @@ -662,12 +662,12 @@ mod checked { let existing_modules_len = current_normalized.len(); let upgrading_modules_len = upgrading_modules.len(); - if context + let disallow_new_modules = context .protocol_config .disallow_new_modules_in_deps_only_packages() - && policy as u8 == UpgradePolicy::DEP_ONLY - && existing_modules_len != upgrading_modules_len - { + && policy as u8 == UpgradePolicy::DEP_ONLY; + + if disallow_new_modules && existing_modules_len != upgrading_modules_len { return Err(ExecutionError::new_with_source( ExecutionErrorKind::PackageUpgradeError { upgrade_error: PackageUpgradeError::IncompatibleUpgrade, @@ -679,7 +679,7 @@ mod checked { )); } - let mut new_normalized = normalize_deserialized_modules(upgrading_modules.into_iter()); + let mut new_normalized = normalize_deserialized_modules(upgrading_modules.iter()); for (name, cur_module) in current_normalized { let Some(new_module) = new_normalized.remove(&name) else { return Err(ExecutionError::new_with_source( @@ -693,6 +693,9 @@ mod checked { check_module_compatibility(&policy, &cur_module, &new_module)?; } + // If we disallow new modules double check that there are no modules left in `new_normalized`. + debug_assert!(!disallow_new_modules || new_normalized.is_empty()); + Ok(()) }