Separate the validation from the execution process for send/mint/burn_coins operations

.changelog/unreleased/breaking-changes/502-add-coins-validation-api.md

@@ -0,0 +1,3 @@
+- Separate the validation from the execution process for `send/mint/burn_coins`
+  operations.
+  ([#502](https://github.com/cosmos/ibc-rs/issues/502))

crates/ibc/src/applications/transfer/context.rs

@@ -6,7 +6,7 @@ use super::error::TokenTransferError;
 use crate::applications::transfer::acknowledgement::TokenTransferAcknowledgement;
 use crate::applications::transfer::events::{AckEvent, AckStatusEvent, RecvEvent, TimeoutEvent};
 use crate::applications::transfer::packet::PacketData;
-use crate::applications::transfer::relay::refund_packet_token;
+use crate::applications::transfer::relay::refund_packet_token_execute;
 use crate::applications::transfer::relay::{
     on_recv_packet::process_recv_packet_execute, refund_packet_token_validate,
 };
@@ -22,32 +22,6 @@ use crate::core::ics04_channel::Version;
 use crate::core::ics24_host::identifier::{ChannelId, ConnectionId, PortId};
 use crate::signer::Signer;
 
-pub trait TokenTransferExecutionContext:
-    TokenTransferValidationContext + SendPacketExecutionContext
-{
-    /// This function should enable sending ibc fungible tokens from one account to another
-    fn send_coins(
-        &mut self,
-        from: &Self::AccountId,
-        to: &Self::AccountId,
-        amt: &PrefixedCoin,
-    ) -> Result<(), TokenTransferError>;
-
-    /// This function to enable minting ibc tokens to a user account
-    fn mint_coins(
-        &mut self,
-        account: &Self::AccountId,
-        amt: &PrefixedCoin,
-    ) -> Result<(), TokenTransferError>;
-
-    /// This function should enable burning of minted tokens in a user account
-    fn burn_coins(
-        &mut self,
-        account: &Self::AccountId,
-        amt: &PrefixedCoin,
-    ) -> Result<(), TokenTransferError>;
-}
-
 pub trait TokenTransferValidationContext: SendPacketValidationContext {
     type AccountId: TryFrom<Signer>;
 
@@ -67,13 +41,61 @@ pub trait TokenTransferValidationContext: SendPacketValidationContext {
     /// Returns true iff receive is enabled.
     fn is_receive_enabled(&self) -> bool;
 
+    /// Validates the sender and receiver accounts and the coin inputs
+    fn send_coins_validate(
+        &self,
+        from_account: &Self::AccountId,
+        to_account: &Self::AccountId,
+        coin: &PrefixedCoin,
+    ) -> Result<(), TokenTransferError>;
+
+    /// Validates the receiver account and the coin input
+    fn mint_coins_validate(
+        &self,
+        account: &Self::AccountId,
+        coin: &PrefixedCoin,
+    ) -> Result<(), TokenTransferError>;
+
+    /// Validates the sender account and the coin input
+    fn burn_coins_validate(
+        &self,
+        account: &Self::AccountId,
+        coin: &PrefixedCoin,
+    ) -> Result<(), TokenTransferError>;
+
     /// Returns a hash of the prefixed denom.
     /// Implement only if the host chain supports hashed denominations.
     fn denom_hash_string(&self, _denom: &PrefixedDenom) -> Option<String> {
         None
     }
 }
 
+pub trait TokenTransferExecutionContext:
+    TokenTransferValidationContext + SendPacketExecutionContext
+{
+    /// This function should enable sending ibc fungible tokens from one account to another
+    fn send_coins_execute(
+        &mut self,
+        from_account: &Self::AccountId,
+        to_account: &Self::AccountId,
+        coin: &PrefixedCoin,
+    ) -> Result<(), TokenTransferError>;
+
+    /// This function to enable minting ibc tokens to a user account
+    fn mint_coins_execute(
+        &mut self,
+        account: &Self::AccountId,
+        coin: &PrefixedCoin,
+    ) -> Result<(), TokenTransferError>;
+
+    /// This function should enable burning of minted tokens in a user account
+    fn burn_coins_execute(
+        &mut self,
+        account: &Self::AccountId,
+        coin: &PrefixedCoin,
+    ) -> Result<(), TokenTransferError>;
+}
+
 // https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-028-public-key-addresses.md
 pub fn cosmos_adr028_escrow_address(port_id: &PortId, channel_id: &ChannelId) -> Vec<u8> {
     let contents = format!("{port_id}/{channel_id}");
@@ -277,7 +299,7 @@ pub fn on_recv_packet_execute(
 }
 
 pub fn on_acknowledgement_packet_validate<Ctx>(
-    _ctx: &Ctx,
+    ctx: &Ctx,
     packet: &Packet,
     acknowledgement: &Acknowledgement,
     _relayer: &Signer,
@@ -293,7 +315,7 @@ where
             .map_err(|_| TokenTransferError::AckDeserialization)?;
 
     if !acknowledgement.is_successful() {
-        refund_packet_token_validate::<Ctx>(&data)?;
+        refund_packet_token_validate(ctx, packet, &data)?;
     }
 
     Ok(())
@@ -327,7 +349,7 @@ pub fn on_acknowledgement_packet_execute(
         };
 
     if !acknowledgement.is_successful() {
-        if let Err(err) = refund_packet_token(ctx, packet, &data) {
+        if let Err(err) = refund_packet_token_execute(ctx, packet, &data) {
             return (ModuleExtras::empty(), Err(err));
         }
     }
@@ -348,7 +370,7 @@ pub fn on_acknowledgement_packet_execute(
 }
 
 pub fn on_timeout_packet_validate<Ctx>(
-    _ctx: &Ctx,
+    ctx: &Ctx,
     packet: &Packet,
     _relayer: &Signer,
 ) -> Result<(), TokenTransferError>
@@ -358,7 +380,7 @@ where
     let data = serde_json::from_slice::<PacketData>(&packet.data)
         .map_err(|_| TokenTransferError::PacketDataDeserialization)?;
 
-    refund_packet_token_validate::<Ctx>(&data)?;
+    refund_packet_token_validate(ctx, packet, &data)?;
 
     Ok(())
 }
@@ -378,7 +400,7 @@ pub fn on_timeout_packet_execute(
         }
     };
 
-    if let Err(err) = refund_packet_token(ctx, packet, &data) {
+    if let Err(err) = refund_packet_token_execute(ctx, packet, &data) {
         return (ModuleExtras::empty(), Err(err));
     }
 

crates/ibc/src/applications/transfer/relay.rs

@@ -10,7 +10,7 @@ use super::context::{TokenTransferExecutionContext, TokenTransferValidationConte
 pub mod on_recv_packet;
 pub mod send_transfer;
 
-pub fn refund_packet_token(
+pub fn refund_packet_token_execute(
     ctx: &mut impl TokenTransferExecutionContext,
     packet: &Packet,
     data: &PacketData,
@@ -30,23 +30,35 @@ pub fn refund_packet_token(
         let escrow_address =
             ctx.get_channel_escrow_address(&packet.port_on_a, &packet.chan_on_a)?;
 
-        ctx.send_coins(&escrow_address, &sender, &data.token)
+        ctx.send_coins_execute(&escrow_address, &sender, &data.token)
     }
     // mint vouchers back to sender
     else {
-        ctx.mint_coins(&sender, &data.token)
+        ctx.mint_coins_execute(&sender, &data.token)
     }
 }
 
-pub fn refund_packet_token_validate<Ctx>(data: &PacketData) -> Result<(), TokenTransferError>
-where
-    Ctx: TokenTransferValidationContext,
-{
-    let _sender: Ctx::AccountId = data
+pub fn refund_packet_token_validate(
+    ctx: &impl TokenTransferValidationContext,
+    packet: &Packet,
+    data: &PacketData,
+) -> Result<(), TokenTransferError> {
+    let sender = data
         .sender
         .clone()
         .try_into()
         .map_err(|_| TokenTransferError::ParseAccountFailure)?;
 
-    Ok(())
+    if is_sender_chain_source(
+        packet.port_on_a.clone(),
+        packet.chan_on_a.clone(),
+        &data.token.denom,
+    ) {
+        let escrow_address =
+            ctx.get_channel_escrow_address(&packet.port_on_a, &packet.chan_on_a)?;
+
+        ctx.send_coins_validate(&escrow_address, &sender, &data.token)
+    } else {
+        ctx.mint_coins_validate(&sender, &data.token)
+    }
 }

crates/ibc/src/applications/transfer/relay/on_recv_packet.rs

@@ -7,6 +7,12 @@ use crate::core::ics04_channel::handler::ModuleExtras;
 use crate::core::ics04_channel::packet::Packet;
 use crate::prelude::*;
 
+/// This function handles the transfer receiving logic.
+///
+/// Note that `send/mint_coins_validate` steps are performed on the host chain
+/// to validate accounts and token info. But the result is then used for
+/// execution on the IBC side, including storing acknowledgements and emitting
+/// events.
 pub fn process_recv_packet_execute<Ctx: TokenTransferExecutionContext>(
     ctx: &mut Ctx,
     packet: &Packet,
@@ -40,7 +46,20 @@ pub fn process_recv_packet_execute<Ctx: TokenTransferExecutionContext>(
             .get_channel_escrow_address(&packet.port_on_b, &packet.chan_on_b)
             .map_err(|token_err| (ModuleExtras::empty(), token_err))?;
 
-        ctx.send_coins(&escrow_address, &receiver_account, &coin)
+        // Note: it is correct to do the validation here because `recv_packet()`
+        // works slightly differently. We do not have a
+        // `on_recv_packet_validate()` callback because regardless of whether or
+        // not the app succeeds to receive the packet, we want to run the
+        // `execute()` phase. And this is because the app failing to receive
+        // does not constitute a failure of the message processing.
+        // Specifically, when the app fails to receive, we need to return
+        // a `TokenTransferAcknowledgement::Error` acknowledgement, which
+        // gets relayed back to the sender so that the escrowed tokens
+        // can be refunded.
+        ctx.send_coins_validate(&escrow_address, &receiver_account, &coin)
+            .map_err(|token_err| (ModuleExtras::empty(), token_err))?;
+
+        ctx.send_coins_execute(&escrow_address, &receiver_account, &coin)
             .map_err(|token_err| (ModuleExtras::empty(), token_err))?;
 
         ModuleExtras::empty()
@@ -63,10 +82,22 @@ pub fn process_recv_packet_execute<Ctx: TokenTransferExecutionContext>(
                 log: Vec::new(),
             }
         };
-        let extras_closure = extras.clone();
 
-        ctx.mint_coins(&receiver_account, &coin)
-            .map_err(|token_err| (extras_closure, token_err))?;
+        // Note: it is correct to do the validation here because `recv_packet()`
+        // works slightly differently. We do not have a
+        // `on_recv_packet_validate()` callback because regardless of whether or
+        // not the app succeeds to receive the packet, we want to run the
+        // `execute()` phase. And this is because the app failing to receive
+        // does not constitute a failure of the message processing.
+        // Specifically, when the app fails to receive, we need to return
+        // a `TokenTransferAcknowledgement::Error` acknowledgement, which
+        // gets relayed back to the sender so that the escrowed tokens
+        // can be refunded.
+        ctx.mint_coins_validate(&receiver_account, &coin)
+            .map_err(|token_err| (extras.clone(), token_err))?;
+
+        ctx.mint_coins_execute(&receiver_account, &coin)
+            .map_err(|token_err| (extras.clone(), token_err))?;
 
         extras
     };

crates/ibc/src/applications/transfer/relay/send_transfer.rs

@@ -62,16 +62,23 @@ where
         .map_err(|_| TokenTransferError::InvalidToken)?;
     let denom = token.denom.clone();
     let coin = Coin {
-        denom,
+        denom: denom.clone(),
         amount: token.amount,
     };
 
-    let _sender: Ctx::AccountId = msg
+    let sender: Ctx::AccountId = msg
         .sender
         .clone()
         .try_into()
         .map_err(|_| TokenTransferError::ParseAccountFailure)?;
 
+    if is_sender_chain_source(msg.port_on_a.clone(), msg.chan_on_a.clone(), &denom) {
+        let escrow_address = ctx_a.get_channel_escrow_address(&msg.port_on_a, &msg.chan_on_a)?;
+        ctx_a.send_coins_validate(&sender, &escrow_address, &coin)?;
+    } else {
+        ctx_a.burn_coins_validate(&sender, &coin)?;
+    }
+
     let data = {
         let data = PacketData {
             token: coin,
@@ -144,9 +151,9 @@ where
 
     if is_sender_chain_source(msg.port_on_a.clone(), msg.chan_on_a.clone(), &denom) {
         let escrow_address = ctx_a.get_channel_escrow_address(&msg.port_on_a, &msg.chan_on_a)?;
-        ctx_a.send_coins(&sender, &escrow_address, &coin)?;
+        ctx_a.send_coins_execute(&sender, &escrow_address, &coin)?;
     } else {
-        ctx_a.burn_coins(&sender, &coin)?;
+        ctx_a.burn_coins_execute(&sender, &coin)?;
     }
 
     let data = {

crates/ibc/src/core/context/recv_packet.rs

@@ -29,6 +29,7 @@ where
     recv_packet::validate(ctx_b, &msg)
 
     // nothing to validate with the module, since `onRecvPacket` cannot fail.
+    // If any error occurs, then an "error acknowledgement" must be returned.
 }
 
 pub(super) fn recv_packet_execute<ExecCtx>(

crates/ibc/src/core/ics26_routing/context.rs

@@ -175,6 +175,11 @@ pub trait Module: Debug {
         Ok(ModuleExtras::empty())
     }
 
+    // Note: no `on_recv_packet_validate()`
+    // the `onRecvPacket` callback always succeeds
+    // if any error occurs, than an "error acknowledgement"
+    // must be returned
+
     fn on_recv_packet_execute(
         &mut self,
         packet: &Packet,