add tests & refactor tx crate

.changelog/unreleased/improvements/3835-refactor-tx-crate.md

@@ -0,0 +1,2 @@
+- Refactored tx crate modules and slightly improved its API.
+  ([\#3835](https://github.com/anoma/namada/pull/3835))

crates/apps_lib/src/client/tx.rs

@@ -1069,7 +1069,8 @@ pub async fn sign_tx<N: Namada>(
 where
     <N::Client as namada_sdk::io::Client>::Error: std::fmt::Display,
 {
-    let tx = if let Ok(transaction) = Tx::deserialize(tx_data.as_ref()) {
+    let tx = if let Ok(transaction) = Tx::try_from_json_bytes(tx_data.as_ref())
+    {
         transaction
     } else {
         edisplay_line!(namada.io(), "Couldn't decode the transaction.");
@@ -1117,8 +1118,9 @@ where
             };
             let signature_path = File::create(&output_path)
                 .expect("Should be able to create signature file.");
-            serde_json::to_writer_pretty(signature_path, &signature)
-                .expect("Signature should be serializable.");
+            signature.to_writer_json(signature_path).expect(
+                "Signature should be serializable and the file writeable.",
+            );
 
             display_line!(
                 namada.io(),

crates/apps_lib/src/client/utils.rs

@@ -1050,7 +1050,8 @@ pub async fn sign_offline(
         safe_exit(1)
     };
 
-    let tx = if let Ok(transaction) = Tx::deserialize(tx_data.as_ref()) {
+    let tx = if let Ok(transaction) = Tx::try_from_json_bytes(tx_data.as_ref())
+    {
         transaction
     } else {
         eprintln!("Couldn't decode the transaction.");

crates/benches/native_vps.rs

@@ -1684,7 +1684,7 @@ fn ibc_vp_validate_action(c: &mut Criterion) {
 
         let verifiers_from_tx =
             shielded_ctx.shell.write().execute_tx(&signed_tx.to_ref());
-        let tx_data = signed_tx.tx.data(&signed_tx.cmt).unwrap();
+        let tx_data = signed_tx.to_ref().data().unwrap();
         let shell_read = shielded_ctx.shell.read();
         let (verifiers, keys_changed) = shell_read
             .state
@@ -1745,7 +1745,7 @@ fn ibc_vp_execute_action(c: &mut Criterion) {
         let verifiers_from_tx =
             shielded_ctx.shell.write().execute_tx(&signed_tx.to_ref());
         let shell_read = shielded_ctx.shell.read();
-        let tx_data = signed_tx.tx.data(&signed_tx.cmt).unwrap();
+        let tx_data = signed_tx.to_ref().data().unwrap();
         let (verifiers, keys_changed) = shell_read
             .state
             .write_log()

crates/ethereum_bridge/src/vp/bridge_pool_vp.rs

@@ -87,7 +87,7 @@ where
                 "Rejecting transaction, since the Ethereum bridge is disabled.",
             ));
         }
-        let Some(tx_data) = batched_tx.tx.data(batched_tx.cmt) else {
+        let Some(tx_data) = batched_tx.data() else {
             return Err(Error::new_const("No transaction data found"));
         };
         let transfer: PendingTransfer =

crates/governance/src/vp/mod.rs

@@ -60,7 +60,7 @@ where
     /// Run the validity predicate
     pub fn validate_tx(
         ctx: &'ctx CTX,
-        tx_data: &BatchedTxRef<'_>,
+        tx: &BatchedTxRef<'_>,
         keys_changed: &BTreeSet<storage::Key>,
         verifiers: &BTreeSet<Address>,
     ) -> Result<()> {
@@ -74,7 +74,7 @@ where
         // Is VP triggered by a governance proposal?
         if is_proposal_accepted(
             &ctx.pre(),
-            tx_data.tx.data(tx_data.cmt).unwrap_or_default().as_ref(),
+            tx.data().unwrap_or_default().as_ref(),
         )? {
             return Ok(());
         }
@@ -168,9 +168,7 @@ where
                 (KeyType::PROPOSAL_COMMIT, _) => {
                     Self::is_valid_proposal_commit(ctx)
                 }
-                (KeyType::PARAMETER, _) => {
-                    Self::is_valid_parameter(ctx, tx_data)
-                }
+                (KeyType::PARAMETER, _) => Self::is_valid_parameter(ctx, tx),
                 (KeyType::BALANCE, _) => {
                     Self::is_valid_balance(ctx, &native_token)
                 }
@@ -988,8 +986,7 @@ where
         ctx: &'ctx CTX,
         batched_tx: &BatchedTxRef<'_>,
     ) -> Result<()> {
-        let BatchedTxRef { tx, cmt } = batched_tx;
-        tx.data(cmt).map_or_else(
+        batched_tx.data().map_or_else(
             || {
                 Err(Error::new_const(
                     "Governance parameter changes require tx data to be \

crates/governance/src/vp/pgf.rs

@@ -55,11 +55,7 @@ where
         // Is VP triggered by a governance proposal?
         if is_proposal_accepted(
             &ctx.pre(),
-            batched_tx
-                .tx
-                .data(batched_tx.cmt)
-                .unwrap_or_default()
-                .as_ref(),
+            batched_tx.data().unwrap_or_default().as_ref(),
         )? {
             return Ok(());
         }
@@ -189,7 +185,7 @@ where
         ctx: &'ctx CTX,
         batched_tx: &BatchedTxRef<'_>,
     ) -> Result<()> {
-        batched_tx.tx.data(batched_tx.cmt).map_or_else(
+        batched_tx.data().map_or_else(
             || {
                 Err(Error::new_const(
                     "PGF parameter changes require tx data to be present",

crates/ibc/src/vp/mod.rs

@@ -148,19 +148,12 @@ where
         // Is VP triggered by a governance proposal?
         if Gov::is_proposal_accepted(
             &self.ctx.pre(),
-            batched_tx
-                .tx
-                .data(batched_tx.cmt)
-                .unwrap_or_default()
-                .as_ref(),
+            batched_tx.data().unwrap_or_default().as_ref(),
         )? {
             return Ok(());
         }
 
-        let tx_data = batched_tx
-            .tx
-            .data(batched_tx.cmt)
-            .ok_or(VpError::NoTxData)?;
+        let tx_data = batched_tx.data().ok_or(VpError::NoTxData)?;
 
         // Pseudo execution and compare them
         self.validate_state(&tx_data, keys_changed)?;

crates/node/src/bench_utils.rs

@@ -1338,7 +1338,7 @@ impl BenchShieldedCtx {
         };
 
         let vectorized_transfer =
-            Transfer::deserialize(&mut tx.tx.data(&tx.cmt).unwrap().as_slice())
+            Transfer::deserialize(&mut tx.to_ref().data().unwrap().as_slice())
                 .unwrap();
         let sources =
             vec![vectorized_transfer.sources.into_iter().next().unwrap()]

crates/node/src/dry_run_tx.rs

@@ -29,7 +29,7 @@ where
     H: 'static + StorageHasher + Sync,
     CA: 'static + WasmCacheAccess + Sync,
 {
-    let tx = Tx::try_from(&request.data[..]).into_storage_result()?;
+    let tx = Tx::try_from_bytes(&request.data[..]).into_storage_result()?;
     tx.validate_tx().into_storage_result()?;
 
     let gas_scale = parameters::get_gas_scale(&state)?;

crates/node/src/protocol.rs

@@ -28,7 +28,7 @@ use namada_sdk::tx::data::{
     BatchedTxResult, ExtendedTxResult, TxResult, VpStatusFlags, VpsResult,
     WrapperTx,
 };
-use namada_sdk::tx::{BatchedTxRef, Tx, TxCommitments};
+use namada_sdk::tx::{BatchedTxRef, InnerTxRef, Tx, TxCommitments};
 use namada_sdk::validation::{
     EthBridgeNutVp, EthBridgePoolVp, EthBridgeVp, GovernanceVp, IbcVp, MaspVp,
     MultitokenVp, NativeVpCtx, ParametersVp, PgfVp, PosVp,
@@ -272,8 +272,11 @@ where
         DispatchArgs::Protocol(protocol_tx) => {
             // No bundles of protocol transactions, only take the first one
             let cmt = tx.first_commitments().ok_or(Error::MissingInnerTxs)?;
-            let batched_tx_result =
-                apply_protocol_tx(protocol_tx.tx, tx.data(cmt), state)?;
+            let batched_tx_result = apply_protocol_tx(
+                protocol_tx.tx,
+                tx.data(InnerTxRef::Commitment(cmt)),
+                state,
+            )?;
 
             Ok({
                 let mut batch_results = TxResult::new();

crates/node/src/shell/finalize_block.rs

@@ -605,16 +605,17 @@ where
         let mut successful_wrappers = vec![];
 
         for (tx_index, processed_tx) in processed_txs.iter().enumerate() {
-            let tx = if let Ok(tx) = Tx::try_from(processed_tx.tx.as_ref()) {
-                tx
-            } else {
-                tracing::error!(
-                    "FinalizeBlock received a tx that could not be \
-                     deserialized to a Tx type. This is likely a protocol \
-                     transaction."
-                );
-                continue;
-            };
+            let tx =
+                if let Ok(tx) = Tx::try_from_bytes(processed_tx.tx.as_ref()) {
+                    tx
+                } else {
+                    tracing::error!(
+                        "FinalizeBlock received a tx that could not be \
+                         deserialized to a Tx type. This is likely a protocol \
+                         transaction."
+                    );
+                    continue;
+                };
 
             let result_code = ResultCode::from_u32(processed_tx.result.code)
                 .expect("Result code conversion should not fail");

crates/node/src/shell/mod.rs

@@ -1057,7 +1057,7 @@ where
         }
 
         // Tx format check
-        let tx = match Tx::try_from(tx_bytes).map_err(Error::TxDecoding) {
+        let tx = match Tx::try_from_bytes(tx_bytes).map_err(Error::TxDecoding) {
             Ok(t) => t,
             Err(msg) => {
                 response.code = ResultCode::InvalidTx.into();
@@ -2108,7 +2108,7 @@ mod shell_tests {
             )
             .await
             .unwrap();
-            let tx = Tx::try_from(&serialized_tx[..]).unwrap();
+            let tx = Tx::try_from_bytes(&serialized_tx[..]).unwrap();
 
             match ethereum_tx_data_variants::ValSetUpdateVext::try_from(&tx) {
                 Ok(signed_valset_upd) => break signed_valset_upd,
@@ -2158,7 +2158,7 @@ mod shell_tests {
         // attempt to receive vote extension tx aggregating
         // all expired events
         let serialized_tx = broadcaster_rx.blocking_recv().unwrap();
-        let tx = Tx::try_from(&serialized_tx[..]).unwrap();
+        let tx = Tx::try_from_bytes(&serialized_tx[..]).unwrap();
 
         // check data inside tx
         let vote_extension =

crates/node/src/shell/prepare_proposal.rs

@@ -289,7 +289,7 @@ where
     H: StorageHasher + Sync + 'static,
     CA: 'static + WasmCacheAccess + Sync,
 {
-    let tx = Tx::try_from(tx_bytes).map_err(|_| ())?;
+    let tx = Tx::try_from_bytes(tx_bytes).map_err(|_| ())?;
     let wrapper = tx.header.wrapper().ok_or(())?;
 
     // If tx doesn't have an expiration it is valid. If time cannot be
@@ -754,7 +754,7 @@ mod test_prepare_proposal {
         assert_eq!(rsp.txs.len(), 1);
 
         let tx_bytes = rsp.txs.remove(0);
-        let got = Tx::try_from(&tx_bytes[..]).unwrap();
+        let got = Tx::try_from_bytes(&tx_bytes[..]).unwrap();
         let eth_tx_data = (&got).try_into().expect("Test failed");
         let rsp_ext = match eth_tx_data {
             EthereumTxData::EthEventsVext(ext) => ext,
@@ -1371,7 +1371,7 @@ mod test_prepare_proposal {
             };
             let proposed_txs =
                 shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| {
-                    Tx::try_from(tx_bytes.as_ref()).expect("Test failed")
+                    Tx::try_from_bytes(tx_bytes.as_ref()).expect("Test failed")
                 });
             // since no events with valid nonces are contained in the vote
             // extension, we drop it from the proposal
@@ -1419,7 +1419,7 @@ mod test_prepare_proposal {
             };
             let proposed_txs =
                 shell.prepare_proposal(req).txs.into_iter().map(|tx_bytes| {
-                    Tx::try_from(tx_bytes.as_ref()).expect("Test failed")
+                    Tx::try_from_bytes(tx_bytes.as_ref()).expect("Test failed")
                 });
             // find the event with the good nonce
             let mut ext = 'ext: {

crates/node/src/shell/process_proposal.rs

@@ -238,7 +238,7 @@ where
             };
         }
 
-        let maybe_tx = Tx::try_from(tx_bytes).map_or_else(
+        let maybe_tx = Tx::try_from_bytes(tx_bytes).map_or_else(
             |err| {
                 tracing::debug!(
                     ?err,

crates/node/src/shell/vote_extensions.rs

@@ -124,7 +124,7 @@ where
     ) -> DrainFilter<'shell, TxBytes, impl FnMut(&mut TxBytes) -> bool + 'shell>
     {
         drain_filter_polyfill::VecExt::drain_filter(txs, move |tx_bytes| {
-            let tx = match Tx::try_from(tx_bytes.as_ref()) {
+            let tx = match Tx::try_from_bytes(tx_bytes.as_ref()) {
                 Ok(tx) => tx,
                 Err(err) => {
                     tracing::warn!(

crates/parameters/src/vp.rs

@@ -31,7 +31,7 @@ where
     ) -> Result<()> {
         keys_changed.iter().try_for_each(|key| {
             let key_type: KeyType = key.into();
-            let data = if let Some(data) = batched_tx.tx.data(batched_tx.cmt) {
+            let data = if let Some(data) = batched_tx.data() {
                 data
             } else {
                 return Err(Error::new_const(

crates/proof_of_stake/src/vp.rs

@@ -10,7 +10,7 @@ use namada_systems::governance;
 use namada_tx::action::{
     Action, Bond, ClaimRewards, PosAction, Redelegation, Unbond, Withdraw,
 };
-use namada_tx::BatchedTxRef;
+use namada_tx::{BatchedTxRef, InnerTxRef};
 use namada_vp_env::{Error, Result, VpEnv};
 use thiserror::Error;
 
@@ -57,7 +57,7 @@ where
         // Check if this is a governance proposal first
         if batched_tx
             .tx
-            .data(batched_tx.cmt)
+            .data(InnerTxRef::Index(0))
             .map(|tx_data| Gov::is_proposal_accepted(&ctx.pre(), &tx_data))
             .transpose()?
             .unwrap_or(false)

crates/sdk/src/masp.rs

@@ -20,7 +20,7 @@ use namada_ibc::{decode_message, extract_masp_tx_from_envelope, IbcMessage};
 use namada_io::client::Client;
 use namada_token::masp::shielded_wallet::ShieldedQueries;
 pub use namada_token::masp::{utils, *};
-use namada_tx::Tx;
+use namada_tx::{InnerTxRef, Tx};
 pub use utilities::{IndexerMaspClient, LedgerMaspClient};
 
 use crate::error::{Error, QueryError};
@@ -72,20 +72,13 @@ fn extract_masp_tx(
                     // Dereference the masp ref to the first instance that
                     // matches is, even if it is not the exact one that produced
                     // the event, the data we extract will be exactly the same
-                    let masp_ibc_tx = tx
-                        .commitments()
-                        .iter()
-                        .find(|cmt| cmt.data_sechash() == hash)
+                    let tx_data = tx
+                        .data(InnerTxRef::CommitmentHash(*hash))
                         .ok_or_else(|| {
                             Error::Other(format!(
-                                "Couldn't find data section with hash {hash}"
+                                "Couldn't find data section with hash {hash}",
                             ))
                         })?;
-                    let tx_data = tx.data(masp_ibc_tx).ok_or_else(|| {
-                        Error::Other(
-                            "Missing expected data section".to_string(),
-                        )
-                    })?;
 
                     let IbcMessage::Envelope(envelope) =
                         decode_message::<token::Transfer>(&tx_data)

crates/sdk/src/masp/utilities.rs

@@ -110,8 +110,9 @@ impl<C: Client + Send + Sync> MaspClient for LedgerMaspClient<C> {
                 masp_refs,
             } in txs_results
             {
-                let tx = Tx::try_from(block[tx_index.0 as usize].as_ref())
-                    .map_err(|e| Error::Other(e.to_string()))?;
+                let tx =
+                    Tx::try_from_bytes(block[tx_index.0 as usize].as_ref())
+                        .map_err(|e| Error::Other(e.to_string()))?;
                 let extracted_masp_txs = extract_masp_tx(&tx, &masp_refs)
                     .map_err(|e| Error::Other(e.to_string()))?;