Clean up ibc-apps errors

[seanchen1991]

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]

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 ⚠️
ibc-apps/ics721-nft-transfer/types/src/packet.rs	0.00%	19 Missing ⚠️
ibc-apps/ics20-transfer/src/module.rs	14.28%	18 Missing ⚠️
ibc-apps/ics20-transfer/types/src/msgs/transfer.rs	0.00%	7 Missing ⚠️
ibc-apps/ics721-nft-transfer/types/src/token.rs	30.00%	7 Missing ⚠️
.../ics721-nft-transfer/src/handler/on_recv_packet.rs	0.00%	6 Missing ⚠️
...pps/ics721-nft-transfer/types/src/msgs/transfer.rs	0.00%	5 Missing ⚠️
...s/ics721-nft-transfer/src/handler/send_transfer.rs	0.00%	4 Missing ⚠️
ibc-apps/ics20-transfer/src/handler/mod.rs	0.00%	2 Missing ⚠️
ibc-apps/ics721-nft-transfer/src/handler/mod.rs	0.00%	2 Missing ⚠️
... and 5 more

Additional details and impacted files

@@                 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     

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

[Farhad-Shabani]

We are now getting a better sense of what exact errors can exist in the system. Nice clean-up!

I requested some changes on ICS-20 TokenTransferError, and most of these also apply to ICS-721 NftTransferError. Although I didn’t add comments there, you can apply the same updates.

[seanchen1991]

@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]

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.

[Farhad-Shabani]

All set from my end.
Let’s merge once applied changes here in basecoin-rs.