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

Refactor error system #255

Merged
merged 44 commits into from
Nov 30, 2022

Conversation

DaviRain-Su
Copy link
Contributor

Closes: #164

Description


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@DaviRain-Su
Copy link
Contributor Author

Do you have a better description of the error?

    /// Ics04 channel error(`{0}`)
    Ics04Channel(channel_error::Error),
    /// invalid port identifier `{context}` error(`{validation_error}`)
    InvalidPortId {
        context: String,
        validation_error: ValidationError,
    },
    /// invalid channel identifier `{context}` error(`{validation_error}`)
    InvalidChannelId {
        context: String,
        validation_error: ValidationError,
    },

@DaviRain-Su DaviRain-Su marked this pull request as ready for review November 23, 2022 17:40
@plafer
Copy link
Contributor

plafer commented Nov 23, 2022

Note: hopefully our solution here also addresses #23. I'm still thinking about how to make everything work together (logs discarded on error issue, no_std support, error sources, small number of Error variants if possible, etc)

EDIT: Actually #23 fixed by the new ValidationContext: logs are no longer returned, but rather given to the context with log_message().

use displaydoc::Display;

#[derive(Debug, Display)]
pub enum ContextError {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know if my understanding of this is in line with the requirements

crates/ibc/Cargo.toml Outdated Show resolved Hide resolved
crates/ibc/src/lib.rs Outdated Show resolved Hide resolved
#[derive(Debug, Display)]
pub enum ClientError {
/// unknown client type: `{client_type}`
UnknownClientType { client_type: String },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having thought about this some more, I like the direction this PR is taking. IMO what's left to be done is:

  1. Remove all unused errors such as this one. Once this is done, we can additionally look to combine redundant errors.
  2. "Roll our own backtrace", as @hu55a1n1 suggested. We should make sure to use the same convention everywhere, as you started doing here: <error message>: {0} (for when the error argument is another Error). When std::error::Error makes it into core, we can start using thiserror.
  3. Make sure there's a Error::Other variant for host methods to use. This PR is getting big enough, we can change host functions in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When std::error::Error makes it into core, we can start using thiserror
Currently thiserror can not be used in no_std, thiserror::Error I think the implementation of this macro can not be used in the current no_std environment, and the current use of displaydoc I think similar to thiserror except that there is no error backtrace, but here we have implemented std::error::Error.

#[cfg(feature = "std")]
impl std::error::Error for Error {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match &self {
            Error::InvalidHeader { error: e, .. } => Some(e),
            Error::InvalidTendermintTrustThreshold(e) => Some(e),
            Error::InvalidChainIdentifier(e) => Some(e),
            Error::InvalidChainId { error: e, .. } => Some(e),
            Error::InvalidRawHeader(e) => Some(e),
            Error::Decode(e) => Some(e),
            Error::TimestampOverflow(e) => Some(e),
            _ => None,
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when thiserror support we just add #derive[thiserror::Error] can remove below this.

#[cfg(feature = "std")]
impl std::error::Error for Error {}

crates/ibc/src/core/ics04_channel/error.rs Outdated Show resolved Hide resolved
@plafer
Copy link
Contributor

plafer commented Nov 28, 2022

Requested additional input from @blasrodri; let's hold until then

@blasrodri
Copy link
Contributor

I like the idea of getting rid of flex-error. Plus, the naming of the errors has become more descriptive, and do not assume that the developer is so acquainted with the different ics. I'm not sure if thiserror is needed at all - would be nice to see a use case that's not currently covered with the current implementation. Seems like something that the library consumers would like to have.

Overall, I think this PR reflects the goals of #164 .

Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this amazing work; we're getting really close to the finish line! 😃

crates/ibc/src/core/ics04_channel/error.rs Show resolved Hide resolved
crates/ibc/src/core/ics02_client/error.rs Show resolved Hide resolved
crates/ibc/src/core/ics03_connection/error.rs Show resolved Hide resolved
crates/ibc/src/core/ics04_channel/error.rs Outdated Show resolved Hide resolved
crates/ibc/src/core/ics04_channel/error.rs Show resolved Hide resolved
crates/ibc/src/core/ics04_channel/error.rs Outdated Show resolved Hide resolved
crates/ibc/src/core/ics04_channel/error.rs Outdated Show resolved Hide resolved
crates/ibc/src/core/ics04_channel/error.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work :)

@plafer plafer merged commit 1056629 into cosmos:main Nov 30, 2022
@DaviRain-Su
Copy link
Contributor Author

Awesome work :)

thx guys

@DaviRain-Su DaviRain-Su deleted the use-displaydoc-as-error-handler branch November 30, 2022 17:33
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
* Redefine Ics20Error by displaydoc

* Redefine Ics02Error by displaydoc

* Update client_state.rs

* Redefine Ics07Error by displaydoc

* Redefine Ics03Error by displaydoc

* Redefine Ics04Error by displaydoc

* Redefine Ics05Error by displaydoc

* Redefine Ics23Error by displaydoc

* Redefine Ics24Error by displaydoc

* Redefine Ics26Error by displaydoc

* Redefine Ics18Error by displaydoc

* Redefine ParseFailure Error

* Redefine HeightError

* Redefine ProofError

* Redefine SignerError

* Redefine TimestampOverflowError, ParseTimestampError

* Redefine event Error

* update ics03Error

* Update Ics04Error

* Update Ics26Error

* Remove flex-error

* Update error.rs

* Create 164-refactor-error-system.md

* Impl std::erorr::Error for IbcError

* Add #![allow(clippy::result_large_err)]

* Rename Ics02Error to ClientError

* Rename Ics03Error to ConnectionError

* Disassembling out PacketError

* Rename Ics04Error to ChannelError

* Rename Ics05Error to PortError

* Rename Ics23Error to CommitmentError

* Rename Ics26Error to RouterError

* Rename Ics18Error to RelayerError

* Rename Ics20Error to TokenTransferError

* Add ContextError for ValidationContext and ExecutionContext

* Update RouterError

* fix clippy

* Update context.rs

* Update lib.rs

* Secondary error display output

* Remove unused Error variant

* Remove unused Error variant

* Update 164-refactor-error-system.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor error system
4 participants