diff --git a/.changelog/unreleased/improvements/4182-tx-result-display.md b/.changelog/unreleased/improvements/4182-tx-result-display.md new file mode 100644 index 0000000000..b96a94c936 --- /dev/null +++ b/.changelog/unreleased/improvements/4182-tx-result-display.md @@ -0,0 +1,4 @@ +- The display of a transaction's result now includes the result code + and avoids displaying the gas unless the transaction was successful. + Improved the display of a dry-run and aligned it with the former. + ([\#4182](https://github.com/anoma/namada/pull/4182)) \ No newline at end of file diff --git a/crates/sdk/src/rpc.rs b/crates/sdk/src/rpc.rs index 32f3c706c8..9f747aa815 100644 --- a/crates/sdk/src/rpc.rs +++ b/crates/sdk/src/rpc.rs @@ -577,21 +577,22 @@ pub async fn dry_run_tx( .await, )? .data; - let result_str = format!("Transaction consumed {} gas", result.1); - let mut cmt_result_str = String::new(); + display_line!(context.io(), "Dry-run result:"); + let mut all_inners_successful = true; for (inner_hash, cmt_result) in result.0.iter() { match cmt_result { Ok(result) => { if result.is_accepted() { - cmt_result_str.push_str(&format!( - "Inner transaction {inner_hash} was successfully \ - applied", - )); + display_line!( + context.io(), + "Transaction {inner_hash} was successfully applied", + ); } else { - cmt_result_str.push_str(&format!( - "Inner transaction {} was rejected by VPs: \ - {}\nErrors: {}\nChanged keys: {}", + display_line!( + context.io(), + "Transaction {} was rejected by VPs: {}\nErrors: \ + {}\nChanged keys: {}", inner_hash, serde_json::to_string_pretty( &result.vps_result.rejected_vps @@ -601,18 +602,33 @@ pub async fn dry_run_tx( .unwrap(), serde_json::to_string_pretty(&result.changed_keys) .unwrap(), - )) + ); + all_inners_successful = false; } } - Err(msg) => cmt_result_str.push_str(&format!( - "Inner transaction {inner_hash} failed with error: {msg}" - )), + Err(msg) => { + display_line!( + context.io(), + "Transaction {inner_hash} failed.\nDetails: {msg}" + ); + all_inners_successful = false; + } } } - display_line!( - context.io(), - "Dry-run result: {result_str}. {cmt_result_str}" - ); + + // Display the gas used only if the entire batch was successful. In all the + // other cases the gas consumed is misleading since most likely the inner + // transactions did not have the chance to run until completion. This could + // trick the user into setting wrong gas limit values when trying to + // resubmit the tx + if all_inners_successful { + display_line!( + context.io(), + "The batch consumed {} gas units.", + result.1 + ); + } + Ok(result) } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 4b3f6cf293..19a6a5ba17 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -161,7 +161,7 @@ pub enum ProcessTxResponse { } impl ProcessTxResponse { - /// Returns a `TxResult` if the transaction applied and was it accepted by + /// Returns a `TxResult` if the transaction was applied and accepted by /// all VPs. Note that this always returns false for dry-run transactions. pub fn is_applied_and_valid( &self, @@ -435,20 +435,59 @@ pub async fn submit_tx( /// Display a result of a tx batch. pub fn display_batch_resp(context: &impl Namada, resp: &TxResponse) { // Wrapper-level logs - display_line!( - context.io(), - "Transaction batch {} was applied at height {}, consuming {} gas \ - units.", - resp.hash, - resp.height, - resp.gas_used - ); + let wrapper_successful = if let ResultCode::Ok = resp.code { + display_line!( + context.io(), + "Transaction batch {} was applied at height {}.", + resp.hash, + resp.height, + ); + true + } else { + let err = match resp.code { + ResultCode::Ok => unreachable!(), + ResultCode::WasmRuntimeError => "wasm runtime", + ResultCode::InvalidTx => "invalid transaction", + ResultCode::InvalidSig => "invalid signature", + ResultCode::AllocationError => "allocation", + ResultCode::ReplayTx => "transaction replay", + ResultCode::InvalidChainId => "invalid chain ID", + ResultCode::ExpiredTx => "transaction expired", + ResultCode::TxGasLimit => "gas limit", + ResultCode::FeeError => "fee", + ResultCode::InvalidVoteExtension => "invalid vote extension", + ResultCode::TooLarge => "transaction too large", + ResultCode::TxNotAllowlisted => "transaction not allowlisted", + }; + let err_msg = if resp.info.is_empty() { + err.to_string() + } else { + format!("{err}, {}", resp.info) + }; + display_line!( + context.io(), + "Transaction batch {} failed at height {} with error: {}.", + resp.hash, + resp.height, + err_msg + ); + false + }; let batch_results = resp.batch_result(); if !batch_results.is_empty() { + if !wrapper_successful { + display_line!( + context.io(), + "Since the batch in its entirety failed, none of the \ + transactions listed below have been committed. Their results \ + are provided for completeness.", + ); + } display_line!(context.io(), "Batch results:"); } // Batch-level logs + let mut all_inners_successful = true; for (inner_hash, result) in batch_results { match result { InnerTxResult::Success(_) => { @@ -477,6 +516,7 @@ pub fn display_batch_resp(context: &impl Namada, resp: &TxResponse) { .unwrap(), serde_json::to_string_pretty(&changed_keys).unwrap(), ); + all_inners_successful = false; } InnerTxResult::OtherFailure(msg) => { edisplay_line!( @@ -485,10 +525,24 @@ pub fn display_batch_resp(context: &impl Namada, resp: &TxResponse) { inner_hash, msg ); + all_inners_successful = false; } } } + // Display the gas used only if the entire batch was successful. In all the + // other cases the gas consumed is misleading since most likely the inner + // transactions did not have the chance to run until completion. This could + // trick the user into setting wrong gas limit values when trying to + // resubmit the tx + if wrapper_successful && all_inners_successful { + edisplay_line!( + context.io(), + "The batch consumed {} gas units.", + resp.gas_used, + ); + } + tracing::debug!( "Full result: {}", serde_json::to_string_pretty(&resp).unwrap() diff --git a/crates/tests/src/e2e/ibc_tests.rs b/crates/tests/src/e2e/ibc_tests.rs index 26500ac32c..c4c09694cc 100644 --- a/crates/tests/src/e2e/ibc_tests.rs +++ b/crates/tests/src/e2e/ibc_tests.rs @@ -1894,13 +1894,13 @@ fn transfer_from_cosmos( fn check_tx_height(test: &Test, client: &mut NamadaCmd) -> Result { let (_unread, matched) = client.exp_regex(r"height .*")?; - // Expecting e.g. "height 1337, consuming x gas units." + // Expecting e.g. "height 1337." let height_str = matched .trim() .split_once(' ') .unwrap() .1 - .split_once(',') + .split_once('.') .unwrap() .0; let height: u32 = height_str.parse().unwrap();