fix(ics20): PrefixedDenom parsing

[rnbguy]

Closes: #1177

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

[rnbguy]

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

ibc-rs/ibc-apps/ics20-transfer/types/src/denom.rs

Lines 396 to 405 in dfac6b5

	assert!(PrefixedDenom::from_str("/uatom").is_err(), "empty prefix");
	assert!(PrefixedDenom::from_str("//uatom").is_err(), "empty ids");
	assert!(
	PrefixedDenom::from_str("transfer/").is_err(),
	"single trace"
	);
	assert!(
	PrefixedDenom::from_str("transfer/atom").is_err(),
	"single trace with base denom"
	);

Note the following are valid cases in this PR:

ibc-rs/ibc-apps/ics20-transfer/types/src/denom.rs

Lines 427 to 438 in dfac6b5

	#[case(
	"transfer/channel-75",
	"factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust"
	)]
	#[case(
	"transfer/channel-75/transfer/channel-123/transfer/channel-1023/transfer/channel-0",
	"factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust"
	)]
	#[case(
	"transfer/channel-75/transfer/channel-123/transfer/channel-1023/transfer/channel-0",
	"//////////////////////dust"
	)]

Update: looks like ibc-go accepts them.

[codecov]

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

❗ 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 ⚠️

Additional details and impacted files

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

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

[seanchen1991]

Looks good for the most part. Would be great if we could add a docstring of the PrefixedDenom::from_str implementation. Feel free to use my suggested description or write a different one.

[seanchen1991]

It would be helpful to add a description of the logic of this from_str implementation, as it's a little bit convoluted. Here's a suggestion:

/// Initializes a `PrefixedDenom` from a string that adheres to the format
/// `{port-id-1/channel-id-1}/{port-id-2/channel-id-2}/.../{port-id-n/channel-id-n}/base-denom`.
/// A `PrefixedDenom` exhibits between 1..n number of `{port-id/channel-id}` pairs.
/// The set of these pairs makes up the `TracePath`.
///
/// This `from_str` implementation peeks the first two segments split on the `/`
/// delimiter and attempts to convert the first segment into a `PortId` and the 
/// second segment into a `ChannelId`. This continues in a loop until a 
/// `{port-id/channel-id}` pair cannot be created from the top two segments. 
/// The remaining parts of the string are then considered the `BaseDenom`.
///
/// For example, given the following denom trace:
/// "transfer/channel-75/factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust",
/// the first two `/`-delimited segments are `"transfer"` and `"channel-75"`. The
/// first is a valid `PortId` and the second is a valid `ChannelId`, so that becomes
/// the first `{port-id/channel-id}` pair that gets added as part of the `TracePath`
/// of the `PrefixedDenom`. The next two segments are `"factory"`, which is not a
/// valid `PortId`, and `"stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4"`, which is
/// not a valid `ChannelId`. The loop breaks at this point, resulting in a
/// `TracePath` of `"transfer/channel-75"` and a `BaseDenom` of
/// `"factory/stars16da2uus9zrsy83h23ur42v3lglg5rmyrpqnju4/dust"`.

Feel free to edit the above to make the description more clear.

[Farhad-Shabani]

I agree with Sean. The above description helps a lot. Great.

The next two segments are "factory", which is not a valid PortId

Just a small note: "factory" is a valid PortId.

[rnbguy]

9d246d4

[Farhad-Shabani]

Thanks @rnbguy for getting into the details! The tests are fantastic.
I have a few suggestions and nitpicks, but overall, everything looks great.

[seanchen1991]

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]

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

[seanchen1991]

Looks good to merge 👍

Farhad is on leave and the fix is on 53fad12 .