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

Consolidate decoding-related errors into new DecodingError type #1325

Merged
merged 73 commits into from
Sep 9, 2024

Conversation

seanchen1991
Copy link
Contributor

@seanchen1991 seanchen1991 commented Aug 28, 2024

Closes: #1319
Closes: #950
Closes: #270

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

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 59.92141% with 204 lines in your changes missing coverage. Please review.

Please upload report for BASE (error-handling@466c13d). Learn more about missing BASE report.

Files with missing lines Patch % Lines
ibc-core/ics25-handler/types/src/msgs.rs 0.00% 19 Missing ⚠️
...clients/ics07-tendermint/types/src/misbehaviour.rs 28.57% 15 Missing ⚠️
...e/ics03-connection/types/src/msgs/conn_open_ack.rs 58.82% 14 Missing ⚠️
...e/ics03-connection/types/src/msgs/conn_open_try.rs 69.44% 11 Missing ⚠️
ibc-apps/ics20-transfer/src/module.rs 0.00% 9 Missing ⚠️
...clients/ics07-tendermint/types/src/client_state.rs 78.57% 9 Missing ⚠️
ibc-core/ics02-client/types/src/error.rs 25.00% 9 Missing ⚠️
ibc-clients/ics07-tendermint/types/src/header.rs 75.00% 7 Missing ⚠️
ibc-core/ics03-connection/types/src/connection.rs 56.25% 7 Missing ⚠️
ibc-apps/ics20-transfer/types/src/error.rs 40.00% 6 Missing ⚠️
... and 31 more
Additional details and impacted files
@@                Coverage Diff                @@
##             error-handling    #1325   +/-   ##
=================================================
  Coverage                  ?   66.96%           
=================================================
  Files                     ?      225           
  Lines                     ?    22930           
  Branches                  ?        0           
=================================================
  Hits                      ?    15354           
  Misses                    ?     7576           
  Partials                  ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ibc-apps/ics20-transfer/types/src/msgs/transfer.rs Outdated Show resolved Hide resolved
ibc-apps/ics721-nft-transfer/types/src/error.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/types/src/status.rs Outdated Show resolved Hide resolved
ibc-core/ics26-routing/types/src/error.rs Outdated Show resolved Hide resolved
ibc-primitives/Cargo.toml Outdated Show resolved Hide resolved
ibc-core/ics02-client/types/src/error.rs Outdated Show resolved Hide resolved
ibc-core/ics24-host/types/Cargo.toml Outdated Show resolved Hide resolved
ibc-apps/ics20-transfer/types/src/coin.rs Outdated Show resolved Hide resolved
ibc-apps/ics20-transfer/types/src/denom.rs Outdated Show resolved Hide resolved
ibc-core/ics03-connection/src/handler/mod.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/types/src/error.rs Outdated Show resolved Hide resolved
Copy link
Member

@Farhad-Shabani Farhad-Shabani 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, Sean, for addressing the previous comments. I've left a few more after a second review.

