From 300e4c16210af5c347739a8d2de449cc837600a7 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Tue, 9 Apr 2024 20:46:07 -0700 Subject: [PATCH 01/10] fix: remove now redundant escrowed_token state + move away from base64-encoding memo --- Cargo.lock | 5 +- clients/sov-celestia/types/Cargo.toml | 1 - mocks/Cargo.toml | 1 - mocks/src/configs.rs | 13 +- mocks/src/relayer/msgs/cosmos.rs | 17 +-- mocks/src/relayer/msgs/rollup.rs | 15 +-- mocks/src/sovereign/rollup.rs | 25 ++-- mocks/src/tests/transfer.rs | 112 ++++++++++-------- .../sov-consensus-state-tracker/Cargo.toml | 1 - modules/sov-ibc-transfer/Cargo.toml | 22 ++-- modules/sov-ibc-transfer/src/context.rs | 93 +++++---------- modules/sov-ibc-transfer/src/lib.rs | 18 +-- modules/sov-ibc-transfer/src/rpc.rs | 23 ---- modules/sov-ibc-transfer/src/utils.rs | 33 ++++++ 14 files changed, 167 insertions(+), 212 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 14466756..ac790b5d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5545,7 +5545,6 @@ dependencies = [ name = "sov-celestia-client-types" version = "0.1.0" dependencies = [ - "base64 0.21.7", "bytes", "derive_more", "hex", @@ -5610,7 +5609,6 @@ name = "sov-consensus-state-tracker" version = "0.1.0" dependencies = [ "anyhow", - "base64 0.21.7", "borsh", "ibc-app-transfer", "ibc-core", @@ -5739,7 +5737,6 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", - "base64 0.21.7", "basecoin", "borsh", "const-rollup-config", @@ -5819,8 +5816,8 @@ name = "sov-ibc-transfer" version = "0.1.0" dependencies = [ "anyhow", - "base64 0.21.7", "borsh", + "derive_more", "ibc-app-transfer", "ibc-core", "jsonrpsee", diff --git a/clients/sov-celestia/types/Cargo.toml b/clients/sov-celestia/types/Cargo.toml index 1875d082..050ade9e 100644 --- a/clients/sov-celestia/types/Cargo.toml +++ b/clients/sov-celestia/types/Cargo.toml @@ -19,7 +19,6 @@ workspace = true [dependencies] # external dependencies -base64 = { workspace = true, features = ["alloc"] } bytes = { workspace = true } hex = { version = "0.4.3" } jmt = { workspace = true } diff --git a/mocks/Cargo.toml b/mocks/Cargo.toml index 38ce041b..687f7486 100644 --- a/mocks/Cargo.toml +++ b/mocks/Cargo.toml @@ -16,7 +16,6 @@ workspace = true # external dependencies anyhow = { workspace = true } async-trait = { version = "0.1.74", default-features = false } -base64 = { workspace = true } borsh = { workspace = true } jmt = { workspace = true } prost = { workspace = true } diff --git a/mocks/src/configs.rs b/mocks/src/configs.rs index 728c0805..6abdd9d4 100644 --- a/mocks/src/configs.rs +++ b/mocks/src/configs.rs @@ -2,10 +2,11 @@ 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; -use sov_bank::{get_token_id, GasTokenConfig, TokenId}; +use sov_bank::{get_token_id, TokenConfig, TokenId, GAS_TOKEN_ID}; #[cfg(feature = "celestia-da")] use sov_consensus_state_tracker::CelestiaService; #[cfg(feature = "mock-da")] @@ -45,11 +46,12 @@ pub struct TestSetupConfig { impl TestSetupConfig { /// Returns list of tokens in the bank configuration - pub fn gas_token_config(&self) -> GasTokenConfig { + pub fn gas_token_config(&self) -> TokenConfig { self.rollup_genesis_config .bank_config .gas_token_config .clone() + .into() } /// Returns the address of the relayer. We use the last address in the list @@ -104,10 +106,8 @@ pub async fn default_config_with_celestia_da() -> TestSetupConfig, /// An arbitrary user address on the rollup. pub sov_address: Address, /// The token name on the Cosmos chain. @@ -119,6 +119,9 @@ 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. diff --git a/mocks/src/relayer/msgs/cosmos.rs b/mocks/src/relayer/msgs/cosmos.rs index 5df5e105..cf6454eb 100644 --- a/mocks/src/relayer/msgs/cosmos.rs +++ b/mocks/src/relayer/msgs/cosmos.rs @@ -2,12 +2,10 @@ use std::str::FromStr; -use base64::engine::general_purpose; -use base64::Engine; use basecoin::modules::ibc::AnyClientState; use ibc_app_transfer::types::msgs::transfer::MsgTransfer; use ibc_app_transfer::types::packet::PacketData; -use ibc_app_transfer::types::{Coin, Memo, PrefixedDenom}; +use ibc_app_transfer::types::{Coin, PrefixedDenom}; use ibc_core::channel::types::msgs::MsgRecvPacket; use ibc_core::channel::types::packet::Packet; use ibc_core::channel::types::timeout::TimeoutHeight; @@ -100,17 +98,6 @@ where /// Builds a Cosmos chain token transfer message; serialized to Any pub fn build_msg_transfer_for_cos(&self, config: &TransferTestConfig) -> MsgTransfer { - let memo = match config.sov_token_id { - Some(token_id) => { - let mut token_id_buf = String::new(); - - general_purpose::STANDARD_NO_PAD.encode_string(token_id, &mut token_id_buf); - - token_id_buf.into() - } - None => Memo::from_str("").unwrap(), - }; - let packet_data = PacketData { token: Coin { denom: PrefixedDenom::from_str(&config.cos_denom).unwrap(), @@ -118,7 +105,7 @@ where }, sender: Signer::from(config.cos_address.clone()), receiver: Signer::from(config.sov_address.to_string()), - memo, + memo: config.memo.clone(), }; MsgTransfer { diff --git a/mocks/src/relayer/msgs/rollup.rs b/mocks/src/relayer/msgs/rollup.rs index 949487b6..d98557d9 100644 --- a/mocks/src/relayer/msgs/rollup.rs +++ b/mocks/src/relayer/msgs/rollup.rs @@ -1,8 +1,6 @@ //! Contains rollup specific message builders for the relayer. use std::str::FromStr; -use base64::engine::general_purpose; -use base64::Engine; use ibc_app_transfer::types::msgs::transfer::MsgTransfer; use ibc_app_transfer::types::packet::PacketData; use ibc_app_transfer::types::{Coin, PrefixedDenom}; @@ -101,14 +99,11 @@ where CallMessage::Core(msg_update_client.to_any()) } - /// Builds a sdk token transfer message wrapped in a `CallMessage` with the given amount - /// Note: keep the amount value lower than the initial balance of the sender address + /// Builds a `MsgTransfer` with the given configuration + /// + /// Note: keep the amount value lower than the initial balance of the sender + /// address pub fn build_msg_transfer_for_sov(&self, config: &TransferTestConfig) -> MsgTransfer { - let mut token_id_buf = String::new(); - - general_purpose::STANDARD_NO_PAD - .encode_string(config.sov_token_id.unwrap(), &mut token_id_buf); - let packet_data = PacketData { token: Coin { denom: PrefixedDenom::from_str(&config.sov_denom).unwrap(), @@ -116,7 +111,7 @@ where }, sender: Signer::from(config.sov_address.to_string()), receiver: Signer::from(config.cos_address.clone()), - memo: token_id_buf.into(), + memo: config.memo.clone(), }; MsgTransfer { diff --git a/mocks/src/sovereign/rollup.rs b/mocks/src/sovereign/rollup.rs index d119f09c..baf8e059 100644 --- a/mocks/src/sovereign/rollup.rs +++ b/mocks/src/sovereign/rollup.rs @@ -7,7 +7,7 @@ use std::sync::{Arc, Mutex}; use ibc_client_tendermint::types::Header; use ibc_core::client::types::Height; use ibc_core::host::types::identifiers::ChainId; -use sov_bank::{CallMessage as BankCallMessage, TokenId}; +use sov_bank::{CallMessage as BankCallMessage, TokenConfig, TokenId}; use sov_celestia_client::types::client_message::test_util::dummy_sov_header; use sov_celestia_client::types::client_message::SovTmHeader; use sov_consensus_state_tracker::{ConsensusStateTracker, HasConsensusState}; @@ -18,6 +18,7 @@ use sov_modules_api::{Context, Spec, WorkingSet}; use sov_rollup_interface::services::da::DaService; use sov_state::{MerkleProofSpec, ProverStorage, Storage}; +use super::DEFAULT_SALT; use crate::cosmos::MockTendermint; use crate::sovereign::runtime::RuntimeCall; use crate::sovereign::Runtime; @@ -153,25 +154,25 @@ where .unwrap() } - /// Returns token ID of an IBC denom - pub fn get_minted_token_id(&self, token_denom: String) -> Option { - let mut working_set = WorkingSet::new(self.prover_storage()); - + pub fn get_token_id(&self, token_config: TokenConfig) -> Option { self.runtime() - .ibc_transfer - .minted_token(token_denom, &mut working_set) - .map(|token| token.token_id) + .bank + .token_id( + token_config.token_name, + token_config.address_and_balances[0].0.clone(), + DEFAULT_SALT, + ) .ok() } - /// Searches the transfer module to retrieve the ID of the token ID in - /// escrow based on its name(denom). - pub fn get_escrowed_token_id(&self, token_denom: String) -> Option { + /// Searches the transfer module by given token denom and returns the token + /// ID if the token has been minted. + pub fn get_minted_token_id(&self, token_denom: String) -> Option { let mut working_set = WorkingSet::new(self.prover_storage()); self.runtime() .ibc_transfer - .escrowed_token(token_denom, &mut working_set) + .minted_token(token_denom, &mut working_set) .map(|token| token.token_id) .ok() } diff --git a/mocks/src/tests/transfer.rs b/mocks/src/tests/transfer.rs index 686a7c1a..d08ce6e1 100644 --- a/mocks/src/tests/transfer.rs +++ b/mocks/src/tests/transfer.rs @@ -4,9 +4,10 @@ use ibc_app_transfer::types::{PrefixedDenom, TracePrefix}; use ibc_core::client::context::client_state::ClientStateCommon; use ibc_core::host::types::identifiers::{ChannelId, PortId}; use ibc_core::primitives::ToProto; -use sov_bank::{TokenConfig, GAS_TOKEN_ID}; +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; @@ -24,62 +25,67 @@ async fn test_escrow_unescrow_on_sov() { // set transfer parameters let gas_token = relayer_builder.setup_cfg().gas_token_config(); - let gas_token_id = GAS_TOKEN_ID; - let mut cfg = TransferTestConfig::builder() - .sov_denom(gas_token.token_name.clone()) - .sov_token_id(Some(GAS_TOKEN_ID)) .sov_address(gas_token.address_and_balances[0].0) .build(); let expected_sender_balance = gas_token.address_and_balances[0].1 - cfg.amount * 2; // ----------------------------------------------------------------------- - // Send a `MsgTransfer` to the rollup + // Send a `MsgTransfer` to the rollup (twice) // ----------------------------------------------------------------------- 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() + CallMessage::Transfer(msg_transfer_on_sov.clone()).into(), + CallMessage::Transfer(msg_transfer_on_sov.clone()).into(), ]) .await; // ----------------------------------------------------------------------- - // Check that the token has been escrowed + // Check the sender balance has been updated correctly // ----------------------------------------------------------------------- - let escrowed_token = rly + let sender_balance = rly .src_chain_ctx() .service() - .get_escrowed_token_id(gas_token_id.to_string()) - .unwrap(); + .get_balance_of(cfg.sov_address, gas_token.token_id); - assert_eq!(escrowed_token, gas_token_id); + assert_eq!(sender_balance, expected_sender_balance); // ----------------------------------------------------------------------- - // Transfer the same token once again + // Send a `MsgRecvPacket` paired with a `MsgUpdateClient` to the Cosmos chain // ----------------------------------------------------------------------- - rly.src_chain_ctx() - .submit_msgs(vec![ - CallMessage::Transfer(msg_transfer_on_sov.clone()).into() - ]) + let target_height = match rly.src_chain_ctx().query(QueryReq::HostHeight).await { + QueryResp::HostHeight(height) => height, + _ => panic!("unexpected response"), + }; + + let msg_update_client = rly.build_msg_update_client_for_cos(target_height).await; + + let msg_recv_packet = rly + .build_msg_recv_packet_for_cos(target_height, msg_transfer_on_sov) .await; - // ----------------------------------------------------------------------- - // Check the sender balance has been updated correctly - // ----------------------------------------------------------------------- - let sender_balance = rly - .src_chain_ctx() + rly.dst_chain_ctx() + .submit_msgs(vec![msg_update_client, msg_recv_packet.to_any()]) + .await; + + let expected_denom_on_cos = + "transfer/channel-0/token_1rwrh8gn2py0dl4vv65twgctmlwck6esm2as9dftumcw89kqqn3nqrduss6"; + + let receiver_balance = rly + .dst_chain_ctx() .service() - .get_balance_of(cfg.sov_address, gas_token_id); + .get_balance_of(expected_denom_on_cos, cfg.cos_address.clone()); - assert_eq!(sender_balance, expected_sender_balance); + assert_eq!(receiver_balance, Some(cfg.amount)); // ----------------------------------------------------------------------- // Transfer another token but with the same name as the previous one // ----------------------------------------------------------------------- - let fake_token_message = rly.build_msg_create_token(&gas_token.clone().into()); + let fake_token_message = rly.build_msg_create_token(&gas_token.clone()); rly.src_chain_ctx() .submit_msgs(vec![fake_token_message.clone().into()]) @@ -89,8 +95,8 @@ async fn test_escrow_unescrow_on_sov() { .setup_cfg() .get_token_id_for_relayer(&gas_token.token_name); - cfg.sov_token_id = Some(fake_token_id); cfg.amount = 50; + cfg.sov_denom = fake_token_id.to_bech32().to_string(); let fake_token_sender_initial_balance = rly .src_chain_ctx() @@ -108,20 +114,12 @@ async fn test_escrow_unescrow_on_sov() { // ----------------------------------------------------------------------- // Check that the token has been escrowed as a distinct asset // ----------------------------------------------------------------------- - let escrowed_token = rly - .src_chain_ctx() - .service() - .get_escrowed_token_id(fake_token_id.to_string()) - .unwrap(); - - assert_eq!(escrowed_token, fake_token_id); - - let sender_genuine_token_balance = rly + let gas_token_sender_balance = rly .src_chain_ctx() .service() - .get_balance_of(cfg.sov_address, gas_token_id); + .get_balance_of(cfg.sov_address, gas_token.token_id); - assert_eq!(sender_genuine_token_balance, expected_sender_balance); + assert_eq!(gas_token_sender_balance, expected_sender_balance); let fake_token_sender_balance = rly .src_chain_ctx() @@ -144,22 +142,26 @@ 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_denom(gas_token.token_name.clone()) .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 let fake_token = TokenConfig { token_name: "transfer/channel-0/basecoin".to_string(), - ..gas_token.into() + ..gas_token }; - let fake_token_message = rly.build_msg_create_token(&fake_token); + let create_fake_token_message = rly.build_msg_create_token(&fake_token); rly.src_chain_ctx() - .submit_msgs(vec![fake_token_message.clone().into()]) + .submit_msgs(vec![create_fake_token_message.clone().into()]) .await; + let fake_token_id = rly.src_chain_ctx().service().get_token_id(fake_token); + // Store the current balance of the sender to check it later after the transfers let initial_sender_balance = rly .dst_chain_ctx() @@ -211,22 +213,22 @@ async fn test_mint_burn_on_sov() { assert_eq!(client_state.latest_height(), target_height); // ----------------------------------------------------------------------- - // Check uniqueness of the created token ID + // Check uniqueness of the token minted on the rollup // ----------------------------------------------------------------------- let denom_path_prefix = TracePrefix::new(PortId::transfer(), ChannelId::zero()); let mut prefixed_denom = PrefixedDenom::from_str(&cfg.cos_denom).unwrap(); prefixed_denom.add_trace_prefix(denom_path_prefix); - let token_id_on_sov = rly + let minted_token_id = rly .src_chain_ctx() .service() .get_minted_token_id(prefixed_denom.to_string()) .unwrap(); - assert_ne!(token_id_on_sov, fake_token.token_id); + assert_ne!(Some(minted_token_id), fake_token_id); // ----------------------------------------------------------------------- - // Transfer the same token once again to the Cosmos chain + // Submit another same `MsgTransfer` to the Cosmos chain // ----------------------------------------------------------------------- rly.dst_chain_ctx() .submit_msgs(vec![msg_transfer_on_cos.clone().to_any()]) @@ -237,6 +239,10 @@ async fn test_mint_burn_on_sov() { _ => panic!("unexpected response"), }; + // ----------------------------------------------------------------------- + // Send a `MsgRecvPacket` paired with a `MsgUpdateClient` to the rollup + // ----------------------------------------------------------------------- + let msg_update_client = rly.build_msg_update_client_for_sov(target_height).await; let msg_recv_packet = rly @@ -253,7 +259,7 @@ async fn test_mint_burn_on_sov() { let receiver_balance = rly .src_chain_ctx() .service() - .get_balance_of(cfg.sov_address, token_id_on_sov); + .get_balance_of(cfg.sov_address, minted_token_id); let mut expected_receiver_balance = cfg.amount * 2; @@ -274,7 +280,9 @@ async fn test_mint_burn_on_sov() { // ----------------------------------------------------------------------- cfg.sov_denom = "transfer/channel-0/basecoin".to_string(); - cfg.sov_token_id = Some(token_id_on_sov); + 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); @@ -302,16 +310,16 @@ async fn test_mint_burn_on_sov() { // ----------------------------------------------------------------------- // Check the token has been burned on rollup and unescrowed on Cosmos chain // ----------------------------------------------------------------------- - let sender_balance = rly + let receiver_balance = rly .src_chain_ctx() .service() - .get_balance_of(cfg.sov_address, token_id_on_sov); + .get_balance_of(cfg.sov_address, minted_token_id); expected_receiver_balance -= cfg.amount; - assert_eq!(sender_balance, expected_receiver_balance); + assert_eq!(receiver_balance, expected_receiver_balance); - let receiver_balance = rly + let sender_balance = rly .dst_chain_ctx() .service() .get_balance_of(&cfg.cos_denom, cfg.cos_address) @@ -319,5 +327,5 @@ async fn test_mint_burn_on_sov() { expected_sender_balance += cfg.amount; - assert_eq!(receiver_balance, expected_sender_balance); + assert_eq!(sender_balance, expected_sender_balance); } diff --git a/modules/sov-consensus-state-tracker/Cargo.toml b/modules/sov-consensus-state-tracker/Cargo.toml index 59a9016d..e296baa5 100644 --- a/modules/sov-consensus-state-tracker/Cargo.toml +++ b/modules/sov-consensus-state-tracker/Cargo.toml @@ -15,7 +15,6 @@ workspace = true [dependencies] # external dependencies anyhow = { workspace = true } -base64 = { workspace = true } borsh = { workspace = true } jsonrpsee = { workspace = true, optional = true } prost = { workspace = true } diff --git a/modules/sov-ibc-transfer/Cargo.toml b/modules/sov-ibc-transfer/Cargo.toml index affedb79..a2810a7b 100644 --- a/modules/sov-ibc-transfer/Cargo.toml +++ b/modules/sov-ibc-transfer/Cargo.toml @@ -14,16 +14,16 @@ workspace = true [dependencies] # external dependencies -anyhow = { workspace = true } -base64 = { workspace = true } -borsh = { workspace = true } -jsonrpsee = { workspace = true, optional = true } -prost = { workspace = true } -schemars = { workspace = true, optional = true } -serde = { workspace = true } -serde_json = { workspace = true, optional = true } -thiserror = { workspace = true } -uint = "0.9" +anyhow = { workspace = true } +borsh = { workspace = true } +derive_more = { workspace = true } +jsonrpsee = { workspace = true, optional = true } +prost = { workspace = true } +schemars = { workspace = true, optional = true } +serde = { workspace = true } +serde_json = { workspace = true } +thiserror = { workspace = true } +uint = "0.9" # ibc dependencies ibc-app-transfer = { workspace = true, features = ["borsh", "schema"] } @@ -36,9 +36,7 @@ sov-rollup-interface = { workspace = true } [features] default = [] -serde = ["serde_json"] native = [ - "serde", "sov-bank/native", "sov-modules-api/native", "sov-rollup-interface/native", diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index ac42f9a1..b91809b6 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/modules/sov-ibc-transfer/src/context.rs @@ -1,9 +1,7 @@ use std::cell::RefCell; use std::rc::Rc; +use std::str::FromStr; -use base64::engine::general_purpose; -use base64::Engine; -use borsh::BorshDeserialize; use ibc_app_transfer::context::{TokenTransferExecutionContext, TokenTransferValidationContext}; use ibc_app_transfer::module::{ on_acknowledgement_packet_validate, on_chan_open_ack_validate, on_chan_open_confirm_validate, @@ -27,7 +25,7 @@ use sov_modules_api::{Context, Spec, WorkingSet}; use uint::FromDecStrErr; use super::IbcTransfer; -use crate::utils::compute_escrow_address; +use crate::utils::{compute_escrow_address, SovereignMemo}; /// 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, @@ -148,11 +146,7 @@ where coin: &PrefixedCoin, memo: &Memo, ) -> Result<(), TokenTransferError> { - let token_id_buf = general_purpose::STANDARD_NO_PAD - .decode(memo.as_ref()) - .map_err(|e| TokenTransferError::Other(format!("Failed to decode memo: {e}")))?; - - let token_id = BorshDeserialize::deserialize(&mut token_id_buf.as_slice()) + let sov_memo: SovereignMemo = serde_json::from_str(memo.as_ref()) .map_err(|e| TokenTransferError::Other(format!("Failed to decode memo: {e}")))?; let expected_token_id = { @@ -164,7 +158,7 @@ where })? }; - if token_id != expected_token_id { + if sov_memo.token_id() != expected_token_id { return Err(TokenTransferError::InvalidCoin { coin: coin.to_string(), }); @@ -175,7 +169,7 @@ where .bank .get_balance_of( account.address.clone(), - token_id, + sov_memo.token_id(), *self.working_set.borrow_mut(), ) .ok_or(TokenTransferError::InvalidCoin { @@ -203,25 +197,16 @@ where coin: &PrefixedCoin, memo: &Memo, ) -> Result<(), TokenTransferError> { - let token_id_buf = general_purpose::STANDARD_NO_PAD - .decode(memo.as_ref()) - .map_err(|e| TokenTransferError::Other(format!("Failed to decode memo: {e}")))?; - - let token_id = BorshDeserialize::deserialize(&mut token_id_buf.as_slice()) - .map_err(|e| TokenTransferError::Other(format!("Failed to decode memo: {e}")))?; - - let token_name = self - .ibc_transfer - .bank - .get_token_name(&token_id, &mut self.working_set.borrow_mut()) - .ok_or(TokenTransferError::Other(format!( - "No token with address {token_id}", - )))?; - - if token_name != coin.denom.to_string() { - return Err(TokenTransferError::InvalidCoin { + 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() { + return Err(TokenTransferError::Other( + "Memo must be empty when escrowing tokens".to_string(), + )); } let sender_balance: u64 = self @@ -271,14 +256,12 @@ where ) -> Result<(), TokenTransferError> { // ensure that escrow account has enough balance let escrow_balance: Amount = { - let token_id = { - self.ibc_transfer - .escrowed_tokens - .get(&coin.denom.to_string(), *self.working_set.borrow_mut()) - .ok_or(TokenTransferError::InvalidCoin { - coin: coin.to_string(), - })? - }; + 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 @@ -428,29 +411,20 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S> port_id: &PortId, channel_id: &ChannelId, coin: &PrefixedCoin, - memo: &Memo, + _memo: &Memo, ) -> Result<(), TokenTransferError> { - let token_id_buf = general_purpose::STANDARD_NO_PAD - .decode(memo.as_ref()) - .map_err(|e| TokenTransferError::Other(format!("Failed to decode memo: {e}")))?; - - let token_id: TokenId = BorshDeserialize::deserialize(&mut token_id_buf.as_slice()) - .map_err(|e| TokenTransferError::Other(format!("Failed to decode memo: {e}")))?; - // The token name on the Sovereign SDK chains is not guaranteed to be // unique, and hence we must use the token ID (which is guaranteed to be // unique) as the ICS-20 denom to ensure uniqueness. - let denom = token_id.to_string(); - - // 1. ensure that token exists in `self.escrowed_tokens` map, which is - // necessary information when unescrowing tokens - self.ibc_transfer - .escrowed_tokens - .set(&denom, &token_id, *self.working_set.borrow_mut()); + let token_id = TokenId::from_str(&coin.denom.to_string()).map_err(|e| { + TokenTransferError::InvalidCoin { + coin: coin.to_string(), + } + })?; - // 2. transfer coins to escrow account let escrow_account = self.obtain_escrow_address(port_id, channel_id); + // transfer coins to escrow account self.transfer( token_id, &from_account.address, @@ -471,18 +445,15 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S> channel_id: &ChannelId, coin: &PrefixedCoin, ) -> Result<(), TokenTransferError> { - let token_id = self - .ibc_transfer - .escrowed_tokens - .get(&coin.denom.to_string(), *self.working_set.borrow_mut()) - .ok_or(TokenTransferError::InvalidCoin { + let token_id = TokenId::from_str(&coin.denom.to_string()).map_err(|e| { + TokenTransferError::InvalidCoin { coin: coin.to_string(), - })?; - - // transfer coins out of escrow account to `to_account` + } + })?; let escrow_account = self.obtain_escrow_address(port_id, channel_id); + // transfer coins out of escrow account to `to_account` self.transfer(token_id, &escrow_account, &to_account.address, &coin.amount)?; Ok(()) diff --git a/modules/sov-ibc-transfer/src/lib.rs b/modules/sov-ibc-transfer/src/lib.rs index f14b2325..5486bbf5 100644 --- a/modules/sov-ibc-transfer/src/lib.rs +++ b/modules/sov-ibc-transfer/src/lib.rs @@ -1,6 +1,6 @@ pub mod context; mod genesis; -mod utils; +pub mod utils; use anyhow::anyhow; use ibc_core::handler::types::events::IbcEvent; @@ -28,23 +28,11 @@ pub struct IbcTransfer { #[module] bank: sov_bank::Bank, - /// Keeps track of the address of each token we minted by token denom. + /// Keeps track of the token IDs minted by the `IbcTransfer` using the + /// specified token denomination in the `MsgRecvPacket` message. #[state] minted_tokens: StateMap, - /// Keeps track of the address of each token we escrowed as a function of - /// the token denom. We need this map because we have the token ID - /// information when escrowing the tokens (i.e. when someone calls a - /// `send_transfer()`), but not when unescrowing tokens (i.e in a - /// `recv_packet`), in which case the only information we have is the ICS 20 - /// denom, and amount. Given that every token that is unescrowed has been - /// previously escrowed, our strategy to get the token ID associated - /// with a denom is - /// 1. when tokens are escrowed, save the mapping `denom -> token ID` - /// 2. when tokens are unescrowed, lookup the token ID by `denom` - #[state] - escrowed_tokens: StateMap, - /// Keeps track of escrow addresses associated with a specific port and /// channel pair, offering an efficient means to access these addresses /// without the need for recomputation during every packet processing. diff --git a/modules/sov-ibc-transfer/src/rpc.rs b/modules/sov-ibc-transfer/src/rpc.rs index 8badcdd2..ce2fbea3 100644 --- a/modules/sov-ibc-transfer/src/rpc.rs +++ b/modules/sov-ibc-transfer/src/rpc.rs @@ -7,11 +7,6 @@ use sov_modules_api::{Spec, WorkingSet}; use super::IbcTransfer; -#[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] -pub struct EscrowedTokenResponse { - pub token_id: TokenId, -} - #[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] pub struct MintedTokenResponse { pub token_id: TokenId, @@ -22,24 +17,6 @@ impl IbcTransfer where S: Spec, { - #[rpc_method(name = "escrowedToken")] - pub fn escrowed_token( - &self, - token_denom: String, - working_set: &mut WorkingSet, - ) -> RpcResult { - let token_id = - self.escrowed_tokens - .get(&token_denom, working_set) - .ok_or(ErrorObjectOwned::owned( - jsonrpsee::types::error::UNKNOWN_ERROR_CODE, - format!("No escrowed token found for denom {token_denom}"), - None::, - ))?; - - Ok(EscrowedTokenResponse { token_id }) - } - #[rpc_method(name = "mintedToken")] pub fn minted_token( &self, diff --git a/modules/sov-ibc-transfer/src/utils.rs b/modules/sov-ibc-transfer/src/utils.rs index 793f669c..ad9efb6d 100644 --- a/modules/sov-ibc-transfer/src/utils.rs +++ b/modules/sov-ibc-transfer/src/utils.rs @@ -1,8 +1,41 @@ 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: /// /// except that the `Hasher` function mandated by the `CryptoSpec` trait in the From fbc13ceb7c06c1e9bc64fb1c160b3b9e5a530071 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Tue, 9 Apr 2024 20:49:31 -0700 Subject: [PATCH 02/10] chore: update basecoin rev --- Cargo.lock | 46 ++++++++++++++++++++++------------------------ mocks/Cargo.toml | 10 +++++----- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac790b5d..97d57348 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -569,7 +569,7 @@ checksum = "8c3c1a368f70d6cf7302d78f8f7093da241fb8e8807c05cc9e51a125895a6d5b" [[package]] name = "basecoin" version = "0.1.0" -source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=90886ab#90886abafbcd65a914fb8396cf850686b7d89f94" +source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=adfebf9#adfebf9e5cd916696544185a4b73bce7e1980bfd" dependencies = [ "basecoin-app", "basecoin-modules", @@ -591,7 +591,7 @@ dependencies = [ [[package]] name = "basecoin-app" version = "0.1.0" -source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=90886ab#90886abafbcd65a914fb8396cf850686b7d89f94" +source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=adfebf9#adfebf9e5cd916696544185a4b73bce7e1980bfd" dependencies = [ "basecoin-modules", "basecoin-store", @@ -610,7 +610,7 @@ dependencies = [ [[package]] name = "basecoin-modules" version = "0.1.0" -source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=90886ab#90886abafbcd65a914fb8396cf850686b7d89f94" +source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=adfebf9#adfebf9e5cd916696544185a4b73bce7e1980bfd" dependencies = [ "base64 0.21.7", "basecoin-store", @@ -627,7 +627,7 @@ dependencies = [ "serde_derive", "serde_json", "sha2 0.10.8", - "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", + "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", "tendermint 0.34.1", "tendermint-rpc", "tonic", @@ -637,7 +637,7 @@ dependencies = [ [[package]] name = "basecoin-store" version = "0.1.0" -source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=90886ab#90886abafbcd65a914fb8396cf850686b7d89f94" +source = "git+https://github.com/informalsystems/basecoin-rs.git?rev=adfebf9#adfebf9e5cd916696544185a4b73bce7e1980bfd" dependencies = [ "displaydoc", "ics23", @@ -5504,7 +5504,7 @@ dependencies = [ [[package]] name = "sov-celestia-client" version = "0.1.0" -source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72#acdfdf72fc5aebdd98ffd868007f531661984bee" +source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16#300e4c16210af5c347739a8d2de449cc837600a7" dependencies = [ "borsh", "derive_more", @@ -5515,7 +5515,7 @@ dependencies = [ "prost", "serde", "sha2 0.10.8", - "sov-celestia-client-types 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", + "sov-celestia-client-types 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", "tendermint 0.34.1", "tendermint-light-client-verifier", "tendermint-proto 0.34.1", @@ -5567,9 +5567,8 @@ dependencies = [ [[package]] name = "sov-celestia-client-types" version = "0.1.0" -source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72#acdfdf72fc5aebdd98ffd868007f531661984bee" +source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16#300e4c16210af5c347739a8d2de449cc837600a7" dependencies = [ - "base64 0.21.7", "bytes", "derive_more", "hex", @@ -5580,7 +5579,7 @@ dependencies = [ "jmt", "prost", "serde", - "sov-ibc-proto 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", + "sov-ibc-proto 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", "sov-rollup-interface", "tendermint 0.34.1", "tendermint-light-client-verifier", @@ -5632,18 +5631,17 @@ dependencies = [ [[package]] name = "sov-consensus-state-tracker" version = "0.1.0" -source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72#acdfdf72fc5aebdd98ffd868007f531661984bee" +source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16#300e4c16210af5c347739a8d2de449cc837600a7" dependencies = [ "anyhow", - "base64 0.21.7", "borsh", "ibc-app-transfer", "ibc-core", "prost", "serde", "sov-celestia-adapter", - "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", - "sov-ibc 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", + "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", + "sov-ibc 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", "sov-mock-da", "sov-modules-api", "sov-modules-core", @@ -5704,7 +5702,7 @@ dependencies = [ [[package]] name = "sov-ibc" version = "0.1.0" -source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72#acdfdf72fc5aebdd98ffd868007f531661984bee" +source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16#300e4c16210af5c347739a8d2de449cc837600a7" dependencies = [ "ahash 0.8.6", "anyhow", @@ -5721,8 +5719,8 @@ dependencies = [ "serde", "serde_json", "sha2 0.10.8", - "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", - "sov-ibc-transfer 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", + "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", + "sov-ibc-transfer 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", "sov-modules-api", "sov-rollup-interface", "sov-state", @@ -5752,11 +5750,11 @@ dependencies = [ "serde_json", "sha2 0.10.8", "sov-bank", - "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", + "sov-celestia-client 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", "sov-chain-state", - "sov-consensus-state-tracker 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", - "sov-ibc 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", - "sov-ibc-transfer 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72)", + "sov-consensus-state-tracker 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", + "sov-ibc 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", + "sov-ibc-transfer 0.1.0 (git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16)", "sov-kernels", "sov-mock-zkvm", "sov-modules-api", @@ -5790,7 +5788,7 @@ dependencies = [ [[package]] name = "sov-ibc-proto" version = "0.1.0" -source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72#acdfdf72fc5aebdd98ffd868007f531661984bee" +source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16#300e4c16210af5c347739a8d2de449cc837600a7" dependencies = [ "ibc-proto", "informalsystems-pbjson", @@ -5835,11 +5833,11 @@ dependencies = [ [[package]] name = "sov-ibc-transfer" version = "0.1.0" -source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=acdfdf72#acdfdf72fc5aebdd98ffd868007f531661984bee" +source = "git+https://github.com/informalsystems/sovereign-ibc.git?rev=300e4c16#300e4c16210af5c347739a8d2de449cc837600a7" dependencies = [ "anyhow", - "base64 0.21.7", "borsh", + "derive_more", "ibc-app-transfer", "ibc-core", "jsonrpsee", diff --git a/mocks/Cargo.toml b/mocks/Cargo.toml index 687f7486..e7232a13 100644 --- a/mocks/Cargo.toml +++ b/mocks/Cargo.toml @@ -31,10 +31,10 @@ tracing = "0.1.36" typed-builder = "0.18.0" # internal dependencies -sov-ibc = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "acdfdf72" } -sov-ibc-transfer = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "acdfdf72" } -sov-consensus-state-tracker = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "acdfdf72" } -sov-celestia-client = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "acdfdf72", features = ["test-util"] } +sov-ibc = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "300e4c16" } +sov-ibc-transfer = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "300e4c16" } +sov-consensus-state-tracker = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "300e4c16" } +sov-celestia-client = { git = "https://github.com/informalsystems/sovereign-ibc.git", rev = "300e4c16", features = ["test-util"] } # ibc dependencies ibc-core = { workspace = true } @@ -45,7 +45,7 @@ ibc-query = { workspace = true } ibc-testkit = { workspace = true } # cosmos dependencies -basecoin = { git = "https://github.com/informalsystems/basecoin-rs.git", rev = "90886ab" } +basecoin = { git = "https://github.com/informalsystems/basecoin-rs.git", rev = "adfebf9" } tendermint = { workspace = true } tendermint-testgen = { workspace = true } From 5e6fdd9a34a01e6bfda13b5a36ce340414415d25 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 10:29:55 -0700 Subject: [PATCH 03/10] imp: move away from memo + introduce minted_token_id_to_name state --- mocks/src/configs.rs | 4 -- mocks/src/relayer/msgs/cosmos.rs | 2 +- mocks/src/relayer/msgs/rollup.rs | 2 +- mocks/src/sovereign/runner.rs | 11 ++-- mocks/src/tests/transfer.rs | 30 ++++++++--- modules/sov-ibc-transfer/src/context.rs | 72 ++++++++++++++++--------- modules/sov-ibc-transfer/src/lib.rs | 15 ++++-- modules/sov-ibc-transfer/src/rpc.rs | 16 +++--- modules/sov-ibc-transfer/src/utils.rs | 33 ------------ 9 files changed, 100 insertions(+), 85 deletions(-) diff --git a/mocks/src/configs.rs b/mocks/src/configs.rs index 6abdd9d4..63d6a489 100644 --- a/mocks/src/configs.rs +++ b/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. diff --git a/mocks/src/relayer/msgs/cosmos.rs b/mocks/src/relayer/msgs/cosmos.rs index cf6454eb..23e7386e 100644 --- a/mocks/src/relayer/msgs/cosmos.rs +++ b/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 { diff --git a/mocks/src/relayer/msgs/rollup.rs b/mocks/src/relayer/msgs/rollup.rs index d98557d9..13213f78 100644 --- a/mocks/src/relayer/msgs/rollup.rs +++ b/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 { diff --git a/mocks/src/sovereign/runner.rs b/mocks/src/sovereign/runner.rs index b346e451..1272cd45 100644 --- a/mocks/src/sovereign/runner.rs +++ b/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); diff --git a/mocks/src/tests/transfer.rs b/mocks/src/tests/transfer.rs index d08ce6e1..925b7fbd 100644 --- a/mocks/src/tests/transfer.rs +++ b/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); } diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index b91809b6..997ac8d5 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/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(), diff --git a/modules/sov-ibc-transfer/src/lib.rs b/modules/sov-ibc-transfer/src/lib.rs index 5486bbf5..c910188f 100644 --- a/modules/sov-ibc-transfer/src/lib.rs +++ b/modules/sov-ibc-transfer/src/lib.rs @@ -28,10 +28,19 @@ pub struct IbcTransfer { #[module] bank: sov_bank::Bank, - /// 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, + minted_token_name_to_id: StateMap, + + /// 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, /// Keeps track of escrow addresses associated with a specific port and /// channel pair, offering an efficient means to access these addresses diff --git a/modules/sov-ibc-transfer/src/rpc.rs b/modules/sov-ibc-transfer/src/rpc.rs index ce2fbea3..2d07a60e 100644 --- a/modules/sov-ibc-transfer/src/rpc.rs +++ b/modules/sov-ibc-transfer/src/rpc.rs @@ -23,14 +23,14 @@ where token_denom: String, working_set: &mut WorkingSet, ) -> RpcResult { - 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::, - ))?; + 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::, + ))?; Ok(MintedTokenResponse { token_id }) } diff --git a/modules/sov-ibc-transfer/src/utils.rs b/modules/sov-ibc-transfer/src/utils.rs index ad9efb6d..793f669c 100644 --- a/modules/sov-ibc-transfer/src/utils.rs +++ b/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: /// /// except that the `Hasher` function mandated by the `CryptoSpec` trait in the From 6b29dc243d5ea1315596dfb778e1e62344cf29a7 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 12:05:59 -0700 Subject: [PATCH 04/10] fix: enforce context to use ibc_transfer address as sender --- mocks/src/sovereign/runner.rs | 21 +++++++-------------- modules/sov-ibc-transfer/src/context.rs | 8 +++++++- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/mocks/src/sovereign/runner.rs b/mocks/src/sovereign/runner.rs index 1272cd45..6db4349d 100644 --- a/mocks/src/sovereign/runner.rs +++ b/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::{ - CallResponse, DispatchCall, Gas, GasMeter, Genesis, KernelWorkingSet, ModuleInfo, SlotData, - Spec, StateCheckpoint, + CallResponse, DispatchCall, Gas, GasMeter, Genesis, KernelWorkingSet, SlotData, Spec, + StateCheckpoint, }; use sov_rollup_interface::da::BlockHeaderTrait; use sov_rollup_interface::services::da::DaService; @@ -15,7 +15,7 @@ use sov_state::{MerkleProofSpec, ProverStorage, Storage}; use tokio::task::JoinHandle; use tracing::{debug, info}; -use super::{GenesisConfig, MockRollup, RuntimeCall}; +use super::{GenesisConfig, MockRollup}; use crate::utils::{wait_for_block, MutexUtil}; impl MockRollup @@ -105,15 +105,11 @@ where let rollup_ctx = self.rollup_ctx(); - for m in self.mempool() { - // Sets the sender address to the address of the 'sov-ibc' - // module, ensuring that the module's address is used for the - // token creation. - if let RuntimeCall::ibc(_) = m { - self.resolve_ctx(self.runtime().ibc.address().clone(), visible_slot); - } + // Resets the sender address to the address of the relayer + self.resolve_ctx(rollup_ctx.sender().clone(), visible_slot); - // NOTE: on failures, we silently ignore the error and continue as + for m in self.mempool() { + // NOTE: on failures, we silently ignore the message and continue as // it is in the real-case scenarios self.runtime() .dispatch_call(m.clone(), &mut working_set, &self.rollup_ctx()) @@ -121,9 +117,6 @@ where 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); } *self.mempool.acquire_mutex() = vec![]; diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index 997ac8d5..8d72009a 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/modules/sov-ibc-transfer/src/context.rs @@ -336,6 +336,12 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S> let minter_address = account.address.clone(); // Only the transfer module is allowed to mint let authorized_minters = vec![self.ibc_transfer.address.clone()]; + // Make sure to use `ibc_transfer` address as the sender + let context = Context::new( + self.ibc_transfer.address.clone(), + self.sdk_context.sequencer().clone(), + self.sdk_context.visible_slot_number(), + ); let new_token_id = self .ibc_transfer .bank @@ -345,7 +351,7 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S> initial_balance, minter_address, authorized_minters, - &self.sdk_context, + &context, &mut self.working_set.borrow_mut(), ) .map_err(|err| TokenTransferError::Other(err.to_string()))?; From b9271531b1b259510603b1c0048725a82dceea78 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 13:25:25 -0700 Subject: [PATCH 05/10] fix: apply review comments --- mocks/src/sovereign/runner.rs | 2 +- modules/sov-ibc-transfer/src/context.rs | 149 +++++++++++++----------- 2 files changed, 82 insertions(+), 69 deletions(-) diff --git a/mocks/src/sovereign/runner.rs b/mocks/src/sovereign/runner.rs index 6db4349d..d26cde6a 100644 --- a/mocks/src/sovereign/runner.rs +++ b/mocks/src/sovereign/runner.rs @@ -114,7 +114,7 @@ where self.runtime() .dispatch_call(m.clone(), &mut working_set, &self.rollup_ctx()) .unwrap_or_else(|e| { - info!("rollup: error executing message: {:?}", e); + info!("rollup: error executing message: {e:?}"); CallResponse::default() }); } diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index 8d72009a..e23de7e9 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/modules/sov-ibc-transfer/src/context.rs @@ -50,6 +50,22 @@ impl<'ws, S: Spec> IbcTransferContext<'ws, S> { } } + /// Stores mapping from "denom to token ID" and vice versa for an IBC-minted + /// token.s + fn record_minted_token(&self, token_id: TokenId, token_name: String) { + self.ibc_transfer.minted_token_id_to_name.set( + &token_id, + &token_name, + *self.working_set.borrow_mut(), + ); + + self.ibc_transfer.minted_token_name_to_id.set( + &token_name, + &token_id, + *self.working_set.borrow_mut(), + ); + } + /// Obtains the escrow address for a given port and channel pair by looking /// up the cache. If the cache does not contain the address, it is computed /// and stored in the cache. @@ -70,6 +86,46 @@ impl<'ws, S: Spec> IbcTransferContext<'ws, S> { }) } + fn create_token( + &self, + token_name: String, + minter_address: S::Address, + authorized_minters: Vec, + ) -> Result { + // Using a different salt will result in a different token + // address. Since ICS-20 tokens coming from other chains are + // guaranteed to have unique names, we don't need to use + // different salt values. + let salt = 0u64; + + let initial_balance = 0; + + // Make sure to use `ibc_transfer` address as the sender + let context = Context::new( + self.ibc_transfer.address.clone(), + self.sdk_context.sequencer().clone(), + self.sdk_context.visible_slot_number(), + ); + + let new_token_id = self + .ibc_transfer + .bank + .create_token( + token_name.clone(), + salt, + initial_balance, + minter_address, + authorized_minters, + &context, + &mut self.working_set.borrow_mut(), + ) + .map_err(|err| TokenTransferError::Other(err.to_string()))?; + + self.record_minted_token(new_token_id, token_name); + + Ok(new_token_id) + } + /// Transfers `amount` tokens from `from_account` to `to_account` fn transfer( &self, @@ -152,14 +208,15 @@ where )); } - let minted_token_id = { - self.ibc_transfer - .minted_token_name_to_id - .get(&coin.denom.to_string(), *self.working_set.borrow_mut()) - .ok_or(TokenTransferError::InvalidCoin { - coin: coin.to_string(), - })? - }; + let token_name = coin.denom.to_string(); + + let minted_token_id = self + .ibc_transfer + .minted_token_name_to_id + .get(&token_name, *self.working_set.borrow_mut()) + .ok_or(TokenTransferError::InvalidCoin { + coin: coin.to_string(), + })?; let sender_balance: u64 = self .ibc_transfer @@ -169,9 +226,7 @@ where minted_token_id, *self.working_set.borrow_mut(), ) - .ok_or(TokenTransferError::InvalidCoin { - coin: coin.denom.to_string(), - })?; + .ok_or(TokenTransferError::InvalidCoin { coin: token_name })?; let sender_balance: Amount = sender_balance.into(); @@ -211,9 +266,10 @@ where .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 escrowed. Use '{token_name}' as denom for sending back an IBC token to the source chain"), - )); + return Err(TokenTransferError::Other(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" + ))); } let sender_balance: u64 = self @@ -272,9 +328,10 @@ where .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") - )); + 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 @@ -311,7 +368,7 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S> account: &Self::AccountId, coin: &PrefixedCoin, ) -> Result<(), TokenTransferError> { - let denom = coin.denom.to_string(); + let token_name = coin.denom.to_string(); // 1. if token ID doesn't exist in `minted_tokens`, then create a new // token and store in `minted_tokens` @@ -319,59 +376,15 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S> let maybe_token_id = self .ibc_transfer .minted_token_name_to_id - .get(&denom, *self.working_set.borrow_mut()); + .get(&token_name, *self.working_set.borrow_mut()); match maybe_token_id { Some(token_id) => token_id, - // Create a new token - None => { - let token_name = coin.denom.to_string(); - // Using a different salt will result in a different token - // address. Since ICS-20 tokens coming from other chains are - // guaranteed to have unique names, we don't need to use - // different salt values. - let salt = 0u64; - let initial_balance = 0; - // Note: unused since initial_balance = 0 - let minter_address = account.address.clone(); - // Only the transfer module is allowed to mint - let authorized_minters = vec![self.ibc_transfer.address.clone()]; - // Make sure to use `ibc_transfer` address as the sender - let context = Context::new( - self.ibc_transfer.address.clone(), - self.sdk_context.sequencer().clone(), - self.sdk_context.visible_slot_number(), - ); - let new_token_id = self - .ibc_transfer - .bank - .create_token( - token_name, - salt, - initial_balance, - minter_address, - authorized_minters, - &context, - &mut self.working_set.borrow_mut(), - ) - .map_err(|err| TokenTransferError::Other(err.to_string()))?; - - // 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 - } + None => self.create_token( + token_name, + account.address.clone(), + vec![self.ibc_transfer.address.clone()], + )?, } }; From 5c9bdafd68d250b1cdd0183a3ee2708f3e03f6d5 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 13:44:05 -0700 Subject: [PATCH 06/10] feat: add transfer_mintedTokenName RPC method --- modules/sov-ibc-transfer/src/rpc.rs | 48 +++++++++++++++++++++++------ modules/sov-ibc/src/rpc/helpers.rs | 11 +------ modules/sov-ibc/src/rpc/methods.rs | 2 +- 3 files changed, 41 insertions(+), 20 deletions(-) diff --git a/modules/sov-ibc-transfer/src/rpc.rs b/modules/sov-ibc-transfer/src/rpc.rs index 2d07a60e..a68e0d75 100644 --- a/modules/sov-ibc-transfer/src/rpc.rs +++ b/modules/sov-ibc-transfer/src/rpc.rs @@ -9,6 +9,7 @@ use super::IbcTransfer; #[derive(Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize, Clone)] pub struct MintedTokenResponse { + pub token_name: String, pub token_id: TokenId, } @@ -17,21 +18,50 @@ impl IbcTransfer where S: Spec, { - #[rpc_method(name = "mintedToken")] + #[rpc_method(name = "mintedTokenName")] + pub fn escrowed_token( + &self, + token_id: TokenId, + working_set: &mut WorkingSet, + ) -> RpcResult { + let token_name = self + .minted_token_id_to_name + .get(&token_id, working_set) + .ok_or(to_jsonrpsee_error(format!( + "No IBC-minted token found for ID: '{token_id}'" + )))?; + + Ok(MintedTokenResponse { + token_name, + token_id, + }) + } + + #[rpc_method(name = "mintedTokenId")] pub fn minted_token( &self, - token_denom: String, + token_name: String, working_set: &mut WorkingSet, ) -> RpcResult { 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::, - ))?; + .get(&token_name, working_set) + .ok_or(to_jsonrpsee_error(format!( + "No IBC-minted token found for denom: '{token_name}'" + )))?; - Ok(MintedTokenResponse { token_id }) + Ok(MintedTokenResponse { + token_name, + token_id, + }) } } + +/// Creates an jsonrpsee error object +pub fn to_jsonrpsee_error(err: impl ToString) -> ErrorObjectOwned { + ErrorObjectOwned::owned( + jsonrpsee::types::error::UNKNOWN_ERROR_CODE, + err.to_string(), + None::, + ) +} diff --git a/modules/sov-ibc/src/rpc/helpers.rs b/modules/sov-ibc/src/rpc/helpers.rs index b7384cf8..fc9993a0 100644 --- a/modules/sov-ibc/src/rpc/helpers.rs +++ b/modules/sov-ibc/src/rpc/helpers.rs @@ -4,7 +4,7 @@ use std::rc::Rc; use ibc_core::client::types::Height; use ibc_core::host::ValidationContext; use jsonrpsee::core::RpcResult; -use jsonrpsee::types::ErrorObjectOwned; +use sov_ibc_transfer::to_jsonrpsee_error; use sov_modules_api::{Spec, WorkingSet}; use crate::context::IbcContext; @@ -31,12 +31,3 @@ impl Ibc { } } } - -/// Creates an jsonrpsee error object -pub fn to_jsonrpsee_error(err: impl ToString) -> ErrorObjectOwned { - ErrorObjectOwned::owned( - jsonrpsee::types::error::UNKNOWN_ERROR_CODE, - err.to_string(), - None::, - ) -} diff --git a/modules/sov-ibc/src/rpc/methods.rs b/modules/sov-ibc/src/rpc/methods.rs index d95f2691..0ad75a0f 100644 --- a/modules/sov-ibc/src/rpc/methods.rs +++ b/modules/sov-ibc/src/rpc/methods.rs @@ -44,13 +44,13 @@ use ibc_query::core::connection::{ use jsonrpsee::core::RpcResult; use sov_celestia_client::client_state::ClientState as HostClientState; use sov_celestia_client::consensus_state::ConsensusState as HostConsensusState; +use sov_ibc_transfer::to_jsonrpsee_error; use sov_modules_api::macros::rpc_gen; use sov_modules_api::{ProvenStateAccessor, Spec, WorkingSet}; use sov_state::storage::{SlotKey, StateCodec, StateItemCodec}; use crate::clients::{AnyClientState, AnyConsensusState}; use crate::context::IbcContext; -use crate::helpers::to_jsonrpsee_error; use crate::Ibc; /// Structure returned by the `client_state` rpc method. From 4a424331cf9084a10d9899cde944bedf490864f2 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 14:28:09 -0700 Subject: [PATCH 07/10] chore: add comment for token_id map check in unescrow_coins_validate --- modules/sov-ibc-transfer/src/context.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index e23de7e9..2eb53d30 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/modules/sov-ibc-transfer/src/context.rs @@ -323,6 +323,9 @@ where } })?; + // NOTE: There is no way to make this fail from an honest counterparty + // chain, as this method only fails when the counterparty chain + // produces a malicious IBC transfer `send_packet()` if let Some(token_name) = self .ibc_transfer .minted_token_id_to_name From b655abd82caa7edf2fb2c3241739b5947601e66d Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 14:34:53 -0700 Subject: [PATCH 08/10] imp: add comments for create_token --- modules/sov-ibc-transfer/src/context.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index 2eb53d30..0420fe09 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/modules/sov-ibc-transfer/src/context.rs @@ -86,11 +86,17 @@ impl<'ws, S: Spec> IbcTransferContext<'ws, S> { }) } + /// Creates a new token with the specified `token_name` and mints an initial + /// balance to the `minter_address`. + /// + /// Note: The mint authority must be held by the `IbcTransfer` module, so the + /// `authorized_minters` is set to the `IbcTransfer` address. Also, remember + /// that the `token_name` is a denom prefixed with IBC and originates from the + /// counterparty chain. fn create_token( &self, token_name: String, minter_address: S::Address, - authorized_minters: Vec, ) -> Result { // Using a different salt will result in a different token // address. Since ICS-20 tokens coming from other chains are @@ -115,7 +121,7 @@ impl<'ws, S: Spec> IbcTransferContext<'ws, S> { salt, initial_balance, minter_address, - authorized_minters, + vec![self.ibc_transfer.address.clone()], &context, &mut self.working_set.borrow_mut(), ) @@ -383,11 +389,7 @@ impl<'ws, S: Spec> TokenTransferExecutionContext for IbcTransferContext<'ws, S> match maybe_token_id { Some(token_id) => token_id, - None => self.create_token( - token_name, - account.address.clone(), - vec![self.ibc_transfer.address.clone()], - )?, + None => self.create_token(token_name, account.address.clone())?, } }; From e9651e12c50544acd44a1d4ce4455c65911aebba Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 19:18:00 -0700 Subject: [PATCH 09/10] chore: clean-ups + comments --- mocks/src/sovereign/rollup.rs | 2 +- modules/sov-ibc-transfer/src/context.rs | 53 +++++++++++++------------ modules/sov-ibc-transfer/src/rpc.rs | 8 ++-- 3 files changed, 33 insertions(+), 30 deletions(-) diff --git a/mocks/src/sovereign/rollup.rs b/mocks/src/sovereign/rollup.rs index baf8e059..bf202612 100644 --- a/mocks/src/sovereign/rollup.rs +++ b/mocks/src/sovereign/rollup.rs @@ -172,7 +172,7 @@ where self.runtime() .ibc_transfer - .minted_token(token_denom, &mut working_set) + .minted_token_id(token_denom, &mut working_set) .map(|token| token.token_id) .ok() } diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index 0420fe09..c2897a9f 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/modules/sov-ibc-transfer/src/context.rs @@ -27,6 +27,15 @@ use uint::FromDecStrErr; use super::IbcTransfer; use crate::utils::compute_escrow_address; +/// Using a different salt will result in a different token address. Since +/// ICS-20 tokens coming from other chains are guaranteed to have unique names, +/// we don't need to use different salt values, thus we just use 0. +const SALT: u64 = 0u64; + +/// Maximum memo size allowed for ICS-20 transfers. This bound corresponds to +/// the `MaximumMemoLength` in the `ibc-go` +const MAXIMUM_MEMO_SIZE: usize = 32768; + /// 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, /// which must be passed to `TokenTransferValidationContext` methods through @@ -50,8 +59,8 @@ impl<'ws, S: Spec> IbcTransferContext<'ws, S> { } } - /// Stores mapping from "denom to token ID" and vice versa for an IBC-minted - /// token.s + /// Stores mapping from "denom to token ID" and vice versa for an + /// IBC-created token. fn record_minted_token(&self, token_id: TokenId, token_name: String) { self.ibc_transfer.minted_token_id_to_name.set( &token_id, @@ -89,23 +98,15 @@ impl<'ws, S: Spec> IbcTransferContext<'ws, S> { /// Creates a new token with the specified `token_name` and mints an initial /// balance to the `minter_address`. /// - /// Note: The mint authority must be held by the `IbcTransfer` module, so the - /// `authorized_minters` is set to the `IbcTransfer` address. Also, remember - /// that the `token_name` is a denom prefixed with IBC and originates from the - /// counterparty chain. + /// Note: The mint authority must be held by the `IbcTransfer` module, so + /// the `authorized_minters` is set to the `IbcTransfer` address. Also, + /// remember that the `token_name` is a denom prefixed with IBC and + /// originates from the counterparty chain. fn create_token( &self, token_name: String, minter_address: S::Address, ) -> Result { - // Using a different salt will result in a different token - // address. Since ICS-20 tokens coming from other chains are - // guaranteed to have unique names, we don't need to use - // different salt values. - let salt = 0u64; - - let initial_balance = 0; - // Make sure to use `ibc_transfer` address as the sender let context = Context::new( self.ibc_transfer.address.clone(), @@ -118,8 +119,8 @@ impl<'ws, S: Spec> IbcTransferContext<'ws, S> { .bank .create_token( token_name.clone(), - salt, - initial_balance, + SALT, + 0, minter_address, vec![self.ibc_transfer.address.clone()], &context, @@ -208,10 +209,11 @@ where coin: &PrefixedCoin, memo: &Memo, ) -> Result<(), TokenTransferError> { - if !memo.as_ref().is_empty() { - return Err(TokenTransferError::Other( - "Memo must be empty when burning tokens".to_string(), - )); + // Disallowing larges memos to prevent potential overloads on the system. + if memo.as_ref().len() > MAXIMUM_MEMO_SIZE { + return Err(TokenTransferError::Other(format!( + "Memo size exceeds maximum allowed size: {MAXIMUM_MEMO_SIZE}" + ))); } let token_name = coin.denom.to_string(); @@ -255,10 +257,11 @@ 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(), - )); + // Disallowing larges memos to prevent potential overloads on the system. + if memo.as_ref().len() > MAXIMUM_MEMO_SIZE { + return Err(TokenTransferError::Other(format!( + "Memo size exceeds maximum allowed size: {MAXIMUM_MEMO_SIZE}" + ))); } let token_id = TokenId::from_str(&coin.denom.to_string()).map_err(|e| { @@ -339,7 +342,7 @@ where { 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" + Use '{token_name}' as denom for receiving an IBC token from the source chain" ))); } diff --git a/modules/sov-ibc-transfer/src/rpc.rs b/modules/sov-ibc-transfer/src/rpc.rs index a68e0d75..81eeec9d 100644 --- a/modules/sov-ibc-transfer/src/rpc.rs +++ b/modules/sov-ibc-transfer/src/rpc.rs @@ -19,7 +19,7 @@ where S: Spec, { #[rpc_method(name = "mintedTokenName")] - pub fn escrowed_token( + pub fn minted_token_name( &self, token_id: TokenId, working_set: &mut WorkingSet, @@ -28,7 +28,7 @@ where .minted_token_id_to_name .get(&token_id, working_set) .ok_or(to_jsonrpsee_error(format!( - "No IBC-minted token found for ID: '{token_id}'" + "No IBC-created token found for ID: '{token_id}'" )))?; Ok(MintedTokenResponse { @@ -38,7 +38,7 @@ where } #[rpc_method(name = "mintedTokenId")] - pub fn minted_token( + pub fn minted_token_id( &self, token_name: String, working_set: &mut WorkingSet, @@ -47,7 +47,7 @@ where .minted_token_name_to_id .get(&token_name, working_set) .ok_or(to_jsonrpsee_error(format!( - "No IBC-minted token found for denom: '{token_name}'" + "No IBC-created token found for denom: '{token_name}'" )))?; Ok(MintedTokenResponse { From 9e16971f3073faa9086f9791a5bf30f186f72064 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Wed, 10 Apr 2024 19:28:34 -0700 Subject: [PATCH 10/10] nit --- modules/sov-ibc-transfer/src/context.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/sov-ibc-transfer/src/context.rs b/modules/sov-ibc-transfer/src/context.rs index c2897a9f..bd94c504 100644 --- a/modules/sov-ibc-transfer/src/context.rs +++ b/modules/sov-ibc-transfer/src/context.rs @@ -32,9 +32,9 @@ use crate::utils::compute_escrow_address; /// we don't need to use different salt values, thus we just use 0. const SALT: u64 = 0u64; -/// Maximum memo size allowed for ICS-20 transfers. This bound corresponds to +/// Maximum memo length allowed for ICS-20 transfers. This bound corresponds to /// the `MaximumMemoLength` in the `ibc-go` -const MAXIMUM_MEMO_SIZE: usize = 32768; +const MAXIMUM_MEMO_LENGTH: usize = 32768; /// 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, @@ -209,10 +209,10 @@ where coin: &PrefixedCoin, memo: &Memo, ) -> Result<(), TokenTransferError> { - // Disallowing larges memos to prevent potential overloads on the system. - if memo.as_ref().len() > MAXIMUM_MEMO_SIZE { + // Disallowing large memos to prevent potential overloads on the system. + if memo.as_ref().len() > MAXIMUM_MEMO_LENGTH { return Err(TokenTransferError::Other(format!( - "Memo size exceeds maximum allowed size: {MAXIMUM_MEMO_SIZE}" + "Memo size exceeds maximum allowed length: {MAXIMUM_MEMO_LENGTH}" ))); } @@ -257,10 +257,10 @@ where coin: &PrefixedCoin, memo: &Memo, ) -> Result<(), TokenTransferError> { - // Disallowing larges memos to prevent potential overloads on the system. - if memo.as_ref().len() > MAXIMUM_MEMO_SIZE { + // Disallowing large memos to prevent potential overloads on the system. + if memo.as_ref().len() > MAXIMUM_MEMO_LENGTH { return Err(TokenTransferError::Other(format!( - "Memo size exceeds maximum allowed size: {MAXIMUM_MEMO_SIZE}" + "Memo size exceeds maximum allowed length: {MAXIMUM_MEMO_LENGTH}" ))); }