From 1d1416b669cf9dd9d1bb5cf263565ab651d78514 Mon Sep 17 00:00:00 2001 From: srdtrk Date: Tue, 9 Jul 2024 18:51:38 +0800 Subject: [PATCH] refactor: cleaned up some code --- contracts/ics02-client/src/contract.rs | 10 ++--- contracts/ics26-router/src/contract.rs | 44 +++++++------------ .../chainconfig/chain_config.go | 4 +- packages/shared/src/types/ibc.rs | 43 ++++++++++++++++++ 4 files changed, 64 insertions(+), 37 deletions(-) diff --git a/contracts/ics02-client/src/contract.rs b/contracts/ics02-client/src/contract.rs index b0455c0..093bb69 100644 --- a/contracts/ics02-client/src/contract.rs +++ b/contracts/ics02-client/src/contract.rs @@ -32,7 +32,6 @@ pub fn instantiate( /// /// # Errors /// Will return an error if the handler returns an error. -#[allow(clippy::needless_pass_by_value)] #[cosmwasm_std::entry_point] pub fn execute( deps: DepsMut, @@ -64,11 +63,10 @@ pub fn execute( /// /// # Errors /// Will return an error if the handler returns an error. -#[allow(clippy::needless_pass_by_value)] #[cosmwasm_std::entry_point] -pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> Result { +pub fn query(deps: Deps, env: Env, msg: QueryMsg) -> Result { match msg { - QueryMsg::ClientInfo { client_id } => query::client_info(deps, client_id), + QueryMsg::ClientInfo { client_id } => query::client_info(deps, env, client_id), } } @@ -184,13 +182,13 @@ mod execute { } mod query { - use super::{state, Binary, ContractError, Deps}; + use super::{state, Binary, ContractError, Deps, Env}; use crate::types::msg::query_responses; /// Returns the address of the client encoded as a JSON binary. #[allow(clippy::needless_pass_by_value)] - pub fn client_info(deps: Deps, client_id: String) -> Result { + pub fn client_info(deps: Deps, _env: Env, client_id: String) -> Result { let address = state::CLIENTS.load(deps.storage, &client_id)?; let counterparty_info = state::COUNTERPARTY.may_load(deps.storage, &client_id)?; let creator = state::CREATORS.load(deps.storage, &client_id)?; diff --git a/contracts/ics26-router/src/contract.rs b/contracts/ics26-router/src/contract.rs index c796c8d..0d4f08e 100644 --- a/contracts/ics26-router/src/contract.rs +++ b/contracts/ics26-router/src/contract.rs @@ -33,7 +33,6 @@ pub fn instantiate( &deps.querier, &env, ics02_client::types::msg::InstantiateMsg {}, - // TODO: ensure there is no DOS attack vector here format!("{}.{}", keys::ICS02_CLIENT_SALT, env.contract.address), None::, keys::ICS02_CLIENT_SALT, @@ -74,7 +73,6 @@ pub fn execute( #[allow(clippy::needless_pass_by_value)] pub fn reply(deps: DepsMut, env: Env, msg: Reply) -> Result { match msg.id { - // TODO: Ensure that events are emitted for all the replies. keys::reply::ON_RECV_PACKET => { reply::write_acknowledgement(deps, env, msg.result, msg.payload) } @@ -149,34 +147,22 @@ mod execute { } } - let source_channel = identifiers::ChannelId::from_str(&msg.source_channel)?; - let source_port = identifiers::PortId::from_str(&msg.source_port)?; - let dest_port = identifiers::PortId::from_str(&msg.dest_port)?; - let dest_channel = identifiers::ChannelId::from_str( - msg.dest_channel.as_ref().unwrap_or(&counterparty_id), - )?; - // Ensure the timeout is valid. utils::timeout::validate(&env, &msg.timeout)?; // Construct the packet. - let sequence: identifiers::Sequence = state::helpers::new_sequence_send( - deps.storage, - source_port.as_str(), - source_channel.as_str(), - )? - .into(); - let packet = ibc::Packet { + let sequence = + state::helpers::new_sequence_send(deps.storage, &msg.source_port, &msg.source_channel)?; + let packet = ibc::Packet::new( sequence, - source_channel, - source_port, - destination_channel: dest_channel, - destination_port: dest_port, - data: msg.data, - timeout: msg.timeout, - }; + &msg.source_channel, + &msg.source_port, + &counterparty_id, + &msg.dest_port, + msg.data, + msg.timeout, + )?; - // TODO: Ensure it is ok to commit packet and emit events before the callback. state::helpers::commit_packet(deps.storage, &packet)?; let send_packet_event = events::send_packet::success(&packet); @@ -200,9 +186,8 @@ mod execute { info: MessageInfo, msg: RecvPacketMsg, ) -> Result { + msg.packet.validate()?; let packet = msg.packet; - let proof_commitment = msg.proof_commitment; - let proof_height = msg.proof_height; let ics02_address = state::ICS02_CLIENT_ADDRESS.load(deps.storage)?; let ics02_contract = ics02_client::helpers::Ics02ClientContract::new(ics02_address); @@ -232,10 +217,10 @@ mod execute { } .to_prefixed_merkle_path(counterparty.merkle_path_prefix)?; let verify_membership_msg = VerifyMembershipMsgRaw { - proof: proof_commitment.into(), + proof: msg.proof_commitment.into(), path: counterparty_commitment_path, value: packet.to_commitment_vec(), - height: proof_height.into(), + height: msg.proof_height.into(), delay_time_period: 0, delay_block_period: 0, }; @@ -259,7 +244,6 @@ mod execute { ) .with_payload(reply_payload); - // TODO: Ensure event emission is reverted if the callback fails. Ok(Response::new() .add_submessage(recv_packet_callback) .add_event(event)) @@ -272,6 +256,7 @@ mod execute { info: MessageInfo, msg: AcknowledgementMsg, ) -> Result { + msg.packet.validate()?; let packet = msg.packet; let ics02_address = state::ICS02_CLIENT_ADDRESS.load(deps.storage)?; @@ -350,6 +335,7 @@ mod execute { info: MessageInfo, msg: TimeoutMsg, ) -> Result { + msg.packet.validate()?; let packet = msg.packet; let ics02_address = state::ICS02_CLIENT_ADDRESS.load(deps.storage)?; diff --git a/e2e/interchaintestv8/chainconfig/chain_config.go b/e2e/interchaintestv8/chainconfig/chain_config.go index 1561f62..183c186 100644 --- a/e2e/interchaintestv8/chainconfig/chain_config.go +++ b/e2e/interchaintestv8/chainconfig/chain_config.go @@ -38,8 +38,8 @@ var DefaultChainSpecs = []*interchaintest.ChainSpec{ ChainID: "simd-1", Images: []ibc.DockerImage{ { - Repository: "ghcr.io/cosmos/ibc-go-simd", // FOR LOCAL IMAGE USE: Docker Image Name - Version: "serdar-xxx-ibc-lite-e2e-debug", // FOR LOCAL IMAGE USE: Docker Image Tag + Repository: "ghcr.io/cosmos/ibc-go-simd", // FOR LOCAL IMAGE USE: Docker Image Name + Version: "ibc-lite", // FOR LOCAL IMAGE USE: Docker Image Tag UidGid: "1025:1025", }, }, diff --git a/packages/shared/src/types/ibc.rs b/packages/shared/src/types/ibc.rs index df11670..92374cd 100644 --- a/packages/shared/src/types/ibc.rs +++ b/packages/shared/src/types/ibc.rs @@ -1,7 +1,10 @@ //! Defines the IBC types used by the contract. +use std::str::FromStr; + use cosmwasm_schema::cw_serde; use cosmwasm_std::{Binary, IbcTimeout}; +use ibc_core_host::types::error::IdentifierError; use sha2::Digest; use super::{error::ContractError, paths::identifiers}; @@ -57,6 +60,46 @@ pub struct Height { // } impl Packet { + /// Creates a new validated packet with the given parameters. + /// + /// # Errors + /// Fails if any of the identifiers are invalid. + pub fn new( + sequence: impl Into, + source_port: &str, + source_channel: &str, + destination_port: &str, + destination_channel: &str, + data: Binary, + timeout: IbcTimeout, + ) -> Result { + let source_port = identifiers::PortId::from_str(source_port)?; + let source_channel = identifiers::ChannelId::from_str(source_channel)?; + let destination_port = identifiers::PortId::from_str(destination_port)?; + let destination_channel = identifiers::ChannelId::from_str(destination_channel)?; + Ok(Self { + sequence: sequence.into(), + source_port, + source_channel, + destination_port, + destination_channel, + data, + timeout, + }) + } + + /// Validates the packet identifiers. + /// + /// # Errors + /// Fails if any of the identifiers are invalid. + pub fn validate(&self) -> Result<(), IdentifierError> { + self.source_port.validate()?; + self.destination_port.validate()?; + identifiers::ChannelId::from_str(self.source_channel.as_str())?; + identifiers::ChannelId::from_str(self.destination_channel.as_str())?; + Ok(()) + } + /// `to_commitment_bytes` serializes the packet to commitment bytes as per [ibc-lite go implementation](https://github.com/cosmos/ibc-go/blob/2b40562bcd59ce820ddd7d6732940728487cf94e/modules/core/04-channel/types/packet.go#L38) #[must_use] pub fn to_commitment_vec(&self) -> Vec {