Merge pull request #3724 from anoma/grarco/fix-masp-expiration

Fixes MASP tx expiration in the SDK

.changelog/unreleased/bug-fixes/3724-fix-masp-expiration.md

@@ -0,0 +1,2 @@
+- Fixed the SDK to generate MASP transactions with the correct expiration (if
+  provided). ([\#3724](https://github.com/anoma/namada/pull/3724))

crates/apps_lib/src/cli.rs

@@ -6836,6 +6836,7 @@ pub mod args {
                 target: chain_ctx.get(&self.target),
                 token: self.token,
                 amount: self.amount,
+                expiration: self.expiration,
                 port_id: self.port_id,
                 channel_id: self.channel_id,
             })
@@ -6849,14 +6850,26 @@ pub mod args {
             let target = TRANSFER_TARGET.parse(matches);
             let token = TOKEN_STR.parse(matches);
             let amount = InputAmount::Unvalidated(AMOUNT.parse(matches));
+            let expiration = EXPIRATION_OPT.parse(matches);
             let port_id = PORT_ID.parse(matches);
             let channel_id = CHANNEL_ID.parse(matches);
+            let no_expiration = NO_EXPIRATION.parse(matches);
+            let expiration = if no_expiration {
+                TxExpiration::NoExpiration
+            } else {
+                match expiration {
+                    Some(exp) => TxExpiration::Custom(exp),
+                    None => TxExpiration::Default,
+                }
+            };
+
             Self {
                 query,
                 output_folder,
                 target,
                 token,
                 amount,
+                expiration,
                 port_id,
                 channel_id,
             }
@@ -6874,6 +6887,26 @@ pub mod args {
                         .def()
                         .help(wrap!("The amount to transfer in decimal.")),
                 )
+                .arg(
+                    EXPIRATION_OPT
+                        .def()
+                        .help(wrap!(
+                            "The expiration datetime of the masp transaction, \
+                             after which the tx won't be accepted anymore. If \
+                             not provided, a default will be set. Example: \
+                             2012-12-12T12:12:12Z"
+                        ))
+                        .conflicts_with_all([NO_EXPIRATION.name]),
+                )
+                .arg(
+                    NO_EXPIRATION
+                        .def()
+                        .help(wrap!(
+                            "Force the construction of the transaction \
+                             without an expiration (highly discouraged)."
+                        ))
+                        .conflicts_with_all([EXPIRATION_OPT.name]),
+                )
                 .arg(PORT_ID.def().help(wrap!(
                     "The port ID via which the token is received."
                 )))
@@ -7315,10 +7348,8 @@ pub mod args {
                     .help(wrap!(
                         "The expiration datetime of the transaction, after \
                          which the tx won't be accepted anymore. If not \
-                         provided, a default will be set. All of these \
-                         examples are \
-                         equivalent:\n2012-12-12T12:12:12Z\n2012-12-12 \
-                         12:12:12Z\n2012-  12-12T12:  12:12Z"
+                         provided, a default will be set. Example: \
+                         2012-12-12T12:12:12Z"
                     ))
                     .conflicts_with_all([NO_EXPIRATION.name]),
             )

crates/node/src/shell/finalize_block.rs

@@ -6090,7 +6090,7 @@ mod test_finalize_block {
         assert_eq!(tx_results.len(), 2);
 
         // all txs should have succeeded
-        assert!(tx_results.are_results_ok());
+        assert!(tx_results.are_results_successfull());
     }
 
     #[test]

crates/node/src/shell/testing/node.rs

@@ -38,7 +38,7 @@ use namada_sdk::tendermint::abci::types::VoteInfo;
 use namada_sdk::tendermint_proto::google::protobuf::Timestamp;
 use namada_sdk::time::DateTimeUtc;
 use namada_sdk::tx::data::ResultCode;
-use namada_sdk::tx::event::Code as CodeAttr;
+use namada_sdk::tx::event::{Batch as BatchAttr, Code as CodeAttr};
 use namada_sdk::{ethereum_structs, governance};
 use regex::Regex;
 use tokio::sync::mpsc;
@@ -247,17 +247,27 @@ pub enum NodeResults {
     Failed(ResultCode),
 }
 
-// TODO: wrap `MockNode` in a single `Arc`
-#[derive(Clone)]
-pub struct MockNode {
-    pub shell: Arc<Mutex<Shell<storage::PersistentDB, Sha256Hasher>>>,
-    pub test_dir: Arc<SalvageableTestDir>,
-    pub results: Arc<Mutex<Vec<NodeResults>>>,
-    pub blocks: Arc<Mutex<HashMap<BlockHeight, block::Response>>>,
-    pub services: Arc<MockServices>,
+pub struct InnerMockNode {
+    pub shell: Mutex<Shell<storage::PersistentDB, Sha256Hasher>>,
+    pub test_dir: SalvageableTestDir,
+    pub tx_result_codes: Mutex<Vec<NodeResults>>,
+    pub tx_results: Mutex<Vec<namada_sdk::tx::data::TxResult<String>>>,
+    pub blocks: Mutex<HashMap<BlockHeight, block::Response>>,
+    pub services: MockServices,
     pub auto_drive_services: bool,
 }
 
+#[derive(Clone)]
+pub struct MockNode(pub Arc<InnerMockNode>);
+
+impl Deref for MockNode {
+    type Target = InnerMockNode;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
 pub struct SalvageableTestDir {
     pub test_dir: ManuallyDrop<TestDir>,
     pub keep_temp: bool,
@@ -505,9 +515,9 @@ impl MockNode {
         };
 
         let resp = locked.finalize_block(req).expect("Test failed");
-        let mut error_codes = resp
+        let mut result_codes = resp
             .events
-            .into_iter()
+            .iter()
             .map(|e| {
                 let code = e
                     .read_attribute_opt::<CodeAttr>()
@@ -520,7 +530,16 @@ impl MockNode {
                 }
             })
             .collect::<Vec<_>>();
-        self.results.lock().unwrap().append(&mut error_codes);
+        let mut tx_results = resp
+            .events
+            .into_iter()
+            .filter_map(|e| e.read_attribute_opt::<BatchAttr<'_>>().unwrap())
+            .collect::<Vec<_>>();
+        self.tx_result_codes
+            .lock()
+            .unwrap()
+            .append(&mut result_codes);
+        self.tx_results.lock().unwrap().append(&mut tx_results);
         locked.commit();
 
         // Cache the block
@@ -599,7 +618,7 @@ impl MockNode {
             })
             .collect();
         if result != tendermint::abci::response::ProcessProposal::Accept {
-            self.results.lock().unwrap().append(&mut errors);
+            self.tx_result_codes.lock().unwrap().append(&mut errors);
             return;
         }
 
@@ -643,7 +662,7 @@ impl MockNode {
         let resp = locked.finalize_block(req).unwrap();
         let mut error_codes = resp
             .events
-            .into_iter()
+            .iter()
             .map(|e| {
                 let code = e
                     .read_attribute_opt::<CodeAttr>()
@@ -656,7 +675,16 @@ impl MockNode {
                 }
             })
             .collect::<Vec<_>>();
-        self.results.lock().unwrap().append(&mut error_codes);
+        let mut txs_results = resp
+            .events
+            .into_iter()
+            .filter_map(|e| e.read_attribute_opt::<BatchAttr<'_>>().unwrap())
+            .collect::<Vec<_>>();
+        self.tx_result_codes
+            .lock()
+            .unwrap()
+            .append(&mut error_codes);
+        self.tx_results.lock().unwrap().append(&mut txs_results);
         self.blocks.lock().unwrap().insert(
             height,
             block::Response {
@@ -702,35 +730,46 @@ impl MockNode {
 
     /// Check that applying a tx succeeded.
     pub fn success(&self) -> bool {
-        self.results
+        self.tx_result_codes
             .lock()
             .unwrap()
             .iter()
             .all(|r| *r == NodeResults::Ok)
+            && self
+                .tx_results
+                .lock()
+                .unwrap()
+                .iter()
+                .all(|inner_results| inner_results.are_results_successfull())
     }
 
     /// Return a tx result if the tx failed in mempool
     pub fn is_broadcast_err(&self) -> Option<TxResult> {
-        self.results.lock().unwrap().iter().find_map(|r| match r {
-            NodeResults::Ok | NodeResults::Failed(_) => None,
-            NodeResults::Rejected(tx_result) => Some(tx_result.clone()),
-        })
+        self.tx_result_codes
+            .lock()
+            .unwrap()
+            .iter()
+            .find_map(|r| match r {
+                NodeResults::Ok | NodeResults::Failed(_) => None,
+                NodeResults::Rejected(tx_result) => Some(tx_result.clone()),
+            })
     }
 
     pub fn clear_results(&self) {
-        self.results.lock().unwrap().clear();
+        self.tx_result_codes.lock().unwrap().clear();
+        self.tx_results.lock().unwrap().clear();
     }
 
     pub fn assert_success(&self) {
         if !self.success() {
             panic!(
                 "Assert failed: The node did not execute \
-                 successfully:\nErrors:\n    {:?}",
-                self.results.lock().unwrap()
+                 successfully:\nErrors:\n    {:?},\nTxs results:\n    {:?}",
+                self.tx_result_codes.lock().unwrap(),
+                self.tx_results.lock().unwrap()
             );
-        } else {
-            self.clear_results();
         }
+        self.clear_results();
     }
 }
 

crates/sdk/src/args.rs

@@ -2892,6 +2892,8 @@ pub struct GenIbcShieldingTransfer<C: NamadaTypes = SdkTypes> {
     pub token: String,
     /// Transferred token amount
     pub amount: InputAmount,
+    /// The optional expiration of the masp shielding transaction
+    pub expiration: TxExpiration,
     /// Port ID via which the token is received
     pub port_id: PortId,
     /// Channel ID via which the token is received

crates/sdk/src/tx.rs

@@ -2616,6 +2616,7 @@ pub async fn build_ibc_transfer(
         masp_transfer_data,
         masp_fee_data,
         !(args.tx.dry_run || args.tx.dry_run_wrapper),
+        args.tx.expiration.to_datetime(),
     )
     .await?;
     let shielded_tx_epoch = shielded_parts.as_ref().map(|trans| trans.0.epoch);
@@ -3080,6 +3081,7 @@ pub async fn build_shielded_transfer<N: Namada>(
         transfer_data,
         masp_fee_data,
         !(args.tx.dry_run || args.tx.dry_run_wrapper),
+        args.tx.expiration.to_datetime(),
     )
     .await?
     .expect("Shielded transfer must have shielded parts");
@@ -3245,6 +3247,7 @@ pub async fn build_shielding_transfer<N: Namada>(
         transfer_data,
         None,
         !(args.tx.dry_run || args.tx.dry_run_wrapper),
+        args.tx.expiration.to_datetime(),
     )
     .await?
     .expect("Shielding transfer must have shielded parts");
@@ -3366,6 +3369,7 @@ pub async fn build_unshielding_transfer<N: Namada>(
         transfer_data,
         masp_fee_data,
         !(args.tx.dry_run || args.tx.dry_run_wrapper),
+        args.tx.expiration.to_datetime(),
     )
     .await?
     .expect("Shielding transfer must have shielded parts");
@@ -3418,13 +3422,13 @@ async fn construct_shielded_parts<N: Namada>(
     data: Vec<MaspTransferData>,
     fee_data: Option<MaspFeeData>,
     update_ctx: bool,
+    expiration: Option<DateTimeUtc>,
 ) -> Result<Option<(ShieldedTransfer, HashSet<AssetData>)>> {
     // Precompute asset types to increase chances of success in decoding
     let token_map = context.wallet().await.get_addresses();
     let tokens = token_map.values().collect();
 
     let stx_result = {
-        let expiration = context.tx_builder().expiration.to_datetime();
         let mut shielded = context.shielded_mut().await;
         _ = shielded
             .precompute_asset_types(context.client(), tokens)
@@ -3789,15 +3793,14 @@ pub async fn gen_ibc_shielding_transfer<N: Namada>(
         amount: validated_amount,
     };
     let shielded_transfer = {
-        let expiration = context.tx_builder().expiration.to_datetime();
         let mut shielded = context.shielded_mut().await;
         shielded
             .gen_shielded_transfer(
                 context,
                 vec![masp_transfer_data],
                 // Fees are paid from the transparent balance of the relayer
                 None,
-                expiration,
+                args.expiration.to_datetime(),
                 true,
             )
             .await

crates/shielded_token/src/masp/shielded_wallet.rs

@@ -910,14 +910,6 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
         expiration: Option<DateTimeUtc>,
         update_ctx: bool,
     ) -> Result<Option<ShieldedTransfer>, TransferErr> {
-        let last_block_height = Self::query_block(context.client())
-            .await
-            .map_err(|e| TransferErr::General(e.to_string()))?
-            .unwrap_or(1);
-        let max_block_time =
-            Self::query_max_block_time_estimate(context.client())
-                .await
-                .map_err(|e| TransferErr::General(e.to_string()))?;
         // Determine epoch in which to submit potential shielded transaction
         let epoch = Self::query_masp_epoch(context.client())
             .await
@@ -949,13 +941,24 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
 
         // TODO: if the user requested the default expiration, there might be a
         // small discrepancy between the datetime we calculate here and the one
-        // we set for the transaction. This should be small enough to not cause
-        // any issue, in case refactor this function to request the precise
-        // datetime to the caller
+        // we set for the transaction (since we compute two different
+        // DateTimeUtc::now()). This should be small enough to not cause
+        // any issue, in case refactor the build process to compute a single
+        // expiration at the beginning and use it both here and for the
+        // transaction
         let expiration_height: u32 = match expiration {
             Some(expiration) => {
                 // Try to match a DateTime expiration with a plausible
                 // corresponding block height
+                let last_block_height = Self::query_block(context.client())
+                    .await
+                    .map_err(|e| TransferErr::General(e.to_string()))?
+                    .unwrap_or(1);
+                let max_block_time =
+                    Self::query_max_block_time_estimate(context.client())
+                        .await
+                        .map_err(|e| TransferErr::General(e.to_string()))?;
+
                 #[allow(clippy::disallowed_methods)]
                 let current_time = DateTimeUtc::now();
                 let delta_time =