Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move host-relevant error variants to a newly-defined HostError type #1320

Closed
34 tasks
Tracked by #270
seanchen1991 opened this issue Aug 23, 2024 · 0 comments · Fixed by #1331 or #1350
Closed
34 tasks
Tracked by #270

Move host-relevant error variants to a newly-defined HostError type #1320

seanchen1991 opened this issue Aug 23, 2024 · 0 comments · Fixed by #1331 or #1350
Assignees
Labels
O: code-hygiene Objective: aims to improve code hygiene O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding O: usability Objective: aims to enhance user experience (UX) and streamline product usability S: errors Scope: related to error handlings
Milestone

Comments

@seanchen1991
Copy link
Contributor

seanchen1991 commented Aug 23, 2024

Feature Summary

Many error variants in ibc-rs are primarily used by hosts and are not relevant to ibc-rs's internals. Such variants should live in one or more dedicated host error types. Such host-relevant error types should live in a workspace that is most-easily accessible by hosts, i.e., in the ics24-host workspace.

Proposal

A new error type specifically for holding host-relevant errors should be defined in ics24-host. Any error variants in ibc-rs that exist primarily for hosts' benefit, i.e., they are not used internally by ibc-rs codepaths, or are only used in testing scenarios, should be moved into this host error type.

pub enum HostError {
    /// some data is invalid
    InvalidData { description: String },
    /// some data is missing
    MissingData { description: String },
    /// failed to update some state
    FailedToUpdateState { description: String },
    /// failed to store some data
    FailedToStoreData { description: String },
    /// failed to parse some data
    FailedToParseData { description: String },
    /// some type that was expected does not exist
    NonexistentType { description: String },
    /// some other classification of error not already covered
    Other { description: String },
}

This host error type aims to capture all possible classes of errors that hosts may want to return / propagate, though it will also include an Other variant as an option for if a host needs to return an error that is not adequately captured by any of the pre-defined variants.

This HostError type is meant to serve as a default error type for hosts to use, and the plan is to have basecoin move away from directly using ibc-rs's error types and use this HostError type instead.

Validation, execution, and dispatch trait methods that currently return ibc-rs's top-level ContextError type will instead return the new HostError type. Additionally, the ContextError type will be renamed to HandlerError to better capture its semantic meaning. A new HostError variant will also be added to it.

The following are all of the basecoin error variants that need to be captured by the HostError type:

basecoin error variants

  • PacketError variants

    • PacketError::AppModule
    • PacketError::MissingPacketReceipt? This variant is used in ibc-core/ics04-channel/handler. Should it be moved to a host-specific error type?
    • PacketError::MissingPacketAcknowledgment
  • ChannelError variants

    • ChannelError::AppModule
    • ChannelError::MissingCounter
    • ChannelError::FailedToUpdateCounter
    • ChannelError::FailedToStoreChannel
    • ChannelError::NonexistentChannel
  • ClientError variants

    • ClientError::InvalidHeaderType
    • ClientError::InvalidRawHeader
    • ClientError::MissingClientState
    • ClientError::MissingUpdateMetaData
    • ClientError::MissingConsensusState? This variant is used in ibc-core/ics02-client/handler. Should it be moved to a host-specific error type?
  • ConnectionError variants

    • ConnectionError::MissingConnection
    • ConnectionError::MissingConnectionCounter
    • ConnectionError::FailedToStoreConnectionIds
    • ConnectionError::FailedToStoreConnectionEnd
    • ConnectionError::FailedToUpdateConnectionCounter
  • RouterError variants

    • RouterError::UnknownPort? This variant is used in multiple places in ibc-core/ics25-handler/entrypoint. Should it be moved to a host-specific error type?
  • TokenTransferError variants

    • TokenTransferError::FailedToParseAccount
  • UpgradeClientError variants

    • UpgradeClientError::InvalidUpgradePlan
    • UpgradeClientError::InvalidUpgradeProposal
    • UpgradeClientError::MissingUpgradedClientState
    • UpgradeClientError::MissingUpgradedConsensusState
    • UpgradeClientError::FailedToStoreUpgradePlan
    • UpgradeClientError::FailedToStoreUpgradedClientState
    • UpgradeClientError::FailedToStoreUpgradedConsensusState

Future Considerations

For the time being, the concrete HostError defined above will serve as the default type for encapsulating errors from hosts. However, this error type does nothing but encode errors as strings, which might not be ideal or optimal for all hosts' error-handling considerations. We may want to give hosts the option of injecting their own error type so that they have the option of defining and using an error type that is better directly suited to their code base.

This can be done by adding an Error associated type to the ValidationContext trait. This associated type will need to be constrained via Into<HandlerError>. We should also consider what other constraints need to be put on this associated type.

@seanchen1991 seanchen1991 changed the title Move host-relevant error variants to a newly-defined HostErrortype Move host-relevant error variants to a newly-defined HostError type Aug 26, 2024
@seanchen1991 seanchen1991 self-assigned this Aug 26, 2024
@seanchen1991 seanchen1991 added S: errors Scope: related to error handlings O: code-hygiene Objective: aims to improve code hygiene O: usability Objective: aims to enhance user experience (UX) and streamline product usability O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding labels Sep 5, 2024
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Sep 5, 2024
@seanchen1991 seanchen1991 added this to the v1 milestone Sep 5, 2024
@seanchen1991 seanchen1991 moved this from 📥 To Do to 🏗️ In Progress in ibc-rs Sep 5, 2024
@github-project-automation github-project-automation bot moved this from 🏗️ In Progress to ✅ Done in ibc-rs Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: code-hygiene Objective: aims to improve code hygiene O: maintainability Objective: cause to ease modification, fault corrections and improve code understanding O: usability Objective: aims to enhance user experience (UX) and streamline product usability S: errors Scope: related to error handlings
Projects
None yet
1 participant