ibc-core/ics03-connection/types/src/error.rs Outdated Show resolved Hide resolved
ibc-core/ics04-channel/types/src/error.rs Outdated Show resolved Hide resolved
ibc-core/ics04-channel/types/src/error.rs Outdated Show resolved Hide resolved
ibc-core/ics04-channel/types/src/error.rs Outdated Show resolved Hide resolved
ibc-apps/ics20-transfer/src/module.rs Outdated Show resolved Hide resolved
ibc-core/ics23-commitment/types/src/commitment.rs Outdated Show resolved Hide resolved
ibc-core/ics02-client/types/src/msgs/upgrade_client.rs Outdated Show resolved Hide resolved
ibc-core/ics24-host/types/Cargo.toml Outdated Show resolved Hide resolved
ibc-core/ics26-routing/types/src/error.rs Outdated Show resolved Hide resolved
@Farhad-Shabani Farhad-Shabani merged commit bfad72f into error-handling Sep 9, 2024
14 checks passed
@Farhad-Shabani Farhad-Shabani deleted the sean/consolidate-error-types branch September 9, 2024 20:24
github-merge-queue bot pushed a commit that referenced this pull request Sep 24, 2024
* Clean up `ibc-apps` errors (#1318)

* Clean up ics20-transfer error

* Clean up NftTransfer error

* Incorporate PR feedback

* Remove redundant TODO comment

* Simplify NftTransferError variant

* Add TODO reminder

* Clean up `ibc-core` and `ibc-clients` error types (#1317)

* Clean up pass of ClientError type

* Clean up pass of ics24 errors

* Clean up pass of ics23 errors

* Clean up pass of ics07 errors

* Clean up ChannelError type

* Clean up PacketError type

* Better organize error variants

* cargo nightly fmt

* Update error variant in integration tests

* Fix ics23 tests

* Fix typo in doc comment

* Update cw-check Cargo.lock

* Clean up ics08 error

* Cargo nightly fmt

* Change single-field error variants to tuple structs

* Propogate error variant changes

* Propogate error variant changes

* Clean up PacketError type

* Clean up IdentifierError type

* Clean up ics26 RouterError

* Clean up ics03 ConnectionError

* Fix error variant naming

* Fix error variant naming

* Remove redundant Other error variant

* Clarify tendermint client error naming

* Rename tendermint client Error to TendermintClientError

* Rename wasm light client Error to WasmClientError

* Rename ics25-handler Error to HandlerError

* Remove unused HandlerError

* Remove TendermintClientError::InvalidHeader error for more specific variant

* Rename LightClientVerifierError variant to FailedToVerifyHeader

* Rename ConsensusStateTimestampGteTrustingPeriod to InsufficientTrustingPeriod

* Remove redundant ChannelError::FailedToParseSequence variant

* Rename ChannelError::FailedChannelVerification to FailedProofVerification

* Replace incorrect usages of FailedPacketVerification

* Define From<CommitmentError> for ClientError impl

* Add IdentifierError::FailedToParse variant

* Replace InvalidPrefix error usage

* Fix doc comment typo

* Improve ChannelError variant names

* Add PacketError::InvalidPathPrefix variant

* Remove unnecessary PacketError variant

* Add error variants used by basecoin

* nit: remove unnecessary map_err in verify_(non_)membership

* imp: define EmptyCommitmentRoot

* nit: remove MissingHostHeight

* fix: remove unnecessary map_err(PacketError::Channel)

* Remove a TODO

---------

Co-authored-by: Farhad Shabani <[email protected]>

* Clean up `QueryError` and `RelayerError` types (#1321)

* Clean up ibc-query QueryError type

* Clean up ibc-testkit RelayerError

* Clean up ibc-testkit RelayerError

* Change wording of a doc comment

* Remove RelayerError

* Consolidate decoding-related errors into new `DecodingError` type (#1325)

* Define DecodingError in ibc-primitives

* Utilize DecodingError in ics721

* Start using DecodingError in ClientError

* Define StatusError type

* Wire up StatusError

* Wiring up DecodingError

* Add DecodingError to RouterError

* Remove unused From impl

* Change format of an import

* Change format of an import

* Change format of an import

* Move DecodingError into ics24

* Cargo nightly fmt

* Use DecodingError in more places

* cargo fmt

* cargo fmt

* Update cw-check Cargo.lock

* Add necessary wasm-client feature attribute

* Move InvalidUri error variant back to NftTransferError

* Regenerate cw hcheck cargo.lock

* Add serde feature attribute

* Remove ClientError::InvalidClientIdentifier error variant in favor of DecodingError::InvalidIdentifier

* Add derive_more::From on NftTransferError

* Stashing changes

* Revert "Add derive_more::From on NftTransferError"

This reverts commit 234ebee.

* Remove RouterError::UnknownMessageTypeUrl

* Add derive_more::From on TokenTransferError

* Add derive_more to NftTransferError

* Remove tendermint-proto dependency from ibc-primitives

* Remove StatusError

* Remove unnecessary ClientError::Decoding wrappings

* Clean up TendermintClientError

* Regenerate cw-check cargo.lock

* taplo fmt

* Apply PR feedback

* Use ibc_proto::Error type instead of tendermint_proto::Error

* Change FailedToParseType fields to make them more clear

* Revert InvalidTrace and InvalidCoin TokenTransferError variants

* Remove NftTransferError variants in favor of DecodingError::InvalidJson

* cargo fmt

* Regenerate cw-check cargo.lock

* Separate out error variants that need to be moved to host-relevant errors

* Consolidate TendermintClientError decoding errors

* Consolidate Connection and Packet decoding errors

* Remove WasmClientError

* Consolidate Identifier variant naming

* Remove HostError annotations in doc comments

* Consolidate CommitmentError variants

* Consolidate ConnectionError variants

* Revert some CommitmentError variants

* Change TryFrom<Any> for MsgEnvelope Error type to DecodingError

* Remove ConnectionError::Identifier variant in favor of DecodingError::Identifier

* Remove ChannelError::Identifier variant in favor of DecodingError::Identifier

* Remove PacketError::Identifier variant in favor of DecodingError::Identifier

* Revert TokenTransferError FailedToDeserializeAck and FailedToDeserializePacketData

* Revert NftTransferError FailedToDeserializeAck and FailedToDeserializePacketData

* Revert ics20 and ics721 on_recv_packet_execute impls

* Remove additional Identifier error variants

* Add TendermintClientError::InsufficientMisbehaviourHeaderHeight variant

* Implement From<IdentifierError> for ClientError and ConnectionError

* Incorporate PR feedback

* Change TryFrom<RawConsensusState> for ConsensusState error to DecodingError

* Remove RouterError::Decoding variant

* Revert AcknowledgementStatus tests

* Cargo fmt

* Fix test_ack_de

* Fix typo in doc comment

* Fix test_ack_de

* Fix test_ack_de

* fix: revert nft721 on module errors

* fix: remove Identifier variant from few more enums

* fix: remove unused tendermint-proto dep

* chore: update Cargo.lock

---------

Co-authored-by: Farhad Shabani <[email protected]>

* imp(ibc-apps): migrate to `DecodingError` in `TryFrom` conversions (#1335)

* imp: migrate to DecodingError in TryFrom conversions, ibc-apps

* imp: apply on TryFrom<RawPacketData>

* fix: missing import

* fix: apply review comments

* imp: move away from From<Infallible>

* Define `HostError` type (#1331)

* Define DecodingError in ibc-primitives

* Utilize DecodingError in ics721

* Start using DecodingError in ClientError

* Define StatusError type

* Wire up StatusError

* Wiring up DecodingError

* Add DecodingError to RouterError

* Remove unused From impl

* Change format of an import

* Change format of an import

* Change format of an import

* Move DecodingError into ics24

* Cargo nightly fmt

* Use DecodingError in more places

* cargo fmt

* cargo fmt

* Update cw-check Cargo.lock

* Add necessary wasm-client feature attribute

* Move InvalidUri error variant back to NftTransferError

* Regenerate cw hcheck cargo.lock

* Add serde feature attribute

* Remove ClientError::InvalidClientIdentifier error variant in favor of DecodingError::InvalidIdentifier

* Add derive_more::From on NftTransferError

* Stashing changes

* Revert "Add derive_more::From on NftTransferError"

This reverts commit ea9c83d.

* Remove RouterError::UnknownMessageTypeUrl

* Add derive_more::From on TokenTransferError

* Add derive_more to NftTransferError

* Remove tendermint-proto dependency from ibc-primitives

* Remove StatusError

* Remove unnecessary ClientError::Decoding wrappings

* Clean up TendermintClientError

* Regenerate cw-check cargo.lock

* taplo fmt

* Apply PR feedback

* Use ibc_proto::Error type instead of tendermint_proto::Error

* Change FailedToParseType fields to make them more clear

* Revert InvalidTrace and InvalidCoin TokenTransferError variants

* Remove NftTransferError variants in favor of DecodingError::InvalidJson

* cargo fmt

* Regenerate cw-check cargo.lock

* Separate out error variants that need to be moved to host-relevant errors

* Consolidate TendermintClientError decoding errors

* Consolidate Connection and Packet decoding errors

* Remove WasmClientError

* Consolidate Identifier variant naming

* Remove HostError annotations in doc comments

* Consolidate CommitmentError variants

* Consolidate ConnectionError variants

* Revert some CommitmentError variants

* Change TryFrom<Any> for MsgEnvelope Error type to DecodingError

* Define HostError type

* Rename ContextError to HandlerError

* Change `pick_version` impl

* Update ValidationContext trait methods to return HostError type

* Rename HandlerError variants throughout the code base

* Remove match arm that is no longer relevant

* Change ClientValidationContext trait methods to return HostErrors

* Change TokenTransferValidationContext and NftTransferValidationContext trait error types

* Updating validation contexts to return HostError types

* Map errors to HostError variants

* Update ValidateSelfClientContext trait

* Update Upgrade validation and execution contexts to return HostErrors

* Fix merge conflicts

* Fix merge conflicts

* Revert execute_upgrade_client_proposal error type to UpgradeClientError

* cargo fmt

* Remove unused alloc::string::ToString

* Import ibc_core::primitives::prelude into handler/mod.rs

* cargo fmt

* Attempt to fix failing testkit test

* Remove RouterError::UnknownPort variant

* Remove some uses of ClientError::Other variants

* Fix recv_packet_validate_happy_path test

* Fix conn_open_ack_no_connection test

* cargo fmt

* fix conn_open_ack_no_connection test without adding new HostError variant

* fix test_create_client_try_from test

* Remove unnecessary import

* Fix test_create_client_try_from

* cargo fmt

* fix: remove now unneeded with_packet_receipt

* fix: revert unintended ack status changes in ICS-20

* fix: revert unintended ack status changes in ICS-721

* fix: remove unintended tendermint-proto

* fix: update Cargo.lock for cw-check

* Fix lingering issues from merge conflicts

* Fix lingering issues from merge conflicts

* Remove From<Infallible> for ClientError impl

* cargo fmt

* Rename HostError InvalidData and MissingData variants

* Define helper functions for HostError

* Use newly-define HostError helpers

* Remove HostError::AppModule variant

* Prune some HostError variants

* Consolidate some HostError variants

* Remove HostError::NonexistentType variant

* Remove HostError::UnexpectedState variant

* Consolidate HostError::UnknownResource variant

* cargo fmt

* Remove unnecessary to_string calls

* Revert `pick_version` method's documentation

* Remove some more HostError variants

* Remove From<DecodingError> for StatusValue impl

* Fix AcknowledgementStatus tests

* fix: revert errors in ICS721 callbacks

* Change references to HandlerError in docs to refer to HostError where appropriate

---------

Signed-off-by: Sean Chen <[email protected]>
Co-authored-by: Farhad Shabani <[email protected]>

* Consolidate `PacketError` and `ChannelError` (#1343)

* Merge PacketError variants into ChannelError

* cargo nightly fmt

* Consolidate ChanelError verification variants

* Convert ics04 try_from impls to use DecodingError

* Add changelog entry

* Incorporate some PR feedback

* Remove ChannelError::MissingProof and MissingProofHeight error variants

* Add `HostError` contexts to module-level error types (#1345)

* Add ConnectionError::Host variant and clean up some other variants

* Migrate some ics03 handlers to return HostErrors

* Revert "Migrate some ics03 handlers to return HostErrors"

This reverts commit f48a1a6.

* Add HostError variant to ChannelError

* Remove HandlerError::Host variant

* Map client handler HostErrors to ClientError::Host

* Revert "Map client handler HostErrors to ClientError::Host"

This reverts commit 247be8a.

* Change ics02-client handlers to return ClientError

* Change ics03-connection handlers to return ConnectionError

* Change ics04-channel handlers to return ChannelError

* Make necessary changes in ics25 dispatch

* imp: misc improvements and fixes

* imp: use derive_more::From for ChannelError

* fix: rename InvalidClientState under ConnectionError

* nit: remove redundant import

---------

Co-authored-by: Farhad Shabani <[email protected]>

* Clean up generic String variants in error types (#1347)

* Clean up most instances of ClientError::Other

* Rename CommitmentError variants

* Clean up some DecodingError calls

* Incorporate some PR feedback

* Incorporate more PR feedback

* A bunch of DecodingError associated type changes

* Eliminate a bunch of redundant map_err calls

* cargo fmt

* Fix ics23 spec tests

* Fix a typo

* Clean up some error messages

* Consolidate MismatchedTypeUrl and MismatchedEventKind variants

* Miscellaneous clean up

* cargo fmt

* Remove unused CommitmentError variants

* Clean up errors to match ADR description

* More clean up

* Add changelog entry

* fix: remove now unused HeightError

* Clean up TimestampError

---------

Co-authored-by: Farhad Shabani <[email protected]>

* nitpicks

* fix: remove ibc-core-connection-types dep under ICS-04

* fix: propogate errors in refund_packet_nft_execute

* fix: revert few errors in ICS-20 module callbacks

* imp: use if let for type_url matches

* nit: minor improvements

* fix: use RouterError in case of ModuleID not found

* chore: add changelog

* chore: add changelog for #1319

---------

Signed-off-by: Sean Chen <[email protected]>
Co-authored-by: Farhad Shabani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants