Merge branch 'mergify/bp/0.44.0/pr-3868' (#3871)

* mergify/bp/0.44.0/pr-3868:
  changelog: add #3835
  tx: test tx sections api
  tx: improve the serialization api
  test/tx: add more unit tests
  tx: split out tx sections into dedicated mod
  tx: move signing related code into sign mod
  tx: rm unsued `SignedTxData`
  refactor and test signature index
  tx/types: rm unused code
  test/e2e: no dbg logs in shielded sync

.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/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/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();
@@ -2109,7 +2109,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,
@@ -2159,7 +2159,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,
@@ -1365,7 +1365,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
@@ -1413,7 +1413,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/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()))?;
 

crates/sdk/src/signing.rs

@@ -224,7 +224,7 @@ where
             .iter()
             .map(|bytes| {
                 let sigidx =
-                    serde_json::from_slice::<SignatureIndex>(bytes).unwrap();
+                    SignatureIndex::try_from_json_bytes(bytes).unwrap();
                 used_pubkeys.insert(sigidx.pubkey.clone());
                 sigidx
             })

crates/sdk/src/tx.rs

@@ -200,7 +200,7 @@ pub fn dump_tx<IO: Io>(io: &IO, args: &args::Tx, tx: Tx) {
             ));
             let out = File::create(&tx_path)
                 .expect("Should be able to create a file to dump tx");
-            serde_json::to_writer_pretty(out, &tx)
+            tx.to_writer_json(out)
                 .expect("Should be able to write to file.");
             display_line!(
                 io,
@@ -3734,7 +3734,7 @@ pub async fn build_custom(
     let fee_amount = validate_fee(context, tx_args).await?;
 
     let mut tx = if let Some(serialized_tx) = serialized_tx {
-        Tx::deserialize(serialized_tx.as_ref()).map_err(|_| {
+        Tx::try_from_json_bytes(serialized_tx.as_ref()).map_err(|_| {
             Error::Other(
                 "Invalid tx deserialization. Please make sure you are passing \
                  a file in .tx format, typically produced from using the \

crates/tests/src/e2e/setup.rs

@@ -17,7 +17,7 @@ use expectrl::session::Session;
 use expectrl::stream::log::LogStream;
 use expectrl::{ControlCode, Eof, WaitStatus};
 use eyre::eyre;
-use itertools::{Either, Itertools};
+use itertools::{peek_nth, Either, Itertools};
 use namada_apps_lib::cli::context::ENV_VAR_CHAIN_ID;
 use namada_apps_lib::client::utils::{
     self, validator_pre_genesis_dir, validator_pre_genesis_txs_file,
@@ -1091,22 +1091,43 @@ where
     I: IntoIterator<Item = S>,
     S: AsRef<OsStr>,
 {
+    let mut args = peek_nth(args);
+    let is_node_ledger = (matches!(bin, Bin::Node)
+        && args
+            .peek()
+            .map(|fst_arg| fst_arg.as_ref() == "ledger")
+            .unwrap_or_default())
+        || (matches!(bin, Bin::Namada)
+            && args
+                .peek()
+                .map(|fst_arg| fst_arg.as_ref() == "node")
+                .unwrap_or_default()
+            && args
+                .peek_nth(1)
+                .map(|snd_arg| snd_arg.as_ref() == "ledger")
+                .unwrap_or_default());
+    let is_shielded_sync = matches!(bin, Bin::Client)
+        && args
+            .peek()
+            .map(|fst_arg| fst_arg.as_ref() == "shielded-sync")
+            .unwrap_or_default();
+
     // Root cargo workspace manifest path
     let (bin_name, log_level) = match bin {
         Bin::Namada => ("namada", "info"),
         Bin::Node => ("namadan", "info"),
-        Bin::Client => ("namadac", "tendermint_rpc=debug"),
+        Bin::Client => (
+            "namadac",
+            if is_shielded_sync {
+                "info"
+            } else {
+                "tendermint_rpc=debug"
+            },
+        ),
         Bin::Wallet => ("namadaw", "info"),
         Bin::Relayer => ("namadar", "info"),
     };
 
-    let mut args = args.into_iter().peekable();
-    let is_node_ledger = matches!(bin, Bin::Node)
-        && args
-            .peek()
-            .map(|fst_arg| fst_arg.as_ref() == "ledger")
-            .unwrap_or_default();
-
     let mut run_cmd = generate_bin_command(
         bin_name,
         &working_dir.as_ref().join("Cargo.toml"),

crates/tests/src/integration/ledger_tests.rs

@@ -27,7 +27,7 @@ use namada_sdk::collections::HashMap;
 use namada_sdk::migrations;
 use namada_sdk::queries::RPC;
 use namada_sdk::token::{self, DenominatedAmount};
-use namada_sdk::tx::{TX_TRANSFER_WASM, VP_USER_WASM};
+use namada_sdk::tx::{self, Tx, TX_TRANSFER_WASM, VP_USER_WASM};
 use namada_test_utils::TestWasms;
 use test_log::test;
 
@@ -1901,9 +1901,9 @@ fn enforce_fee_payment() -> Result<()> {
 
     let mut txs = vec![];
     for bytes in txs_bytes {
-        let mut tx = namada_sdk::tx::Tx::deserialize(&bytes).unwrap();
+        let mut tx = Tx::try_from_json_bytes(&bytes).unwrap();
         tx.add_wrapper(
-            namada_sdk::tx::data::wrapper::Fee {
+            tx::data::wrapper::Fee {
                 amount_per_gas_unit: DenominatedAmount::native(
                     token::Amount::native_whole(1),
                 ),

crates/tests/src/integration/masp.rs

@@ -16,7 +16,8 @@ use namada_sdk::state::{StorageRead, StorageWrite};
 use namada_sdk::time::DateTimeUtc;
 use namada_sdk::token::storage_key::masp_token_map_key;
 use namada_sdk::token::{self, DenominatedAmount};
-use namada_sdk::DEFAULT_GAS_LIMIT;
+use namada_sdk::tx::Tx;
+use namada_sdk::{tx, DEFAULT_GAS_LIMIT};
 use test_log::test;
 
 use super::setup;
@@ -1503,9 +1504,9 @@ fn multiple_unfetched_txs_same_block() -> Result<()> {
         .clone();
     let mut txs = vec![];
     for bytes in txs_bytes {
-        let mut tx = namada_sdk::tx::Tx::deserialize(&bytes).unwrap();
+        let mut tx = Tx::try_from_json_bytes(&bytes).unwrap();
         tx.add_wrapper(
-            namada_sdk::tx::data::wrapper::Fee {
+            tx::data::wrapper::Fee {
                 amount_per_gas_unit: DenominatedAmount::native(1.into()),
                 token: native_token.clone(),
             },
@@ -1640,7 +1641,7 @@ fn expired_masp_tx() -> Result<()> {
         .in_mem()
         .native_token
         .clone();
-    let mut tx = namada_sdk::tx::Tx::deserialize(&tx_bytes).unwrap();
+    let mut tx = Tx::try_from_json_bytes(&tx_bytes).unwrap();
     // Remove the expiration field to avoid a failure because of it, we only
     // want to check the expiration in the masp vp
     tx.header.expiration = None;

crates/tx/src/action.rs

@@ -69,7 +69,7 @@ pub enum PgfAction {
 /// MASP tx actions.
 #[derive(Clone, Debug, BorshDeserialize, BorshSerialize, PartialEq)]
 pub enum MaspAction {
-    /// The hash of the masp [`crate::types::Section`]
+    /// The hash of the masp [`crate::Section`]
     MaspSectionRef(MaspTxId),
     /// A required authorizer for the transaction
     MaspAuthorizer(Address),

crates/tx/src/data/mod.rs

@@ -37,7 +37,7 @@ use sha2::{Digest, Sha256};
 pub use wrapper::*;
 
 use crate::data::protocol::ProtocolTx;
-use crate::types::TxCommitments;
+use crate::TxCommitments;
 
 /// The different result codes that the ledger may send back to a client
 /// indicating the status of their submitted tx.

crates/tx/src/lib.rs

@@ -21,19 +21,25 @@ pub mod action;
 pub mod data;
 pub mod event;
 pub mod proto;
+mod section;
 mod sign;
 mod types;
 
 use data::TxType;
 pub use either;
 pub use event::new_tx_event;
 pub use namada_core::key::SignableEthMessage;
-pub use sign::{SigIndexDecodeError, SignatureIndex};
+pub use section::{
+    Authorization, Code, Commitment, CompressedAuthorization, Data, Header,
+    MaspBuilder, Memo, Section, Signer, TxCommitments,
+};
+pub use sign::{
+    standalone_signature, verify_standalone_sig, SignatureIndex, Signed,
+    VerifySigError,
+};
 pub use types::{
-    standalone_signature, verify_standalone_sig, Authorization, BatchedTx,
-    BatchedTxRef, Code, Commitment, CompressedAuthorization, Data, DecodeError,
-    Header, IndexedTx, IndexedTxRange, MaspBuilder, Memo, Section, Signed,
-    Signer, Tx, TxCommitments, TxError, VerifySigError,
+    BatchedTx, BatchedTxRef, DecodeError, IndexedTx, IndexedTxRange, Tx,
+    TxError,
 };
 
 /// Length of the transaction sections salt