Merge branch 'mergify/bp/0.44.0/pr-3882' (#3888)

* origin/mergify/bp/0.44.0/pr-3882:
  Changelog #3882
  Extends batch sections test to check duplicate sections
  Custom `PartialEq` impl for `SigningTxData`
  Fixes cmt reference in `add_inner_tx`
  Avoids duplicated sections when contructing a batch

.changelog/unreleased/improvements/3882-better-batch-construction.md

@@ -0,0 +1,2 @@
+- Improved batch construction to reduce the size of the resulting tx.
+  ([\#3882](https://github.com/anoma/namada/pull/3882))

crates/sdk/src/signing.rs

@@ -61,7 +61,7 @@ use crate::wallet::{Wallet, WalletIo};
 use crate::{args, rpc, Namada};
 
 /// A structure holding the signing data to craft a transaction
-#[derive(Clone, PartialEq)]
+#[derive(Clone)]
 pub struct SigningTxData {
     /// The address owning the transaction
     pub owner: Option<Address>,
@@ -75,6 +75,27 @@ pub struct SigningTxData {
     pub fee_payer: common::PublicKey,
 }
 
+impl PartialEq for SigningTxData {
+    fn eq(&self, other: &Self) -> bool {
+        if !(self.owner == other.owner
+            && self.threshold == other.threshold
+            && self.account_public_keys_map == other.account_public_keys_map
+            && self.fee_payer == other.fee_payer)
+        {
+            return false;
+        }
+
+        // Check equivalence of the public keys ignoring the specific ordering
+        if self.public_keys.len() != other.public_keys.len() {
+            return false;
+        }
+
+        self.public_keys
+            .iter()
+            .all(|pubkey| other.public_keys.contains(pubkey))
+    }
+}
+
 /// Find the public key for the given address and try to load the keypair
 /// for it from the wallet.
 ///
@@ -290,7 +311,7 @@ where
         Ok(fee_payer_keypair) => {
             tx.sign_wrapper(fee_payer_keypair);
         }
-        // The case where tge fee payer also signs the inner transaction
+        // The case where the fee payer also signs the inner transaction
         Err(_)
             if signing_data.public_keys.contains(&signing_data.fee_payer) =>
         {

crates/tx/src/types.rs

@@ -145,16 +145,49 @@ impl Tx {
 
     /// Add a new inner tx to the transaction. Returns `false` if the
     /// commitments already existed in the collection. This function expects a
-    /// transaction carrying a single inner tx as input
-    pub fn add_inner_tx(&mut self, other: Tx, cmt: TxCommitments) -> bool {
-        if !self.header.batch.insert(cmt) {
+    /// transaction carrying a single inner tx as input and the provided
+    /// commitment is assumed to be present in the transaction without further
+    /// validation
+    pub fn add_inner_tx(&mut self, other: Tx, mut cmt: TxCommitments) -> bool {
+        if self.header.batch.contains(&cmt) {
             return false;
         }
 
-        // TODO: avoid duplicated sections to reduce the size of the message
-        self.sections.extend(other.sections);
+        for section in other.sections {
+            // PartialEq implementation of Section relies on an implementation
+            // on the inner types that doesn't account for the possible salt
+            // which is the correct behavior for this logic
+            if let Some(duplicate) =
+                self.sections.iter().find(|&sec| sec == &section)
+            {
+                // Avoid pushing a duplicated section. Adjust the commitment of
+                // this inner tx for the different section's salt if needed
+                match duplicate {
+                    Section::Code(_) => {
+                        if cmt.code_hash == section.get_hash() {
+                            cmt.code_hash = duplicate.get_hash();
+                        }
+                    }
+                    Section::Data(_) => {
+                        if cmt.data_hash == section.get_hash() {
+                            cmt.data_hash = duplicate.get_hash();
+                        }
+                    }
+                    Section::ExtraData(_) => {
+                        if cmt.memo_hash == section.get_hash() {
+                            cmt.memo_hash = duplicate.get_hash();
+                        }
+                    }
+                    // Other sections don't have a direct commitment in the
+                    // header
+                    _ => (),
+                }
+            } else {
+                self.sections.push(section);
+            }
+        }
 
-        true
+        self.header.batch.insert(cmt)
     }
 
     /// Get the transaction header
@@ -1324,6 +1357,11 @@ mod test {
         let data_bytes2 = "WASD".as_bytes();
         let memo_bytes2 = "hjkl".as_bytes();
 
+        // Some duplicated sections
+        let code_bytes3 = code_bytes1;
+        let data_bytes3 = data_bytes2;
+        let memo_bytes3 = memo_bytes1;
+
         let inner_tx1 = {
             let mut tx = Tx::default();
 
@@ -1352,10 +1390,25 @@ mod test {
             tx
         };
 
+        let inner_tx3 = {
+            let mut tx = Tx::default();
+
+            let code = Code::new(code_bytes3.to_owned(), None);
+            tx.set_code(code);
+
+            let data = Data::new(data_bytes3.to_owned());
+            tx.set_data(data);
+
+            tx.add_memo(memo_bytes3);
+
+            tx
+        };
+
         let cmt1 = inner_tx1.first_commitments().unwrap().to_owned();
-        let cmt2 = inner_tx2.first_commitments().unwrap().to_owned();
+        let mut cmt2 = inner_tx2.first_commitments().unwrap().to_owned();
+        let mut cmt3 = inner_tx3.first_commitments().unwrap().to_owned();
 
-        // Batch `inner_tx1` and `inner_tx1` into `tx`
+        // Batch `inner_tx1`, `inner_tx2` and `inner_tx3` into `tx`
         let tx = {
             let mut tx = Tx::default();
 
@@ -1364,10 +1417,22 @@ mod test {
             assert_eq!(tx.header.batch.len(), 1);
 
             tx.add_inner_tx(inner_tx2, cmt2.clone());
+            // Update cmt2 with the hash of cmt1 code section
+            cmt2.code_hash = cmt1.code_hash;
             assert_eq!(tx.first_commitments().unwrap(), &cmt1);
             assert_eq!(tx.header.batch.len(), 2);
             assert_eq!(tx.header.batch.get_index(1).unwrap(), &cmt2);
 
+            tx.add_inner_tx(inner_tx3, cmt3.clone());
+            // Update cmt3 with the hash of cmt1 code and memo sections and the
+            // hash of cmt2 data section
+            cmt3.code_hash = cmt1.code_hash;
+            cmt3.data_hash = cmt2.data_hash;
+            cmt3.memo_hash = cmt1.memo_hash;
+            assert_eq!(tx.first_commitments().unwrap(), &cmt1);
+            assert_eq!(tx.header.batch.len(), 3);
+            assert_eq!(tx.header.batch.get_index(2).unwrap(), &cmt3);
+
             tx
         };
 
@@ -1390,5 +1455,40 @@ mod test {
 
         assert!(tx.memo(&cmt2).is_some());
         assert_eq!(tx.memo(&cmt2).unwrap(), memo_bytes2);
+
+        // Check sections of `inner_tx3`
+        assert!(tx.code(&cmt3).is_some());
+        assert_eq!(tx.code(&cmt3).unwrap(), code_bytes3);
+
+        assert!(tx.data(&cmt3).is_some());
+        assert_eq!(tx.data(&cmt3).unwrap(), data_bytes3);
+
+        assert!(tx.memo(&cmt3).is_some());
+        assert_eq!(tx.memo(&cmt3).unwrap(), memo_bytes3);
+
+        // Check that the redundant sections have been included only once in the
+        // batch
+        assert_eq!(tx.sections.len(), 5);
+        assert_eq!(
+            tx.sections
+                .iter()
+                .filter(|section| section.code_sec().is_some())
+                .count(),
+            1
+        );
+        assert_eq!(
+            tx.sections
+                .iter()
+                .filter(|section| section.data().is_some())
+                .count(),
+            2
+        );
+        assert_eq!(
+            tx.sections
+                .iter()
+                .filter(|section| section.extra_data_sec().is_some())
+                .count(),
+            2
+        );
     }
 }