Clean up `ibc-apps` errors

seanchen1991 commented 3 months ago

Part of: #1316

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

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 15.78947% with 96 lines in your changes missing coverage. Please review.

Project coverage is 66.99%. Comparing base (60ff9bd) to head (a36348b). Report is 1 commits behind head on error-handling.

Files	Patch %	Lines
ibc-apps/ics721-nft-transfer/src/module.rs	17.39%	19 Missing :warning:
ibc-apps/ics721-nft-transfer/types/src/packet.rs	0.00%	19 Missing :warning:
ibc-apps/ics20-transfer/src/module.rs	14.28%	18 Missing :warning:
ibc-apps/ics20-transfer/types/src/msgs/transfer.rs	0.00%	7 Missing :warning:
ibc-apps/ics721-nft-transfer/types/src/token.rs	30.00%	7 Missing :warning:
.../ics721-nft-transfer/src/handler/on_recv_packet.rs	0.00%	6 Missing :warning:
...pps/ics721-nft-transfer/types/src/msgs/transfer.rs	0.00%	5 Missing :warning:
...s/ics721-nft-transfer/src/handler/send_transfer.rs	0.00%	4 Missing :warning:
ibc-apps/ics20-transfer/src/handler/mod.rs	0.00%	2 Missing :warning:
ibc-apps/ics721-nft-transfer/src/handler/mod.rs	0.00%	2 Missing :warning:
... and 5 more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## error-handling #1318 +/- ## =================================================== + Coverage 0 66.99% +66.99% =================================================== Files 0 227 +227 Lines 0 23451 +23451 =================================================== + Hits 0 15712 +15712 - Misses 0 7739 +7739 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

seanchen1991 commented 3 months ago

@Farhad-Shabani A broader question. With an error variant like this one:

/// invalid URI: `{uri}`, validation error: `{validation_error}`
InvalidUri {
    uri: String,
    validation_error: http::uri::InvalidUri,
},

it's not clear whether the wrapped validation_error is useful and should be kept, or if this could be simplified to the following to be more concise:

/// invalid URI: `{0}`
InvalidUri(String),

Do you have thoughts on this?

Farhad-Shabani commented 3 months ago

it's not clear whether the wrapped validation_error is useful and should be kept, or if this could be simplified Do you have thoughts on this?

I’m actually leaning towards something between:

/// invalid URI with error: `{0}`
InvalidUri(http::uri::InvalidUri),

Which is more like the way we handle e.g. InvalidIdentifier(IdentifierError)

By the way, I think it would be a good idea to briefly document our error definition convention before finalizing these PRs. Any kind of variations can be clarified there, if that makes sense to you.

cosmos / ibc-rs