Adds support for MASP frontend sus fee over IBC

Author: grarco

crates/apps_lib/src/cli.rs

@@ -5229,7 +5229,13 @@ pub mod args {
                 ibc_memo: self.ibc_memo,
                 gas_spending_key,
                 tx_code_path: self.tx_code_path.to_path_buf(),
-                frontend_sus_fee: None,
+                frontend_sus_fee: self.frontend_sus_fee.map(|fee| {
+                    TxTransparentTarget {
+                        target: chain_ctx.get(&fee.target),
+                        token: chain_ctx.get(&fee.token),
+                        amount: fee.amount,
+                    }
+                }),
             })
         }
     }
@@ -5258,8 +5264,6 @@ pub mod args {
             let ibc_memo = IBC_MEMO.parse(matches);
             let gas_spending_key = GAS_SPENDING_KEY.parse(matches);
             let tx_code_path = PathBuf::from(TX_IBC_WASM);
-            // FIXME: this api is to confusing, split the amount into two,
-            // source amount and target amount
             #[cfg(any(test, feature = "testing"))]
             let frontend_sus_fee =
                 FRONTEND_SUS_FEE.parse(matches).map(|target|
@@ -5278,8 +5282,6 @@ pub mod args {
             #[cfg(not(any(test, feature = "testing")))]
             let frontend_sus_fee = None;
 
-            eprintln!("AMOUNT IN CLI: {:#?}", amount); //FIXME: remove
-
             Self {
                 tx,
                 source,
@@ -5290,7 +5292,6 @@ pub mod args {
                 channel_id,
                 timeout_height,
                 timeout_sec_offset,
-                // FIXME: check this refund, we should not refund the fee
                 refund_target,
                 ibc_shielding_data,
                 ibc_memo,
@@ -5301,7 +5302,8 @@ pub mod args {
         }
 
         fn def(app: App) -> App {
-            app.add_args::<Tx<CliTypes>>()
+            let app = app
+                .add_args::<Tx<CliTypes>>()
                 .arg(SOURCE.def().help(wrap!(
                     "The source account address. The source's key is used to \
                      produce the signature."
@@ -5345,7 +5347,15 @@ pub mod args {
                     "The optional spending key that will be used for gas \
                      payment (if this is a shielded action).  When not \
                      provided the source spending key will be used."
-                )))
+                )));
+
+            #[cfg(any(test, feature = "testing"))]
+            let app = app.arg(FRONTEND_SUS_FEE.def().help(wrap!(
+                "The optional address of the frontend provider that will take \
+                 the masp sustainability fee."
+            )));
+
+            app
         }
     }
 
@@ -7389,14 +7399,11 @@ pub mod args {
                     None => TxExpiration::Default,
                 }
             };
-            // FIXME: this api is to confusing, split the amount into two,
-            // source amount and target amount
             #[cfg(any(test, feature = "testing"))]
             let frontend_sus_fee = FRONTEND_SUS_FEE_IBC.parse(matches).map(
                 |target|                        // Take a constant fee of 1 on top of the input amount
                 (
                     target,
-                    //FIXME: this means we can't do anything when it comes to nfts for this frontend fee
                              InputAmount::Unvalidated(
                                 token::DenominatedAmount::new(
                                     1.into(),
@@ -7409,8 +7416,6 @@ pub mod args {
             #[cfg(not(any(test, feature = "testing")))]
             let frontend_sus_fee = None;
 
-            eprintln!("AMOUNT IN CLI: {:#?}", amount); //FIXME: remove
-
             Self {
                 query,
                 output_folder,

crates/apps_lib/src/client/tx.rs

@@ -2011,17 +2011,18 @@ pub async fn gen_ibc_shielding_transfer(
 ) -> Result<(), error::Error> {
     let output_folder = args.output_folder.clone();
 
-    if let Some(masp_tx) = tx::gen_ibc_shielding_transfer(context, args).await?
+    if let Some(shielding_data) =
+        tx::gen_ibc_shielding_transfer(context, args).await?
     {
-        let tx_id = masp_tx.txid().to_string();
+        let tx_id = shielding_data.0.txid().to_string();
         let filename = format!("ibc_masp_tx_{}.memo", tx_id);
         let output_path = match output_folder {
             Some(path) => path.join(filename),
             None => filename.into(),
         };
         let mut out = File::create(&output_path)
             .expect("Creating a new file for IBC MASP transaction failed.");
-        let bytes = convert_masp_tx_to_ibc_memo(&masp_tx);
+        let bytes = convert_masp_tx_to_ibc_memo(shielding_data);
         out.write_all(bytes.as_bytes())
             .expect("Writing IBC MASP transaction file failed.");
         println!(

crates/ibc/src/lib.rs

@@ -239,11 +239,13 @@ where
 {
     fn try_extract_masp_tx_from_envelope<Transfer: BorshDeserialize>(
         tx_data: &[u8],
-    ) -> StorageResult<Option<masp_primitives::transaction::Transaction>> {
+    ) -> StorageResult<
+        Option<(masp_primitives::transaction::Transaction, Option<Address>)>,
+    > {
         let msg = decode_message::<Transfer>(tx_data)
             .into_storage_result()
             .ok();
-        let tx = if let Some(IbcMessage::Envelope(envelope)) = msg {
+        let data = if let Some(IbcMessage::Envelope(envelope)) = msg {
             Some(extract_masp_tx_from_envelope(&envelope).ok_or_else(|| {
                 StorageError::new_const(
                     "Missing MASP transaction in IBC message",
@@ -252,7 +254,7 @@ where
         } else {
             None
         };
-        Ok(tx)
+        Ok(data.map(|shielding_data| (shielding_data.0, shielding_data.1)))
     }
 
     fn apply_ibc_packet<Transfer: BorshDeserialize>(
@@ -576,9 +578,10 @@ where
 #[derive(Debug)]
 pub struct InternalData<Transfer> {
     /// The transparent transfer that happens in parallel to IBC processes
+    // FIXME: can't we use this to encode the masp fee payment?
     pub transparent: Option<Transfer>,
     /// The shielded transaction that happens in parallel to IBC processes
-    pub shielded: Option<MaspTransaction>,
+    pub shielded: Option<(MaspTransaction, Option<Address>)>,
     /// IBC tokens that are credited/debited to internal accounts
     pub ibc_tokens: BTreeSet<Address>,
 }
@@ -737,7 +740,7 @@ where
                     .map_err(|e| Error::Handler(Box::new(e)))?;
 
                 // Extract MASP tx from the memo in the packet if needed
-                let (masp_tx, tokens) = match &*envelope {
+                let (shielding_data, tokens) = match &*envelope {
                     MsgEnvelope::Packet(PacketMsg::Recv(msg))
                         if self
                             .is_receiving_success(msg)?
@@ -773,7 +776,7 @@ where
                 };
                 Ok(InternalData {
                     transparent: None,
-                    shielded: masp_tx,
+                    shielded: shielding_data.map(|data| (data.0, data.1)),
                     ibc_tokens: tokens,
                 })
             }

crates/ibc/src/msg.rs

@@ -238,7 +238,10 @@ impl<Transfer: BorshSchema> BorshSchema for MsgNftTransfer<Transfer> {
 
 /// Shielding data in IBC packet memo
 #[derive(Debug, Clone, BorshDeserialize, BorshSerialize)]
-pub struct IbcShieldingData(pub MaspTransaction);
+pub struct IbcShieldingData(
+    pub MaspTransaction,
+    pub Option<namada_vp::Address>,
+);
 
 impl From<&IbcShieldingData> for String {
     fn from(data: &IbcShieldingData) -> Self {
@@ -272,7 +275,7 @@ impl FromStr for IbcShieldingData {
 /// Extract MASP transaction from IBC envelope
 pub fn extract_masp_tx_from_envelope(
     envelope: &MsgEnvelope,
-) -> Option<MaspTransaction> {
+) -> Option<IbcShieldingData> {
     match envelope {
         MsgEnvelope::Packet(PacketMsg::Recv(msg)) => {
             extract_masp_tx_from_packet(&msg.packet)
@@ -299,9 +302,11 @@ pub fn decode_ibc_shielding_data(
 }
 
 /// Extract MASP transaction from IBC packet memo
-pub fn extract_masp_tx_from_packet(packet: &Packet) -> Option<MaspTransaction> {
+pub fn extract_masp_tx_from_packet(
+    packet: &Packet,
+) -> Option<IbcShieldingData> {
     let memo = extract_memo_from_packet(packet, &packet.port_id_on_b)?;
-    decode_ibc_shielding_data(memo).map(|data| data.0)
+    decode_ibc_shielding_data(memo)
 }
 
 fn extract_memo_from_packet(
@@ -366,6 +371,8 @@ pub fn extract_traces_from_recv_msg(
 }
 
 /// Get IBC memo string from MASP transaction for receiving
-pub fn convert_masp_tx_to_ibc_memo(transaction: &MaspTransaction) -> String {
-    IbcShieldingData(transaction.clone()).into()
+pub fn convert_masp_tx_to_ibc_memo(
+    ibc_shielding_data: IbcShieldingData,
+) -> String {
+    ibc_shielding_data.into()
 }

crates/sdk/src/args.rs

@@ -521,12 +521,10 @@ pub enum Slippage {
     },
 }
 
-// FIXME: do we need a new event for the frontend fee?
-// FIXME: should the fee be taken shielded? It might avoid us to update the ibc
-// methods actually. Yes but it would produce a substantial amount of notes with
-// very little amounts FIXME: what happens to the sus fee if we wanto to
+// FIXME: what happens to the sus fee if we wanto to
 // shield/unshield more than one asset? It should probably be a vector of
-// targets, one for every asset
+// targets, one for every asset, yes but only for non-ibc txs, for ibc we only
+// support a single asset
 /// An token swap on Osmosis
 #[derive(Debug, Clone)]
 pub struct TxOsmosisSwap<C: NamadaTypes = SdkTypes> {
@@ -748,9 +746,7 @@ impl TxOsmosisSwap<SdkTypes> {
                 let memo = assert_json_obj(
                     serde_json::to_value(&NamadaMemo {
                         namada: NamadaMemoData::OsmosisSwap {
-                            shielding_data: StringEncoded::new(
-                                IbcShieldingData(shielding_tx),
-                            ),
+                            shielding_data: StringEncoded::new(shielding_tx),
                             shielded_amount: amount_to_shield,
                             overflow_receiver,
                         },
@@ -830,8 +826,6 @@ pub struct TxIbcTransfer<C: NamadaTypes = SdkTypes> {
     /// Refund target address when the shielded transfer failure
     pub refund_target: Option<C::TransferTarget>,
     /// IBC shielding transfer data for the destination chain
-    // FIXME: here the shielding transaction to reapply to namada, it should
-    // carry the sus fee
     pub ibc_shielding_data: Option<IbcShieldingData>,
     /// Memo for IBC transfer packet
     pub ibc_memo: Option<String>,
@@ -841,7 +835,6 @@ pub struct TxIbcTransfer<C: NamadaTypes = SdkTypes> {
     // FIXME: this should probably be an either with ibc_shielding_data. Yes
     // but there would still be the room for errors, maybe need marker traits?
     // Not sure...
-    // FIXME: support this in the client for testing only
     pub frontend_sus_fee: Option<TxTransparentTarget<C>>,
     /// Path to the TX WASM code file
     pub tx_code_path: PathBuf,

crates/sdk/src/masp.rs

@@ -83,7 +83,7 @@ fn extract_masp_tx(
 
             if let Some(transaction) = extract_masp_tx_from_envelope(&envelope)
             {
-                Ok(transaction)
+                Ok(transaction.0)
             } else {
                 Err(Error::Other(
                     "Failed to retrieve MASP over IBC transaction".to_string(),

crates/sdk/src/tx.rs

@@ -8,7 +8,6 @@ use std::time::Duration;
 
 use borsh::BorshSerialize;
 use masp_primitives::asset_type::AssetType;
-use masp_primitives::transaction::Transaction as MaspTransaction;
 use masp_primitives::transaction::builder::Builder;
 use masp_primitives::transaction::components::I128Sum;
 use masp_primitives::transaction::components::sapling::builder::{
@@ -55,7 +54,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::{MsgNftTransfer, MsgTransfer};
+use namada_ibc::{IbcShieldingData, MsgNftTransfer, MsgTransfer};
 use namada_io::{Client, Io, display_line, edisplay_line};
 use namada_proof_of_stake::parameters::{
     MAX_VALIDATOR_METADATA_LEN, PosParams,
@@ -2827,7 +2826,6 @@ pub async fn build_ibc_transfer(
         masp_fee_data
     };
 
-    // FIXME: adjust this
     if let Some(TxTransparentTarget {
         target,
         token,
@@ -2844,14 +2842,20 @@ pub async fn build_ibc_transfer(
                     args.tx.force,
                 )
                 .await?;
+                masp_transfer_data.sources.push((
+                    args.source.clone(),
+                    args.token.clone(),
+                    validated_amount,
+                ));
                 masp_transfer_data.targets.push((
                     TransferTarget::Address(target.to_owned()),
                     token.to_owned(),
                     validated_amount,
                 ));
 
                 transfer = transfer
-                    .credit(
+                    .transfer(
+                        source.to_owned(),
                         target.to_owned(),
                         token.to_owned(),
                         validated_amount,
@@ -4206,7 +4210,7 @@ pub async fn build_custom(
 pub async fn gen_ibc_shielding_transfer<N: Namada>(
     context: &N,
     args: args::GenIbcShieldingTransfer,
-) -> Result<Option<MaspTransaction>> {
+) -> Result<Option<IbcShieldingData>> {
     let source = IBC;
 
     let token = match args.asset {
@@ -4247,26 +4251,28 @@ pub async fn gen_ibc_shielding_transfer<N: Namada>(
         .precompute_asset_types(context.client(), tokens)
         .await;
 
-    let (extra_target, source_amount) = match &args.frontend_sus_fee {
-        Some((target, amount)) => {
-            // Validate the amount given
-            let validated_fee_amount =
-                validate_amount(context, amount.to_owned(), &token, false)
-                    .await?;
-            let source_amount =
-                checked!(validated_amount + validated_fee_amount)?;
-
-            (
-                vec![(
-                    target.to_owned(),
-                    token.to_owned(),
-                    validated_fee_amount,
-                )],
-                source_amount,
-            )
-        }
-        None => (vec![], validated_amount),
-    };
+    let (extra_recipient, extra_target, source_amount) =
+        match &args.frontend_sus_fee {
+            Some((target, amount)) => {
+                // Validate the amount given
+                let validated_fee_amount =
+                    validate_amount(context, amount.to_owned(), &token, false)
+                        .await?;
+                let source_amount =
+                    checked!(validated_amount + validated_fee_amount)?;
+
+                (
+                    target.address(),
+                    vec![(
+                        target.to_owned(),
+                        token.to_owned(),
+                        validated_fee_amount,
+                    )],
+                    source_amount,
+                )
+            }
+            None => (None, vec![], validated_amount),
+        };
 
     let masp_transfer_data = MaspTransferData {
         sources: vec![(
@@ -4281,9 +4287,6 @@ pub async fn gen_ibc_shielding_transfer<N: Namada>(
         .concat(),
     };
 
-    // eprintln!("MASP TRANSFER DATA: {:#?}", masp_transfer_data); //FIXME:
-    // remove
-
     let shielded_transfer = {
         let mut shielded = context.shielded_mut().await;
         shielded
@@ -4299,10 +4302,8 @@ pub async fn gen_ibc_shielding_transfer<N: Namada>(
             .map_err(|err| TxSubmitError::MaspError(err.to_string()))?
     };
 
-    // eprintln!("GENERATED MASP BUNDLE: {:#?}", shielded_transfer); //FIXME:
-    // remove
-
-    Ok(shielded_transfer.map(|st| st.masp_tx))
+    Ok(shielded_transfer
+        .map(|st| IbcShieldingData(st.masp_tx, extra_recipient)))
 }
 
 pub(crate) async fn get_ibc_src_port_channel(