fix: review sov-ibc-transfer implementation and apply fixes

mocks/src/configs.rs

@@ -2,7 +2,6 @@ use std::fs::File;
 use std::io::Read;
 use std::path::Path;
 
-use ibc_app_transfer::types::Memo;
 use ibc_core::host::types::identifiers::ChainId;
 use ibc_testkit::fixtures::core::signer::dummy_bech32_account;
 use serde::de::DeserializeOwned;
@@ -119,9 +118,6 @@ pub struct TransferTestConfig {
     /// The amount to transfer.
     #[builder(default = 100)]
     pub amount: u64,
-    /// The memo to attach to the transfer.
-    #[builder(default = "".into())]
-    pub memo: Memo,
 }
 
 /// Reads toml file as a specific type.

mocks/src/relayer/msgs/cosmos.rs

@@ -105,7 +105,7 @@ where
             },
             sender: Signer::from(config.cos_address.clone()),
             receiver: Signer::from(config.sov_address.to_string()),
-            memo: config.memo.clone(),
+            memo: "".into(),
         };
 
         MsgTransfer {

mocks/src/relayer/msgs/rollup.rs

@@ -111,7 +111,7 @@ where
             },
             sender: Signer::from(config.sov_address.to_string()),
             receiver: Signer::from(config.cos_address.clone()),
-            memo: config.memo.clone(),
+            memo: "".into(),
         };
 
         MsgTransfer {

mocks/src/sovereign/runner.rs

@@ -5,8 +5,8 @@ use sov_consensus_state_tracker::HasConsensusState;
 use sov_kernels::basic::BasicKernelGenesisConfig;
 use sov_modules_api::runtime::capabilities::{Kernel, KernelSlotHooks};
 use sov_modules_api::{
-    DispatchCall, Gas, GasMeter, Genesis, KernelWorkingSet, ModuleInfo, SlotData, Spec,
-    StateCheckpoint,
+    CallResponse, DispatchCall, Gas, GasMeter, Genesis, KernelWorkingSet, ModuleInfo, SlotData,
+    Spec, StateCheckpoint,
 };
 use sov_rollup_interface::da::BlockHeaderTrait;
 use sov_rollup_interface::services::da::DaService;
@@ -113,9 +113,14 @@ where
                 self.resolve_ctx(self.runtime().ibc.address().clone(), visible_slot);
             }
 
+            // NOTE: on failures, we silently ignore the error and continue as
+            // it is in the real-case scenarios
             self.runtime()
                 .dispatch_call(m.clone(), &mut working_set, &self.rollup_ctx())
-                .unwrap();
+                .unwrap_or_else(|e| {
+                    info!("rollup: error executing message: {:?}", e);
+                    CallResponse::default()
+                });
 
             // Resets the sender address to the address of the relayer
             self.resolve_ctx(rollup_ctx.sender().clone(), visible_slot);

mocks/src/tests/transfer.rs

@@ -7,7 +7,6 @@ use ibc_core::primitives::ToProto;
 use sov_bank::TokenConfig;
 use sov_ibc::call::CallMessage;
 use sov_ibc::clients::AnyClientState;
-use sov_ibc_transfer::utils::SovereignMemo;
 use test_log::test;
 
 use crate::configs::TransferTestConfig;
@@ -142,10 +141,8 @@ async fn test_mint_burn_on_sov() {
 
     // set transfer parameters
     let gas_token = relayer_builder.setup_cfg().gas_token_config();
-    let sov_memo = serde_json::to_string(&SovereignMemo::new(gas_token.token_id)).unwrap();
     let mut cfg = TransferTestConfig::builder()
         .sov_address(gas_token.address_and_balances[0].0)
-        .memo(sov_memo.into())
         .build();
 
     // Fake token with the same parameters as the gas token but a different name
@@ -280,11 +277,8 @@ async fn test_mint_burn_on_sov() {
     // -----------------------------------------------------------------------
 
     cfg.sov_denom = "transfer/channel-0/basecoin".to_string();
-    cfg.memo = serde_json::to_string(&SovereignMemo::new(minted_token_id))
-        .unwrap()
-        .into();
 
-    let msg_transfer_on_sov = rly.build_msg_transfer_for_sov(&cfg);
+    let msg_transfer_on_sov = rly.build_msg_transfer_for_sov(&cfg.clone());
 
     rly.src_chain_ctx()
         .submit_msgs(vec![
@@ -322,10 +316,30 @@ async fn test_mint_burn_on_sov() {
     let sender_balance = rly
         .dst_chain_ctx()
         .service()
-        .get_balance_of(&cfg.cos_denom, cfg.cos_address)
+        .get_balance_of(&cfg.cos_denom, cfg.cos_address.clone())
         .unwrap();
 
     expected_sender_balance += cfg.amount;
 
     assert_eq!(sender_balance, expected_sender_balance);
+
+    // -----------------------------------------------------------------------
+    // Check if sending back a token with the same ID as IBC-minted token fails
+    // -----------------------------------------------------------------------
+    cfg.sov_denom = minted_token_id.to_bech32().to_string();
+
+    let msg_transfer_on_sov = rly.build_msg_transfer_for_sov(&cfg);
+
+    rly.src_chain_ctx()
+        .submit_msgs(vec![
+            CallMessage::Transfer(msg_transfer_on_sov.clone()).into()
+        ])
+        .await;
+
+    let receiver_balance = rly
+        .src_chain_ctx()
+        .service()
+        .get_balance_of(cfg.sov_address, minted_token_id);
+
+    assert_eq!(receiver_balance, expected_receiver_balance);
 }

modules/sov-ibc-transfer/src/context.rs

@@ -25,7 +25,7 @@ use sov_modules_api::{Context, Spec, WorkingSet};
 use uint::FromDecStrErr;
 
 use super::IbcTransfer;
-use crate::utils::{compute_escrow_address, SovereignMemo};
+use crate::utils::compute_escrow_address;
 
 /// We need to create a wrapper around the `Transfer` module and `WorkingSet`,
 /// because we only get the `WorkingSet` at call-time from the Sovereign SDK,
@@ -146,30 +146,27 @@ where
         coin: &PrefixedCoin,
         memo: &Memo,
     ) -> Result<(), TokenTransferError> {
-        let sov_memo: SovereignMemo = serde_json::from_str(memo.as_ref())
-            .map_err(|e| TokenTransferError::Other(format!("Failed to decode memo: {e}")))?;
+        if !memo.as_ref().is_empty() {
+            return Err(TokenTransferError::Other(
+                "Memo must be empty when burning tokens".to_string(),
+            ));
+        }
 
-        let expected_token_id = {
+        let minted_token_id = {
             self.ibc_transfer
-                .minted_tokens
+                .minted_token_name_to_id
                 .get(&coin.denom.to_string(), *self.working_set.borrow_mut())
                 .ok_or(TokenTransferError::InvalidCoin {
                     coin: coin.to_string(),
                 })?
         };
 
-        if sov_memo.token_id() != expected_token_id {
-            return Err(TokenTransferError::InvalidCoin {
-                coin: coin.to_string(),
-            });
-        }
-
         let sender_balance: u64 = self
             .ibc_transfer
             .bank
             .get_balance_of(
                 account.address.clone(),
-                sov_memo.token_id(),
+                minted_token_id,
                 *self.working_set.borrow_mut(),
             )
             .ok_or(TokenTransferError::InvalidCoin {
@@ -197,15 +194,25 @@ where
         coin: &PrefixedCoin,
         memo: &Memo,
     ) -> Result<(), TokenTransferError> {
+        if !memo.as_ref().is_empty() {
+            return Err(TokenTransferError::Other(
+                "Memo must be empty when escrowing tokens".to_string(),
+            ));
+        }
+
         let token_id = TokenId::from_str(&coin.denom.to_string()).map_err(|e| {
             TokenTransferError::InvalidCoin {
                 coin: coin.to_string(),
             }
         })?;
 
-        if !memo.as_ref().is_empty() {
+        if let Some(token_name) = self
+            .ibc_transfer
+            .minted_token_id_to_name
+            .get(&token_id, *self.working_set.borrow_mut())
+        {
             return Err(TokenTransferError::Other(
-                "Memo must be empty when escrowing tokens".to_string(),
+                format!("Token with ID '{token_id}' is an IBC-created token and cannot be escrowed. Use '{token_name}' as denom for sending back an IBC token to the source chain"),
             ));
         }
 
@@ -254,14 +261,24 @@ where
         channel_id: &ChannelId,
         coin: &PrefixedCoin,
     ) -> Result<(), TokenTransferError> {
+        let token_id = TokenId::from_str(&coin.denom.to_string()).map_err(|e| {
+            TokenTransferError::InvalidCoin {
+                coin: coin.to_string(),
+            }
+        })?;
+
+        if let Some(token_name) = self
+            .ibc_transfer
+            .minted_token_id_to_name
+            .get(&token_id, *self.working_set.borrow_mut())
+        {
+            return Err(TokenTransferError::Other(
+                    format!("Token with ID '{token_id}' is an IBC-created token and cannot be unescrowed. Use '{token_name}' as denom for receiving an IBC token from the source chain")
+                ));
+        }
+
         // ensure that escrow account has enough balance
         let escrow_balance: Amount = {
-            let token_id = TokenId::from_str(&coin.denom.to_string()).map_err(|e| {
-                TokenTransferError::InvalidCoin {
-                    coin: coin.to_string(),
-                }
-            })?;
-
             let escrow_address = self.obtain_escrow_address(port_id, channel_id);
 
             let escrow_balance = self
@@ -301,7 +318,7 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S>
         let token_id = {
             let maybe_token_id = self
                 .ibc_transfer
-                .minted_tokens
+                .minted_token_name_to_id
                 .get(&denom, *self.working_set.borrow_mut());
 
             match maybe_token_id {
@@ -333,13 +350,20 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S>
                         )
                         .map_err(|err| TokenTransferError::Other(err.to_string()))?;
 
-                    // Store the new address in `minted_tokens`
-                    self.ibc_transfer.minted_tokens.set(
+                    // Stores mapping from "denom to token ID" of the IBC-minted token
+                    self.ibc_transfer.minted_token_name_to_id.set(
                         &denom,
                         &new_token_id,
                         *self.working_set.borrow_mut(),
                     );
 
+                    // Stores mapping from "token ID to denom" of the IBC-minted token
+                    self.ibc_transfer.minted_token_id_to_name.set(
+                        &new_token_id,
+                        &denom,
+                        *self.working_set.borrow_mut(),
+                    );
+
                     new_token_id
                 }
             }
@@ -380,7 +404,7 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S>
 
         let token_id = {
             self.ibc_transfer
-                .minted_tokens
+                .minted_token_name_to_id
                 .get(&denom, *self.working_set.borrow_mut())
                 .ok_or(TokenTransferError::InvalidCoin {
                     coin: coin.to_string(),

modules/sov-ibc-transfer/src/lib.rs

@@ -28,10 +28,19 @@ pub struct IbcTransfer<S: Spec> {
     #[module]
     bank: sov_bank::Bank<S>,
 
-    /// Keeps track of the token IDs minted by the `IbcTransfer` using the
-    /// specified token denomination in the `MsgRecvPacket` message.
+    /// Maps the token name to its corresponding token ID for tokens minted
+    /// through IBC. This mapping is used during mint/burn processes to validate
+    /// if the token exists and gives out needed ID for the minting/burning
+    /// process.
     #[state]
-    minted_tokens: StateMap<String, TokenId>,
+    minted_token_name_to_id: StateMap<String, TokenId>,
+
+    /// Maps the token ID to its corresponding token name for tokens minted
+    /// through IBC. This mapping is used during escrows/un-escrows to verify if
+    /// the `TokenId` extracted from the `denom` is not an IBC-minted token,
+    /// indicating it is a native token for escrow/un-escrows purposes.
+    #[state]
+    minted_token_id_to_name: StateMap<TokenId, String>,
 
     /// Keeps track of escrow addresses associated with a specific port and
     /// channel pair, offering an efficient means to access these addresses

modules/sov-ibc-transfer/src/rpc.rs

@@ -23,14 +23,14 @@ where
         token_denom: String,
         working_set: &mut WorkingSet<S>,
     ) -> RpcResult<MintedTokenResponse> {
-        let token_id =
-            self.minted_tokens
-                .get(&token_denom, working_set)
-                .ok_or(ErrorObjectOwned::owned(
-                    jsonrpsee::types::error::UNKNOWN_ERROR_CODE,
-                    format!("No minted token found for denom {token_denom}"),
-                    None::<String>,
-                ))?;
+        let token_id = self
+            .minted_token_name_to_id
+            .get(&token_denom, working_set)
+            .ok_or(ErrorObjectOwned::owned(
+                jsonrpsee::types::error::UNKNOWN_ERROR_CODE,
+                format!("No minted token found for denom {token_denom}"),
+                None::<String>,
+            ))?;
 
         Ok(MintedTokenResponse { token_id })
     }

modules/sov-ibc-transfer/src/utils.rs

@@ -1,41 +1,8 @@
 use ibc_app_transfer::types::VERSION;
 use ibc_core::host::types::identifiers::{ChannelId, PortId};
-use serde::{Deserialize, Serialize};
-use sov_bank::TokenId;
 use sov_modules_api::digest::Digest;
 use sov_modules_api::{CryptoSpec, Spec};
 
-/// The high-level memo type for the Sovereign SDK rollups.
-#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, derive_more::From)]
-pub struct SovereignMemo {
-    /// the ICS-20 transfer namespace
-    pub transfer: TransferMemo,
-}
-
-impl SovereignMemo {
-    pub fn new(token_id: TokenId) -> Self {
-        Self {
-            transfer: TransferMemo::new(token_id),
-        }
-    }
-
-    pub fn token_id(&self) -> TokenId {
-        self.transfer.token_id
-    }
-}
-
-/// The memo type for the ICS-20 transfer module.
-#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, derive_more::From)]
-pub struct TransferMemo {
-    pub token_id: TokenId,
-}
-
-impl TransferMemo {
-    pub fn new(token_id: TokenId) -> Self {
-        Self { token_id }
-    }
-}
-
 /// The escrow address follows the format as outlined in Cosmos SDK's ADR 028:
 /// <https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-028-public-key-addresses.md/>
 /// except that the `Hasher` function mandated by the `CryptoSpec` trait in the