fix(ibc)!: handle ibc withdrawals correctly

This commit adds a `use_compat_address` bool to withdrawals,
to allow issuing compat address withdrawals. It also implements correct
error handling on withdrawal attemtps when counterparty chains
return an error.

crates/bin/pcli/src/command/tx.rs

@@ -253,6 +253,10 @@ pub enum TxCmd {
         /// The selected fee tier to multiply the fee amount by.
         #[clap(short, long, default_value_t)]
         fee_tier: FeeTier,
+        /// Whether to use a Bech32(non-m) address for the withdrawal.
+        /// Required for some chains for a successful acknowledgement.
+        #[clap(long)]
+        use_compat_address: bool,
     },
     /// Broadcast a saved transaction to the network
     #[clap(display_order = 1000)]
@@ -974,6 +978,7 @@ impl TxCmd {
                 channel,
                 source,
                 fee_tier,
+                use_compat_address,
             } => {
                 let destination_chain_address = to;
 
@@ -1087,6 +1092,7 @@ impl TxCmd {
                     return_address: ephemeral_return_address,
                     // TODO: impl From<u64> for ChannelId
                     source_channel: ChannelId::from_str(format!("channel-{}", channel).as_ref())?,
+                    use_compat_address: *use_compat_address,
                 };
 
                 let plan = Planner::new(OsRng)

crates/core/component/ibc/src/component/app_handler.rs

@@ -55,7 +55,10 @@ pub trait AppHandlerExecute: Send + Sync {
 
     async fn recv_packet_execute<S: StateWrite>(state: S, msg: &MsgRecvPacket) -> Result<()>;
     async fn timeout_packet_execute<S: StateWrite>(state: S, msg: &MsgTimeout) -> Result<()>;
-    async fn acknowledge_packet_execute<S: StateWrite>(state: S, msg: &MsgAcknowledgement);
+    async fn acknowledge_packet_execute<S: StateWrite>(
+        state: S,
+        msg: &MsgAcknowledgement,
+    ) -> Result<()>;
 }
 
 pub trait AppHandler: AppHandlerCheck + AppHandlerExecute {}

crates/core/component/ibc/src/component/client.rs

@@ -577,7 +577,12 @@ mod tests {
         async fn timeout_packet_execute<S: StateWrite>(_state: S, _msg: &MsgTimeout) -> Result<()> {
             Ok(())
         }
-        async fn acknowledge_packet_execute<S: StateWrite>(_state: S, _msg: &MsgAcknowledgement) {}
+        async fn acknowledge_packet_execute<S: StateWrite>(
+            _state: S,
+            _msg: &MsgAcknowledgement,
+        ) -> Result<()> {
+            Ok(())
+        }
     }
 
     #[async_trait]

crates/core/component/ibc/src/component/msg_handler/acknowledgement.rs

@@ -119,7 +119,7 @@ impl MsgHandler for MsgAcknowledgement {
 
         let transfer = PortId::transfer();
         if self.packet.port_on_b == transfer {
-            AH::acknowledge_packet_execute(state, self).await;
+            AH::acknowledge_packet_execute(state, self).await?;
         } else {
             anyhow::bail!("invalid port id");
         }

crates/core/component/shielded-pool/Cargo.toml

@@ -51,7 +51,7 @@ decaf377-rdsa = {workspace = true}
 futures = {workspace = true}
 hex = {workspace = true}
 ibc-proto = {workspace = true, default-features = false}
-ibc-types = {workspace = true, default-features = false}
+ibc-types = {workspace = true, features = ["with_serde"], default-features = false}
 im = {workspace = true}
 metrics = {workspace = true}
 once_cell = {workspace = true}

crates/core/component/shielded-pool/src/component/transfer.rs

@@ -7,6 +7,7 @@ use crate::{
 use anyhow::{Context, Result};
 use async_trait::async_trait;
 use cnidarium::{StateRead, StateWrite};
+use ibc_types::core::channel::Packet;
 use ibc_types::{
     core::channel::{
         channel::Order as ChannelOrder,
@@ -406,8 +407,8 @@ async fn recv_transfer_packet_inner<S: StateWrite>(
 }
 
 // see: https://github.com/cosmos/ibc/blob/8326e26e7e1188b95c32481ff00348a705b23700/spec/app/ics-020-fungible-token-transfer/README.md?plain=1#L297
-async fn timeout_packet_inner<S: StateWrite>(mut state: S, msg: &MsgTimeout) -> Result<()> {
-    let packet_data: FungibleTokenPacketData = serde_json::from_slice(msg.packet.data.as_slice())?;
+async fn timeout_packet_inner<S: StateWrite>(mut state: S, packet: &Packet) -> Result<()> {
+    let packet_data: FungibleTokenPacketData = serde_json::from_slice(packet.data.as_slice())?;
     let denom: asset::Metadata = packet_data // CRITICAL: verify that this denom is validated in upstream timeout handling
         .denom
         .as_str()
@@ -430,11 +431,11 @@ async fn timeout_packet_inner<S: StateWrite>(mut state: S, msg: &MsgTimeout) ->
         asset_id: denom.id(),
     };
 
-    if is_source(&msg.packet.port_on_a, &msg.packet.chan_on_a, &denom, true) {
+    if is_source(&packet.port_on_a, &packet.chan_on_a, &denom, true) {
         // sender was source chain, unescrow tokens back to sender
         let value_balance: Amount = state
             .get(&state_key::ics20_value_balance::by_asset_id(
-                &msg.packet.chan_on_a,
+                &packet.chan_on_a,
                 &denom.id(),
             ))
             .await?
@@ -449,8 +450,8 @@ async fn timeout_packet_inner<S: StateWrite>(mut state: S, msg: &MsgTimeout) ->
                 value,
                 &receiver,
                 CommitmentSource::Ics20Transfer {
-                    packet_seq: msg.packet.sequence.0,
-                    channel_id: msg.packet.chan_on_a.0.clone(),
+                    packet_seq: packet.sequence.0,
+                    channel_id: packet.chan_on_a.0.clone(),
                     sender: packet_data.sender.clone(),
                 },
             )
@@ -460,7 +461,7 @@ async fn timeout_packet_inner<S: StateWrite>(mut state: S, msg: &MsgTimeout) ->
         // update the value balance
         let value_balance: Amount = state
             .get(&state_key::ics20_value_balance::by_asset_id(
-                &msg.packet.chan_on_a,
+                &packet.chan_on_a,
                 &denom.id(),
             ))
             .await?
@@ -471,13 +472,13 @@ async fn timeout_packet_inner<S: StateWrite>(mut state: S, msg: &MsgTimeout) ->
             .checked_sub(&amount)
             .context("underflow in ics20 timeout packet value balance subtraction")?;
         state.put(
-            state_key::ics20_value_balance::by_asset_id(&msg.packet.chan_on_a, &denom.id()),
+            state_key::ics20_value_balance::by_asset_id(&packet.chan_on_a, &denom.id()),
             new_value_balance,
         );
     } else {
         let value_balance: Amount = state
             .get(&state_key::ics20_value_balance::by_asset_id(
-                &msg.packet.chan_on_a,
+                &packet.chan_on_a,
                 &denom.id(),
             ))
             .await?
@@ -489,8 +490,8 @@ async fn timeout_packet_inner<S: StateWrite>(mut state: S, msg: &MsgTimeout) ->
                 &receiver,
                 // NOTE: should this be Ics20TransferTimeout?
                 CommitmentSource::Ics20Transfer {
-                    packet_seq: msg.packet.sequence.0,
-                    channel_id: msg.packet.chan_on_a.0.clone(),
+                    packet_seq: packet.sequence.0,
+                    channel_id: packet.chan_on_a.0.clone(),
                     sender: packet_data.sender.clone(),
                 },
             )
@@ -499,7 +500,7 @@ async fn timeout_packet_inner<S: StateWrite>(mut state: S, msg: &MsgTimeout) ->
 
         let new_value_balance = value_balance.saturating_add(&value.amount);
         state.put(
-            state_key::ics20_value_balance::by_asset_id(&msg.packet.chan_on_a, &denom.id()),
+            state_key::ics20_value_balance::by_asset_id(&packet.chan_on_a, &denom.id()),
             new_value_balance,
         );
     }
@@ -540,14 +541,30 @@ impl AppHandlerExecute for Ics20Transfer {
 
     async fn timeout_packet_execute<S: StateWrite>(mut state: S, msg: &MsgTimeout) -> Result<()> {
         // timeouts may fail due to counterparty chains sending transfers of u128-1
-        timeout_packet_inner(&mut state, msg)
+        timeout_packet_inner(&mut state, &msg.packet)
             .await
             .context("able to timeout packet")?;
 
         Ok(())
     }
 
-    async fn acknowledge_packet_execute<S: StateWrite>(_state: S, _msg: &MsgAcknowledgement) {}
+    async fn acknowledge_packet_execute<S: StateWrite>(
+        mut state: S,
+        msg: &MsgAcknowledgement,
+    ) -> Result<()> {
+        let ack: TokenTransferAcknowledgement =
+            serde_json::from_slice(msg.acknowledgement.as_slice())?;
+        if !ack.is_successful() {
+            // in the case where a counterparty chain acknowledges a packet with an error,
+            // for example due to a middleware processing issue or other behavior,
+            // the funds should be unescrowed back to the packet sender.
+            timeout_packet_inner(&mut state, &msg.packet)
+                .await
+                .context("unable to refund packet acknowledgement")?;
+        }
+
+        Ok(())
+    }
 }
 
 impl AppHandler for Ics20Transfer {}

crates/core/component/shielded-pool/src/ics20_withdrawal.rs

@@ -37,6 +37,10 @@ pub struct Ics20Withdrawal {
     pub timeout_time: u64,
     // the source channel used for the withdrawal
     pub source_channel: ChannelId,
+
+    // Whether to use a "compat" (bech32, non-m) address for the return address in the withdrawal,
+    // for compatability with chains that expect to be able to parse the return address as bech32.
+    pub use_compat_address: bool,
 }
 
 #[cfg(feature = "component")]
@@ -113,6 +117,7 @@ impl From<Ics20Withdrawal> for pb::Ics20Withdrawal {
             timeout_height: Some(w.timeout_height.into()),
             timeout_time: w.timeout_time,
             source_channel: w.source_channel.to_string(),
+            use_compat_address: w.use_compat_address,
         }
     }
 }
@@ -142,17 +147,23 @@ impl TryFrom<pb::Ics20Withdrawal> for Ics20Withdrawal {
                 .try_into()?,
             timeout_time: s.timeout_time,
             source_channel: ChannelId::from_str(&s.source_channel)?,
+            use_compat_address: s.use_compat_address,
         })
     }
 }
 
 impl From<Ics20Withdrawal> for pb::FungibleTokenPacketData {
     fn from(w: Ics20Withdrawal) -> Self {
+        let receiver = match w.use_compat_address {
+            true => w.return_address.compat_encoding(),
+            false => w.return_address.to_string(),
+        };
+
         pb::FungibleTokenPacketData {
             amount: w.value().amount.to_string(),
             denom: w.denom.to_string(),
-            receiver: w.destination_chain_address,
-            sender: w.return_address.to_string(),
+            receiver,
+            sender: w.return_address.compat_encoding(),
             memo: "".to_string(),
         }
     }

crates/proto/src/gen/penumbra.core.component.ibc.v1.rs

@@ -69,6 +69,10 @@ pub struct Ics20Withdrawal {
     /// The source channel used for the withdrawal
     #[prost(string, tag = "7")]
     pub source_channel: ::prost::alloc::string::String,
+    /// Whether to use a "compat" (bech32, non-m) address for the return address in the withdrawal,
+    /// for compatability with chains that expect to be able to parse the return address as bech32.
+    #[prost(bool, tag = "8")]
+    pub use_compat_address: bool,
 }
 impl ::prost::Name for Ics20Withdrawal {
     const NAME: &'static str = "Ics20Withdrawal";

crates/proto/src/gen/penumbra.core.component.ibc.v1.serde.rs

@@ -1054,6 +1054,9 @@ impl serde::Serialize for Ics20Withdrawal {
         if !self.source_channel.is_empty() {
             len += 1;
         }
+        if self.use_compat_address {
+            len += 1;
+        }
         let mut struct_ser = serializer.serialize_struct("penumbra.core.component.ibc.v1.Ics20Withdrawal", len)?;
         if let Some(v) = self.amount.as_ref() {
             struct_ser.serialize_field("amount", v)?;
@@ -1077,6 +1080,9 @@ impl serde::Serialize for Ics20Withdrawal {
         if !self.source_channel.is_empty() {
             struct_ser.serialize_field("sourceChannel", &self.source_channel)?;
         }
+        if self.use_compat_address {
+            struct_ser.serialize_field("useCompatAddress", &self.use_compat_address)?;
+        }
         struct_ser.end()
     }
 }
@@ -1099,6 +1105,8 @@ impl<'de> serde::Deserialize<'de> for Ics20Withdrawal {
             "timeoutTime",
             "source_channel",
             "sourceChannel",
+            "use_compat_address",
+            "useCompatAddress",
         ];
 
         #[allow(clippy::enum_variant_names)]
@@ -1110,6 +1118,7 @@ impl<'de> serde::Deserialize<'de> for Ics20Withdrawal {
             TimeoutHeight,
             TimeoutTime,
             SourceChannel,
+            UseCompatAddress,
             __SkipField__,
         }
         impl<'de> serde::Deserialize<'de> for GeneratedField {
@@ -1139,6 +1148,7 @@ impl<'de> serde::Deserialize<'de> for Ics20Withdrawal {
                             "timeoutHeight" | "timeout_height" => Ok(GeneratedField::TimeoutHeight),
                             "timeoutTime" | "timeout_time" => Ok(GeneratedField::TimeoutTime),
                             "sourceChannel" | "source_channel" => Ok(GeneratedField::SourceChannel),
+                            "useCompatAddress" | "use_compat_address" => Ok(GeneratedField::UseCompatAddress),
                             _ => Ok(GeneratedField::__SkipField__),
                         }
                     }
@@ -1165,6 +1175,7 @@ impl<'de> serde::Deserialize<'de> for Ics20Withdrawal {
                 let mut timeout_height__ = None;
                 let mut timeout_time__ = None;
                 let mut source_channel__ = None;
+                let mut use_compat_address__ = None;
                 while let Some(k) = map_.next_key()? {
                     match k {
                         GeneratedField::Amount => {
@@ -1211,6 +1222,12 @@ impl<'de> serde::Deserialize<'de> for Ics20Withdrawal {
                             }
                             source_channel__ = Some(map_.next_value()?);
                         }
+                        GeneratedField::UseCompatAddress => {
+                            if use_compat_address__.is_some() {
+                                return Err(serde::de::Error::duplicate_field("useCompatAddress"));
+                            }
+                            use_compat_address__ = Some(map_.next_value()?);
+                        }
                         GeneratedField::__SkipField__ => {
                             let _ = map_.next_value::<serde::de::IgnoredAny>()?;
                         }
@@ -1224,6 +1241,7 @@ impl<'de> serde::Deserialize<'de> for Ics20Withdrawal {
                     timeout_height: timeout_height__,
                     timeout_time: timeout_time__.unwrap_or_default(),
                     source_channel: source_channel__.unwrap_or_default(),
+                    use_compat_address: use_compat_address__.unwrap_or_default(),
                 })
             }
         }

proto/penumbra/penumbra/core/component/ibc/v1/ibc.proto

@@ -50,6 +50,10 @@ message Ics20Withdrawal {
 
   // The source channel used for the withdrawal
   string source_channel = 7;
+
+  // Whether to use a "compat" (bech32, non-m) address for the return address in the withdrawal,
+  // for compatability with chains that expect to be able to parse the return address as bech32.
+  bool use_compat_address = 8;
 }
 
 message ClientData {