From d14f781f1745112853266631074be145143ef490 Mon Sep 17 00:00:00 2001 From: Tim Zakian <2895723+tzakian@users.noreply.github.com> Date: Tue, 18 Jun 2024 17:14:50 -0700 Subject: [PATCH] [CLI] Add clever error support to Sui CLI (#17920) ## Description Enables clever error rendering in the Sui CLI. It also abstracts out some of the rendering logic for constants into their own file so it can be reused across the graphql and Sui CLI renderer. This also updates the Move disassembler to use this new (nicer!) renderer to that we support rendered values for constants in the Move disassembler instead of just strings in constants. ## Test plan Added new tests for this to the Sui CLI, and updated/added additional tests for the disassembler. --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [X] CLI: Add better rendering for Move aborts, and added support for rendering clever errors. - [ ] Rust SDK: --- Cargo.lock | 2 + crates/sui-package-resolver/src/lib.rs | 36 +--- crates/sui/src/clever_error_rendering.rs | 193 ++++++++++++++++++ crates/sui/src/client_commands.rs | 75 ++++++- crates/sui/src/lib.rs | 1 + ...ing__tests__parse_abort_status_string.snap | 51 +++++ crates/sui/tests/cli_tests.rs | 135 ++++++++++++ crates/sui/tests/data/clever_errors/Move.toml | 6 + .../clever_errors/sources/clever_errors.move | 28 +++ .../tests/snapshots/cli_tests__body_fn.snap | 21 ++ external-crates/move/Cargo.lock | 2 + .../build_tests/disassemble_module/args.exp | 22 +- .../disassemble_module/sources/m.move | 5 +- .../sandbox_tests/package_basics/args.exp | 4 +- .../move-command-line-common/Cargo.toml | 2 + .../move-command-line-common/src/display.rs | 63 ++++++ .../move-command-line-common/src/lib.rs | 1 + .../move-disassembler/src/disassembler.rs | 90 +++++--- 18 files changed, 657 insertions(+), 80 deletions(-) create mode 100644 crates/sui/src/clever_error_rendering.rs create mode 100644 crates/sui/src/snapshots/sui__clever_error_rendering__tests__parse_abort_status_string.snap create mode 100644 crates/sui/tests/data/clever_errors/Move.toml create mode 100644 crates/sui/tests/data/clever_errors/sources/clever_errors.move create mode 100644 crates/sui/tests/snapshots/cli_tests__body_fn.snap create mode 100644 external-crates/move/crates/move-command-line-common/src/display.rs diff --git a/Cargo.lock b/Cargo.lock index 79807bf0487f96..6415e144b91be4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6787,9 +6787,11 @@ name = "move-command-line-common" version = "0.1.0" dependencies = [ "anyhow", + "bcs", "difference", "dirs-next", "hex", + "move-binary-format", "move-core-types", "num-bigint 0.4.4", "once_cell", diff --git a/crates/sui-package-resolver/src/lib.rs b/crates/sui-package-resolver/src/lib.rs index d58b1e6b9f4071..c2cb4017d6dc3d 100644 --- a/crates/sui-package-resolver/src/lib.rs +++ b/crates/sui-package-resolver/src/lib.rs @@ -7,10 +7,10 @@ use move_binary_format::file_format::{ AbilitySet, DatatypeTyParameter, EnumDefinitionIndex, FunctionDefinitionIndex, Signature as MoveSignature, SignatureIndex, Visibility, }; -use move_command_line_common::error_bitset::ErrorBitset; +use move_command_line_common::display::RenderResult; +use move_command_line_common::{display::try_render_constant, error_bitset::ErrorBitset}; use move_core_types::annotated_value::MoveEnumLayout; use move_core_types::language_storage::ModuleId; -use move_core_types::u256::U256; use std::collections::BTreeSet; use std::num::NonZeroUsize; use std::sync::{Arc, Mutex}; @@ -594,40 +594,16 @@ impl Resolver { .and_then(|x| String::from_utf8(x).ok())?; let bytes = error_value_constant.data.clone(); - let rendered = match &error_value_constant.type_ { - SignatureToken::Vector(inner_ty) if inner_ty.as_ref() == &SignatureToken::U8 => { - bcs::from_bytes::>(&bytes) - .ok() - .and_then(|x| String::from_utf8(x).ok()) - } - SignatureToken::U8 => bcs::from_bytes::(&bytes).ok().map(|x| x.to_string()), - SignatureToken::U16 => bcs::from_bytes::(&bytes).ok().map(|x| x.to_string()), - SignatureToken::U32 => bcs::from_bytes::(&bytes).ok().map(|x| x.to_string()), - SignatureToken::U64 => bcs::from_bytes::(&bytes).ok().map(|x| x.to_string()), - SignatureToken::U128 => bcs::from_bytes::(&bytes).ok().map(|x| x.to_string()), - SignatureToken::U256 => bcs::from_bytes::(&bytes).ok().map(|x| x.to_string()), - SignatureToken::Address => bcs::from_bytes::(&bytes) - .ok() - .map(|x| x.to_canonical_string(true)), - SignatureToken::Bool => bcs::from_bytes::(&bytes).ok().map(|x| x.to_string()), - - SignatureToken::Signer - | SignatureToken::Vector(_) - | SignatureToken::Datatype(_) - | SignatureToken::DatatypeInstantiation(_) - | SignatureToken::Reference(_) - | SignatureToken::MutableReference(_) - | SignatureToken::TypeParameter(_) => None, - }; + let rendered = try_render_constant(error_value_constant); let error_info = match rendered { - None => ErrorConstants::Raw { + RenderResult::NotRendered => ErrorConstants::Raw { identifier: error_identifier, bytes, }, - Some(error_constant) => ErrorConstants::Rendered { + RenderResult::AsString(s) | RenderResult::AsValue(s) => ErrorConstants::Rendered { identifier: error_identifier, - constant: error_constant, + constant: s, }, }; diff --git a/crates/sui/src/clever_error_rendering.rs b/crates/sui/src/clever_error_rendering.rs new file mode 100644 index 00000000000000..725a48367aea3f --- /dev/null +++ b/crates/sui/src/clever_error_rendering.rs @@ -0,0 +1,193 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//! This module provides a function to render a Move abort status string into a more human-readable +//! error message using the clever error rendering logic. +//! +//! The logic in this file is largely a stop-gap to provide Clever Error rendering in the CLI while +//! it still uses the JSON-RPC API. The new GraphQL API already renderd Clever Errors on the server +//! side in a much more robust and efficient way. +//! +//! Once the CLI is updated to use the GraphQL API, this file can be removed, and the GraphQL-based +//! rendering logic for Clever Errors should be used instead. + +use fastcrypto::encoding::{Base64, Encoding}; +use move_binary_format::{ + binary_config::BinaryConfig, file_format::SignatureToken, CompiledModule, +}; +use move_command_line_common::{ + display::{try_render_constant, RenderResult}, + error_bitset::ErrorBitset, +}; +use move_core_types::account_address::AccountAddress; +use sui_json_rpc_types::{SuiObjectDataOptions, SuiRawData}; +use sui_sdk::apis::ReadApi; +use sui_types::{base_types::ObjectID, Identifier}; + +/// Take a Move abort status string and render it into a more human-readable error message using +/// by parsing the string (as best we can) and seeing if the abort code is a Clever Error abort +/// code. If it is, we attempt to render the error in a more huma-readable manner using the Read +/// API and decoding the Clever Error encoding in the abort code. +/// +/// This function is used to render Clever Errors for on-chain errors only within the Sui CLI. This +/// function is _not_ used at all for off-chain errors or Move unit tests. You should only use this +/// function within this crate. +pub(crate) async fn render_clever_error_opt( + error_string: &str, + read_api: &ReadApi, +) -> Option { + let (address, module_name, function_name, instruction, abort_code, command_index) = + parse_abort_status_string(error_string).ok()?; + + let error = 'error: { + let Some(error_bitset) = ErrorBitset::from_u64(abort_code) else { + break 'error format!( + "function '{}::{}::{}' at instruction {} with code {}", + address.to_canonical_display(true), + module_name, + function_name, + instruction, + abort_code + ); + }; + + let line_number = error_bitset.line_number()?; + + if error_bitset.constant_index().is_none() && error_bitset.identifier_index().is_none() { + break 'error format!( + "function '{}::{}::{}' at line {}", + address.to_canonical_display(true), + module_name, + function_name, + line_number + ); + } + + let SuiRawData::Package(package) = read_api + .get_object_with_options( + ObjectID::from_address(address), + SuiObjectDataOptions::bcs_lossless(), + ) + .await + .ok()? + .into_object() + .ok()? + .bcs? + else { + return None; + }; + + let module = package.module_map.get(module_name.as_str())?; + let module = + CompiledModule::deserialize_with_config(module, &BinaryConfig::standard()).ok()?; + + let error_identifier_constant = module + .constant_pool() + .get(error_bitset.identifier_index()? as usize)?; + let error_value_constant = module + .constant_pool() + .get(error_bitset.constant_index()? as usize)?; + + if !matches!(&error_identifier_constant.type_, SignatureToken::Vector(x) if x.as_ref() == &SignatureToken::U8) + { + return None; + }; + + let error_identifier = bcs::from_bytes::>(&error_identifier_constant.data) + .ok() + .and_then(|x| String::from_utf8(x).ok())?; + + let const_str = match try_render_constant(error_value_constant) { + RenderResult::NotRendered => { + format!("'{}'", Base64::encode(&error_value_constant.data)) + } + RenderResult::AsString(s) => format!("'{s}'"), + RenderResult::AsValue(v_str) => v_str, + }; + + format!( + "function '{}::{}::{}' at line {}. Aborted with '{}' -- {}", + address.to_canonical_display(true), + module_name, + function_name, + line_number, + error_identifier, + const_str + ) + }; + + // Convert the command index into an ordinal. + let command = command_index + 1; + let suffix = match command % 10 { + 1 => "st", + 2 => "nd", + 3 => "rd", + _ => "th", + }; + + Some(format!("{command}{suffix} command aborted within {error}")) +} + +/// Parsing the error with a regex is not great, but it's the best we can do with the current +/// JSON-RPC API since we only get error messages as strings. This function attempts to parse a +/// Move abort status string into its different parts, and then parses it back into the structured +/// format that we can then use to render a Clever Error. +/// +/// If we are able to parse the string, we return a tuple with the address, module name, function +/// name, instruction, abort code, and command index. If we are unable to parse the string, we +/// return `Err`. +/// +/// You should delete this function with glee once the CLI is updated to use the GraphQL API. +fn parse_abort_status_string( + s: &str, +) -> Result<(AccountAddress, Identifier, Identifier, u16, u64, u16), anyhow::Error> { + use regex::Regex; + let re = Regex::new(r#"MoveAbort.*address:\s*(.*?),.* name:.*Identifier\((.*?)\).*instruction:\s+(\d+),.*function_name:.*Some\((.*?)\).*},\s*(\d+).*in command\s*(\d+)"#).unwrap(); + let Some(captures) = re.captures(s) else { + anyhow::bail!( + "Cannot parse abort status string: {} as a move abort string", + s + ); + }; + + // Remove any escape characters from the string if present. + let clean_string = |s: &str| s.replace(['\\', '\"'], ""); + + let address = AccountAddress::from_hex(&captures[1])?; + let module_name = Identifier::new(clean_string(&captures[2]))?; + let instruction = captures[3].parse::()?; + let function_name = Identifier::new(clean_string(&captures[4]))?; + let abort_code = captures[5].parse::()?; + let command_index = captures[6].parse::()?; + Ok(( + address, + module_name, + function_name, + instruction, + abort_code, + command_index, + )) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_parse_abort_status_string() { + let corpus = vec![ + r#"Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 0, instruction: 1, function_name: Some(\"aborter\") }, 0) in command 0" }"#, + r#"Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 1, instruction: 1, function_name: Some(\"aborter_line_no\") }, 9223372105574252543) in command 0" }"#, + r#"Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 2, instruction: 1, function_name: Some(\"clever_aborter\") }, 9223372118459154433) in command 0" }"#, + r#"Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 3, instruction: 1, function_name: Some(\"clever_aborter_not_a_string\") }, 9223372135639154691) in command 0" }"#, + r#"MoveAbort(MoveLocation { module: ModuleId { address: 24bf9e624820625ac1e38076901421d2630b2b225b638aaf0b85264b857a608b, name: Identifier(\"tester\") }, function: 0, instruction: 1, function_name: Some(\"test\") }, 9223372071214514177) in command 0"#, + r#"MoveAbort(MoveLocation { module: ModuleId { address: 24bf9e624820625ac1e38076901421d2630b2b225b638aaf0b85264b857a608b, name: Identifier("tester") }, function: 0, instruction: 1, function_name: Some("test") }, 9223372071214514177) in command 0"#, + ]; + let parsed: Vec<_> = corpus.into_iter().map(|c| { + let (address, module_name, function_name, instruction, abort_code, command_index) = + parse_abort_status_string(c).unwrap(); + format!("original abort message: {}\n address: {}\n module_name: {}\n function_name: {}\n instruction: {}\n abort_code: {}\n command_index: {}", c, address, module_name, function_name, instruction, abort_code, command_index) + }).collect(); + insta::assert_snapshot!(parsed.join("\n------\n")); + } +} diff --git a/crates/sui/src/client_commands.rs b/crates/sui/src/client_commands.rs index 62316af6f9578e..b72713ecc15f4e 100644 --- a/crates/sui/src/client_commands.rs +++ b/crates/sui/src/client_commands.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 use crate::{ + clever_error_rendering::render_clever_error_opt, client_ptb::ptb::PTB, displays::Pretty, key_identity::{get_identity_address, KeyIdentity}, @@ -42,7 +43,8 @@ use sui_json_rpc_types::{ Coin, DryRunTransactionBlockResponse, DynamicFieldPage, SuiCoinMetadata, SuiData, SuiExecutionStatus, SuiObjectData, SuiObjectDataOptions, SuiObjectResponse, SuiObjectResponseQuery, SuiParsedData, SuiProtocolConfigValue, SuiRawData, - SuiTransactionBlockEffectsAPI, SuiTransactionBlockResponse, SuiTransactionBlockResponseOptions, + SuiTransactionBlockEffects, SuiTransactionBlockEffectsAPI, SuiTransactionBlockResponse, + SuiTransactionBlockResponseOptions, }; use sui_keys::keystore::AccountKeystore; use sui_move_build::{ @@ -663,7 +665,7 @@ impl SuiClientCommands { self, context: &mut WalletContext, ) -> Result { - let ret = Ok(match self { + let ret = match self { SuiClientCommands::ProfileTransaction { tx_digest, profile_output, @@ -1583,8 +1585,9 @@ impl SuiClientCommands { ptb.execute(context).await?; SuiClientCommandResult::NoOutput } - }); - ret + }; + let client = context.get_client().await?; + Ok(ret.prerender_clever_errors(client.read_api()).await) } pub fn switch_env(config: &mut SuiClientConfig, env: &str) -> Result<(), anyhow::Error> { @@ -2204,6 +2207,42 @@ impl SuiClientCommandResult { _ => None, } } + + pub async fn prerender_clever_errors(mut self, read_api: &ReadApi) -> Self { + match &mut self { + SuiClientCommandResult::DryRun(DryRunTransactionBlockResponse { effects, .. }) + | SuiClientCommandResult::TransactionBlock(SuiTransactionBlockResponse { + effects: Some(effects), + .. + }) => prerender_clever_errors(effects, read_api).await, + + SuiClientCommandResult::TransactionBlock(SuiTransactionBlockResponse { + effects: None, + .. + }) => (), + SuiClientCommandResult::ActiveAddress(_) + | SuiClientCommandResult::ActiveEnv(_) + | SuiClientCommandResult::Addresses(_) + | SuiClientCommandResult::Balance(_, _) + | SuiClientCommandResult::ChainIdentifier(_) + | SuiClientCommandResult::DynamicFieldQuery(_) + | SuiClientCommandResult::Envs(_, _) + | SuiClientCommandResult::Gas(_) + | SuiClientCommandResult::NewAddress(_) + | SuiClientCommandResult::NewEnv(_) + | SuiClientCommandResult::NoOutput + | SuiClientCommandResult::Object(_) + | SuiClientCommandResult::Objects(_) + | SuiClientCommandResult::RawObject(_) + | SuiClientCommandResult::SerializedSignedTransaction(_) + | SuiClientCommandResult::SerializedUnsignedTransaction(_) + | SuiClientCommandResult::Switch(_) + | SuiClientCommandResult::SyncClientState + | SuiClientCommandResult::VerifyBytecodeMeter { .. } + | SuiClientCommandResult::VerifySource => (), + } + self + } } #[derive(Serialize)] @@ -2568,7 +2607,10 @@ pub async fn execute_dry_run( .dry_run_transaction_block(dry_run_tx_data) .await .map_err(|e| anyhow!("Dry run failed: {e}"))?; - Ok(SuiClientCommandResult::DryRun(response)) + let resp = SuiClientCommandResult::DryRun(response) + .prerender_clever_errors(client.read_api()) + .await; + Ok(resp) } /// Call a dry run with the transaction data to estimate the gas budget. @@ -2718,17 +2760,32 @@ pub(crate) async fn dry_run_or_execute_or_serialize( )) } else { let transaction = Transaction::new(sender_signed_data); - let response = context.execute_transaction_may_fail(transaction).await?; + let mut response = context.execute_transaction_may_fail(transaction).await?; + if let Some(effects) = response.effects.as_mut() { + prerender_clever_errors(effects, client.read_api()).await; + } let effects = response.effects.as_ref().ok_or_else(|| { anyhow!("Effects from SuiTransactionBlockResult should not be empty") })?; - if matches!(effects.status(), SuiExecutionStatus::Failure { .. }) { + if let SuiExecutionStatus::Failure { error } = effects.status() { return Err(anyhow!( - "Error executing transaction: {:#?}", - effects.status() + "Error executing transaction '{}': {error}", + response.digest )); } Ok(SuiClientCommandResult::TransactionBlock(response)) } } } + +pub(crate) async fn prerender_clever_errors( + effects: &mut SuiTransactionBlockEffects, + read_api: &ReadApi, +) { + let SuiTransactionBlockEffects::V1(effects) = effects; + if let SuiExecutionStatus::Failure { error } = &mut effects.status { + if let Some(rendered) = render_clever_error_opt(error, read_api).await { + *error = rendered; + } + } +} diff --git a/crates/sui/src/lib.rs b/crates/sui/src/lib.rs index 170e7e55826de7..8fdafa059819f8 100644 --- a/crates/sui/src/lib.rs +++ b/crates/sui/src/lib.rs @@ -5,6 +5,7 @@ pub mod client_commands; #[macro_use] pub mod client_ptb; +mod clever_error_rendering; pub mod console; pub mod displays; pub mod fire_drill; diff --git a/crates/sui/src/snapshots/sui__clever_error_rendering__tests__parse_abort_status_string.snap b/crates/sui/src/snapshots/sui__clever_error_rendering__tests__parse_abort_status_string.snap new file mode 100644 index 00000000000000..d6590d51596a39 --- /dev/null +++ b/crates/sui/src/snapshots/sui__clever_error_rendering__tests__parse_abort_status_string.snap @@ -0,0 +1,51 @@ +--- +source: crates/sui/src/clever_error_rendering.rs +expression: "parsed.join(\"\\n------\\n\")" +--- +original abort message: Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 0, instruction: 1, function_name: Some(\"aborter\") }, 0) in command 0" } + address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20 + module_name: clever_errors + function_name: aborter + instruction: 1 + abort_code: 0 + command_index: 0 +------ +original abort message: Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 1, instruction: 1, function_name: Some(\"aborter_line_no\") }, 9223372105574252543) in command 0" } + address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20 + module_name: clever_errors + function_name: aborter_line_no + instruction: 1 + abort_code: 9223372105574252543 + command_index: 0 +------ +original abort message: Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 2, instruction: 1, function_name: Some(\"clever_aborter\") }, 9223372118459154433) in command 0" } + address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20 + module_name: clever_errors + function_name: clever_aborter + instruction: 1 + abort_code: 9223372118459154433 + command_index: 0 +------ +original abort message: Failure { error: "MoveAbort(MoveLocation { module: ModuleId { address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20, name: Identifier(\"clever_errors\") }, function: 3, instruction: 1, function_name: Some(\"clever_aborter_not_a_string\") }, 9223372135639154691) in command 0" } + address: 60197a0c146e31dd12689e890208767fe2fefb2f726710b4a9fa0b857f7a2c20 + module_name: clever_errors + function_name: clever_aborter_not_a_string + instruction: 1 + abort_code: 9223372135639154691 + command_index: 0 +------ +original abort message: MoveAbort(MoveLocation { module: ModuleId { address: 24bf9e624820625ac1e38076901421d2630b2b225b638aaf0b85264b857a608b, name: Identifier(\"tester\") }, function: 0, instruction: 1, function_name: Some(\"test\") }, 9223372071214514177) in command 0 + address: 24bf9e624820625ac1e38076901421d2630b2b225b638aaf0b85264b857a608b + module_name: tester + function_name: test + instruction: 1 + abort_code: 9223372071214514177 + command_index: 0 +------ +original abort message: MoveAbort(MoveLocation { module: ModuleId { address: 24bf9e624820625ac1e38076901421d2630b2b225b638aaf0b85264b857a608b, name: Identifier("tester") }, function: 0, instruction: 1, function_name: Some("test") }, 9223372071214514177) in command 0 + address: 24bf9e624820625ac1e38076901421d2630b2b225b638aaf0b85264b857a608b + module_name: tester + function_name: test + instruction: 1 + abort_code: 9223372071214514177 + command_index: 0 diff --git a/crates/sui/tests/cli_tests.rs b/crates/sui/tests/cli_tests.rs index c773db7a479211..924ea8892f9254 100644 --- a/crates/sui/tests/cli_tests.rs +++ b/crates/sui/tests/cli_tests.rs @@ -3669,3 +3669,138 @@ async fn test_gas_estimation() -> Result<(), anyhow::Error> { } Ok(()) } + +#[sim_test] +async fn test_clever_errors() -> Result<(), anyhow::Error> { + // Publish the package + 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("clever_errors"); + let build_config = BuildConfig::new_for_testing().config; + let resp = SuiClientCommands::Publish { + package_path: package_path.clone(), + build_config, + skip_dependency_verification: false, + with_unpublished_dependencies: false, + opts: OptsWithGas::for_testing(Some(gas_obj_id), rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH), + } + .execute(context) + .await?; + + // Print it out to CLI/logs + resp.print(true); + + let SuiClientCommandResult::TransactionBlock(response) = resp else { + unreachable!("Invalid response"); + }; + + let SuiTransactionBlockEffects::V1(effects) = response.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 elide_transaction_digest = |s: String| -> String { + let mut x = s.splitn(5, '\'').collect::>(); + x[1] = "ELIDED_TRANSACTION_DIGEST"; + let tmp = format!("ELIDED_ADDRESS{}", &x[3][66..]); + x[3] = &tmp; + x.join("'") + }; + + // Normal abort + let non_clever_abort = SuiClientCommands::Call { + package: package.reference.object_id, + module: "clever_errors".to_string(), + function: "aborter".to_string(), + type_args: vec![], + opts: OptsWithGas::for_testing(None, rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH), + gas_price: None, + args: vec![], + } + .execute(context) + .await + .unwrap_err(); + + // Line-only abort + let line_only_abort = SuiClientCommands::Call { + package: package.reference.object_id, + module: "clever_errors".to_string(), + function: "aborter_line_no".to_string(), + type_args: vec![], + opts: OptsWithGas::for_testing(None, rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH), + gas_price: None, + args: vec![], + } + .execute(context) + .await + .unwrap_err(); + + // Full clever error with utf-8 string + let clever_error_utf8 = SuiClientCommands::Call { + package: package.reference.object_id, + module: "clever_errors".to_string(), + function: "clever_aborter".to_string(), + type_args: vec![], + opts: OptsWithGas::for_testing(None, rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH), + gas_price: None, + args: vec![], + } + .execute(context) + .await + .unwrap_err(); + + // Full clever error with non-utf-8 string + let clever_error_non_utf8 = SuiClientCommands::Call { + package: package.reference.object_id, + module: "clever_errors".to_string(), + function: "clever_aborter_not_a_string".to_string(), + type_args: vec![], + opts: OptsWithGas::for_testing(None, rgp * TEST_ONLY_GAS_UNIT_FOR_PUBLISH), + gas_price: None, + args: vec![], + } + .execute(context) + .await + .unwrap_err(); + + let error_string = format!( + "Non-clever-abort\n---\n{}\n---\nLine-only-abort\n---\n{}\n---\nClever-error-utf8\n---\n{}\n---\nClever-error-non-utf8\n---\n{}\n---\n", + elide_transaction_digest(non_clever_abort.to_string()), + elide_transaction_digest(line_only_abort.to_string()), + elide_transaction_digest(clever_error_utf8.to_string()), + elide_transaction_digest(clever_error_non_utf8.to_string()) + ); + + insta::assert_snapshot!(error_string); + Ok(()) +} diff --git a/crates/sui/tests/data/clever_errors/Move.toml b/crates/sui/tests/data/clever_errors/Move.toml new file mode 100644 index 00000000000000..d597ae9c2749b0 --- /dev/null +++ b/crates/sui/tests/data/clever_errors/Move.toml @@ -0,0 +1,6 @@ +[package] +name = "clever_errors" +edition = "2024.beta" # edition = "legacy" to use legacy (pre-2024) Move + +[addresses] +clever_errors = "0x0" diff --git a/crates/sui/tests/data/clever_errors/sources/clever_errors.move b/crates/sui/tests/data/clever_errors/sources/clever_errors.move new file mode 100644 index 00000000000000..b8ac3774cf7312 --- /dev/null +++ b/crates/sui/tests/data/clever_errors/sources/clever_errors.move @@ -0,0 +1,28 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Do _not_ edit this file (yes, even whitespace!). Editing this file will +// cause the tests that use this module to fail. +module clever_errors::clever_errors { + #[error] + const ENotFound: vector = b"Element not found in vector 💥 🚀 🌠"; + + #[error] + const ENotAString: vector = vector[1,2,3,4]; + + public fun aborter() { + abort 0 + } + + public fun aborter_line_no() { + assert!(false); + } + + public fun clever_aborter() { + assert!(false, ENotFound); + } + + public fun clever_aborter_not_a_string() { + assert!(false, ENotAString); + } +} diff --git a/crates/sui/tests/snapshots/cli_tests__body_fn.snap b/crates/sui/tests/snapshots/cli_tests__body_fn.snap new file mode 100644 index 00000000000000..32fb1f76f42e75 --- /dev/null +++ b/crates/sui/tests/snapshots/cli_tests__body_fn.snap @@ -0,0 +1,21 @@ +--- +source: crates/sui/tests/cli_tests.rs +expression: error_string +--- +Non-clever-abort +--- +Error executing transaction 'ELIDED_TRANSACTION_DIGEST': 1st command aborted within function 'ELIDED_ADDRESS::clever_errors::aborter' at instruction 1 with code 0 +--- +Line-only-abort +--- +Error executing transaction 'ELIDED_TRANSACTION_DIGEST': 1st command aborted within function 'ELIDED_ADDRESS::clever_errors::aborter_line_no' at line 18 +--- +Clever-error-utf8 +--- +Error executing transaction 'ELIDED_TRANSACTION_DIGEST': 1st command aborted within function 'ELIDED_ADDRESS::clever_errors::clever_aborter' at line 22. Aborted with 'ENotFound' -- 'Element not found in vector 💥 🚀 🌠' +--- +Clever-error-non-utf8 +--- +Error executing transaction 'ELIDED_TRANSACTION_DIGEST': 1st command aborted within function 'ELIDED_ADDRESS::clever_errors::clever_aborter_not_a_string' at line 26. Aborted with 'ENotAString' -- 'BAEAAAAAAAAAAgAAAAAAAAADAAAAAAAAAAQAAAAAAAAA' +--- + diff --git a/external-crates/move/Cargo.lock b/external-crates/move/Cargo.lock index dd1b5c2c109bc5..1ad62df6d7640a 100644 --- a/external-crates/move/Cargo.lock +++ b/external-crates/move/Cargo.lock @@ -1713,9 +1713,11 @@ name = "move-command-line-common" version = "0.1.0" dependencies = [ "anyhow", + "bcs", "difference", "dirs-next", "hex", + "move-binary-format", "move-core-types", "num-bigint", "once_cell", diff --git a/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/args.exp b/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/args.exp index 1e7edaaedeb844..236f9bd53fcdc6 100644 --- a/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/args.exp +++ b/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/args.exp @@ -14,19 +14,33 @@ struct As { public zf(): u64 { B0: - 0: LdConst[0](U64: 01000000..) + 0: LdConst[0](u64: 1) 1: Ret } public af(): u32 { B0: - 0: LdConst[1](U32: 00000000) + 0: LdConst[1](u32: 0) + 1: Ret + +} +public sf(): vector { +B0: + 0: LdConst[2](vector: "Thi..) + 1: Ret + +} +public nf(): vector { +B0: + 0: LdConst[3](vector: 0401..) 1: Ret } Constants [ - 0 => u64: 0100000000000000 - 1 => u32: 00000000 + 0 => u64: 1 + 1 => u32: 0 + 2 => vector: "This is some emojis 🤔🤔🤔" // interpreted as UTF8 string + 3 => vector: 040100020003000400 ] } diff --git a/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/sources/m.move b/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/sources/m.move index 4f255689cf67a7..35af0da97addcd 100644 --- a/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/sources/m.move +++ b/external-crates/move/crates/move-cli/tests/build_tests/disassemble_module/sources/m.move @@ -5,8 +5,11 @@ struct As {} const Zc: u64 = 1; const Ac: u32 = 0; +const AString: vector = b"This is some emojis 🤔🤔🤔"; +const NotAString: vector = vector[1,2,3,4]; public fun zf(): u64 { Zc } public fun af(): u32 { Ac } - +public fun sf(): vector { AString } +public fun nf(): vector { NotAString } } diff --git a/external-crates/move/crates/move-cli/tests/sandbox_tests/package_basics/args.exp b/external-crates/move/crates/move-cli/tests/sandbox_tests/package_basics/args.exp index dc9256b55eaff6..a46298ad886990 100644 --- a/external-crates/move/crates/move-cli/tests/sandbox_tests/package_basics/args.exp +++ b/external-crates/move/crates/move-cli/tests/sandbox_tests/package_basics/args.exp @@ -63,7 +63,7 @@ B0: B1: [4] 4: Branch(7) B2: -[2] 5: LdConst[0](U64: 00000000..) +[2] 5: LdConst[0](u64: 0) [2] 6: Abort B3: [4] 7: CopyLoc[0](x#0#0: u64) @@ -74,7 +74,7 @@ B3: } Constants [ - 0 => u64: 0000000000000000 + 0 => u64: 0 ] } Command `disassemble --package MoveStdlib --name signer`: diff --git a/external-crates/move/crates/move-command-line-common/Cargo.toml b/external-crates/move/crates/move-command-line-common/Cargo.toml index ff8756f2e28b30..08679ba8a6e312 100644 --- a/external-crates/move/crates/move-command-line-common/Cargo.toml +++ b/external-crates/move/crates/move-command-line-common/Cargo.toml @@ -20,8 +20,10 @@ once_cell.workspace = true serde.workspace = true dirs-next.workspace = true vfs.workspace = true +bcs.workspace = true move-core-types.workspace = true +move-binary-format.workspace = true [dev-dependencies] proptest.workspace = true diff --git a/external-crates/move/crates/move-command-line-common/src/display.rs b/external-crates/move/crates/move-command-line-common/src/display.rs new file mode 100644 index 00000000000000..f55ecde4638b9b --- /dev/null +++ b/external-crates/move/crates/move-command-line-common/src/display.rs @@ -0,0 +1,63 @@ +// Copyright (c) The Move Contributors +// SPDX-License-Identifier: Apache-2.0 + +use move_binary_format::file_format::{Constant, SignatureToken}; +use move_core_types::{account_address::AccountAddress, u256::U256}; + +pub enum RenderResult { + AsString(String), + AsValue(String), + NotRendered, +} + +pub fn try_render_constant(constant: &Constant) -> RenderResult { + let bytes = &constant.data; + match &constant.type_ { + SignatureToken::Vector(inner_ty) if inner_ty.as_ref() == &SignatureToken::U8 => { + bcs::from_bytes::>(bytes) + .ok() + .and_then(|x| String::from_utf8(x).ok()) + .map_or(RenderResult::NotRendered, RenderResult::AsString) + } + SignatureToken::U8 => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_string()) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + SignatureToken::U16 => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_string()) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + SignatureToken::U32 => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_string()) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + SignatureToken::U64 => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_string()) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + SignatureToken::U128 => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_string()) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + SignatureToken::U256 => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_string()) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + SignatureToken::Address => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_canonical_string(true)) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + SignatureToken::Bool => bcs::from_bytes::(bytes) + .ok() + .map(|x| x.to_string()) + .map_or(RenderResult::NotRendered, RenderResult::AsValue), + + SignatureToken::Signer + | SignatureToken::Vector(_) + | SignatureToken::Datatype(_) + | SignatureToken::DatatypeInstantiation(_) + | SignatureToken::Reference(_) + | SignatureToken::MutableReference(_) + | SignatureToken::TypeParameter(_) => RenderResult::NotRendered, + } +} diff --git a/external-crates/move/crates/move-command-line-common/src/lib.rs b/external-crates/move/crates/move-command-line-common/src/lib.rs index 22253b9c4deeaf..2b087266cf27f7 100644 --- a/external-crates/move/crates/move-command-line-common/src/lib.rs +++ b/external-crates/move/crates/move-command-line-common/src/lib.rs @@ -6,6 +6,7 @@ pub mod address; pub mod character_sets; +pub mod display; pub mod env; pub mod error_bitset; pub mod files; diff --git a/external-crates/move/crates/move-disassembler/src/disassembler.rs b/external-crates/move/crates/move-disassembler/src/disassembler.rs index 3b986b61791762..e03a684a579f5c 100644 --- a/external-crates/move/crates/move-disassembler/src/disassembler.rs +++ b/external-crates/move/crates/move-disassembler/src/disassembler.rs @@ -22,11 +22,14 @@ use move_bytecode_source_map::{ mapping::SourceMapping, source_map::{FunctionSourceMap, SourceName}, }; +use move_command_line_common::display::{try_render_constant, RenderResult}; use move_compiler::compiled_unit::CompiledUnit; use move_core_types::{identifier::IdentStr, language_storage::ModuleId}; use move_coverage::coverage_map::{ExecCoverageMap, FunctionCoverage}; use move_ir_types::location::Loc; +const PREVIEW_LEN: usize = 4; + /// Holds the various options that we support while disassembling code. #[derive(Debug, Default, Parser)] pub struct DisassemblerOptions { @@ -437,10 +440,19 @@ impl<'a> Disassembler<'a> { } fn preview_const(slice: &[u8]) -> String { - if slice.len() <= 4 { + // Account for the .. in the preview + if slice.len() <= PREVIEW_LEN + 2 { hex::encode(slice) } else { - format!("{}..", hex::encode(&slice[..4])) + format!("{}..", hex::encode(&slice[..PREVIEW_LEN])) + } + } + + fn preview_string(s: &str) -> String { + if s.len() <= PREVIEW_LEN + 2 { + s.to_string() + } else { + format!("{}..", &s[..PREVIEW_LEN]) } } @@ -522,6 +534,7 @@ impl<'a> Disassembler<'a> { fn disassemble_instruction( &self, parameters: &Signature, + constants: &[(String, String)], instruction: &Bytecode, locals_sigs: &Signature, function_source_map: &FunctionSourceMap, @@ -529,13 +542,20 @@ impl<'a> Disassembler<'a> { ) -> Result { match instruction { Bytecode::LdConst(idx) => { - let constant = self.source_mapper.bytecode.constant_at(*idx); - Ok(format!( - "LdConst[{}]({:?}: {})", - idx, - &constant.type_, - Self::preview_const(&constant.data), - )) + let constant = constants + .get(idx.0 as usize) + .map(|(ty_str, val_str)| { + format!("{}: {}", ty_str, Self::preview_string(val_str)) + }) + .unwrap_or_else(|| { + let constant = self.source_mapper.bytecode.constant_at(*idx); + format!( + "{:?}: {}", + &constant.type_, + Self::preview_const(&constant.data) + ) + }); + Ok(format!("LdConst[{}]({})", idx, constant,)) } Bytecode::CopyLoc(local_idx) => { let name = @@ -958,6 +978,7 @@ impl<'a> Disassembler<'a> { function_source_map: &FunctionSourceMap, function_name: &IdentStr, parameters: SignatureIndex, + constants: &[(String, String)], code: &CodeUnit, ) -> Result> { if !self.options.print_code { @@ -976,6 +997,7 @@ impl<'a> Disassembler<'a> { .map(|instruction| { self.disassemble_instruction( parameters, + constants, instruction, locals_sigs, function_source_map, @@ -1097,6 +1119,7 @@ impl<'a> Disassembler<'a> { type_parameters: &[AbilitySet], parameters: SignatureIndex, code: Option<&CodeUnit>, + constants: &[(String, String)], ) -> Result { debug_assert_eq!( function_source_map.parameters.len(), @@ -1171,8 +1194,13 @@ impl<'a> Disassembler<'a> { Some(code) => { let locals = self.disassemble_locals(function_source_map, code.locals, params.len())?; - let bytecode = - self.disassemble_bytecode(function_source_map, name, parameters, code)?; + let bytecode = self.disassemble_bytecode( + function_source_map, + name, + parameters, + constants, + code, + )?; let jump_tables = self.disassemble_jump_tables(code)?; Self::format_function_body(locals, bytecode, jump_tables) } @@ -1346,25 +1374,14 @@ impl<'a> Disassembler<'a> { )) } - pub fn disassemble_constant( - &self, - constant_index: usize, - Constant { type_, data }: &Constant, - ) -> Result { - let data_str = match type_ { - SignatureToken::Vector(x) if x.as_ref() == &SignatureToken::U8 => { - match bcs::from_bytes::>(data) - .ok() - .and_then(|data| String::from_utf8(data).ok()) - { - Some(str) => "\"".to_owned() + &str + "\" // interpreted as UTF8 string", - None => hex::encode(data), - } - } - _ => hex::encode(data), + pub fn disassemble_constant(&self, constant: &Constant) -> Result<(String, String)> { + let data_str = match try_render_constant(constant) { + RenderResult::NotRendered => hex::encode(&constant.data), + RenderResult::AsValue(v_str) => v_str, + RenderResult::AsString(s) => "\"".to_owned() + &s + "\" // interpreted as UTF8 string", }; - let type_str = self.disassemble_sig_tok(type_.clone(), &[])?; - Ok(format!("\t{constant_index} => {}: {}", type_str, data_str)) + let type_str = self.disassemble_sig_tok(constant.type_.clone(), &[])?; + Ok((type_str, data_str)) } pub fn disassemble(&self) -> Result { @@ -1388,14 +1405,13 @@ impl<'a> Disassembler<'a> { .map(|i| self.disassemble_enum_def(EnumDefinitionIndex(i as TableIndex))) .collect::>>()?; - let constants: Vec = self + let constants: Vec<(String, String)> = self .source_mapper .bytecode .constant_pool() .iter() - .enumerate() - .map(|(i, constant)| self.disassemble_constant(i, constant)) - .collect::>>()?; + .map(|constant| self.disassemble_constant(constant)) + .collect::>>()?; let function_defs: Vec = (0..self.source_mapper.bytecode.function_defs.len()) .map(|i| { @@ -1416,6 +1432,7 @@ impl<'a> Disassembler<'a> { &function_handle.type_parameters, function_handle.parameters, function_definition.code.as_ref(), + &constants, ) }) .collect::>>()?; @@ -1429,7 +1446,12 @@ impl<'a> Disassembler<'a> { } else { format!( "\nConstants [\n{constants}\n]\n", - constants = constants.join("\n") + constants = constants + .into_iter() + .enumerate() + .map(|(i, (ty, s))| format!("\t{i} => {ty}: {s}")) + .collect::>() + .join("\n") ) }; Ok(format!(