Process write acknowledgements on packet_callback (#2439)

* add write_acknowledgement process on packet_callback

* fix bug

* extract write_ack logic to process_write_ack function

* add issue #2424 and 2438 to changelog

* Derive From/Into for opaque Ack

* Use generic Ack as writeAck input

* Simplify Ack trait

* Process write ack using conversion to generic Ack

* Allow `packet_callback()` to emit IbcEvents

Co-authored-by: hu55a1n1 <sufialhussaini@gmail.com>

.changelog/unreleased/bug-fixes/ibc/2424-add-write-acknowledgement-packet-process-and-store.md

@@ -0,0 +1,2 @@
+- Add Write Acknowledgement Process on packet callback
+  ([#2424](https://github.com/informalsystems/ibc-rs/issues/2424)).

.changelog/unreleased/bug-fixes/ibc/2428-fix-handle-process-write-acknowledgement-incorrect-store-type.md

@@ -0,0 +1,2 @@
+- Mirror fix write_acknowledgement handler incorrectly uses packet's source_{port, channel} as key for storing acks
+  ([#2428](https://github.com/informalsystems/ibc-rs/issues/2428))

modules/src/core/ics04_channel/context.rs

@@ -94,7 +94,7 @@ pub trait ChannelReader {
     }
 
     fn ack_commitment(&self, ack: Acknowledgement) -> AcknowledgementCommitment {
-        self.hash(ack.into_bytes()).into()
+        self.hash(ack.into()).into()
     }
 
     /// A hashing function for packet commitments

modules/src/core/ics04_channel/handler.rs

@@ -1,13 +1,15 @@
 //! This module implements the processing logic for ICS4 (channel) messages.
+use crate::prelude::*;
 
 use crate::core::ics04_channel::channel::ChannelEnd;
 use crate::core::ics04_channel::context::ChannelReader;
 use crate::core::ics04_channel::error::Error;
 use crate::core::ics04_channel::msgs::ChannelMsg;
+use crate::core::ics04_channel::packet::Packet;
 use crate::core::ics04_channel::{msgs::PacketMsg, packet::PacketResult};
 use crate::core::ics24_host::identifier::{ChannelId, PortId};
 use crate::core::ics26_routing::context::{
-    Ics26Context, ModuleId, ModuleOutputBuilder, OnRecvPacketAck, Router,
+    Acknowledgement, Ics26Context, ModuleId, ModuleOutputBuilder, OnRecvPacketAck, Router,
 };
 use crate::handler::{HandlerOutput, HandlerOutputBuilder};
 
@@ -192,11 +194,28 @@ pub fn packet_callback<Ctx>(
     ctx: &mut Ctx,
     module_id: &ModuleId,
     msg: &PacketMsg,
-    module_output: &mut ModuleOutputBuilder,
+    output: &mut HandlerOutputBuilder<()>,
 ) -> Result<(), Error>
 where
     Ctx: Ics26Context,
 {
+    let mut module_output = ModuleOutputBuilder::new();
+    let mut core_output = HandlerOutputBuilder::new();
+
+    let result = do_packet_callback(ctx, module_id, msg, &mut module_output, &mut core_output);
+    output.merge(module_output);
+    output.merge(core_output);
+
+    result
+}
+
+fn do_packet_callback(
+    ctx: &mut impl Ics26Context,
+    module_id: &ModuleId,
+    msg: &PacketMsg,
+    module_output: &mut ModuleOutputBuilder,
+    core_output: &mut HandlerOutputBuilder<()>,
+) -> Result<(), Error> {
     let cb = ctx
         .router_mut()
         .get_route_mut(module_id)
@@ -206,24 +225,53 @@ where
         PacketMsg::RecvPacket(msg) => {
             let result = cb.on_recv_packet(module_output, &msg.packet, &msg.signer);
             match result {
-                OnRecvPacketAck::Nil(write_fn) | OnRecvPacketAck::Successful(_, write_fn) => {
+                OnRecvPacketAck::Nil(write_fn) => {
+                    write_fn(cb.as_any_mut()).map_err(Error::app_module)
+                }
+                OnRecvPacketAck::Successful(ack, write_fn) => {
                     write_fn(cb.as_any_mut()).map_err(Error::app_module)?;
+
+                    process_write_ack(ctx, msg.packet.clone(), ack.as_ref(), core_output)
+                }
+                OnRecvPacketAck::Failed(ack) => {
+                    process_write_ack(ctx, msg.packet.clone(), ack.as_ref(), core_output)
                 }
-                OnRecvPacketAck::Failed(_) => {}
             }
         }
         PacketMsg::AckPacket(msg) => cb.on_acknowledgement_packet(
             module_output,
             &msg.packet,
             &msg.acknowledgement,
             &msg.signer,
-        )?,
-        PacketMsg::ToPacket(msg) => {
-            cb.on_timeout_packet(module_output, &msg.packet, &msg.signer)?
-        }
+        ),
+        PacketMsg::ToPacket(msg) => cb.on_timeout_packet(module_output, &msg.packet, &msg.signer),
         PacketMsg::ToClosePacket(msg) => {
-            cb.on_timeout_packet(module_output, &msg.packet, &msg.signer)?
+            cb.on_timeout_packet(module_output, &msg.packet, &msg.signer)
         }
-    };
+    }
+}
+
+fn process_write_ack(
+    ctx: &mut impl Ics26Context,
+    packet: Packet,
+    acknowledgement: &dyn Acknowledgement,
+    core_output: &mut HandlerOutputBuilder<()>,
+) -> Result<(), Error> {
+    let HandlerOutput {
+        result,
+        log,
+        events,
+    } = write_acknowledgement::process(ctx, packet, acknowledgement.as_ref().to_vec().into())?;
+
+    // store write ack result
+    ctx.store_packet_result(result)?;
+
+    core_output.merge_output(
+        HandlerOutput::builder()
+            .with_log(log)
+            .with_events(events)
+            .with_result(()),
+    );
+
     Ok(())
 }

modules/src/core/ics04_channel/handler/write_acknowledgement.rs

@@ -1,6 +1,7 @@
 use crate::core::ics04_channel::channel::State;
 use crate::core::ics04_channel::commitment::AcknowledgementCommitment;
 use crate::core::ics04_channel::events::WriteAcknowledgement;
+use crate::core::ics04_channel::msgs::acknowledgement::Acknowledgement;
 use crate::core::ics04_channel::packet::{Packet, PacketResult, Sequence};
 use crate::core::ics04_channel::{context::ChannelReader, error::Error};
 use crate::core::ics24_host::identifier::{ChannelId, PortId};
@@ -21,7 +22,7 @@ pub struct WriteAckPacketResult {
 pub fn process(
     ctx: &dyn ChannelReader,
     packet: Packet,
-    ack: Vec<u8>,
+    ack: Acknowledgement,
 ) -> HandlerResult<PacketResult, Error> {
     let mut output = HandlerOutput::builder();
 
@@ -56,18 +57,18 @@ pub fn process(
     }
 
     let result = PacketResult::WriteAck(WriteAckPacketResult {
-        port_id: packet.source_port.clone(),
-        channel_id: packet.source_channel.clone(),
+        port_id: packet.destination_port.clone(),
+        channel_id: packet.destination_channel.clone(),
         seq: packet.sequence,
-        ack_commitment: ctx.ack_commitment(ack.clone().into()),
+        ack_commitment: ctx.ack_commitment(ack.clone()),
     });
 
     output.log("success: packet write acknowledgement");
 
     output.emit(IbcEvent::WriteAcknowledgement(WriteAcknowledgement {
         height: ctx.host_height(),
         packet,
-        ack,
+        ack: ack.into(),
     }));
 
     Ok(output.with_result(result))
@@ -176,7 +177,7 @@ mod tests {
         .collect();
 
         for test in tests {
-            let res = process(&test.ctx, test.packet.clone(), test.ack);
+            let res = process(&test.ctx, test.packet.clone(), test.ack.into());
             // Additionally check the events and the output objects in the result.
             match res {
                 Ok(proto_output) => {

modules/src/core/ics04_channel/msgs/acknowledgement.rs

@@ -1,8 +1,8 @@
 use crate::prelude::*;
 
-use tendermint_proto::Protobuf;
-
+use derive_more::{From, Into};
 use ibc_proto::ibc::core::channel::v1::MsgAcknowledgement as RawMsgAcknowledgement;
+use tendermint_proto::Protobuf;
 
 use crate::core::ics04_channel::error::Error;
 use crate::core::ics04_channel::packet::Packet;
@@ -13,22 +13,12 @@ use crate::tx_msg::Msg;
 pub const TYPE_URL: &str = "/ibc.core.channel.v1.MsgAcknowledgement";
 
 /// A generic Acknowledgement type that modules may interpret as they like.
-#[derive(Clone, Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq, From, Into)]
 pub struct Acknowledgement(Vec<u8>);
 
 impl Acknowledgement {
-    pub fn into_bytes(self) -> Vec<u8> {
-        self.0
-    }
-
-    pub fn from_bytes(bytes: Vec<u8>) -> Self {
-        bytes.into()
-    }
-}
-
-impl From<Vec<u8>> for Acknowledgement {
-    fn from(bytes: Vec<u8>) -> Self {
-        Self(bytes)
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
     }
 }
 
@@ -123,7 +113,7 @@ impl From<MsgAcknowledgement> for RawMsgAcknowledgement {
     fn from(domain_msg: MsgAcknowledgement) -> Self {
         RawMsgAcknowledgement {
             packet: Some(domain_msg.packet.into()),
-            acknowledgement: domain_msg.acknowledgement.into_bytes(),
+            acknowledgement: domain_msg.acknowledgement.into(),
             signer: domain_msg.signer.to_string(),
             proof_height: Some(domain_msg.proofs.height().into()),
             proof_acked: domain_msg.proofs.object_proof().clone().into(),

modules/src/core/ics26_routing/handler.rs

@@ -111,9 +111,7 @@ where
                 return Ok(handler_builder.with_result(()));
             }
 
-            let mut module_output = ModuleOutputBuilder::new();
-            let cb_result = ics4_packet_callback(ctx, &module_id, &msg, &mut module_output);
-            handler_builder.merge(module_output);
+            let cb_result = ics4_packet_callback(ctx, &module_id, &msg, &mut handler_builder);
             cb_result.map_err(Error::ics04_channel)?;
 
             // Apply any results to the host chain store.