wip2

there is still a bug. when refunding a packet, we're charging
unshielding fees, for some reason :\

Author: sug0

crates/ibc/src/context/middlewares.rs

@@ -88,8 +88,8 @@ pub fn send_transfer_execute<SendPacketCtx, TokenCtx>(
 ) -> Result<(), TokenTransferError>
 where
     SendPacketCtx: SendPacketExecutionContext,
-    TokenCtx: TokenTransferExecutionContext
-        + MaspUnshieldingFeesExecutionContext<MsgTransfer>,
+    TokenCtx:
+        TokenTransferExecutionContext + MaspUnshieldingFeesExecutionContext,
 {
     macro_rules! assemble_middlewares {
         ($base:expr) => { $base };
@@ -106,11 +106,11 @@ where
 }
 
 /// Context that handles ICS-20 MASP unshielding fees.
-pub trait MaspUnshieldingFeesExecutionContext<Msg> {
+pub trait MaspUnshieldingFeesExecutionContext {
     /// Apply a MASP unshielding fee over the given ICS-20 packet.
     fn apply_masp_unshielding_fee(
         &self,
-        msg: &mut Msg,
+        msg: &mut MsgTransfer,
     ) -> Result<(), TokenTransferError>;
 }
 
@@ -128,8 +128,8 @@ where
         MsgTransfer,
     ) -> Result<(), TokenTransferError>,
     SendPacketCtx: SendPacketExecutionContext,
-    TokenCtx: TokenTransferExecutionContext
-        + MaspUnshieldingFeesExecutionContext<MsgTransfer>,
+    TokenCtx:
+        TokenTransferExecutionContext + MaspUnshieldingFeesExecutionContext,
 {
     |send_packet_ctx_a, token_ctx_a, mut msg| {
         token_ctx_a.apply_masp_unshielding_fee(&mut msg)?;

crates/ibc/src/context/token_transfer.rs

@@ -776,60 +776,7 @@ where
     )?))
 }
 
-#[allow(missing_docs)]
-pub struct ParamsStorageAdapter<S, Params>(S, PhantomData<Params>);
-
-impl<S, Params> ParamsStorageAdapter<S, Params> {
-    #[allow(missing_docs)]
-    pub const fn adapt(storage: S) -> Self {
-        Self(storage, PhantomData)
-    }
-}
-
-impl<S, Params> MaspUnshieldingFeesExecutionContext<crate::IbcTransferInfo>
-    for ParamsStorageAdapter<S, Params>
-where
-    Params: namada_systems::parameters::Read<S>,
-{
-    fn apply_masp_unshielding_fee(
-        &self,
-        msg: &mut crate::IbcTransferInfo,
-    ) -> Result<(), TokenTransferError> {
-        for trace in msg.ibc_traces.iter() {
-            let ibc_token =
-                crate::trace::convert_to_address(trace).map_err(|err| {
-                    HostError::Other {
-                        description: format!(
-                            "Failed to convert {trace:?} to address: {err}"
-                        ),
-                    }
-                })?;
-
-            if let Some(fee) = get_masp_unshielding_fee::<_, Params>(
-                &self.0,
-                &ibc_token,
-                &msg.amount,
-            )
-            .map_err(TokenTransferError::Host)?
-            .filter(|fee| !fee.is_zero())
-            {
-                msg.amount =
-                    // NOTE: we're adding the fee, because we want to recreate the
-                    // original packet
-                    msg.amount.checked_add(fee).ok_or_else(|| HostError::Other {
-                        description: "Unshielding fee greater than withdrawn \
-                                      amount"
-                            .to_string(),
-                    })?;
-            }
-        }
-
-        Ok(())
-    }
-}
-
-impl<C, Params, Token, ShieldedToken>
-    MaspUnshieldingFeesExecutionContext<MsgTransfer>
+impl<C, Params, Token, ShieldedToken> MaspUnshieldingFeesExecutionContext
     for TokenTransferContext<C, Params, Token, ShieldedToken>
 where
     C: IbcCommonContext,
