From 89e3602c099520859eb3aeab6378866ec361cc4b Mon Sep 17 00:00:00 2001 From: stefan-mysten <135084671+stefan-mysten@users.noreply.github.com> Date: Thu, 27 Feb 2025 11:22:22 -0800 Subject: [PATCH] Rework errors around fetching move packages --- Cargo.lock | 1 + crates/sui/src/client_commands.rs | 97 +++++++++++---------- crates/sui/src/client_ptb/builder.rs | 22 +++-- crates/sui/src/client_ptb/ptb.rs | 2 +- crates/sui/src/sui_commands.rs | 5 +- crates/sui/src/upgrade_compatibility/mod.rs | 7 +- crates/sui/tests/cli_tests.rs | 2 +- 7 files changed, 69 insertions(+), 67 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 13c1a57734317..40867ab733f92 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12424,6 +12424,7 @@ dependencies = [ "move-core-types", "move-ir-types", "move-package", + "move-symbol-pool", "move-vm-config", "move-vm-profiler", "msim", diff --git a/crates/sui/src/client_commands.rs b/crates/sui/src/client_commands.rs index 0061668efb45a..7a883de745618 100644 --- a/crates/sui/src/client_commands.rs +++ b/crates/sui/src/client_commands.rs @@ -10,7 +10,7 @@ use crate::{ verifier_meter::{AccumulatingMeter, Accumulator}, }; use std::{ - collections::{btree_map::Entry, BTreeMap}, + collections::{btree_map::Entry, BTreeMap, BTreeSet}, fmt::{Debug, Display, Formatter, Write}, fs, path::{Path, PathBuf}, @@ -67,7 +67,7 @@ use sui_types::{ base_types::{ObjectID, SequenceNumber, SuiAddress}, crypto::{EmptySignInfo, SignatureScheme}, digests::TransactionDigest, - error::SuiError, + error::{SuiError, SuiObjectResponseError}, gas::GasCostSummary, gas_coin::GasCoin, message_envelope::Envelope, @@ -891,12 +891,9 @@ impl SuiClientCommands { let sender = context.try_get_object_owner(&opts.gas).await?; let sender = sender.unwrap_or(context.active_address()?); let client = context.get_client().await?; - let chain_id = client.read_api().get_chain_identifier().await.ok(); - let protocol_version = client - .read_api() - .get_protocol_config(None) - .await? - .protocol_version; + let read_api = client.read_api(); + let chain_id = read_api.get_chain_identifier().await.ok(); + let protocol_version = read_api.get_protocol_config(None).await?.protocol_version; let protocol_config = ProtocolConfig::get_for_version( protocol_version, match chain_id @@ -908,7 +905,7 @@ impl SuiClientCommands { }, ); - check_protocol_version_and_warn(&client).await?; + check_protocol_version_and_warn(read_api).await?; let package_path = package_path .canonicalize() @@ -935,7 +932,7 @@ impl SuiClientCommands { check_dep_verification_flags(skip_dependency_verification, verify_deps)?; let upgrade_result = upgrade_package( - &client, + read_api, build_config.clone(), &package_path, upgrade_capability, @@ -967,7 +964,7 @@ impl SuiClientCommands { if verify_compatibility { check_compatibility( - &client, + read_api, package_id, compiled_package, package_path, @@ -1040,9 +1037,10 @@ impl SuiClientCommands { let sender = context.try_get_object_owner(&opts.gas).await?; let sender = sender.unwrap_or(context.active_address()?); let client = context.get_client().await?; - let chain_id = client.read_api().get_chain_identifier().await.ok(); + let read_api = client.read_api(); + let chain_id = read_api.get_chain_identifier().await.ok(); - check_protocol_version_and_warn(&client).await?; + check_protocol_version_and_warn(read_api).await?; let package_path = package_path .canonicalize() @@ -1064,7 +1062,7 @@ impl SuiClientCommands { check_dep_verification_flags(skip_dependency_verification, verify_deps)?; let compile_result = compile_package( - &client, + read_api, build_config.clone(), &package_path, with_unpublished_dependencies, @@ -1123,6 +1121,7 @@ impl SuiClientCommands { build_config, } => { let client = context.get_client().await?; + let read_api = client.read_api(); let protocol_version = protocol_version.map_or(ProtocolVersion::MAX, ProtocolVersion::new); let protocol_config = @@ -1151,7 +1150,7 @@ impl SuiClientCommands { (_, package_path) => { let package_path = package_path.unwrap_or_else(|| PathBuf::from(".")); let package = - compile_package_simple(&client, build_config, &package_path, None) + compile_package_simple(read_api, build_config, &package_path, None) .await?; let name = package .package @@ -1767,7 +1766,7 @@ fn check_dep_verification_flags( } async fn compile_package_simple( - client: &SuiClient, + read_api: &ReadApi, build_config: MoveBuildConfig, package_path: &Path, chain_id: Option, @@ -1781,13 +1780,13 @@ async fn compile_package_simple( let resolution_graph = config.resolution_graph(package_path, chain_id.clone())?; let mut compiled_package = build_from_resolution_graph(resolution_graph, false, false, chain_id)?; - pkg_tree_shake(client, false, &mut compiled_package).await?; + pkg_tree_shake(read_api, false, &mut compiled_package).await?; Ok(compiled_package) } pub(crate) async fn upgrade_package( - client: &SuiClient, + read_api: &ReadApi, build_config: MoveBuildConfig, package_path: &Path, upgrade_capability: ObjectID, @@ -1796,7 +1795,7 @@ pub(crate) async fn upgrade_package( env_alias: Option, ) -> Result<(u8, CompiledPackage), anyhow::Error> { let mut compiled_package = compile_package( - client, + read_api, build_config, package_path, with_unpublished_dependencies, @@ -1804,7 +1803,12 @@ pub(crate) async fn upgrade_package( ) .await?; - pkg_tree_shake(client, with_unpublished_dependencies, &mut compiled_package).await?; + pkg_tree_shake( + read_api, + with_unpublished_dependencies, + &mut compiled_package, + ) + .await?; compiled_package.published_at.as_ref().map_err(|e| match e { PublishedAtError::NotPresent => { @@ -1832,8 +1836,7 @@ pub(crate) async fn upgrade_package( } })?; - let resp = client - .read_api() + let resp = read_api .get_object_with_options( upgrade_capability, SuiObjectDataOptions::default().with_bcs().with_owner(), @@ -1861,13 +1864,12 @@ pub(crate) async fn upgrade_package( } pub(crate) async fn compile_package( - client: &SuiClient, + read_api: &ReadApi, build_config: MoveBuildConfig, package_path: &Path, with_unpublished_dependencies: bool, skip_dependency_verification: bool, ) -> Result { - let read_api = client.read_api(); let config = resolve_lock_file_path(build_config, Some(package_path))?; let run_bytecode_verifier = true; let print_diags_to_stderr = true; @@ -1891,7 +1893,12 @@ pub(crate) async fn compile_package( chain_id, )?; - pkg_tree_shake(client, with_unpublished_dependencies, &mut compiled_package).await?; + pkg_tree_shake( + read_api, + with_unpublished_dependencies, + &mut compiled_package, + ) + .await?; let protocol_config = read_api.get_protocol_config(None).await?; @@ -3086,8 +3093,8 @@ pub(crate) async fn prerender_clever_errors( } /// Warn the user if the CLI falls behind more than 2 protocol versions. -async fn check_protocol_version_and_warn(client: &SuiClient) -> Result<(), anyhow::Error> { - let protocol_cfg = client.read_api().get_protocol_config(None).await?; +async fn check_protocol_version_and_warn(read_api: &ReadApi) -> Result<(), anyhow::Error> { + let protocol_cfg = read_api.get_protocol_config(None).await?; let on_chain_protocol_version = protocol_cfg.protocol_version.as_u64(); let cli_protocol_version = ProtocolVersion::MAX.as_u64(); if (cli_protocol_version + 2) < on_chain_protocol_version { @@ -3111,12 +3118,11 @@ async fn check_protocol_version_and_warn(client: &SuiClient) -> Result<(), anyho /// Fetch move packages based on the provided package IDs. async fn fetch_move_packages( - client: &SuiClient, + read_api: &ReadApi, package_ids: &[ObjectID], pkg_id_to_name: &BTreeMap<&ObjectID, &Symbol>, ) -> Result, anyhow::Error> { - let objects = client - .read_api() + let objects = read_api .multi_get_object_with_options(package_ids.to_vec(), SuiObjectDataOptions::bcs_lossless()) .await?; @@ -3124,30 +3130,30 @@ async fn fetch_move_packages( .into_iter() .map(|o| { let o = o.into_object().map_err(|e| match e { - sui_types::error::SuiObjectResponseError::NotExists { object_id } => { + SuiObjectResponseError::NotExists { object_id } => { anyhow!( - "Package {:?} with object ID {object_id} does not exist", - pkg_id_to_name.get(&object_id) + "Package {} with object ID {object_id} does not exist", + pkg_id_to_name.get(&object_id).unwrap() ) } - sui_types::error::SuiObjectResponseError::Deleted { + SuiObjectResponseError::Deleted { object_id, version, digest, } => { anyhow!( - "Package {:?} with object ID {object_id} was deleted at version {version} \ + "Package {} with object ID {object_id} was deleted at version {version} \ with digest {digest}", - pkg_id_to_name.get(&object_id) + pkg_id_to_name.get(&object_id).unwrap() ) } - _ => anyhow!("Error fetching package: {e}"), + _ => anyhow!("Cannot convert data into an object: {e}"), })?; let Some(SuiRawData::Package(p)) = o.bcs else { bail!( - "Expected package {:?} with object ID {} but got something else", - pkg_id_to_name.get(&o.object_id), + "Expected package {} with object ID {} but got something else", + pkg_id_to_name.get(&o.object_id).unwrap(), o.object_id ); }; @@ -3159,7 +3165,7 @@ async fn fetch_move_packages( /// Filter out a package's dependencies which are not referenced in the source code. pub(crate) async fn pkg_tree_shake( - client: &SuiClient, + read_api: &ReadApi, with_unpublished_dependencies: bool, compiled_package: &mut CompiledPackage, ) -> Result<(), anyhow::Error> { @@ -3177,15 +3183,12 @@ pub(crate) async fn pkg_tree_shake( .map(|(pkg_name, module)| (pkg_name, ObjectID::from(module.unit.address.into_inner()))) .collect(); - let published_deps_packages = fetch_move_packages(client, &pkg_ids, &pkg_id_to_name).await?; + let published_deps_packages = fetch_move_packages(read_api, &pkg_ids, &pkg_id_to_name).await?; - let linkage_table_ids = published_deps_packages + let linkage_table_ids: BTreeSet<_> = published_deps_packages .iter() - .flat_map(|pkg| { - let linkage_table = pkg.linkage_table(); - linkage_table.keys().cloned().collect::>() - }) - .collect::>(); + .flat_map(|pkg| pkg.linkage_table().keys()) + .collect(); compiled_package .dependency_ids diff --git a/crates/sui/src/client_ptb/builder.rs b/crates/sui/src/client_ptb/builder.rs index 027794b8f458a..17ca35c773cc5 100644 --- a/crates/sui/src/client_ptb/builder.rs +++ b/crates/sui/src/client_ptb/builder.rs @@ -28,7 +28,7 @@ use std::{collections::BTreeMap, path::Path}; use sui_json::{is_receiving_argument, primitive_type}; use sui_json_rpc_types::{SuiObjectData, SuiObjectDataOptions, SuiRawData}; use sui_move::manage_package::resolve_lock_file_path; -use sui_sdk::SuiClient; +use sui_sdk::apis::ReadApi; use sui_types::{ base_types::{is_primitive_type_tag, ObjectID, TxContext, TxContextKind}, move_package::MovePackage, @@ -223,7 +223,7 @@ pub struct PTBBuilder<'a> { /// transaction arguments. resolved_arguments: BTreeMap, /// Read API for reading objects from chain. Needed for object resolution. - client: &'a SuiClient, + reader: &'a ReadApi, /// The last command that we have added. This is used to support assignment commands. last_command: Option, /// The actual PTB that we are building up. @@ -275,14 +275,14 @@ impl ArgWithHistory { } impl<'a> PTBBuilder<'a> { - pub fn new(starting_env: BTreeMap, client: &'a SuiClient) -> Self { + pub fn new(starting_env: BTreeMap, reader: &'a ReadApi) -> Self { Self { addresses: starting_env, identifiers: BTreeMap::new(), arguments_to_resolve: BTreeMap::new(), resolved_arguments: BTreeMap::new(), ptb: ProgrammableTransactionBuilder::new(), - client, + reader, last_command: None, errors: Vec::new(), } @@ -412,8 +412,7 @@ impl<'a> PTBBuilder<'a> { loc: Span, ) -> PTBResult { let object = self - .client - .read_api() + .reader .get_object_with_options(package_id, SuiObjectDataOptions::bcs_lossless()) .await .map_err(|e| err!(loc, "{e}"))? @@ -764,8 +763,7 @@ impl<'a> PTBBuilder<'a> { /// Fetch the `SuiObjectData` for an object ID -- this is used for object resolution. async fn get_object(&self, object_id: ObjectID, obj_loc: Span) -> PTBResult { let res = self - .client - .read_api() + .reader .get_object_with_options( object_id, SuiObjectDataOptions::new().with_type().with_owner(), @@ -933,7 +931,7 @@ impl<'a> PTBBuilder<'a> { self.last_command = Some(res); } ParsedPTBCommand::Publish(sp!(pkg_loc, package_path)) => { - let chain_id = self.client.read_api().get_chain_identifier().await.ok(); + let chain_id = self.reader.get_chain_identifier().await.ok(); let build_config = BuildConfig::default(); let package_path = Path::new(&package_path); let build_config = resolve_lock_file_path(build_config.clone(), Some(package_path)) @@ -960,7 +958,7 @@ impl<'a> PTBBuilder<'a> { .map_err(|e| err!(pkg_loc, "{e}"))?; } let compiled_package = compile_package( - self.client, + self.reader, build_config.clone(), package_path, false, /* with_unpublished_dependencies */ @@ -1001,7 +999,7 @@ impl<'a> PTBBuilder<'a> { ) .await?; - let chain_id = self.client.read_api().get_chain_identifier().await.ok(); + let chain_id = self.reader.get_chain_identifier().await.ok(); let build_config = BuildConfig::default(); let package_path = Path::new(&package_path); let build_config = resolve_lock_file_path(build_config.clone(), Some(package_path)) @@ -1029,7 +1027,7 @@ impl<'a> PTBBuilder<'a> { } let (upgrade_policy, compiled_package) = upgrade_package( - self.client, + self.reader, build_config.clone(), package_path, ObjectID::from_address(upgrade_cap_id.into_inner()), diff --git a/crates/sui/src/client_ptb/ptb.rs b/crates/sui/src/client_ptb/ptb.rs index d8ca4fb14ea7e..4a41762a13854 100644 --- a/crates/sui/src/client_ptb/ptb.rs +++ b/crates/sui/src/client_ptb/ptb.rs @@ -235,7 +235,7 @@ impl PTB { .into_iter() .map(|(sa, alias)| (alias.alias.clone(), AccountAddress::from(*sa))) .collect(); - let builder = PTBBuilder::new(starting_addresses, &client); + let builder = PTBBuilder::new(starting_addresses, client.read_api()); builder.build(program).await } diff --git a/crates/sui/src/sui_commands.rs b/crates/sui/src/sui_commands.rs index c788796743b9f..0cf861b002c84 100644 --- a/crates/sui/src/sui_commands.rs +++ b/crates/sui/src/sui_commands.rs @@ -523,6 +523,7 @@ impl SuiCommand { let Ok(client) = context.get_client().await else { bail!("`sui move build --dump-bytecode-as-base64` requires a connection to the network. Current active network is {} but failed to connect to it.", context.config.active_env.as_ref().unwrap()); }; + let read_api = client.read_api(); if let Err(e) = client.check_api_version() { eprintln!("{}", format!("[warning] {e}").yellow().bold()); @@ -532,7 +533,7 @@ impl SuiCommand { // for tests it's useful to ignore the chain id! None } else { - client.read_api().get_chain_identifier().await.ok() + read_api.get_chain_identifier().await.ok() }; let rerooted_path = move_cli::base::reroot_path(package_path.as_deref())?; @@ -553,7 +554,7 @@ impl SuiCommand { check_unpublished_dependencies(&pkg.dependency_ids.unpublished)?; } - pkg_tree_shake(&client, with_unpublished_deps, &mut pkg).await?; + pkg_tree_shake(read_api, with_unpublished_deps, &mut pkg).await?; println!( "{}", diff --git a/crates/sui/src/upgrade_compatibility/mod.rs b/crates/sui/src/upgrade_compatibility/mod.rs index f9da6d38137bc..a2853adc5f6b7 100644 --- a/crates/sui/src/upgrade_compatibility/mod.rs +++ b/crates/sui/src/upgrade_compatibility/mod.rs @@ -45,7 +45,7 @@ use move_package::compilation::compiled_package::CompiledUnitWithSource; use sui_json_rpc_types::{SuiObjectDataOptions, SuiRawData}; use sui_move_build::CompiledPackage; use sui_protocol_config::ProtocolConfig; -use sui_sdk::SuiClient; +use sui_sdk::apis::ReadApi; use sui_types::move_package::UpgradePolicy; use sui_types::{base_types::ObjectID, execution_config_utils::to_binary_config}; @@ -654,15 +654,14 @@ upgrade_codes!( /// Check the upgrade compatibility of a new package with an existing on-chain package. pub(crate) async fn check_compatibility( - client: &SuiClient, + read_api: &ReadApi, package_id: ObjectID, new_package: CompiledPackage, package_path: PathBuf, upgrade_policy: u8, protocol_config: ProtocolConfig, ) -> Result<(), Error> { - let existing_obj_read = client - .read_api() + let existing_obj_read = read_api .get_object_with_options(package_id, SuiObjectDataOptions::new().with_bcs()) .await .context("Unable to get existing package")?; diff --git a/crates/sui/tests/cli_tests.rs b/crates/sui/tests/cli_tests.rs index be14bea645d02..1266054dc45a2 100644 --- a/crates/sui/tests/cli_tests.rs +++ b/crates/sui/tests/cli_tests.rs @@ -1879,7 +1879,7 @@ async fn test_package_publish_nonexistent_dependency() -> Result<(), anyhow::Err let err = result.unwrap_err().to_string(); assert!( - err.contains("Object 0x0000000000000000000000000000000000000000000000000000000000abc123 does not exist"), + err.contains("Package Nonexistent with object ID 0x0000000000000000000000000000000000000000000000000000000000abc123 does not exist"), "{}", err );