Merge pull request #4189 from anoma/mergify/bp/maint-libs-0.46/pr-4182

Improve display of transactions' results (backport #4182)

.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))

crates/sdk/src/rpc.rs

@@ -577,21 +577,22 @@ pub async fn dry_run_tx<N: Namada>(
             .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<N: Namada>(
                             .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)
 }
 

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()

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<u32> {
     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();