@@ -839,31 +786,34 @@ where
         &self,
         msg: &mut MsgTransfer,
     ) -> Result<(), TokenTransferError> {
+        tracing::warn!(?msg, "called apply_masp_unshielding_fee");
+
         // no fee is taken if this is not a masp unshielding op
         let has_masp_tx = self
             .config
             .contains(TokenTransferContextConfig::HAS_MASP_TX);
         if !has_masp_tx {
+            tracing::warn!("apply_masp_unshielding_fee has no masp tx");
             return Ok(());
         }
 
-        let (ibc_token, mut amount) = get_token_amount(&msg.packet_data.token)
+        let (ibc_token, amount) = get_token_amount(&msg.packet_data.token)
             .map_err(TokenTransferError::Host)?;
 
         if let Some(fee) = self
             .get_masp_unshielding_fee(&ibc_token, &amount)
             .map_err(TokenTransferError::Host)?
             .filter(|fee| !fee.is_zero())
         {
-            amount =
+            let new_amount =
                 amount.checked_sub(fee).ok_or_else(|| HostError::Other {
                     description: "Unshielding fee greater than withdrawn \
                                   amount"
                         .to_string(),
                 })?;
 
             // commit the updated amount to the packet
-            msg.packet_data.token.amount = amount.into();
+            msg.packet_data.token.amount = new_amount.into();
 
             // transfer the fee to PGF, and trigger its vp
             self.insert_verifier(&PGF);
@@ -872,6 +822,15 @@ where
                 .transfer_token(&MASP, &PGF, &ibc_token, fee)
                 .map_err(HostError::from)
                 .map_err(TokenTransferError::Host)?;
+
+            tracing::warn!(
+                %ibc_token,
+                %fee,
+                original_amount = %amount,
+                %new_amount,
+                new_msg = ?msg,
+                "apply_masp_unshielding_fee fees applied"
+            );
         }
 
         Ok(())

crates/ibc/src/lib.rs

@@ -113,10 +113,7 @@ use trace::{
     is_sender_chain_source,
 };
 
-use crate::context::middlewares::{
-    MaspUnshieldingFeesExecutionContext, send_transfer_execute,
-};
-use crate::context::token_transfer::ParamsStorageAdapter;
+use crate::context::middlewares::send_transfer_execute;
 use crate::storage::{
     channel_counter_key, client_counter_key, connection_counter_key,
     deposit_prefix, nft_class_key, nft_metadata_key, withdraw_prefix,
@@ -164,6 +161,7 @@ impl From<Error> for StorageError {
     }
 }
 
+#[derive(Debug)]
 struct IbcTransferInfo {
     src_port_id: PortId,
     src_channel_id: ChannelId,
@@ -183,6 +181,11 @@ impl TryFrom<IbcMsgTransfer> for IbcTransferInfo {
     ) -> std::result::Result<Self, Self::Error> {
         let packet_data = serde_json::to_vec(&message.packet_data)
             .map_err(StorageError::new)?;
+        tracing::warn!(
+            ?message,
+            ?packet_data,
+            "parsing IbcTransferInfo from IbcMsgTransfer"
+        );
         let ibc_traces = vec![message.packet_data.token.denom.to_string()];
         let amount: Amount = message.packet_data.token.amount.into();
         let receiver = message.packet_data.receiver.to_string();
@@ -328,6 +331,7 @@ where
             Some(IbcMessage::Transfer(msg)) => {
                 // Get the packet commitment from post-storage that corresponds
                 // to this event
+                tracing::warn!(msg = ?msg.message, "calling apply_transfer_msg");
                 let ibc_transfer = IbcTransferInfo::try_from(msg.message)?;
                 let receiver = ibc_transfer.receiver.clone();
                 let addr = TAddrData::Ibc(receiver.clone());
@@ -337,14 +341,60 @@ where
                     accum,
                     ibc_transfer,
                     keys_changed,
-                    |ibc_transfer| {
-                        // Recreate the original packet, by applying middlewares
-                        let adapter = ParamsStorageAdapter::<_, Params>::adapt(
-                            storage_pre,
-                        );
-                        adapter
-                            .apply_masp_unshielding_fee(ibc_transfer)
-                            .into_storage_result()
+                    |packet_data_vec| {
+                        let mut packet_data =
+                            serde_json::from_slice::<PacketData>(
+                                packet_data_vec,
+                            )
+                            .map_err(StorageError::new)?;
+
+                        let token = trace::convert_to_address(
+                            packet_data.token.denom.to_string(),
+                        )
+                        .map_err(|e| {
+                            Error::Trace(format!(
+                                "Failed to convert IBC trace to Namada \
+                                 address: {e}"
+                            ))
+                        })
+                        .into_storage_result()?;
+
+                        let Some(fee_percentage) =
+                            Params::ibc_unshielding_fee_percentage(
+                                &storage_pre,
+                                &token,
+                            )?
+                        else {
+                            return Ok(());
+                        };
+
+                        let old_amount: Amount =
+                            packet_data.token.amount.into();
+                        let fee = old_amount
+                            .checked_mul_dec(fee_percentage)
+                            .ok_or(StorageError::new_const(
+                                "IBC unshielding fee overflow",
+                            ))?;
+
+                        if fee.is_zero() {
+                            return Ok(());
+                        }
+
+                        packet_data.token.amount = old_amount
+                            .checked_sub(fee)
+                            .ok_or_else(|| {
+                                StorageError::new_const(
+                                    "Unshielding fee greater than withdrawn \
+                                     amount",
+                                )
+                            })?
+                            .into();
+
+                        packet_data_vec.clear();
+                        serde_json::to_writer(packet_data_vec, &packet_data)
+                            .map_err(StorageError::new)?;
+
+                        Ok(())
                     },
                 )?;
             }
@@ -525,14 +575,17 @@ impl fmt::Display for IbcAccountId {
     }
 }
 
-fn check_ibc_transfer<S>(
+fn check_ibc_transfer<S, F>(
     storage: &S,
     ibc_transfer: &IbcTransferInfo,
     keys_changed: &BTreeSet<Key>,
+    alter_packet_data: F,
 ) -> StorageResult<()>
 where
     S: StorageRead,
+    F: FnOnce(&mut Vec<u8>) -> StorageResult<()>,
 {
+    tracing::warn!(?ibc_transfer, "calling check_ibc_transfer");
     let IbcTransferInfo {
         src_port_id,
         src_channel_id,
@@ -553,6 +606,9 @@ where
         )));
     }
 
+    let mut packet_data = packet_data.clone();
+    alter_packet_data(&mut packet_data)?;
+
     // The commitment is also validated in IBC VP. Make sure that for when
     // IBC VP isn't triggered.
     let actual: PacketCommitment = storage
@@ -563,7 +619,7 @@ where
         )))?
         .into();
     let expected = compute_packet_commitment(
-        packet_data,
+        &packet_data,
         timeout_height,
         timeout_timestamp,
     );
@@ -601,16 +657,20 @@ fn check_packet_receiving(
 fn apply_transfer_msg<S, F>(
     storage: &S,
     mut accum: ChangedBalances,
-    mut ibc_transfer: IbcTransferInfo,
+    ibc_transfer: IbcTransferInfo,
     keys_changed: &BTreeSet<Key>,
-    alter_ibc_transfer: F,
+    alter_packet_data: F,
 ) -> StorageResult<ChangedBalances>
 where
     S: StorageRead,
-    F: FnOnce(&mut IbcTransferInfo) -> StorageResult<()>,
+    F: FnOnce(&mut Vec<u8>) -> StorageResult<()>,
 {
-    check_ibc_transfer(storage, &ibc_transfer, keys_changed)?;
-    alter_ibc_transfer(&mut ibc_transfer)?;
+    check_ibc_transfer(
+        storage,
+        &ibc_transfer,
+        keys_changed,
+        alter_packet_data,
+    )?;
 
     let IbcTransferInfo {
         ref ibc_traces,