search

cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
197 stars 79 forks source link

fix(ics20): `PrefixedDenom` parsing #1178

Closed rnbguy closed 5 months ago

rnbguy commented 5 months ago

Closes: #1177

Description

PR author checklist:

Reviewer checklist:

rnbguy commented 5 months ago

I added the client validation logic. But now, the question remains - what happens in the following cases:

https://github.com/cosmos/ibc-rs/blob/dfac6b5f9bc2fd133ee219ef88762537ba44d347/ibc-apps/ics20-transfer/types/src/denom.rs#L396-L405

Note the following are valid cases in this PR:

https://github.com/cosmos/ibc-rs/blob/dfac6b5f9bc2fd133ee219ef88762537ba44d347/ibc-apps/ics20-transfer/types/src/denom.rs#L427-L438

Update: looks like ibc-go accepts them.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 96.69421% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 64.34%. Comparing base (cdf6cf5) to head (6f3eddb).

:exclamation: Current head 6f3eddb differs from pull request most recent head 53fad12. Consider uploading reports for the commit 53fad12 to get more accurate results

Files Patch % Lines
ibc-apps/ics721-nft-transfer/types/src/class.rs 84.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1178 +/- ## ========================================== - Coverage 64.40% 64.34% -0.06% ========================================== Files 229 229 Lines 22109 22005 -104 ========================================== - Hits 14240 14160 -80 + Misses 7869 7845 -24 ```

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

penso commented 5 months ago

Thanks @rnbguy - love the use of rstest. I had that PR in mind since I have found the issue and would have used json!() to make it easy to include a bunch of tests. This is obviously a way better way.

rnbguy commented 5 months ago

@penso thanks for the review ! :pray: really appreciate discussing the issue on Twitter. :+1:

If you come across any other issue with ibc-rs, feel free to open an issue on Github or directly contact us on Slack (shared ibc-rs channel) or Twitter :raised_hands:

Farhad-Shabani commented 5 months ago

As a remark:

The same case applies to the PrefixedClassId under the ICS-721 implementation. Perhaps it would be better to handle that as a separate PR, But that should be part of the next release too.

rnbguy commented 5 months ago

@seanchen1991 do you think it makes sense to include the same changes, needed for ics-721, in this PR?

Also, I introduced TracePrefix::strip and TracePath::trim to keep things cleaner.

seanchen1991 commented 5 months ago

do you think it makes sense to include the same changes, needed for ics-721, in this PR?

I'm fine either way. If we want to include the ics-721 changes in this PR, then go ahead 👍

rnbguy commented 5 months ago

hey @seanchen1991, in recent commits, I added ibc-app-transfer-types dependency to ibc-app-nft-transfer-types and reused TracePath and TracePrefix implementations.

I also updated the algorithm subtly - to handle another incorrect parsing (7408994).