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.

Sign up for GitHub

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

Jump to bottom

Consolidate PacketError and ChannelError #1343

Merged
merged 9 commits into from
Sep 17, 2024
Merged

Consolidate PacketError and ChannelError #1343

merged 9 commits into from
Sep 17, 2024

Conversation

seanchen1991
Copy link
Contributor

Closes: #1339

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

@seanchen1991 seanchen1991 changed the base branch from main to error-handling September 17, 2024 13:00
@codecov Codecov
Copy link

codecov bot commented Sep 17, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 54.28571% with 64 lines in your changes missing coverage. Please review.

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

Files with missing lines Patch % Lines
ibc-core/ics04-channel/types/src/channel.rs 65.51% 10 Missing ⚠️
ibc-core/ics04-channel/types/src/packet.rs 27.27% 8 Missing ⚠️
ibc-testkit/src/testapp/ibc/core/types.rs 0.00% 8 Missing ⚠️
...ics04-channel/types/src/msgs/chan_close_confirm.rs 33.33% 6 Missing ⚠️
...e/ics04-channel/types/src/msgs/timeout_on_close.rs 54.54% 5 Missing ⚠️
ibc-core/ics04-channel/src/handler/recv_packet.rs 33.33% 4 Missing ⚠️
ibc-core/ics04-channel/src/handler/timeout.rs 42.85% 4 Missing ⚠️
...core/ics04-channel/src/handler/timeout_on_close.rs 42.85% 4 Missing ⚠️
...rc/testapp/ibc/applications/nft_transfer/module.rs 0.00% 4 Missing ⚠️
ibc-core/ics04-channel/types/src/msgs/timeout.rs 50.00% 3 Missing ⚠️
... and 5 more
Additional details and impacted files 
@@                Coverage Diff                @@
##             error-handling    #1343   +/-   ##
=================================================
  Coverage                  ?   66.89%           
=================================================
  Files                     ?      225           
  Lines                     ?    22958           
  Branches                  ?        0           
=================================================
  Hits                      ?    15357           
  Misses                    ?     7601           
  Partials                  ?        0

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

@Farhad-Shabani Farhad-Shabani added this to the 0.55.0 milestone Sep 17, 2024
@Farhad-Shabani Farhad-Shabani merged commit a4e409a into error-handling Sep 17, 2024
14 checks passed
@Farhad-Shabani Farhad-Shabani deleted the imp/packet-error-into-channel-error branch September 17, 2024 17:20
@Farhad-Shabani Farhad-Shabani mentioned this pull request Sep 24, 2024
Merged
15 tasks
github-merge-queue bot pushed a commit that referenced this pull request Sep 24, 2024
@seanchen1991 @Farhad-Shabani
imp(ibc)!: refactor error handling throughout codebase (#1350)
ede380f 
* 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
Reviewers

@Farhad-Shabani Farhad-Shabani Farhad-Shabani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
ibc-rs
Status: ✅ Done
Milestone
0.55.0
Development

Successfully merging this pull request may close these issues.

Merge PacketError type into ChannelError
2 participants
@seanchen1991 @Farhad-Shabani