Merge branch 'yuji/rename-ibc-transfer-memo-opt' (#3519)

* yuji/rename-ibc-transfer-memo-opt:
  rename and split --memo-path of ibc-transfer

.changelog/unreleased/improvements/3517-rename-ibc-transfer-memo-opt.md

@@ -0,0 +1,2 @@
+- Rename and split --memo-path into --ibc-shielding-data and --ibc-memo
+  ([\#3517](https://github.com/anoma/namada/issues/3517))

crates/apps_lib/src/cli.rs

@@ -3328,7 +3328,9 @@ pub mod args {
         flag("allow-non-compliant");
     pub const HD_PROMPT_BIP39_PASSPHRASE: ArgFlag = flag("bip39-passphrase");
     pub const HISTORIC: ArgFlag = flag("historic");
-    pub const IBC_TRANSFER_MEMO_PATH: ArgOpt<PathBuf> = arg_opt("memo-path");
+    pub const IBC_SHIELDING_DATA_PATH: ArgOpt<PathBuf> =
+        arg_opt("ibc-shielding-data");
+    pub const IBC_MEMO: ArgOpt<String> = arg_opt("ibc-memo");
     pub const INPUT_OPT: ArgOpt<PathBuf> = arg_opt("input");
     pub const LEDGER_ADDRESS_ABOUT: &str = textwrap_macros::fill!(
         "Address of a ledger node as \"{scheme}://{host}:{port}\". If the \
@@ -4763,7 +4765,8 @@ pub mod args {
                 timeout_height: self.timeout_height,
                 timeout_sec_offset: self.timeout_sec_offset,
                 refund_target: chain_ctx.get_opt(&self.refund_target),
-                memo: self.memo,
+                ibc_shielding_data: self.ibc_shielding_data,
+                ibc_memo: self.ibc_memo,
                 gas_spending_keys,
                 tx_code_path: self.tx_code_path.to_path_buf(),
             })
@@ -4782,10 +4785,14 @@ pub mod args {
             let timeout_height = TIMEOUT_HEIGHT.parse(matches);
             let timeout_sec_offset = TIMEOUT_SEC_OFFSET.parse(matches);
             let refund_target = REFUND_TARGET.parse(matches);
-            let memo = IBC_TRANSFER_MEMO_PATH.parse(matches).map(|path| {
-                std::fs::read_to_string(path)
-                    .expect("Expected a file at given path")
-            });
+            let ibc_shielding_data =
+                IBC_SHIELDING_DATA_PATH.parse(matches).map(|path| {
+                    let data = std::fs::read_to_string(path)
+                        .expect("Failed to open IBC shielding data file");
+                    namada::ibc::decode_ibc_shielding_data(data)
+                        .expect("Failed to decode IBC shielding data")
+                });
+            let ibc_memo = IBC_MEMO.parse(matches);
             let mut gas_spending_keys = vec![];
             if let Some(key) = GAS_SPENDING_KEY.parse(matches) {
                 gas_spending_keys.push(key);
@@ -4802,7 +4809,8 @@ pub mod args {
                 timeout_height,
                 timeout_sec_offset,
                 refund_target,
-                memo,
+                ibc_shielding_data,
+                ibc_memo,
                 gas_spending_keys,
                 tx_code_path,
             }
@@ -4837,9 +4845,16 @@ pub mod args {
                     "The refund target address when IBC shielded transfer \
                      failure."
                 )))
-                .arg(IBC_TRANSFER_MEMO_PATH.def().help(wrap!(
-                    "The path for the memo field of ICS20 transfer."
+                .arg(IBC_SHIELDING_DATA_PATH.def().help(wrap!(
+                    "The file path of the IBC shielding data for the \
+                     destination Namada. This can't be set with --ibc-memo at \
+                     the same time."
                 )))
+                .arg(
+                    IBC_MEMO
+                        .def()
+                        .help(wrap!("The memo for IBC transfer packet.")),
+                )
                 .arg(GAS_SPENDING_KEY.def().help(wrap!(
                     "The optional spending key that will be used in addition \
                      to the source for gas payment (if this is a shielded \

crates/ibc/src/msg.rs

@@ -163,11 +163,11 @@ pub fn extract_masp_tx_from_envelope(
     }
 }
 
-/// Decode IBC shielding data from the memo string
-pub fn decode_masp_tx_from_memo(
-    memo: impl AsRef<str>,
+/// Decode IBC shielding data from the string
+pub fn decode_ibc_shielding_data(
+    s: impl AsRef<str>,
 ) -> Option<IbcShieldingData> {
-    let bytes = HEXUPPER.decode(memo.as_ref().as_bytes()).ok()?;
+    let bytes = HEXUPPER.decode(s.as_ref().as_bytes()).ok()?;
     IbcShieldingData::try_from_slice(&bytes).ok()
 }
 
@@ -182,7 +182,7 @@ pub fn extract_masp_tx_from_packet(
         &packet.port_id_on_b
     };
     let memo = extract_memo_from_packet(packet, port_id)?;
-    let shielding_data = decode_masp_tx_from_memo(memo)?;
+    let shielding_data = decode_ibc_shielding_data(memo)?;
     if is_sender {
         shielding_data.refund
     } else {

crates/sdk/src/args.rs

@@ -19,6 +19,7 @@ use namada_core::{storage, token};
 use namada_governance::cli::onchain::{
     DefaultProposal, PgfFundingProposal, PgfStewardProposal,
 };
+use namada_ibc::IbcShieldingData;
 use namada_tx::data::GasLimit;
 use namada_tx::Memo;
 use serde::{Deserialize, Serialize};
@@ -441,8 +442,10 @@ pub struct TxIbcTransfer<C: NamadaTypes = SdkTypes> {
     pub timeout_sec_offset: Option<u64>,
     /// Refund target address when the shielded transfer failure
     pub refund_target: Option<C::TransferTarget>,
-    /// Memo
-    pub memo: Option<String>,
+    /// IBC shielding transfer data for the destination chain
+    pub ibc_shielding_data: Option<IbcShieldingData>,
+    /// Memo for IBC transfer packet
+    pub ibc_memo: Option<String>,
     /// Optional additional keys for gas payment
     pub gas_spending_keys: Vec<C::SpendingKey>,
     /// Path to the TX WASM code file
@@ -516,10 +519,18 @@ impl<C: NamadaTypes> TxIbcTransfer<C> {
         }
     }
 
-    /// Memo
-    pub fn memo(self, memo: String) -> Self {
+    /// IBC shielding transfer data
+    pub fn ibc_shielding_data(self, shielding_data: IbcShieldingData) -> Self {
         Self {
-            memo: Some(memo),
+            ibc_shielding_data: Some(shielding_data),
+            ..self
+        }
+    }
+
+    /// Memo for IBC transfer packet
+    pub fn ibc_memo(self, ibc_memo: String) -> Self {
+        Self {
+            ibc_memo: Some(ibc_memo),
             ..self
         }
     }

crates/sdk/src/lib.rs

@@ -324,7 +324,8 @@ pub trait Namada: Sized + MaybeSync + MaybeSend {
             timeout_height: None,
             timeout_sec_offset: None,
             refund_target: None,
-            memo: None,
+            ibc_shielding_data: None,
+            ibc_memo: None,
             gas_spending_keys: Default::default(),
             tx: self.tx_builder(),
             tx_code_path: PathBuf::from(TX_IBC_WASM),

crates/sdk/src/tx.rs

@@ -53,9 +53,7 @@ use namada_governance::storage::proposal::{
 use namada_governance::storage::vote::ProposalVote;
 use namada_ibc::storage::channel_key;
 use namada_ibc::trace::is_nft_trace;
-use namada_ibc::{
-    decode_masp_tx_from_memo, IbcShieldingData, MsgNftTransfer, MsgTransfer,
-};
+use namada_ibc::{IbcShieldingData, MsgNftTransfer, MsgTransfer};
 use namada_proof_of_stake::parameters::{
     PosParams, MAX_VALIDATOR_METADATA_LEN,
 };
@@ -2463,6 +2461,14 @@ pub async fn build_ibc_transfer(
     context: &impl Namada,
     args: &args::TxIbcTransfer,
 ) -> Result<(Tx, SigningTxData, Option<MaspEpoch>)> {
+    if args.ibc_shielding_data.is_some() && args.ibc_memo.is_some() {
+        return Err(Error::Other(
+            "The memo field of the IBC packet can't be used for both \
+             shielding transfer and another purpose at the same time"
+                .to_string(),
+        ));
+    }
+
     let refund_target =
         get_refund_target(context, &args.source, &args.refund_target).await?;
 
@@ -2643,28 +2649,20 @@ pub async fn build_ibc_transfer(
         )
         .await?
         .map(|(shielded_transfer, _)| shielded_transfer.masp_tx);
-        let shielding_data = match &args.memo {
-            Some(memo) => {
-                if let Some(mut shielding_data) = decode_masp_tx_from_memo(memo)
-                {
-                    shielding_data.refund = masp_tx;
-                    shielding_data
-                } else {
-                    return Err(Error::Other(
-                        "The memo has been already set. The refunding for the \
-                         spending key can't be set"
-                            .to_string(),
-                    ));
-                }
-            }
-            None => IbcShieldingData {
-                shielding: None,
-                refund: masp_tx,
-            },
+        let shielding_data = IbcShieldingData {
+            shielding: args
+                .ibc_shielding_data
+                .as_ref()
+                .and_then(|data| data.shielding.clone()),
+            refund: masp_tx,
         };
         Some(shielding_data.into())
     } else {
-        args.memo.clone()
+        args.ibc_shielding_data
+            .as_ref()
+            .map_or(args.ibc_memo.clone(), |shielding_data| {
+                Some(shielding_data.clone().into())
+            })
     };
     // If the refund address is given, set the refund address. It is used only
     // when refunding and won't affect the actual transfer because the actual
@@ -2717,7 +2715,7 @@ pub async fn build_ibc_transfer(
             token_data: None,
             sender,
             receiver: args.receiver.clone().into(),
-            memo: args.memo.clone().map(|m| m.into()),
+            memo: memo.map(|s| s.into()),
         };
         let message = IbcMsgNftTransfer {
             port_id_on_a: args.port_id.clone(),

crates/tests/src/e2e/ibc_tests.rs

@@ -302,7 +302,7 @@ fn run_ledger_ibc_with_hermes() -> Result<()> {
     // Shieded transfer from Chain A to Chain B
     std::env::set_var(ENV_VAR_CHAIN_ID, test_a.net.chain_id.to_string());
     let token_addr = find_address(&test_a, BTC)?.to_string();
-    let memo_path = gen_masp_tx(
+    let shielding_data_path = gen_ibc_shielding_data(
         &test_b,
         AB_PAYMENT_ADDRESS,
         token_addr,
@@ -320,7 +320,7 @@ fn run_ledger_ibc_with_hermes() -> Result<()> {
         &port_id_a,
         &channel_id_a,
         None,
-        Some(memo_path),
+        Some(shielding_data_path),
         None,
         false,
     )?;
@@ -494,7 +494,7 @@ fn ibc_namada_gaia() -> Result<()> {
     check_gaia_balance(&test_gaia, GAIA_USER, GAIA_COIN, 900)?;
 
     // Shielding transfer from Gaia to Namada
-    let memo_path = gen_masp_tx(
+    let memo_path = gen_ibc_shielding_data(
         &test,
         AA_PAYMENT_ADDRESS,
         GAIA_COIN,
@@ -696,7 +696,7 @@ fn proposal_ibc_token_inflation() -> Result<()> {
     // Transfer 1 from Chain A to a z-address on Chain B
     std::env::set_var(ENV_VAR_CHAIN_ID, test_a.net.chain_id.to_string());
     let token_addr = find_address(&test_a, APFEL)?.to_string();
-    let memo_path = gen_masp_tx(
+    let shielding_data_path = gen_ibc_shielding_data(
         &test_b,
         AB_PAYMENT_ADDRESS,
         token_addr,
@@ -714,7 +714,7 @@ fn proposal_ibc_token_inflation() -> Result<()> {
         &port_id_a,
         &channel_id_a,
         None,
-        Some(memo_path),
+        Some(shielding_data_path),
         None,
         false,
     )?;
@@ -1991,7 +1991,7 @@ fn transfer(
     port_id: &PortId,
     channel_id: &ChannelId,
     timeout_sec: Option<Duration>,
-    memo_path: Option<PathBuf>,
+    shielding_data_path: Option<PathBuf>,
     expected_err: Option<&str>,
     wait_reveal_pk: bool,
 ) -> Result<u32> {
@@ -2033,12 +2033,12 @@ fn transfer(
         tx_args.push(&timeout);
     }
 
-    let memo = memo_path
+    let memo = shielding_data_path
         .as_ref()
         .map(|path| path.to_string_lossy().to_string())
         .unwrap_or_default();
-    if memo_path.is_some() {
-        tx_args.push("--memo-path");
+    if shielding_data_path.is_some() {
+        tx_args.push("--ibc-shielding-data");
         tx_args.push(&memo);
     }
 
@@ -2692,8 +2692,9 @@ fn shielded_sync(test: &Test, viewing_key: impl AsRef<str>) -> Result<()> {
     Ok(())
 }
 
-/// Get masp proof for the following IBC transfer from the destination chain
-fn gen_masp_tx(
+/// Get IBC shielding data for the following IBC transfer from the destination
+/// chain
+fn gen_ibc_shielding_data(
     dst_test: &Test,
     receiver: impl AsRef<str>,
     token: impl AsRef<str>,