modules: Clean up modules' errors (#1334)

* clean up redundant errors

* add a log

* remove ics02_error from ics04_error

* fix a test to check the correct error

.changelog/unreleased/improvements/ibc/1333-modules-error.md

@@ -0,0 +1,9 @@
+<!--
+    Add your entry's details here (in Markdown format).
+
+    If you don't change this message, or if this file is empty, the entry will
+    not be created.
+-->
+- Clean up modules' errors ([#1333])
+
+[#1333]: https://github.com/informalsystems/ibc-rs/issues/1333

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) },

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) },

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<std::num::ParseIntError> ]
@@ -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" },

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)
                     }

modules/src/mock/context.rs

@@ -461,10 +461,7 @@ impl ChannelReader for MockContext {
     }
 
     fn connection_end(&self, cid: &ConnectionId) -> Result<ConnectionEnd, Ics04Error> {
-        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<AnyClientState, Ics04Error> {
         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<AnyConsensusState, Ics04Error> {
         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<Capability, Ics04Error> {
@@ -513,7 +510,7 @@ impl ChannelReader for MockContext {
     ) -> Result<Sequence, Ics04Error> {
         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<Sequence, Ics04Error> {
         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<Sequence, Ics04Error> {
         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<AnyClientState, Ics03Error> {
         // 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<AnyConsensusState, Ics03Error> {
         // 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<AnyConsensusState, Ics03Error> {

modules/tests/runner/mod.rs

@@ -480,6 +480,10 @@ impl modelator::step_runner::StepRunner<Step> 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<Step> 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<Step> 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(),
         };
 

modules/tests/runner/step.rs

@@ -124,22 +124,20 @@ pub enum ActionOutcome {
     Ics02CreateOk,
     Ics02UpdateOk,
     Ics02ClientNotFound,
+    Ics02ConsensusStateNotFound,
     Ics02HeaderVerificationFailure,
 
     Ics07UpgradeOk,
     Ics07ClientNotFound,
     Ics07HeaderVerificationFailure,
 
     Ics03ConnectionOpenInitOk,
-    Ics03MissingClient,
     Ics03ConnectionOpenTryOk,
     Ics03InvalidConsensusHeight,
     Ics03ConnectionNotFound,
     Ics03ConnectionMismatch,
-    Ics03MissingClientConsensusState,
     Ics03InvalidProof,
     Ics03ConnectionOpenAckOk,
-    Ics03UninitializedConnection,
     Ics03ConnectionOpenConfirmOk,
 }
 

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",