Generalize and consolidate duplicated error variants

[seanchen1991]

Feature Summary

Duplicated error variants scattered across the ibc-rs codebase should be consolidated as much as possible. These should then live in a more centralized location in the repository, depending on how general they are and on how many ibc-rs crates make use of them.

Proposal

One concrete example of this is mentioned in this comment. All error variants that have to do with surfacing decoding errors can be consolidated under one DecodingError type that would live in the ibc-primitives crate:

pub enum DecodingError {
    /// invalid identifier: `{0}`
    InvalidIdentifier(IdentifierError),
    /// invalid field: `{0}`
    InvalidFeild(String)
    /// missing field: `{0}`
    MissingFeild(String)
    /// mismatched type URLs: expected `{expected}`, actual `{actual}`
    MismatchedTypeUrls { expected: String, actual: String },
     /// failed to decode with error: `{description}`
    FailedtoDecode { description: String },

    // ... any other relevant variants.
}

Tasks

Beta Give feedback

1. Consolidate decoding-related errors into new DecodingError type #1325
   +0

[seanchen1991]

@Farhad-Shabani I just realized that if this DecodingError type is going to belong in ibc-primitives, the IdentiferError variant can't contain an IdentifierError type, since that would then introduce a dependency on ics24-host. Would it make sense to move the IdentifierError type into ibc-primitives?

[Farhad-Shabani]

I just realized that if this DecodingError type is going to belong in ibc-primitives, the IdentiferError variant can't contain an IdentifierError type, since that would then introduce a dependency on ics24-host. Would it make sense to move the IdentifierError type into ibc-primitives?

Since DecodingError and IdentifierError mostly occur during Protobuf deserialization, it makes sense to keep them together. Though, I'm more inclined to place both under the ics24-host. After we recently refactored the Timestamp and differentiated the TimeoutTimestamp placing it under ics-04, the codebase under ibc-primitives now appears would be more suitable for merging into ics-24 in the future.

[seanchen1991]

the codebase under ibc-primitives now appears would be more suitable for merging into ics-24 in the future

So ics24 will become the foundational dependency that other ibc-rs workspaces / crates rely upon?

[Farhad-Shabani]

So ics24 will become the foundational dependency that other ibc-rs workspaces / crates rely upon?

Exactly. It seems like an unnecessary breakdown of the codebase, especially since there's no clear definition of what should be placed under the ibc-primitives. When looking at the existing codebase, the components there, like Signer, Timestamp, serialization, and conversions, appear to be more host-related utilities for interacting with the host. We should definitely investigate and make a decision on this later as a separate issue.

[seanchen1991]

Is the idea then that IdentifierErrors will be subsumed by DecodingError? I.e., every current instance of an IdentifierError will become a DecodingError::InvalidIdentifier(IdentifierError)? Or is it that DecodingError::InvalidIdentifier(IdentifierError) should only be used for when an invalid identifier causes a decoding error?

[Farhad-Shabani]

Is the idea then that IdentifierErrors will be subsumed by DecodingError? I.e., every current instance of an IdentifierError will become a DecodingError::InvalidIdentifier(IdentifierError)? Or is it that DecodingError::InvalidIdentifier(IdentifierError) should only be used for when an invalid identifier causes a decoding error?

I’d prefer the latter one. To use it for cases where an invalid identifier causes a decoding error, especially considering various IBC clients and applications can exist. We can’t be certain that an IdentifierError will be exclusively happening in decoding situations.