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.

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

Parse ICS20 denomination using channel identifier format #1730

Conversation

colin-axner
Copy link
Contributor

@colin-axner colin-axner commented Jul 19, 2022

Description

closes: #1698


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

Change IBC token denomination parsing to use the channel identifier rather than the port
Remove unfixable parsing validation test
Fix migration tests
Add check to migration of traces to panic upon denomination state corruption
modules/apps/transfer/keeper/migrations.go Outdated Show resolved Hide resolved
@@ -108,3 +107,16 @@ func (suite *KeeperTestSuite) TestMigratorMigrateTraces() {
})
}
}

func (suite *KeeperTestSuite) TestMigratorMigrateTracesCorruptionDetection() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made as a separate test since it is the only test case which should panic

{"trace info with multiple '/'s in base denom", "transfer/channelToA/gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1", Path: "transfer/channelToA"}},
{"trace info with multiple double '/'s in base denom", "transfer/channelToA/gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1", Path: "transfer/channelToA"}},
{"trace info with multiple port/channel pairs", "transfer/channelToA/transfer/channelToB/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}},
{"trace info", "transfer/channel-0/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-0"}},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed channelToA -> channel-1, channelToB -> channel-2

modules/apps/transfer/types/trace_test.go Outdated Show resolved Hide resolved
{"empty denom", "", true},
{"single trace identifier", "transfer/", true},
{"invalid channel ID", "transfer/(channelToA)/uatom", true},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to remove this test, the ValidatePrefixedDenom relies on the parsing function. As the documentation indicates, an invalid channel identifier will result in the channel identifier being treated as a base denomination, thus making the test case "invalid channel id" always pass (we don't do validation on the base denomination, since it isn't ours to validate)

@colin-axner colin-axner requested a review from seantking as a code owner July 19, 2022 12:31
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🚀

Comment on lines 113 to 115
corruptedDenomTrace := transfertypes.DenomTrace{
BaseDenom: "customport/channel-0/uatom", Path: "",
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mega nit but could we separate fields onto different lines for readability, e.g.

corruptedDenomTrace := transfertypes.DenomTrace{
	BaseDenom: "customport/channel-0/uatom", 
        Path:      "",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh definitely! Didn't even notice it was two fields

modules/apps/transfer/types/trace.go Outdated Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

// The new form of parsing will result in a token denomination change.
// A bank migration is required. A panic should occur to prevent the
// chain from using corrupted state.
panic(fmt.Sprintf("migration will result in corrupted state. Previous IBC token (%s) requires a bank migration. Expected denom trace (%s)", dt, newTrace))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect this to not occur correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct. From my understanding, this should only be possible if any chains using the latest minor versions (1.5, 2.3, 3.1), use custom ports for transfer and allow receives of foreign assets. I'm believe there is no single case of this, but to be safe, the check is added to ensure that if this assumption is wrong we can fix the issue before it worsens

@codecov-commenter
Copy link

Codecov Report

Merging #1730 (2be1b63) into charly/denom_traces_migration_handler (7660328) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                          Coverage Diff                           @@
##           charly/denom_traces_migration_handler    #1730   +/-   ##
======================================================================
  Coverage                                  80.01%   80.02%           
======================================================================
  Files                                        165      165           
  Lines                                      12336    12353   +17     
======================================================================
+ Hits                                        9871     9885   +14     
- Misses                                      2001     2003    +2     
- Partials                                     464      465    +1     
Impacted Files Coverage Δ
modules/apps/transfer/keeper/migrations.go 88.23% <100.00%> (+2.02%) ⬆️
modules/apps/transfer/types/trace.go 91.17% <100.00%> (+0.85%) ⬆️
modules/apps/transfer/keeper/grpc_query.go 73.07% <0.00%> (-3.85%) ⬇️

colin-axner and others added 2 commits July 19, 2022 15:10
….com:cosmos/ibc-go into colin/1698-parse-ics20-via-channelidentifier
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superb work, @colin-axner!

@crodriguezvega crodriguezvega merged commit b86cdd8 into charly/denom_traces_migration_handler Jul 20, 2022
@crodriguezvega crodriguezvega deleted the colin/1698-parse-ics20-via-channelidentifier branch July 20, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants