From cf0056a2f58d3c8963740246b6071c617dc0830b Mon Sep 17 00:00:00 2001 From: Rijnard van Tonder Date: Sun, 26 May 2024 19:37:34 -0400 Subject: [PATCH] move: automated address mgmt for pkg upgrades --- Cargo.lock | 2 + .../unit_tests/move_package_publish_tests.rs | 10 +- crates/sui-move-build/Cargo.toml | 1 + crates/sui-move-build/src/lib.rs | 19 ++- crates/sui-package-management/Cargo.toml | 1 + crates/sui-package-management/src/lib.rs | 86 +++++++++- crates/sui/src/client_commands.rs | 31 +++- crates/sui/src/client_ptb/builder.rs | 1 + crates/sui/tests/cli_tests.rs | 151 ++++++++++++++---- 9 files changed, 253 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 051cc5dea6af2..a17fc474e40d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13021,6 +13021,7 @@ dependencies = [ "move-package", "move-symbol-pool", "serde-reflection", + "sui-package-management", "sui-protocol-config", "sui-types", "sui-verifier-latest", @@ -13267,6 +13268,7 @@ version = "1.27.0" dependencies = [ "anyhow", "move-package", + "move-symbol-pool", "sui-json-rpc-types", "sui-sdk", "tracing", diff --git a/crates/sui-core/src/unit_tests/move_package_publish_tests.rs b/crates/sui-core/src/unit_tests/move_package_publish_tests.rs index b397e90d9b421..b1ebdd5e4843b 100644 --- a/crates/sui-core/src/unit_tests/move_package_publish_tests.rs +++ b/crates/sui-core/src/unit_tests/move_package_publish_tests.rs @@ -301,11 +301,11 @@ async fn test_custom_property_check_unpublished_dependencies() { .resolution_graph_for_package(&path, &mut std::io::sink()) .expect("Could not build resolution graph."); - let SuiError::ModulePublishFailure { error } = - check_unpublished_dependencies(&gather_published_ids(&resolution_graph).1.unpublished) - .err() - .unwrap() - else { + let SuiError::ModulePublishFailure { error } = check_unpublished_dependencies( + &gather_published_ids(&resolution_graph, None).1.unpublished, + ) + .err() + .unwrap() else { panic!("Expected ModulePublishFailure") }; diff --git a/crates/sui-move-build/Cargo.toml b/crates/sui-move-build/Cargo.toml index 4896cf3c2164f..c58e3ea6e2a0e 100644 --- a/crates/sui-move-build/Cargo.toml +++ b/crates/sui-move-build/Cargo.toml @@ -18,6 +18,7 @@ sui-verifier = { path = "../../sui-execution/latest/sui-verifier", package = "su serde-reflection.workspace = true sui-types.workspace = true sui-protocol-config.workspace = true +sui-package-management.workspace = true move-binary-format.workspace = true move-bytecode-utils.workspace = true diff --git a/crates/sui-move-build/src/lib.rs b/crates/sui-move-build/src/lib.rs index 3fc87e9750adc..4304d46b7c913 100644 --- a/crates/sui-move-build/src/lib.rs +++ b/crates/sui-move-build/src/lib.rs @@ -40,6 +40,7 @@ use move_package::{ }; use move_symbol_pool::Symbol; use serde_reflection::Registry; +use sui_package_management::{resolve_published_id, PublishedAtError}; use sui_protocol_config::{Chain, ProtocolConfig, ProtocolVersion}; use sui_types::{ base_types::ObjectID, @@ -223,7 +224,7 @@ pub fn build_from_resolution_graph( run_bytecode_verifier: bool, print_diags_to_stderr: bool, ) -> SuiResult { - let (published_at, dependency_ids) = gather_published_ids(&resolution_graph); + let (published_at, dependency_ids) = gather_published_ids(&resolution_graph, None); let result = if print_diags_to_stderr { BuildConfig::compile_package(resolution_graph, &mut std::io::stderr()) @@ -634,12 +635,8 @@ pub struct PackageDependencies { pub unpublished: BTreeSet, /// Set of dependencies with invalid `published-at` addresses. pub invalid: BTreeMap, -} - -#[derive(Debug, Clone)] -pub enum PublishedAtError { - Invalid(String), - NotPresent, + /// Set of dependencies that have conflicting `published-at` addresses in Move.lock and Move.toml. + pub conflicting: BTreeMap, } /// Partition packages in `resolution_graph` into one of four groups: @@ -649,16 +646,18 @@ pub enum PublishedAtError { /// - The names of packages that have a `published-at` field that isn't filled with a valid address. pub fn gather_published_ids( resolution_graph: &ResolvedGraph, + chain_id: Option, ) -> (Result, PackageDependencies) { let root = resolution_graph.root_package(); let mut published = BTreeMap::new(); let mut unpublished = BTreeSet::new(); let mut invalid = BTreeMap::new(); + let mut conflicting = BTreeMap::new(); let mut published_at = Err(PublishedAtError::NotPresent); for (name, package) in &resolution_graph.package_table { - let property = published_at_property(package); + let property = resolve_published_id(package, chain_id.clone()); if name == &root { // Separate out the root package as a special case published_at = property; @@ -675,6 +674,9 @@ pub fn gather_published_ids( Err(PublishedAtError::Invalid(value)) => { invalid.insert(*name, value); } + Err(PublishedAtError::Conflict(id_lock, id_manifest)) => { + conflicting.insert(*name, (id_lock, id_manifest)); + } }; } @@ -684,6 +686,7 @@ pub fn gather_published_ids( published, unpublished, invalid, + conflicting, }, ) } diff --git a/crates/sui-package-management/Cargo.toml b/crates/sui-package-management/Cargo.toml index 853d587138340..e1b34d8ba3308 100644 --- a/crates/sui-package-management/Cargo.toml +++ b/crates/sui-package-management/Cargo.toml @@ -17,3 +17,4 @@ sui-json-rpc-types.workspace = true sui-sdk.workspace = true move-package.workspace = true +move-symbol-pool.workspace = true diff --git a/crates/sui-package-management/src/lib.rs b/crates/sui-package-management/src/lib.rs index e36e392deaa85..d7b2269625e76 100644 --- a/crates/sui-package-management/src/lib.rs +++ b/crates/sui-package-management/src/lib.rs @@ -3,16 +3,31 @@ use anyhow::{bail, Context}; use std::path::PathBuf; +use std::str::FromStr; -use move_package::lock_file::{self, LockFile}; +use move_package::{ + lock_file::{self, schema::ManagedPackage, LockFile}, + resolution::resolution_graph::Package, + source_package::layout::SourcePackageLayout, +}; +use move_symbol_pool::Symbol; use sui_json_rpc_types::{get_new_package_obj_from_response, SuiTransactionBlockResponse}; -use sui_sdk::wallet_context::WalletContext; +use sui_sdk::{types::base_types::ObjectID, wallet_context::WalletContext}; + +const PUBLISHED_AT_MANIFEST_FIELD: &str = "published-at"; pub enum LockCommand { Publish, Upgrade, } +#[derive(Debug, Clone)] +pub enum PublishedAtError { + Invalid(String), + Conflict(String, String), + NotPresent, +} + /// Update the `Move.lock` file with automated address management info. /// Expects a wallet context, the publish or upgrade command, its response. /// The `Move.lock` principally file records the published address (i.e., package ID) of @@ -73,3 +88,70 @@ pub async fn update_lock_file( lock.commit(lock_file)?; Ok(()) } + +/// Find the published on-chain ID in the `Move.lock` or `Move.toml` file. +/// A chain ID of `None` means that we will only try to resolve a published ID from the Move.toml. +/// The published ID is resolved from the `Move.toml` if the Move.lock does not exist. +/// Else, we resolve from the `Move.lock`, where addresses are automatically +/// managed. If conflicting IDs are found in the `Move.lock` vs. `Move.toml`, a +/// "Conflict" error message returns. +pub fn resolve_published_id( + package: &Package, + chain_id: Option, +) -> Result { + // Look up a valid `published-at` in the `Move.toml` first, which we'll + // return if the Move.lock does not manage addresses. + let published_id_in_manifest = match published_at_property(package) { + Ok(v) => Some(v), + Err(PublishedAtError::NotPresent) => None, + Err(e) => return Err(e), // An existing but invalid `published-at` in `Move.toml` should fail early. + }; + + let lock = package.package_path.join(SourcePackageLayout::Lock.path()); + let mut lock_file = match std::fs::File::open(lock.clone()) { + Ok(f) => f, + Err(_) => match published_id_in_manifest { + Some(v) => { + return ObjectID::from_str(v.as_str()) + .map_err(|_| PublishedAtError::Invalid(v.to_owned())) + } + None => return Err(PublishedAtError::NotPresent), + }, + }; + let managed_packages = ManagedPackage::read(&mut lock_file).ok(); + // Find the environment and ManagedPackage data for this chain_id. + let env_for_chain_id = managed_packages + .and_then(|m| { + m.into_iter().find(|(_, v)| { + if let Some(chain_id) = &chain_id { + v.chain_id == *chain_id + } else { + false + } + }) + }) + .map(|(k, v)| (k, v.latest_published_id)); + + let package_id = match (env_for_chain_id, published_id_in_manifest) { + (Some((_env, id_lock)), Some(id_manifest)) if id_lock != id_manifest => { + return Err(PublishedAtError::Conflict(id_lock, id_manifest)) + } + (Some((_, id_lock)), _) => id_lock, + (None, Some(id_manifest)) => id_manifest, /* No info in Move.lock: Fall back to manifest */ + _ => return Err(PublishedAtError::NotPresent), /* Neither in Move.toml nor Move.lock */ + }; + ObjectID::from_str(package_id.as_str()) + .map_err(|_| PublishedAtError::Invalid(package_id.to_owned())) +} + +fn published_at_property(package: &Package) -> Result { + let Some(value) = package + .source_package + .package + .custom_properties + .get(&Symbol::from(PUBLISHED_AT_MANIFEST_FIELD)) + else { + return Err(PublishedAtError::NotPresent); + }; + Ok(value.to_string()) +} diff --git a/crates/sui/src/client_commands.rs b/crates/sui/src/client_commands.rs index 1ded42100f6c2..d0a2528e96e14 100644 --- a/crates/sui/src/client_commands.rs +++ b/crates/sui/src/client_commands.rs @@ -47,9 +47,9 @@ use sui_json_rpc_types::{ use sui_keys::keystore::AccountKeystore; use sui_move_build::{ build_from_resolution_graph, check_invalid_dependencies, check_unpublished_dependencies, - gather_published_ids, BuildConfig, CompiledPackage, PackageDependencies, PublishedAtError, + gather_published_ids, BuildConfig, CompiledPackage, PackageDependencies, }; -use sui_package_management::LockCommand; +use sui_package_management::{LockCommand, PublishedAtError}; use sui_replay::ReplayToolCommand; use sui_sdk::{ apis::ReadApi, @@ -864,6 +864,11 @@ impl SuiClientCommands { .map_err(|e| SuiError::ModulePublishFailure { error: format!("Failed to canonicalize package path: {}", e), })?; + let env_alias = context + .config + .get_active_env() + .map(|e| e.alias.clone()) + .ok(); let (package_id, compiled_modules, dependencies, package_digest, upgrade_policy) = upgrade_package( client.read_api(), @@ -872,6 +877,7 @@ impl SuiClientCommands { upgrade_capability, with_unpublished_dependencies, skip_dependency_verification, + env_alias, ) .await?; let tx_kind = client @@ -1622,6 +1628,7 @@ pub(crate) async fn upgrade_package( upgrade_capability: ObjectID, with_unpublished_dependencies: bool, skip_dependency_verification: bool, + env_alias: Option, ) -> Result<(ObjectID, Vec>, PackageDependencies, [u8; 32], u8), anyhow::Error> { let (dependencies, compiled_modules, compiled_package, package_id) = compile_package( read_api, @@ -1634,12 +1641,25 @@ pub(crate) async fn upgrade_package( let package_id = package_id.map_err(|e| match e { PublishedAtError::NotPresent => { - anyhow!("No 'published-at' field in manifest for package to be upgraded.") + anyhow!("No 'published-at' field in Move.toml or Move.lock for package to be upgraded.") } PublishedAtError::Invalid(v) => anyhow!( - "Invalid 'published-at' field in manifest of package to be upgraded. \ + "Invalid 'published-at' field in Move.toml or Move.lock of package to be upgraded. \ Expected an on-chain address, but found: {v:?}" ), + PublishedAtError::Conflict(id_lock, id_manifest) => { + let env_alias = format!("(currently {})", env_alias.unwrap_or_default()); + anyhow!( + "Conflicting published package address: `Move.toml` contains published-at address \ + {id_manifest} but `Move.lock` file contains published-at address {id_lock}. \ + You may want to: + + - delete the published-at address in the `Move.toml` if the `Move.lock` address is correct; OR + - update the `Move.lock` address using the `sui manage-package` command to be the same as the `Move.toml`; OR + - check that your `sui active-env` {env_alias} corresponds to the chain on which the package is published (i.e., devnet, testnet, mainnet); OR + - contact the maintainer if this package is a dependency and request resolving the conflict." + ) + } })?; let resp = read_api @@ -1700,7 +1720,8 @@ pub(crate) async fn compile_package( print_diags_to_stderr, }; let resolution_graph = config.resolution_graph(&package_path)?; - let (package_id, dependencies) = gather_published_ids(&resolution_graph); + let chain_id = read_api.get_chain_identifier().await.ok(); + let (package_id, dependencies) = gather_published_ids(&resolution_graph, chain_id); check_invalid_dependencies(&dependencies.invalid)?; if !with_unpublished_dependencies { check_unpublished_dependencies(&dependencies.unpublished)?; diff --git a/crates/sui/src/client_ptb/builder.rs b/crates/sui/src/client_ptb/builder.rs index 9fd53c877dd8d..cb3c31298eb7a 100644 --- a/crates/sui/src/client_ptb/builder.rs +++ b/crates/sui/src/client_ptb/builder.rs @@ -974,6 +974,7 @@ impl<'a> PTBBuilder<'a> { ObjectID::from_address(upgrade_cap_id.into_inner()), false, /* with_unpublished_dependencies */ false, /* skip_dependency_verification */ + None, ) .await .map_err(|e| err!(path_loc, "{e}"))?; diff --git a/crates/sui/tests/cli_tests.rs b/crates/sui/tests/cli_tests.rs index c773db7a47921..56e3b845a21ca 100644 --- a/crates/sui/tests/cli_tests.rs +++ b/crates/sui/tests/cli_tests.rs @@ -1849,20 +1849,12 @@ async fn test_package_management_on_upgrade_command() -> Result<(), anyhow::Erro assert!(effects.status.is_ok()); assert_eq!(effects.gas_object().object_id(), gas_obj_id); - let package = effects - .created() - .iter() - .find(|refe| matches!(refe.owner, Owner::Immutable)) - .unwrap(); - let cap = effects .created() .iter() .find(|refe| matches!(refe.owner, Owner::AddressOwner(_))) .unwrap(); - // Hacky for now: we need to add the correct `published-at` field to the Move toml file. - // In the future once we have automated address management replace this logic! let tmp_dir = tempfile::tempdir().unwrap(); fs_extra::dir::copy( &package_path, @@ -1872,29 +1864,8 @@ async fn test_package_management_on_upgrade_command() -> Result<(), anyhow::Erro .unwrap(); let mut upgrade_pkg_path = tmp_dir.path().to_path_buf(); upgrade_pkg_path.extend(["dummy_modules_upgrade", "Move.toml"]); - let mut move_toml = std::fs::File::options() - .read(true) - .write(true) - .open(&upgrade_pkg_path) - .unwrap(); upgrade_pkg_path.pop(); - let mut buf = String::new(); - move_toml.read_to_string(&mut buf).unwrap(); - - // Add a `published-at = "0x"` to the Move manifest. - let mut lines: Vec = buf.split('\n').map(|x| x.to_string()).collect(); - let idx = lines.iter().position(|s| s == "[package]").unwrap(); - lines.insert( - idx + 1, - format!( - "published-at = \"{}\"", - package.reference.object_id.to_hex_uncompressed() - ), - ); - let new = lines.join("\n"); - move_toml.write_at(new.as_bytes(), 0).unwrap(); - // Create a new build config for the upgrade. Initialize its lock file // to the package we published. let build_config_upgrade = BuildConfig::new_for_testing().config; @@ -1960,6 +1931,128 @@ async fn test_package_management_on_upgrade_command() -> Result<(), anyhow::Erro Ok(()) } +#[sim_test] +async fn test_package_management_on_upgrade_command_conflict() -> Result<(), anyhow::Error> { + move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks)); + let mut test_cluster = TestClusterBuilder::new().build().await; + let rgp = test_cluster.get_reference_gas_price().await; + let address = test_cluster.get_address_0(); + let context = &mut test_cluster.wallet; + let client = context.get_client().await?; + let object_refs = client + .read_api() + .get_owned_objects( + address, + Some(SuiObjectResponseQuery::new_with_options( + SuiObjectDataOptions::new() + .with_type() + .with_owner() + .with_previous_transaction(), + )), + None, + None, + ) + .await? + .data; + + // Check log output contains all object ids. + let gas_obj_id = object_refs.first().unwrap().object().unwrap().object_id; + + // Provide path to well formed package sources + let mut package_path = PathBuf::from(TEST_DATA_DIR); + package_path.push("dummy_modules_upgrade"); + let build_config_publish = BuildConfig::new_for_testing().config; + let resp = SuiClientCommands::Publish { + package_path: package_path.clone(), + build_config: build_config_publish.clone(), + opts: OptsWithGas::for_testing(Some(gas_obj_id), rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH), + skip_dependency_verification: false, + with_unpublished_dependencies: false, + } + .execute(context) + .await?; + + let SuiClientCommandResult::TransactionBlock(publish_response) = resp else { + unreachable!("Invalid response"); + }; + + let SuiTransactionBlockEffects::V1(effects) = publish_response.clone().effects.unwrap(); + + assert!(effects.status.is_ok()); + assert_eq!(effects.gas_object().object_id(), gas_obj_id); + let package = effects + .created() + .iter() + .find(|refe| matches!(refe.owner, Owner::Immutable)) + .unwrap(); + + let cap = effects + .created() + .iter() + .find(|refe| matches!(refe.owner, Owner::AddressOwner(_))) + .unwrap(); + + // Set up a temporary working directory for upgrading. + let tmp_dir = tempfile::tempdir().unwrap(); + fs_extra::dir::copy( + &package_path, + tmp_dir.path(), + &fs_extra::dir::CopyOptions::default(), + ) + .unwrap(); + let mut upgrade_pkg_path = tmp_dir.path().to_path_buf(); + upgrade_pkg_path.extend(["dummy_modules_upgrade", "Move.toml"]); + let mut move_toml = std::fs::File::options() + .read(true) + .write(true) + .open(&upgrade_pkg_path) + .unwrap(); + upgrade_pkg_path.pop(); + let mut buf = String::new(); + move_toml.read_to_string(&mut buf).unwrap(); + let mut lines: Vec = buf.split('\n').map(|x| x.to_string()).collect(); + let idx = lines.iter().position(|s| s == "[package]").unwrap(); + // Purposely add a conflicting `published-at` address to the Move manifest. + lines.insert(idx + 1, "published-at = \"0xbad\"".to_string()); + let new = lines.join("\n"); + move_toml.write_at(new.as_bytes(), 0).unwrap(); + + // Create a new build config for the upgrade. Initialize its lock file to the package we published. + let build_config_upgrade = BuildConfig::new_for_testing().config; + let mut upgrade_lock_file_path = upgrade_pkg_path.clone(); + upgrade_lock_file_path.push("Move.lock"); + let publish_lock_file_path = build_config_publish.lock_file.unwrap(); + std::fs::copy( + publish_lock_file_path.clone(), + upgrade_lock_file_path.clone(), + )?; + + // Now run the upgrade + let upgrade_response = SuiClientCommands::Upgrade { + package_path: upgrade_pkg_path, + upgrade_capability: cap.reference.object_id, + build_config: build_config_upgrade.clone(), + opts: OptsWithGas::for_testing(Some(gas_obj_id), rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH), + skip_dependency_verification: false, + with_unpublished_dependencies: false, + } + .execute(context) + .await; + + let err_string = upgrade_response.unwrap_err().to_string(); + let err_string = err_string.replace(&package.object_id().to_string(), ""); + + let expect = expect![[r#" + Conflicting published package address: `Move.toml` contains published-at address 0xbad but `Move.lock` file contains published-at address . You may want to: + + - delete the published-at address in the `Move.toml` if the `Move.lock` address is correct; OR + - update the `Move.lock` address using the `sui manage-package` command to be the same as the `Move.toml`; OR + - check that your `sui active-env` (currently localnet) corresponds to the chain on which the package is published (i.e., devnet, testnet, mainnet); OR + - contact the maintainer if this package is a dependency and request resolving the conflict."#]]; + expect.assert_eq(&err_string); + Ok(()) +} + #[sim_test] async fn test_native_transfer() -> Result<(), anyhow::Error> { let mut test_cluster = TestClusterBuilder::new().build().await;