From 28f649f6477e002fea99d089afc03462f8c829f3 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Fri, 19 Apr 2024 17:38:06 +0200 Subject: [PATCH] Cleanup error enum (#490) wip --- light-clients/cf-guest-cw/src/context.rs | 45 +++++++------- light-clients/cf-guest-cw/src/contract.rs | 73 +++++++---------------- light-clients/cf-guest-cw/src/error.rs | 40 ++++--------- light-clients/cf-guest-cw/src/msg.rs | 4 +- light-clients/cf-guest/src/error.rs | 16 ++--- 5 files changed, 60 insertions(+), 118 deletions(-) diff --git a/light-clients/cf-guest-cw/src/context.rs b/light-clients/cf-guest-cw/src/context.rs index b09159058..dbd4c2ecf 100644 --- a/light-clients/cf-guest-cw/src/context.rs +++ b/light-clients/cf-guest-cw/src/context.rs @@ -73,22 +73,19 @@ impl<'a> Context<'a> { impl<'a> Context<'a> { pub fn processed_timestamp(&self, height: Height) -> Result { - let processed_state = ReadonlyProcessedStates::new(self.storage()); - match processed_state.get_processed_time(height, &mut Vec::new()) { - Some(time) => Ok(time), - None => Err(Error::implementation_specific( - "problem getting processed timestamp".to_string(), - )), - } + ReadonlyProcessedStates::new(self.storage()) + .get_processed_time(height, &mut Vec::new()) + .ok_or_else(|| { + Error::implementation_specific("problem getting processed timestamp".into()) + }) } pub fn processed_height(&self, height: Height) -> Result { - let processed_state = ReadonlyProcessedStates::new(self.storage()); - match processed_state.get_processed_height(height, &mut Vec::new()) { - Some(p_height) => Ok(p_height), - None => - Err(Error::implementation_specific("problem getting processed height".to_string())), - } + ReadonlyProcessedStates::new(self.storage()) + .get_processed_height(height, &mut Vec::new()) + .ok_or_else(|| { + Error::implementation_specific("problem getting processed height".into()) + }) } pub fn consensus_state_prefixed( @@ -99,13 +96,12 @@ impl<'a> Context<'a> { let bytes = ReadonlyConsensusStates::new(self.storage()) .get_prefixed(height, prefix) .ok_or_else(|| { - ContractError::Tendermint(format!( + ContractError::Other(format!( "no consensus state found for height {height} and prefix {prefix:?}", )) })?; - Context::decode_consensus_state(&bytes).map_err(|e| { - ContractError::Tendermint(format!("error decoding consensus state: {e:?}")) - }) + Context::decode_consensus_state(&bytes) + .map_err(|e| ContractError::Other(format!("error decoding consensus state: {e:?}"))) } pub fn store_consensus_state_prefixed( @@ -125,10 +121,10 @@ impl<'a> Context<'a> { ) -> Result, ContractError> { let bytes = ReadonlyClientStates::new(self.storage()).get_prefixed(prefix).ok_or_else(|| { - ContractError::Tendermint(format!("no client state found for prefix {prefix:?}",)) + ContractError::Other(format!("no client state found for prefix {prefix:?}",)) })?; Context::decode_client_state(&bytes) - .map_err(|e| ContractError::Tendermint(format!("error decoding client state: {e:?}"))) + .map_err(|e| ContractError::Other(format!("error decoding client state: {e:?}"))) } pub fn store_client_state_prefixed( @@ -137,12 +133,11 @@ impl<'a> Context<'a> { prefix: &[u8], ) -> Result<(), ContractError> { let client_states = ReadonlyClientStates::new(self.storage()); - let data = client_states.get_prefixed(prefix).ok_or_else(|| { - ContractError::Tendermint("no client state found for prefix".to_string()) - })?; - let encoded = Context::encode_client_state(client_state, data).map_err(|e| { - ContractError::Tendermint(format!("error encoding client state: {e:?}")) - })?; + let data = client_states + .get_prefixed(prefix) + .ok_or_else(|| ContractError::Other("no client state found for prefix".to_string()))?; + let encoded = Context::encode_client_state(client_state, data) + .map_err(|e| ContractError::Other(format!("error encoding client state: {e:?}")))?; let mut client_states = ClientStates::new(self.storage_mut()); client_states.insert_prefixed(encoded, prefix); Ok(()) diff --git a/light-clients/cf-guest-cw/src/contract.rs b/light-clients/cf-guest-cw/src/contract.rs index ffcf1c1eb..38b23991e 100644 --- a/light-clients/cf-guest-cw/src/contract.rs +++ b/light-clients/cf-guest-cw/src/contract.rs @@ -53,14 +53,10 @@ pub fn instantiate( let _client = GuestClient::::default(); let mut ctx = Context::new(deps, env); let client_id = ClientId::from_str("08-wasm-0").expect("client id is valid"); - let client_state = ctx - .client_state(&client_id) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + let client_state = ctx.client_state(&client_id)?; let latest_height = client_state.latest_height(); - ctx.store_update_height(client_id.clone(), latest_height, ctx.host_height()) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; - ctx.store_update_time(client_id, latest_height, ctx.host_timestamp()) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + ctx.store_update_height(client_id.clone(), latest_height, ctx.host_height())?; + ctx.store_update_time(client_id, latest_height, ctx.host_timestamp())?; Ok(Response::default()) } @@ -90,20 +86,15 @@ fn process_message( //log!(ctx, "process_message: {:?}", msg); let result = match msg { ExecuteMsg::VerifyMembership(msg) => { - let client_state = ctx - .client_state(&client_id) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + let client_state = ctx.client_state(&client_id)?; let msg = VerifyMembershipMsg::try_from(msg)?; crate::helpers::verify_delay_passed( ctx, msg.height, msg.delay_time_period, msg.delay_block_period, - ) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; - let consensus_state = ctx - .consensus_state(&client_id, msg.height) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + )?; + let consensus_state = ctx.consensus_state(&client_id, msg.height)?; // TODO(blas) verify( &CommitmentPrefix::default(), @@ -111,25 +102,19 @@ fn process_message( &consensus_state.root(), msg.path, Some(msg.value.as_ref()), - ) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + )?; Ok(()).map(|_| to_binary(&ContractResult::success())) }, ExecuteMsg::VerifyNonMembership(msg) => { - let client_state = ctx - .client_state(&client_id) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + let client_state = ctx.client_state(&client_id)?; let msg = VerifyNonMembershipMsg::try_from(msg)?; crate::helpers::verify_delay_passed( ctx, msg.height, msg.delay_time_period, msg.delay_block_period, - ) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; - let consensus_state = ctx - .consensus_state(&client_id, msg.height) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + )?; + let consensus_state = ctx.consensus_state(&client_id, msg.height)?; verify( &CommitmentPrefix::default(), @@ -138,47 +123,38 @@ fn process_message( msg.path, None, ) - .map_err(|e| ContractError::Tendermint(e.to_string())) + .map_err(ContractError::from) .map(|_| to_binary(&ContractResult::success())) }, ExecuteMsg::VerifyClientMessage(msg) => { - let client_state = ctx - .client_state(&client_id) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + let client_state = ctx.client_state(&client_id)?; let msg = VerifyClientMessage::try_from(msg)?; client .verify_client_message(ctx, client_id, client_state, msg.client_message) - .map_err(|e| ContractError::Tendermint(format!("{e:?}"))) + .map_err(ContractError::from) .map(|_| to_binary(&ContractResult::success())) }, ExecuteMsg::CheckForMisbehaviour(msg) => { - let client_state = ctx - .client_state(&client_id) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + let client_state = ctx.client_state(&client_id)?; let msg = CheckForMisbehaviourMsg::try_from(msg)?; client .check_for_misbehaviour(ctx, client_id, client_state, msg.client_message) - .map_err(|e| ContractError::Tendermint(e.to_string())) + .map_err(ContractError::from) .map(|result| to_binary(&ContractResult::success().misbehaviour(result))) }, ExecuteMsg::UpdateStateOnMisbehaviour(msg_raw) => { - let client_state = ctx - .client_state(&client_id) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + let client_state = ctx.client_state(&client_id)?; let msg = UpdateStateOnMisbehaviourMsg::try_from(msg_raw)?; client .update_state_on_misbehaviour(client_state, msg.client_message) - .map_err(|e| ContractError::Tendermint(e.to_string())) + .map_err(ContractError::from) .and_then(|cs| { - ctx.store_client_state(client_id, cs) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + ctx.store_client_state(client_id, cs)?; Ok(to_binary(&ContractResult::success())) }) }, ExecuteMsg::UpdateState(msg_raw) => { - let client_state = ctx - .client_state(&client_id) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + let client_state = ctx.client_state(&client_id)?; let msg = UpdateStateMsg::try_from(msg_raw)?; let latest_revision_height = client_state.latest_height().revision_height; helpers::prune_oldest_consensus_state( @@ -188,23 +164,20 @@ fn process_message( ); client .update_state(ctx, client_id.clone(), client_state, msg.client_message) - .map_err(|e| ContractError::Tendermint(e.to_string())) + .map_err(ContractError::from) .and_then(|(cs, cu)| { let height = cs.latest_height(); match cu { ConsensusUpdateResult::Single(cs) => { - ctx.store_consensus_state(client_id.clone(), height, cs) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + ctx.store_consensus_state(client_id.clone(), height, cs)?; }, ConsensusUpdateResult::Batch(css) => for (height, cs) in css { - ctx.store_consensus_state(client_id.clone(), height, cs) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + ctx.store_consensus_state(client_id.clone(), height, cs)?; }, } if u64::from(cs.0.latest_height) > latest_revision_height { - ctx.store_client_state(client_id, cs) - .map_err(|e| ContractError::Tendermint(e.to_string()))?; + ctx.store_client_state(client_id, cs)?; } Ok(to_binary(&ContractResult::success())) }) diff --git a/light-clients/cf-guest-cw/src/error.rs b/light-clients/cf-guest-cw/src/error.rs index fd3779b91..b23dc3328 100644 --- a/light-clients/cf-guest-cw/src/error.rs +++ b/light-clients/cf-guest-cw/src/error.rs @@ -13,44 +13,24 @@ // See the License for the specific language governing permissions and // limitations under the License. -use cf_guest::error::Error as CfGuestError; use cosmwasm_std::StdError; -use derive_more::{Display, From}; use std::error::Error; -#[derive(From, Display, Debug)] +#[derive(derive_more::From, derive_more::Display, Debug)] pub enum ContractError { Std(StdError), - #[display(fmt = "Unauthorized")] - Unauthorized {}, - // Add any other custom errors you like here. - // Look at https://docs.rs/thiserror/1.0.21/thiserror/ for details. - #[display(fmt = "Storage error")] - StorageError, - // TODO: use `ics07-tendermint`'s error type here - #[display(fmt = "Tendermint error: {_0}")] - #[from(ignore)] - Tendermint(String), - #[display(fmt = "Protobuf error: {_0}")] - Protobuf(ibc::protobuf::Error), - #[display(fmt = "IBC validation error: {_0}")] - Validation(ibc::core::ics24_host::error::ValidationError), - #[display(fmt = "IBC path error: {_0}")] + + ProofVerification(cf_guest::proof::VerifyError), + + Client(ibc::core::ics02_client::error::Error), Path(ibc::core::ics24_host::path::PathError), - #[display(fmt = "IBC proof error: {_0}")] Proof(ibc::proofs::ProofError), - #[display(fmt = "IBC commitment error: {_0}")] Commitment(ibc::core::ics23_commitment::error::Error), - #[display(fmt = "Proto decode error: {_0}")] - ProtoDecode(prost::DecodeError), - #[display(fmt = "From UTF8 error: {_0}")] - FromUtf8(alloc::string::FromUtf8Error), -} + Protobuf(ibc::protobuf::Error), -impl Error for ContractError {} + Prost(prost::DecodeError), -impl From for ContractError { - fn from(e: CfGuestError) -> Self { - ContractError::Tendermint(e.to_string()) - } + Other(String), } + +impl Error for ContractError {} diff --git a/light-clients/cf-guest-cw/src/msg.rs b/light-clients/cf-guest-cw/src/msg.rs index cd52bcde4..373450870 100644 --- a/light-clients/cf-guest-cw/src/msg.rs +++ b/light-clients/cf-guest-cw/src/msg.rs @@ -358,9 +358,7 @@ impl TryFrom for VerifyUpgradeAndUpdateStateM let upgrade_client_state: ClientState = ClientState::decode_vec(&any.value)?; if upgrade_client_state.0.is_frozen { - return ibc::prelude::Err(ContractError::Tendermint( - "Upgrade client state not zeroed".to_string(), - )) + return Err(ContractError::Other("Upgrade client state not zeroed".into())) } Ok(VerifyUpgradeAndUpdateStateMsg { diff --git a/light-clients/cf-guest/src/error.rs b/light-clients/cf-guest/src/error.rs index 699595f11..7c6cee943 100644 --- a/light-clients/cf-guest/src/error.rs +++ b/light-clients/cf-guest/src/error.rs @@ -17,11 +17,7 @@ use alloc::{ fmt, string::{String, ToString}, }; -use ibc::{ - core::{ics02_client::error::Error as Ics02Error, ics24_host::identifier::ClientId}, - timestamp::Timestamp, - Height, -}; +use ibc::{core::ics24_host::identifier::ClientId, timestamp::Timestamp, Height}; #[derive(Clone, Debug)] pub enum Error { @@ -35,13 +31,13 @@ pub enum Error { } impl fmt::Display for Error { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{:?}", self) + fn fmt(&self, fmtr: &mut fmt::Formatter) -> fmt::Result { + fmt::Debug::fmt(self, fmtr) } } -impl From for Ics02Error { - fn from(e: Error) -> Self { - Ics02Error::client_error(CLIENT_TYPE.to_string(), e.to_string()) +impl From for ibc::core::ics02_client::error::Error { + fn from(err: Error) -> Self { + Self::client_error(CLIENT_TYPE.into(), err.to_string()) } }