Merge branch 'grarco/better-gas-interface' (#3428)

* origin/grarco/better-gas-interface:
  Logs tx used gas
  Increases masp gas limit in genesis files and removes custom setup in integration tests
  update Hermes
  Returns `WholeGas` from `dry_run_tx`
  Adds conversion from `WholeGas` to `GasLimit`
  Updates gas in ibc e2e test
  Fixes gas payment in masp integration tests
  Updates gas limit in unit tests
  Changelog #3428
  Changelog #3428
  Removes misleading gas message
  Fixes ibc e2e test
  Fixes unit tests
  Clippy + fmt
  Compacts `BatchResults` into `TxResult`
  Remove gas from `TxResult`. Adjusts dry run result
  Reduces gas scale param in genesis
  Introduces `WholeGas` type

.changelog/unreleased/improvements/3428-better-gas-interface.md

@@ -0,0 +1,2 @@
+- Improved the interface of the gas type. Removed the duplicated gas used from
+  events. ([\#3428](https://github.com/anoma/namada/pull/3428))

.github/workflows/scripts/hermes.txt

@@ -1 +1 @@
-1.9.0-namada-beta13-rc2
+1.9.0-namada-beta14-rc

crates/gas/src/event.rs

@@ -2,13 +2,13 @@
 
 use namada_events::extend::EventAttributeEntry;
 
-use super::Gas;
+use crate::WholeGas;
 
 /// Extend an [`namada_events::Event`] with gas used data.
-pub struct GasUsed(pub Gas);
+pub struct GasUsed(pub WholeGas);
 
 impl EventAttributeEntry<'static> for GasUsed {
-    type Value = Gas;
+    type Value = WholeGas;
     type ValueOwned = Self::Value;
 
     const KEY: &'static str = "gas_used";

crates/gas/src/lib.rs

@@ -116,9 +116,10 @@ pub const MASP_PARALLEL_GAS_DIVIDER: u64 = PARALLEL_GAS_DIVIDER / 2;
 /// Gas module result for functions that may fail
 pub type Result<T> = std::result::Result<T, Error>;
 
-/// Representation of gas in sub-units. This effectively decouples gas metering
-/// from fee payment, allowing higher resolution when accounting for gas while,
-/// at the same time, providing a contained gas value when paying fees.
+/// Representation of tracking gas in sub-units. This effectively decouples gas
+/// metering from fee payment, allowing higher resolution when accounting for
+/// gas while, at the same time, providing a contained gas value when paying
+/// fees.
 #[derive(
     Clone,
     Copy,
@@ -154,8 +155,8 @@ impl Gas {
     }
 
     /// Converts the sub gas units to whole ones. If the sub units are not a
-    /// multiple of the `SCALE` than ceil the quotient
-    pub fn get_whole_gas_units(&self, scale: u64) -> u64 {
+    /// multiple of the scale than ceil the quotient
+    pub fn get_whole_gas_units(&self, scale: u64) -> WholeGas {
         let quotient = self
             .sub
             .checked_div(scale)
@@ -166,18 +167,18 @@ impl Gas {
             .expect("Gas quotient remainder should not overflow")
             == 0
         {
-            quotient
+            quotient.into()
         } else {
             quotient
                 .checked_add(1)
                 .expect("Cannot overflow as the quotient is scaled down u64")
+                .into()
         }
     }
 
-    /// Generates a `Gas` instance from a whole amount
-    pub fn from_whole_units(whole: u64, scale: u64) -> Option<Self> {
-        let sub = whole.checked_mul(scale)?;
-        Some(Self { sub })
+    /// Generates a `Gas` instance from a `WholeGas` amount
+    pub fn from_whole_units(whole: WholeGas, scale: u64) -> Option<Self> {
+        scale.checked_mul(whole.into()).map(|sub| Self { sub })
     }
 }
 
@@ -193,20 +194,46 @@ impl From<Gas> for u64 {
     }
 }
 
-impl Display for Gas {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        // Need to do this now that the gas scale is a parameter. Should
-        // manually scale gas first before calling this
-        write!(f, "{}", self.sub)
+/// Gas represented in whole units. Used for fee payment and to display
+/// information to the user.
+#[derive(
+    Debug,
+    Clone,
+    Copy,
+    PartialEq,
+    BorshSerialize,
+    BorshDeserialize,
+    BorshDeserializer,
+    BorshSchema,
+    Serialize,
+    Deserialize,
+    Eq,
+)]
+pub struct WholeGas(u64);
+
+impl From<u64> for WholeGas {
+    fn from(amount: u64) -> WholeGas {
+        Self(amount)
     }
 }
 
-impl FromStr for Gas {
+impl From<WholeGas> for u64 {
+    fn from(whole: WholeGas) -> u64 {
+        whole.0
+    }
+}
+
+impl FromStr for WholeGas {
     type Err = GasParseError;
 
     fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
-        let raw: u64 = s.parse().map_err(GasParseError::Parse)?;
-        Ok(Self { sub: raw })
+        Ok(Self(s.parse().map_err(GasParseError::Parse)?))
+    }
+}
+
+impl Display for WholeGas {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.0)
     }
 }
 

crates/light_sdk/src/reading/asynchronous/tx.rs

@@ -1,6 +1,6 @@
 use namada_sdk::events::Event;
 use namada_sdk::rpc::{TxEventQuery, TxResponse};
-use namada_sdk::tx::data::TxResult;
+use namada_sdk::tx::data::DryRunResult;
 
 use super::*;
 
@@ -25,7 +25,7 @@ pub async fn query_tx_events(
 pub async fn dry_run_tx(
     tendermint_addr: &str,
     tx_bytes: Vec<u8>,
-) -> Result<TxResult<String>, Error> {
+) -> Result<DryRunResult, Error> {
     let client = HttpClient::new(
         TendermintAddress::from_str(tendermint_addr)
             .map_err(|e| Error::Other(e.to_string()))?,

crates/light_sdk/src/reading/blocking/tx.rs

@@ -26,7 +26,7 @@ pub fn query_tx_events(
 pub fn dry_run_tx(
     tendermint_addr: &str,
     tx_bytes: Vec<u8>,
-) -> Result<TxResult, Error> {
+) -> Result<DryRunResult, Error> {
     let client = HttpClient::new(
         TendermintAddress::from_str(tendermint_addr)
             .map_err(|e| Error::Other(e.to_string()))?,

crates/namada/src/ledger/mod.rs

@@ -25,7 +25,7 @@ mod dry_run_tx {
 
     use namada_sdk::queries::{EncodedResponseQuery, RequestCtx, RequestQuery};
     use namada_state::{DBIter, ResultExt, StorageHasher, DB};
-    use namada_tx::data::{ExtendedTxResult, GasLimit, TxResult};
+    use namada_tx::data::{DryRunResult, ExtendedTxResult, GasLimit, TxResult};
 
     use super::protocol;
     use crate::vm::wasm::{TxCache, VpCache};
@@ -132,18 +132,22 @@ mod dry_run_tx {
             } else {
                 temp_state.write_log_mut().drop_tx();
             }
-            tx_result.batch_results.insert_inner_tx_result(
+            tx_result.insert_inner_tx_result(
                 wrapper_hash.as_ref(),
                 either::Right(cmt),
                 batched_tx_result,
             );
         }
         // Account gas for both batch and wrapper
-        tx_result.gas_used = tx_gas_meter.borrow().get_tx_consumed_gas();
+        let gas_used = tx_gas_meter
+            .borrow()
+            .get_tx_consumed_gas()
+            .get_whole_gas_units(gas_scale);
         let tx_result_string = tx_result.to_result_string();
-        let data = tx_result_string.serialize_to_vec();
+        let dry_run_result = DryRunResult(tx_result_string, gas_used);
+
         Ok(EncodedResponseQuery {
-            data,
+            data: dry_run_result.serialize_to_vec(),
             proof: None,
             info: Default::default(),
             height: ctx.state.in_mem().get_last_block_height(),
@@ -331,7 +335,7 @@ mod test {
         assert!(
             result
                 .data
-                .batch_results
+                .0
                 .get_inner_tx_result(None, either::Right(cmt))
                 .unwrap()
                 .as_ref()

crates/namada/src/ledger/protocol/mod.rs

@@ -19,8 +19,8 @@ use namada_token::utils::is_masp_transfer;
 use namada_tx::action::Read;
 use namada_tx::data::protocol::{ProtocolTx, ProtocolTxType};
 use namada_tx::data::{
-    BatchResults, BatchedTxResult, ExtendedTxResult, TxResult, VpStatusFlags,
-    VpsResult, WrapperTx,
+    BatchedTxResult, ExtendedTxResult, TxResult, VpStatusFlags, VpsResult,
+    WrapperTx,
 };
 use namada_tx::{BatchedTxRef, Tx, TxCommitments};
 use namada_vote_ext::EthereumTxData;
@@ -275,17 +275,14 @@ where
                     },
                 )?;
 
-                Ok(TxResult {
-                    gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(),
-                    batch_results: {
-                        let mut batch_results = BatchResults::new();
-                        batch_results.insert_inner_tx_result(
-                            wrapper_hash,
-                            either::Right(cmt),
-                            Ok(batched_tx_result),
-                        );
-                        batch_results
-                    },
+                Ok({
+                    let mut batch_results = TxResult::new();
+                    batch_results.insert_inner_tx_result(
+                        wrapper_hash,
+                        either::Right(cmt),
+                        Ok(batched_tx_result),
+                    );
+                    batch_results
                 }
                 .to_extended_result(None))
             }
@@ -296,17 +293,14 @@ where
             let batched_tx_result =
                 apply_protocol_tx(protocol_tx.tx, tx.data(cmt), state)?;
 
-            Ok(TxResult {
-                batch_results: {
-                    let mut batch_results = BatchResults::new();
-                    batch_results.insert_inner_tx_result(
-                        None,
-                        either::Right(cmt),
-                        Ok(batched_tx_result),
-                    );
-                    batch_results
-                },
-                ..Default::default()
+            Ok({
+                let mut batch_results = TxResult::new();
+                batch_results.insert_inner_tx_result(
+                    None,
+                    either::Right(cmt),
+                    Ok(batched_tx_result),
+                );
+                batch_results
             }
             .to_extended_result(None))
         }
@@ -382,16 +376,11 @@ where
         ) {
             Err(Error::GasError(ref msg)) => {
                 // Gas error aborts the execution of the entire batch
-                extended_tx_result.tx_result.gas_used =
-                    tx_gas_meter.borrow().get_tx_consumed_gas();
-                extended_tx_result
-                    .tx_result
-                    .batch_results
-                    .insert_inner_tx_result(
-                        wrapper_hash,
-                        either::Right(cmt),
-                        Err(Error::GasError(msg.to_owned())),
-                    );
+                extended_tx_result.tx_result.insert_inner_tx_result(
+                    wrapper_hash,
+                    either::Right(cmt),
+                    Err(Error::GasError(msg.to_owned())),
+                );
                 state.write_log_mut().drop_tx();
                 return Err(DispatchError {
                     error: Error::GasError(msg.to_owned()),
@@ -402,16 +391,11 @@ where
                 let is_accepted =
                     matches!(&res, Ok(result) if result.is_accepted());
 
-                extended_tx_result
-                    .tx_result
-                    .batch_results
-                    .insert_inner_tx_result(
-                        wrapper_hash,
-                        either::Right(cmt),
-                        res,
-                    );
-                extended_tx_result.tx_result.gas_used =
-                    tx_gas_meter.borrow().get_tx_consumed_gas();
+                extended_tx_result.tx_result.insert_inner_tx_result(
+                    wrapper_hash,
+                    either::Right(cmt),
+                    res,
+                );
                 if is_accepted {
                     // If the transaction was a masp one append the
                     // transaction refs for the events
@@ -488,9 +472,9 @@ where
     shell_params.state.write_log_mut().commit_batch();
 
     let (batch_results, masp_tx_refs) = payment_result.map_or_else(
-        || (BatchResults::default(), None),
+        || (TxResult::default(), None),
         |(batched_result, masp_section_ref)| {
-            let mut batch = BatchResults::default();
+            let mut batch = TxResult::default();
             batch.insert_inner_tx_result(
                 // Ok to unwrap cause if we have a batched result it means
                 // we've executed the first tx in the batch
@@ -508,11 +492,7 @@ where
         .add_wrapper_gas(tx_bytes)
         .map_err(|err| Error::GasError(err.to_string()))?;
 
-    Ok(TxResult {
-        gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(),
-        batch_results,
-    }
-    .to_extended_result(masp_tx_refs))
+    Ok(batch_results.to_extended_result(masp_tx_refs))
 }
 
 /// Perform the actual transfer of fees from the fee payer to the block
@@ -702,7 +682,7 @@ where
     let gas_scale = get_gas_scale(&**state).map_err(Error::StorageError)?;
 
     let mut gas_meter = TxGasMeter::new(
-        namada_gas::Gas::from_whole_units(max_gas_limit, gas_scale)
+        namada_gas::Gas::from_whole_units(max_gas_limit.into(), gas_scale)
             .ok_or_else(|| {
                 Error::GasError("Overflow in gas expansion".to_string())
             })?,

crates/node/src/bench_utils.rs

@@ -77,9 +77,7 @@ use namada::masp::MaspTxRefs;
 use namada::state::StorageRead;
 use namada::token::{Amount, DenominatedAmount, Transfer};
 use namada::tx::data::pos::Bond;
-use namada::tx::data::{
-    BatchResults, BatchedTxResult, Fee, TxResult, VpsResult,
-};
+use namada::tx::data::{BatchedTxResult, Fee, TxResult, VpsResult};
 use namada::tx::event::{new_tx_event, Batch};
 use namada::tx::{
     Authorization, BatchedTx, BatchedTxRef, Code, Data, Section, Tx,
@@ -919,24 +917,19 @@ impl Client for BenchShell {
                     .iter()
                     .enumerate()
                     .map(|(idx, (tx, changed_keys))| {
-                        let tx_result = TxResult::<String> {
-                            gas_used: 0.into(),
-                            batch_results: {
-                                let mut batch_results = BatchResults::new();
-                                batch_results.insert_inner_tx_result(
-                                    tx.wrapper_hash().as_ref(),
-                                    either::Right(
-                                        tx.first_commitments().unwrap(),
-                                    ),
-                                    Ok(BatchedTxResult {
-                                        changed_keys: changed_keys.to_owned(),
-                                        vps_result: VpsResult::default(),
-                                        initialized_accounts: vec![],
-                                        events: BTreeSet::default(),
-                                    }),
-                                );
-                                batch_results
-                            },
+                        let tx_result = {
+                            let mut batch_results = TxResult::new();
+                            batch_results.insert_inner_tx_result(
+                                tx.wrapper_hash().as_ref(),
+                                either::Right(tx.first_commitments().unwrap()),
+                                Ok(BatchedTxResult {
+                                    changed_keys: changed_keys.to_owned(),
+                                    vps_result: VpsResult::default(),
+                                    initialized_accounts: vec![],
+                                    events: BTreeSet::default(),
+                                }),
+                            );
+                            batch_results
                         };
                         let event: Event = new_tx_event(tx, height.value())
                             .with(Batch(&tx_result))