imp: refactor send-coins-*() and add memo field to escrow-* and burn-* methods (#836)

* split send_coins_*() functions

and introduce extra data

* arg name change

* add error variant

* add error

* add `Other` variant to `TokenTransferError`

* fmt

* fix: restore `no_std` support for `serde` feature (#790)

* fix: restore no_std support for serde feature

* deps: bump ibc-proto-rs to v0.34.0

* fix: reflect changes b/c of ibc-proto-rs

* imp: comply naming + use better domain type for host_consensus_state

* fix: apply host_cons_state_proof changes for conn_open_ack

* fix: use domain TmEvent instead of proto for upgrade client proposal handler

* misc: add a unclog for ibc-proto-rs upgrade

* bump ibc-proto to v0.34.1 and borsh to v0.10 (#844)

* bump ibc-proto and borsh

* changelog

* add borsh derive for `MsgTransfer`

* derive JsonSchema for MsgTransfer

* fix JsonSchema derive for MsgTransfer

Fix is in implementing it properly for our `Timestamp`

* execute() methods must have &mut self

* nit: spell checker

* imp: simplify escrow/unescrow abstraction + using memo instead

* chore: dress up with unclogs

* nit: adjust unlogs

* nit: remove confusing InternalTransferFailed error variant

* imp: add memo field to burn-coins-*

* docs: adjust & add missing docstrings

---------

Co-authored-by: Farhad Shabani <[email protected]>

Author: plafer

.changelog/unreleased/993-bump-ibc-proto-rs.md

@@ -0,0 +1,5 @@
+- `[ibc-app-transfer]` Refactor `send-coins-*()` methods by breaking them down
+  into distinct escrow and unescrow methods, enhancing both clarity and
+  specificity in functionality.
+  ([\#837](https://github.com/cosmos/ibc-rs/issues/837))
+  

.changelog/unreleased/breaking-changes/837-split-sent-coins-method copy.md

@@ -0,0 +1,5 @@
+- `[ibc-app-transfer]` Add `memo` field to `escrow-coins-*()` and
+  `burn-coins-*()` methods, allowing implementors to pass in arbitrary data
+  necessary for their use case.
+  ([\#839](https://github.com/cosmos/ibc-rs/issues/837))
+  

.changelog/unreleased/breaking-changes/839-add-memo-field-for-token-escrow.md

@@ -1,2 +1,2 @@
-- Optimize `IdentifierError` variants and make them mutually exclusive.
-  ([\#978](https://github.com/cosmos/ibc-rs/issues/978))
+- `[ibc-core-host-type]` Optimize `IdentifierError` variants and make them
+  mutually exclusive. ([\#978](https://github.com/cosmos/ibc-rs/issues/978))

.changelog/unreleased/breaking-changes/978-identifier-error.md

@@ -0,0 +1,2 @@
+- `[ibc-data-types]` Bump ibc-proto-rs dependency to v0.39.1.
+  ([\#993](https://github.com/cosmos/ibc-rs/issues/993))

.changelog/unreleased/breaking-changes/993-bump-ibc-proto-rs.md

@@ -1,4 +1,4 @@
-- [`ibc`] Minimize `prost` dependency by introducing `ToVec` trait
+- `[ibc]` Minimize `prost` dependency by introducing `ToVec` trait
   - Now `prost` is only imported in `ibc-primitives` crate
   - Remove error variants originating from `prost` (Breaking change)
   - Eliminate the need for the `bytes` dependency

.changelog/unreleased/breaking-changes/997-minimize-prost-dependency.md

@@ -1,3 +1,3 @@
-- [`cw-check`] More rigorous CosmWasm check by upgrading dependencies and
+- `[cw-check]` More rigorous CosmWasm check by upgrading dependencies and
   including `std` and `schema` features for `ibc-core`.
   ([\#992](https://github.com/cosmos/ibc-rs/pull/992))

.changelog/unreleased/improvements/992-more-rigorous-cosmwasm-check.md

@@ -1,7 +1,7 @@
 //! Defines the main context traits and IBC module callbacks
 
 use ibc_app_transfer_types::error::TokenTransferError;
-use ibc_app_transfer_types::{PrefixedCoin, PrefixedDenom};
+use ibc_app_transfer_types::{Memo, PrefixedCoin, PrefixedDenom};
 use ibc_core::host::types::identifiers::{ChannelId, PortId};
 use ibc_core::primitives::prelude::*;
 use ibc_core::primitives::Signer;
@@ -13,24 +13,31 @@ pub trait TokenTransferValidationContext {
     /// get_port returns the portID for the transfer module.
     fn get_port(&self) -> Result<PortId, TokenTransferError>;
 
-    /// Returns the escrow account id for a port and channel combination
-    fn get_escrow_account(
-        &self,
-        port_id: &PortId,
-        channel_id: &ChannelId,
-    ) -> Result<Self::AccountId, TokenTransferError>;
-
     /// Returns Ok() if the host chain supports sending coins.
     fn can_send_coins(&self) -> Result<(), TokenTransferError>;
 
     /// Returns Ok() if the host chain supports receiving coins.
     fn can_receive_coins(&self) -> Result<(), TokenTransferError>;
 
-    /// Validates the sender and receiver accounts and the coin inputs
-    fn send_coins_validate(
+    /// Validates that the tokens can be escrowed successfully.
+    ///
+    /// `memo` field allows to incorporate additional contextual details in the
+    /// escrow validation.
+    fn escrow_coins_validate(
         &self,
         from_account: &Self::AccountId,
+        port_id: &PortId,
+        channel_id: &ChannelId,
+        coin: &PrefixedCoin,
+        memo: &Memo,
+    ) -> Result<(), TokenTransferError>;
+
+    /// Validates that the tokens can be unescrowed successfully.
+    fn unescrow_coins_validate(
+        &self,
         to_account: &Self::AccountId,
+        port_id: &PortId,
+        channel_id: &ChannelId,
         coin: &PrefixedCoin,
     ) -> Result<(), TokenTransferError>;
 
@@ -41,11 +48,15 @@ pub trait TokenTransferValidationContext {
         coin: &PrefixedCoin,
     ) -> Result<(), TokenTransferError>;
 
-    /// Validates the sender account and the coin input
+    /// Validates the sender account and the coin input before burning.
+    ///
+    /// `memo` field allows to incorporate additional contextual details in the
+    /// burn validation.
     fn burn_coins_validate(
         &self,
         account: &Self::AccountId,
         coin: &PrefixedCoin,
+        memo: &Memo,
     ) -> Result<(), TokenTransferError>;
 
     /// Returns a hash of the prefixed denom.
@@ -55,27 +66,45 @@ pub trait TokenTransferValidationContext {
     }
 }
 
-/// Methods required in token transfer execution, to be implemented by the host
+/// Methods required in token transfer execution, to be implemented by the host.
 pub trait TokenTransferExecutionContext: TokenTransferValidationContext {
-    /// This function should enable sending ibc fungible tokens from one account to another
-    fn send_coins_execute(
+    /// Executes the escrow of the tokens in a user account.
+    ///
+    /// `memo` field allows to incorporate additional contextual details in the
+    /// escrow execution.
+    fn escrow_coins_execute(
         &mut self,
         from_account: &Self::AccountId,
+        port_id: &PortId,
+        channel_id: &ChannelId,
+        coin: &PrefixedCoin,
+        memo: &Memo,
+    ) -> Result<(), TokenTransferError>;
+
+    /// Executes the unescrow of the tokens in a user account.
+    fn unescrow_coins_execute(
+        &mut self,
         to_account: &Self::AccountId,
+        port_id: &PortId,
+        channel_id: &ChannelId,
         coin: &PrefixedCoin,
     ) -> Result<(), TokenTransferError>;
 
-    /// This function to enable minting ibc tokens to a user account
+    /// Executes minting of the tokens in 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
+    /// Executes burning of the tokens in a user account.
+    ///
+    /// `memo` field allows to incorporate additional contextual details in the
+    /// burn execution.
     fn burn_coins_execute(
         &mut self,
         account: &Self::AccountId,
         coin: &PrefixedCoin,
+        memo: &Memo,
     ) -> Result<(), TokenTransferError>;
 }

ibc-apps/ics20-transfer/src/context.rs

@@ -27,11 +27,12 @@ pub fn refund_packet_token_execute(
         packet.chan_id_on_a.clone(),
         &data.token.denom,
     ) {
-        // unescrow tokens back to sender
-        let escrow_address =
-            ctx_a.get_escrow_account(&packet.port_id_on_a, &packet.chan_id_on_a)?;
-
-        ctx_a.send_coins_execute(&escrow_address, &sender, &data.token)
+        ctx_a.unescrow_coins_execute(
+            &sender,
+            &packet.port_id_on_a,
+            &packet.chan_id_on_a,
+            &data.token,
+        )
     }
     // mint vouchers back to sender
     else {
@@ -55,10 +56,12 @@ pub fn refund_packet_token_validate(
         packet.chan_id_on_a.clone(),
         &data.token.denom,
     ) {
-        let escrow_address =
-            ctx_a.get_escrow_account(&packet.port_id_on_a, &packet.chan_id_on_a)?;
-
-        ctx_a.send_coins_validate(&escrow_address, &sender, &data.token)
+        ctx_a.unescrow_coins_validate(
+            &sender,
+            &packet.port_id_on_a,
+            &packet.chan_id_on_a,
+            &data.token,
+        )
     } else {
         ctx_a.mint_coins_validate(&sender, &data.token)
     }

ibc-apps/ics20-transfer/src/handler/mod.rs

@@ -43,10 +43,6 @@ pub fn process_recv_packet_execute<Ctx: TokenTransferExecutionContext>(
             c
         };
 
-        let escrow_address = ctx_b
-            .get_escrow_account(&packet.port_id_on_b, &packet.chan_id_on_b)
-            .map_err(|token_err| (ModuleExtras::empty(), 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
@@ -58,11 +54,20 @@ pub fn process_recv_packet_execute<Ctx: TokenTransferExecutionContext>(
         // gets relayed back to the sender so that the escrowed tokens
         // can be refunded.
         ctx_b
-            .send_coins_validate(&escrow_address, &receiver_account, &coin)
+            .unescrow_coins_validate(
+                &receiver_account,
+                &packet.port_id_on_b,
+                &packet.chan_id_on_b,
+                &coin,
+            )
             .map_err(|token_err| (ModuleExtras::empty(), token_err))?;
-
         ctx_b
-            .send_coins_execute(&escrow_address, &receiver_account, &coin)
+            .unescrow_coins_execute(
+                &receiver_account,
+                &packet.port_id_on_b,
+                &packet.chan_id_on_b,
+                &coin,
+            )
             .map_err(|token_err| (ModuleExtras::empty(), token_err))?;
 
         ModuleExtras::empty()

ibc-apps/ics20-transfer/src/handler/on_recv_packet.rs

@@ -68,11 +68,15 @@ where
         msg.chan_id_on_a.clone(),
         &token.denom,
     ) {
-        let escrow_address =
-            token_ctx_a.get_escrow_account(&msg.port_id_on_a, &msg.chan_id_on_a)?;
-        token_ctx_a.send_coins_validate(&sender, &escrow_address, token)?;
+        token_ctx_a.escrow_coins_validate(
+            &sender,
+            &msg.port_id_on_a,
+            &msg.chan_id_on_a,
+            token,
+            &msg.packet_data.memo,
+        )?;
     } else {
-        token_ctx_a.burn_coins_validate(&sender, token)?;
+        token_ctx_a.burn_coins_validate(&sender, token, &msg.packet_data.memo)?;
     }
 
     let packet = {
@@ -137,11 +141,15 @@ where
         msg.chan_id_on_a.clone(),
         &token.denom,
     ) {
-        let escrow_address =
-            token_ctx_a.get_escrow_account(&msg.port_id_on_a, &msg.chan_id_on_a)?;
-        token_ctx_a.send_coins_execute(&sender, &escrow_address, token)?;
+        token_ctx_a.escrow_coins_execute(
+            &sender,
+            &msg.port_id_on_a,
+            &msg.chan_id_on_a,
+            token,
+            &msg.packet_data.memo,
+        )?;
     } else {
-        token_ctx_a.burn_coins_execute(&sender, token)?;
+        token_ctx_a.burn_coins_execute(&sender, token, &msg.packet_data.memo)?;
     }
 
     let packet = {

ibc-apps/ics20-transfer/src/handler/send_transfer.rs

@@ -11,12 +11,19 @@ use ibc_core::host::types::identifiers::{ChannelId, PortId};
 use ibc_core::primitives::prelude::*;
 use uint::FromDecStrErr;
 
+use crate::Amount;
+
 #[derive(Display, Debug)]
 pub enum TokenTransferError {
     /// context error: `{0}`
     ContextError(ContextError),
     /// invalid identifier: `{0}`
     InvalidIdentifier(IdentifierError),
+    /// insufficient funds: tried to send `{send_attempt}`, sender only has `{available_funds}`
+    InsufficientFunds {
+        send_attempt: Amount,
+        available_funds: Amount,
+    },
     /// destination channel not found in the counterparty of port_id `{port_id}` and channel_id `{channel_id}`
     DestinationChannelNotFound {
         port_id: PortId,
@@ -70,6 +77,8 @@ pub enum TokenTransferError {
     InvalidCoin { coin: String },
     /// decoding raw bytes as UTF8 string error: `{0}`
     Utf8Decode(Utf8Error),
+    /// other error: `{0}`
+    Other(String),
 }
 
 #[cfg(feature = "std")]

ibc-apps/ics20-transfer/types/src/error.rs

@@ -32,6 +32,7 @@ pub(crate) const TYPE_URL: &str = "/ibc.applications.transfer.v1.MsgTransfer";
     feature = "borsh",
     derive(borsh::BorshSerialize, borsh::BorshDeserialize)
 )]
+#[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
 pub struct MsgTransfer {
     /// the port on which the packet will be sent
     pub port_id_on_a: PortId,

ibc-apps/ics20-transfer/types/src/msgs/transfer.rs

@@ -1,10 +1,8 @@
 use ibc::apps::transfer::context::{TokenTransferExecutionContext, TokenTransferValidationContext};
 use ibc::apps::transfer::types::error::TokenTransferError;
-use ibc::apps::transfer::types::PrefixedCoin;
+use ibc::apps::transfer::types::{Memo, PrefixedCoin};
 use ibc::core::host::types::identifiers::{ChannelId, PortId};
 use ibc::core::primitives::Signer;
-use ibc::cosmos_host::utils::cosmos_adr028_escrow_address;
-use subtle_encoding::bech32;
 
 use super::types::DummyTransferModule;
 
@@ -15,27 +13,29 @@ impl TokenTransferValidationContext for DummyTransferModule {
         Ok(PortId::transfer())
     }
 
-    fn get_escrow_account(
-        &self,
-        port_id: &PortId,
-        channel_id: &ChannelId,
-    ) -> Result<Self::AccountId, TokenTransferError> {
-        let addr = cosmos_adr028_escrow_address(port_id, channel_id);
-        Ok(bech32::encode("cosmos", addr).into())
-    }
-
     fn can_send_coins(&self) -> Result<(), TokenTransferError> {
         Ok(())
     }
 
     fn can_receive_coins(&self) -> Result<(), TokenTransferError> {
         Ok(())
     }
-
-    fn send_coins_validate(
+    fn escrow_coins_validate(
         &self,
         _from_account: &Self::AccountId,
+        _port_id: &PortId,
+        _channel_id: &ChannelId,
+        _coin: &PrefixedCoin,
+        _memo: &Memo,
+    ) -> Result<(), TokenTransferError> {
+        Ok(())
+    }
+
+    fn unescrow_coins_validate(
+        &self,
         _to_account: &Self::AccountId,
+        _port_id: &PortId,
+        _channel_id: &ChannelId,
         _coin: &PrefixedCoin,
     ) -> Result<(), TokenTransferError> {
         Ok(())
@@ -53,16 +53,29 @@ impl TokenTransferValidationContext for DummyTransferModule {
         &self,
         _account: &Self::AccountId,
         _coin: &PrefixedCoin,
+        _memo: &Memo,
     ) -> Result<(), TokenTransferError> {
         Ok(())
     }
 }
 
 impl TokenTransferExecutionContext for DummyTransferModule {
-    fn send_coins_execute(
+    fn escrow_coins_execute(
         &mut self,
         _from_account: &Self::AccountId,
+        _port_id: &PortId,
+        _channel_id: &ChannelId,
+        _coin: &PrefixedCoin,
+        _memo: &Memo,
+    ) -> Result<(), TokenTransferError> {
+        Ok(())
+    }
+
+    fn unescrow_coins_execute(
+        &mut self,
         _to_account: &Self::AccountId,
+        _port_id: &PortId,
+        _channel_id: &ChannelId,
         _coin: &PrefixedCoin,
     ) -> Result<(), TokenTransferError> {
         Ok(())
@@ -80,6 +93,7 @@ impl TokenTransferExecutionContext for DummyTransferModule {
         &mut self,
         _account: &Self::AccountId,
         _coin: &PrefixedCoin,
+        _memo: &Memo,
     ) -> Result<(), TokenTransferError> {
         Ok(())
     }