diff --git a/.changelog/unreleased/improvements/ibc/1333-modules-error.md b/.changelog/unreleased/improvements/ibc/1333-modules-error.md new file mode 100644 index 0000000000..103394ce6f --- /dev/null +++ b/.changelog/unreleased/improvements/ibc/1333-modules-error.md @@ -0,0 +1,9 @@ + +- Clean up modules' errors ([#1333]) + +[#1333]: https://github.com/informalsystems/ibc-rs/issues/1333 diff --git a/modules/src/application/ics20_fungible_token_transfer/error.rs b/modules/src/application/ics20_fungible_token_transfer/error.rs index c4a98b7167..772b83fd74 100644 --- a/modules/src/application/ics20_fungible_token_transfer/error.rs +++ b/modules/src/application/ics20_fungible_token_transfer/error.rs @@ -14,14 +14,6 @@ define_error! { [ channel_error::Error ] |_ | { "Ics04 channel error" }, - SequenceSendNotFound - { port_id: PortId, channel_id: ChannelId } - | e | { format_args!("sending sequence number not found for port {0} and channel {1}", e.port_id, e.channel_id) }, - - ChannelNotFound - { port_id: PortId, channel_id: ChannelId } - | e | { format_args!("sending sequence number not found for port {0} and channel {1}", e.port_id, e.channel_id) }, - DestinationChannelNotFound { port_id: PortId, channel_id: ChannelId } | e | { format_args!("destination channel not found in the counterparty of port_id {0} and channel_id {1} ", e.port_id, e.channel_id) }, diff --git a/modules/src/ics03_connection/error.rs b/modules/src/ics03_connection/error.rs index 3fc1782e5c..d5e07c48aa 100644 --- a/modules/src/ics03_connection/error.rs +++ b/modules/src/ics03_connection/error.rs @@ -30,13 +30,6 @@ define_error! { e.connection_id) }, - UninitializedConnection - { connection_id: ConnectionId } - | e | { - format_args!("connection end for identifier {0} was never initialized", - e.connection_id) - }, - InvalidConsensusHeight { target_height: Height, @@ -120,13 +113,6 @@ define_error! { MissingCounterpartyPrefix | _ | { "missing counterparty prefix" }, - MissingClient - { client_id: ClientId } - | e | { - format_args!("the client id does not match any client state: {0}", - e.client_id) - }, - NullClientProof | _ | { "client proof must be present" }, @@ -140,16 +126,6 @@ define_error! { ConnectionVerificationFailure | _ | { "the connection proof verification failed" }, - MissingClientConsensusState - { - height: Height, - client_id: ClientId, - } - | e | { - format_args!("the consensus state at height {0} for client id {1} could not be retrieved", - e.height, e.client_id) - }, - MissingLocalConsensusState { height: Height } | e | { format_args!("the local consensus state could not be retrieved for height {}", e.height) }, diff --git a/modules/src/ics04_channel/error.rs b/modules/src/ics04_channel/error.rs index 398280f22b..82887f1e1d 100644 --- a/modules/src/ics04_channel/error.rs +++ b/modules/src/ics04_channel/error.rs @@ -1,5 +1,6 @@ use super::packet::Sequence; use crate::ics02_client::error as client_error; +use crate::ics03_connection::error as connection_error; use crate::ics04_channel::channel::State; use crate::ics24_host::error::ValidationError; use crate::ics24_host::identifier::{ChannelId, ClientId, ConnectionId, PortId}; @@ -12,6 +13,10 @@ use tendermint_proto::Error as TendermintError; define_error! { #[derive(Debug, PartialEq, Eq)] Error { + Ics03Connection + [ connection_error::Error ] + | _ | { "ics03 connection error" }, + UnknownState { state: i32 } | e | { format_args!("channel state unknown: {}", e.state) }, @@ -58,7 +63,12 @@ define_error! { | _ | { "invalid proof: missing height" }, MissingNextRecvSeq - | _ | { "Missing sequence number for receiving packets" }, + { port_channel_id: (PortId, ChannelId) } + | e | { + format_args!("Missing sequence number for receiving packets on port {0} and channel {1}", + e.port_channel_id.0, + e.port_channel_id.1) + }, ZeroPacketSequence | _ | { "packet sequence cannot be 0" }, @@ -95,14 +105,6 @@ define_error! { MissingChannel | _ | { "missing channel end" }, - MissingConnection - { connection_id: ConnectionId } - | e | { - format_args!( - "given connection hop {0} does not exist", - e.connection_id) - }, - NoPortCapability { port_id: PortId } | e | { @@ -178,17 +180,14 @@ define_error! { e.sequence) }, - MissingClientState - { client_id: ClientId } + MissingNextSendSeq + { port_channel_id: (PortId, ChannelId) } | e | { - format_args!( - "No client state associated with client id {0}", - e.client_id) + format_args!("Missing sequence number for sending packets on port {0} and channel {1}", + e.port_channel_id.0, + e.port_channel_id.1) }, - MissingNextSendSeq - | _ | { "Missing sequence number for send packets" }, - InvalidStringAsSequence { value: String } [ TraceError ] @@ -259,21 +258,11 @@ define_error! { "Client with id {0} is frozen", e.client_id) }, - MissingClientConsensusState - { client_id: ClientId, height: Height } - | e | { - format_args!( - "Missing client consensus state for client id {0} at height {1}", - e.client_id, e.height) - }, InvalidCounterpartyChannelId [ ValidationError ] | _ | { "Invalid channel id in counterparty" }, - ClientNotFound - | _ | { "Client not found in chan open verification" }, - InvalidChannelState { channel_id: ChannelId, state: State } | e | { @@ -326,7 +315,12 @@ define_error! { }, MissingNextAckSeq - | _ | { "Missing sequence number for ack packets" }, + { port_channel_id: (PortId, ChannelId) } + | e | { + format_args!("Missing sequence number for ack packets on port {0} and channel {1}", + e.port_channel_id.0, + e.port_channel_id.1) + }, ImplementationSpecific | _ | { "implementation specific error" }, diff --git a/modules/src/ics04_channel/handler/chan_open_try.rs b/modules/src/ics04_channel/handler/chan_open_try.rs index 83fe9d4d8d..5191e0efba 100644 --- a/modules/src/ics04_channel/handler/chan_open_try.rs +++ b/modules/src/ics04_channel/handler/chan_open_try.rs @@ -155,9 +155,11 @@ mod tests { use crate::events::IbcEvent; use crate::ics02_client::client_type::ClientType; + use crate::ics02_client::error as ics02_error; use crate::ics03_connection::connection::ConnectionEnd; use crate::ics03_connection::connection::Counterparty as ConnectionCounterparty; use crate::ics03_connection::connection::State as ConnectionState; + use crate::ics03_connection::error as ics03_error; use crate::ics03_connection::msgs::test_util::get_dummy_raw_counterparty; use crate::ics03_connection::version::get_compatible_versions; use crate::ics04_channel::channel::{ChannelEnd, State}; @@ -273,8 +275,13 @@ mod tests { match_error: { let connection_id = msg.channel().connection_hops()[0].clone(); Box::new(move |e| match e { - error::ErrorDetail::MissingConnection(e) => { - assert_eq!(e.connection_id, connection_id); + error::ErrorDetail::Ics03Connection(e) => { + assert_eq!( + e.source, + ics03_error::ErrorDetail::ConnectionNotFound( + ics03_error::ConnectionNotFoundSubdetail { connection_id } + ) + ); } _ => { panic!("Expected MissingConnection, instead got {}", e) @@ -363,7 +370,20 @@ mod tests { msg: ChannelMsg::ChannelOpenTry(msg.clone()), want_pass: false, match_error: Box::new(|e| match e { - error::ErrorDetail::MissingClientState(_) => {} + error::ErrorDetail::Ics03Connection(e) => { + assert_eq!( + e.source, + ics03_error::ErrorDetail::Ics02Client( + ics03_error::Ics02ClientSubdetail { + source: ics02_error::ErrorDetail::ClientNotFound( + ics02_error::ClientNotFoundSubdetail { + client_id: ClientId::new(ClientType::Mock, 45).unwrap() + } + ) + } + ) + ); + } _ => { panic!("Expected MissingClientState, instead got {}", e) } diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index 6ef0298166..449ec38931 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -461,10 +461,7 @@ impl ChannelReader for MockContext { } fn connection_end(&self, cid: &ConnectionId) -> Result { - match self.connections.get(cid) { - Some(connection_end) => Ok(connection_end.clone()), - None => Err(Ics04Error::missing_connection(cid.clone())), - } + ConnectionReader::connection_end(self, cid).map_err(Ics04Error::ics03_connection) } fn connection_channels( @@ -479,7 +476,7 @@ impl ChannelReader for MockContext { fn client_state(&self, client_id: &ClientId) -> Result { ClientReader::client_state(self, client_id) - .map_err(|_| Ics04Error::missing_client_state(client_id.clone())) + .map_err(|e| Ics04Error::ics03_connection(Ics03Error::ics02_client(e))) } fn client_consensus_state( @@ -488,7 +485,7 @@ impl ChannelReader for MockContext { height: Height, ) -> Result { ClientReader::consensus_state(self, client_id, height) - .map_err(|_| Ics04Error::missing_client_consensus_state(client_id.clone(), height)) + .map_err(|e| Ics04Error::ics03_connection(Ics03Error::ics02_client(e))) } fn authenticated_capability(&self, port_id: &PortId) -> Result { @@ -513,7 +510,7 @@ impl ChannelReader for MockContext { ) -> Result { match self.next_sequence_send.get(port_channel_id) { Some(sequence) => Ok(*sequence), - None => Err(Ics04Error::missing_next_send_seq()), + None => Err(Ics04Error::missing_next_send_seq(port_channel_id.clone())), } } @@ -523,7 +520,7 @@ impl ChannelReader for MockContext { ) -> Result { match self.next_sequence_recv.get(port_channel_id) { Some(sequence) => Ok(*sequence), - None => Err(Ics04Error::missing_next_recv_seq()), + None => Err(Ics04Error::missing_next_recv_seq(port_channel_id.clone())), } } @@ -533,7 +530,7 @@ impl ChannelReader for MockContext { ) -> Result { match self.next_sequence_ack.get(port_channel_id) { Some(sequence) => Ok(*sequence), - None => Err(Ics04Error::missing_next_ack_seq()), + None => Err(Ics04Error::missing_next_ack_seq(port_channel_id.clone())), } } @@ -698,8 +695,7 @@ impl ConnectionReader for MockContext { fn client_state(&self, client_id: &ClientId) -> Result { // Forward method call to the Ics2 Client-specific method. - ClientReader::client_state(self, client_id) - .map_err(|_| Ics03Error::missing_client(client_id.clone())) + ClientReader::client_state(self, client_id).map_err(Ics03Error::ics02_client) } fn host_current_height(&self) -> Height { @@ -722,7 +718,7 @@ impl ConnectionReader for MockContext { ) -> Result { // Forward method call to the Ics2Client-specific method. self.consensus_state(client_id, height) - .map_err(|_| Ics03Error::missing_client_consensus_state(height, client_id.clone())) + .map_err(Ics03Error::ics02_client) } fn host_consensus_state(&self, height: Height) -> Result { diff --git a/modules/tests/runner/mod.rs b/modules/tests/runner/mod.rs index 4d082dd27c..7be0a3f2ca 100644 --- a/modules/tests/runner/mod.rs +++ b/modules/tests/runner/mod.rs @@ -480,6 +480,10 @@ impl modelator::step_runner::StepRunner for IbcTestRunner { Self::extract_ics02_error_kind(result), client_error::ErrorDetail::ClientNotFound(_) ), + ActionOutcome::Ics02ConsensusStateNotFound => matches!( + Self::extract_ics02_error_kind(result), + client_error::ErrorDetail::ConsensusStateNotFound(_) + ), ActionOutcome::Ics02HeaderVerificationFailure => { matches!( Self::extract_ics02_error_kind(result), @@ -498,10 +502,6 @@ impl modelator::step_runner::StepRunner for IbcTestRunner { ) } ActionOutcome::Ics03ConnectionOpenInitOk => result.is_ok(), - ActionOutcome::Ics03MissingClient => matches!( - Self::extract_ics03_error_kind(result), - connection_error::ErrorDetail::MissingClient(_) - ), ActionOutcome::Ics03ConnectionOpenTryOk => result.is_ok(), ActionOutcome::Ics03InvalidConsensusHeight => matches!( Self::extract_ics03_error_kind(result), @@ -515,19 +515,11 @@ impl modelator::step_runner::StepRunner for IbcTestRunner { Self::extract_ics03_error_kind(result), connection_error::ErrorDetail::ConnectionMismatch(_) ), - ActionOutcome::Ics03MissingClientConsensusState => matches!( - Self::extract_ics03_error_kind(result), - connection_error::ErrorDetail::MissingClientConsensusState(_) - ), ActionOutcome::Ics03InvalidProof => matches!( Self::extract_ics03_error_kind(result), connection_error::ErrorDetail::InvalidProof(_) ), ActionOutcome::Ics03ConnectionOpenAckOk => result.is_ok(), - ActionOutcome::Ics03UninitializedConnection => matches!( - Self::extract_ics03_error_kind(result), - connection_error::ErrorDetail::UninitializedConnection(_) - ), ActionOutcome::Ics03ConnectionOpenConfirmOk => result.is_ok(), }; diff --git a/modules/tests/runner/step.rs b/modules/tests/runner/step.rs index 408a7d32b4..e156ffda88 100644 --- a/modules/tests/runner/step.rs +++ b/modules/tests/runner/step.rs @@ -124,6 +124,7 @@ pub enum ActionOutcome { Ics02CreateOk, Ics02UpdateOk, Ics02ClientNotFound, + Ics02ConsensusStateNotFound, Ics02HeaderVerificationFailure, Ics07UpgradeOk, @@ -131,15 +132,12 @@ pub enum ActionOutcome { Ics07HeaderVerificationFailure, Ics03ConnectionOpenInitOk, - Ics03MissingClient, Ics03ConnectionOpenTryOk, Ics03InvalidConsensusHeight, Ics03ConnectionNotFound, Ics03ConnectionMismatch, - Ics03MissingClientConsensusState, Ics03InvalidProof, Ics03ConnectionOpenAckOk, - Ics03UninitializedConnection, Ics03ConnectionOpenConfirmOk, } diff --git a/modules/tests/support/model_based/IBC.tla b/modules/tests/support/model_based/IBC.tla index 04e5e30189..e1d276f5ba 100644 --- a/modules/tests/support/model_based/IBC.tla +++ b/modules/tests/support/model_based/IBC.tla @@ -136,13 +136,11 @@ ActionOutcomes == { "Ics07HeaderVerificationFailure", \* ICS03_ConnectionOpenInit outcomes: "Ics03ConnectionOpenInitOk", - "Ics03MissingClient", \* ICS03_ConnectionOpenTry outcomes: "Ics03ConnectionOpenTryOk", "Ics03InvalidConsensusHeight", "Ics03ConnectionNotFound", "Ics03ConnectionMismatch", - "Ics03MissingClientConsensusState", "Ics03InvalidProof", \* ICS03_ConnectionOpenAck outcomes: "Ics03ConnectionOpenAckOk", diff --git a/modules/tests/support/model_based/IBCTests.tla b/modules/tests/support/model_based/IBCTests.tla index fd52e4957c..a8f0285fcb 100644 --- a/modules/tests/support/model_based/IBCTests.tla +++ b/modules/tests/support/model_based/IBCTests.tla @@ -31,9 +31,6 @@ ICS07HeaderVerificationFailureTest == ICS03ConnectionOpenInitOKTest == /\ actionOutcome = "Ics03ConnectionOpenInitOk" -ICS03MissingClientTest == - /\ actionOutcome = "Ics03MissingClient" - \* ICS03ConnectionOpenTry tests ICS03ConnectionOpenTryOKTest == /\ actionOutcome = "Ics03ConnectionOpenTryOk" @@ -47,9 +44,6 @@ ICS03ConnectionNotFoundTest == ICS03ConnectionMismatchTest == /\ actionOutcome = "Ics03ConnectionMismatch" -ICS03MissingClientConsensusStateTest == - /\ actionOutcome = "Ics03MissingClientConsensusState" - \* TODO: the following test should fail but doesn't because proofs are not yet \* verified in the implementation \* ICS03InvalidProofTest == diff --git a/modules/tests/support/model_based/ICS03.tla b/modules/tests/support/model_based/ICS03.tla index 05276dd631..cf8496f1bd 100644 --- a/modules/tests/support/model_based/ICS03.tla +++ b/modules/tests/support/model_based/ICS03.tla @@ -35,7 +35,7 @@ ICS03_ConnectionOpenInit( connections |-> chain.connections, connectionIdCounter |-> chain.connectionIdCounter, action |-> action_, - outcome |-> "Ics03MissingClient" + outcome |-> "Ics02ClientNotFound" ] ELSE \* if the client exists, @@ -170,7 +170,7 @@ ICS03_ConnectionOpenTry( connections |-> chain.connections, connectionIdCounter |-> chain.connectionIdCounter, action |-> action_, - outcome |-> "Ics03MissingClient" + outcome |-> "Ics02ClientNotFound" ] ELSE \* check if the client has a consensus state with this height @@ -183,7 +183,7 @@ ICS03_ConnectionOpenTry( connections |-> chain.connections, connectionIdCounter |-> chain.connectionIdCounter, action |-> action_, - outcome |-> "Ics03MissingClientConsensusState" + outcome |-> "Ics02ConsensusStateNotFound" ] ELSE \* check if there was an open init at the remote chain @@ -296,7 +296,7 @@ ICS03_ConnectionOpenAck( [ connections |-> connections, action |-> action_, - outcome |-> "Ics03MissingClientConsensusState" + outcome |-> "Ics02ConsensusStateNotFound" ] ELSE \* check if there was an open try at the remote chain @@ -385,7 +385,7 @@ ICS03_ConnectionOpenConfirm( [ connections |-> connections, action |-> action_, - outcome |-> "Ics03MissingClientConsensusState" + outcome |-> "Ics02ConsensusStateNotFound" ] ELSE \* check if there was an open ack at the remote chain