fix: ica handshake reopening channel - use `GetAppVersion` in favour of `channel.Version` #2302

damiannolan · 2022-09-19T10:14:47Z

Description

Adding unwrapping of channel version to ica controller handshake reopening flow

closes: #XXXX

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

…ening flow

crodriguezvega

Can we add a changelog entry, @damiannolan? :)

damiannolan · 2022-09-19T10:17:38Z

Can we add a changelog entry, @damiannolan? :)

Done! ✅

chatton · 2022-09-19T10:51:46Z

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

-		if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) {
+		appVersion, found := k.GetAppVersion(ctx, portID, activeChannelID)
+		if !found {
+			panic(fmt.Sprintf("active channel mapping set for %s, but channel does not exist in channel store", activeChannelID))


looks like this panic introduces multiple conventions in the function (error return + panic) does it make more sense to return an error instead?

Do we need to panic?

As mentioned earlier in person I just followed essentially the same used above here - as in some places in ibc-go code we just panic because it's unreachable code.

maybe we could add a comment indicating as such? The message we panic with seems like it would be a regular error that we could run into.

colin-axner

Is it possible to add a test case for the exact issue we had?

AdityaSripal

Nice work!

damiannolan · 2022-09-19T18:31:27Z

I've been modifying the existing the test TestControlAccountAfterChannelClose to use an ICS29/ICS27 channel version.

It looks like we need to handle the same unwrapping of the channel version on the host in the OnChanOpenTry callback: https://github.com/cosmos/ibc-go/blob/main/modules/apps/27-interchain-accounts/host/keeper/handshake.go#L62.

This requires us to compose the hostKeeper with an ics4Wrapper and pass the feeKeeper similarly to how we do with the controller: https://github.com/cosmos/ibc-go/blob/main/testing/simapp/app.go#L401
This has to be done to unwrap the ICS27 app version from the fee enabled version correctly.

I'll make the changes locally and see if it resolves the failing test before pushing an update.

Edit: This seemed to do the trick. I've pushed an update with the changes to the host submodule and simapp, as well as updated/moved the changelog entry to API breaking. I guess we can't backport this fix. cc. @crodriguezvega @colin-axner

…el version in OnChanOpenTry handshake cb

codecov-commenter · 2022-09-19T19:01:27Z

Codecov Report

Merging #2302 (3559f5b) into main (b97729d) will increase coverage by 0.03%.
The diff coverage is 77.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2302      +/-   ##
==========================================
+ Coverage   79.54%   79.58%   +0.03%     
==========================================
  Files         175      175              
  Lines       12069    12131      +62     
==========================================
+ Hits         9600     9654      +54     
- Misses       2045     2049       +4     
- Partials      424      428       +4

Impacted Files	Coverage Δ
...7-interchain-accounts/controller/ibc_middleware.go	`70.96% <42.85%> (-9.56%)`	⬇️
...interchain-accounts/controller/keeper/handshake.go	`89.85% <50.00%> (+1.97%)`	⬆️
...27-interchain-accounts/controller/keeper/keeper.go	`92.53% <75.00%> (-0.21%)`	⬇️
...nterchain-accounts/controller/keeper/migrations.go	`81.25% <75.00%> (+1.25%)`	⬆️
...ps/27-interchain-accounts/host/keeper/handshake.go	`89.55% <90.47%> (+4.93%)`	⬆️
...7-interchain-accounts/controller/keeper/account.go	`85.00% <100.00%> (ø)`
...7-interchain-accounts/controller/keeper/genesis.go	`91.30% <100.00%> (ø)`
...apps/27-interchain-accounts/host/keeper/account.go	`100.00% <100.00%> (+23.07%)`	⬆️
.../apps/27-interchain-accounts/host/keeper/keeper.go	`83.80% <100.00%> (+0.47%)`	⬆️
modules/apps/27-interchain-accounts/module.go	`54.00% <100.00%> (+1.42%)`	⬆️
... and 8 more

jackzampolin · 2022-09-19T20:44:09Z

Thank you for this fix ibc-go team!

crodriguezvega · 2022-09-20T08:52:40Z

@damiannolan Could you please document this API change in the PR for the migration docs of v5?

chatton

LGTM just left a very minor nit!

charleenfei

nice!

charleenfei · 2022-09-21T09:07:02Z

modules/apps/27-interchain-accounts/controller/keeper/handshake.go

-		if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) {
+		appVersion, found := k.GetAppVersion(ctx, portID, activeChannelID)
+		if !found {
+			panic(fmt.Sprintf("active channel mapping set for %s, but channel does not exist in channel store", activeChannelID))


a nit: channel does not exist in channel store is kind of confusing if the error is GetAppVersion returning not found -- maybe something like channel version does not exist?

…of `channel.Version` (#2302) * adding unwrapping of channel version to ica controller handshake reopening flow * adding changelog * adding ics4Wrapper to ics27 host submodule, handling unwrapping channel version in OnChanOpenTry handshake cb * updating changelog Co-authored-by: colin axnér <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 9e6246f) # Conflicts: # CHANGELOG.md # modules/apps/27-interchain-accounts/host/keeper/keeper.go

…of `channel.Version` (backport #2302) (#2357) * fix: ica handshake reopening channel - use `GetAppVersion` in favour of `channel.Version` (#2302) * adding unwrapping of channel version to ica controller handshake reopening flow * adding changelog * adding ics4Wrapper to ics27 host submodule, handling unwrapping channel version in OnChanOpenTry handshake cb * updating changelog Co-authored-by: colin axnér <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 9e6246f) # Conflicts: # CHANGELOG.md # modules/apps/27-interchain-accounts/host/keeper/keeper.go * fixing imports and interface type Co-authored-by: Damian Nolan <[email protected]>

…of `channel.Version` (backport #2302) (#2358) * fix: ica handshake reopening channel - use `GetAppVersion` in favour of `channel.Version` (#2302) * adding unwrapping of channel version to ica controller handshake reopening flow * adding changelog * adding ics4Wrapper to ics27 host submodule, handling unwrapping channel version in OnChanOpenTry handshake cb * updating changelog Co-authored-by: colin axnér <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> (cherry picked from commit 9e6246f) # Conflicts: # CHANGELOG.md # modules/apps/27-interchain-accounts/host/keeper/keeper.go * fixing merge conflict error - reinstate interface type for msg router Co-authored-by: Damian Nolan <[email protected]>

adding unwrapping of channel version to ica controller handshake reop…

871a799

…ening flow

damiannolan requested review from seantking, colin-axner and AdityaSripal as code owners September 19, 2022 10:14

damiannolan requested review from charleenfei, chatton and crodriguezvega September 19, 2022 10:15

crodriguezvega added backport-to-v5.0.x labels Sep 19, 2022

crodriguezvega reviewed Sep 19, 2022

View reviewed changes

adding changelog

5e9be5b

crodriguezvega added the backport-to-v3.3.x label Sep 19, 2022

chatton reviewed Sep 19, 2022

View reviewed changes

colin-axner reviewed Sep 19, 2022

View reviewed changes

AdityaSripal approved these changes Sep 19, 2022

View reviewed changes

damiannolan added 2 commits September 19, 2022 20:45

adding ics4Wrapper to ics27 host submodule, handling unwrapping chann…

6d6bdaf

…el version in OnChanOpenTry handshake cb

updating changelog

c18da2c

crodriguezvega removed backport-to-v3.3.x labels Sep 20, 2022

colin-axner approved these changes Sep 20, 2022

View reviewed changes

damiannolan and others added 2 commits September 20, 2022 15:34

Merge branch 'main' into damian/fix-reopening-channel-handshake

03c665b

Merge branch 'main' into damian/fix-reopening-channel-handshake

59e0509

colin-axner requested review from AdityaSripal and chatton September 20, 2022 16:55

Merge branch 'main' into damian/fix-reopening-channel-handshake

4c243e1

chatton approved these changes Sep 21, 2022

View reviewed changes

charleenfei approved these changes Sep 21, 2022

View reviewed changes

damiannolan mentioned this pull request Sep 21, 2022

docs: include ics4Wrapper in hostkeeper.NewKeeper ics27 integration docs #2351

Merged

9 tasks

AdityaSripal approved these changes Sep 21, 2022

View reviewed changes

Carlos Rodriguez and others added 2 commits September 21, 2022 12:43

Merge branch 'main' into damian/fix-reopening-channel-handshake

3559f5b

Merge branch 'main' into damian/fix-reopening-channel-handshake

7ac47d8

damiannolan enabled auto-merge (squash) September 21, 2022 12:54

damiannolan merged commit 9e6246f into main Sep 21, 2022

damiannolan deleted the damian/fix-reopening-channel-handshake branch September 21, 2022 12:59

mergify bot mentioned this pull request Sep 21, 2022

fix: ica handshake reopening channel - use GetAppVersion in favour of channel.Version (backport #2302) #2357

Merged

mergify bot mentioned this pull request Sep 21, 2022

fix: ica handshake reopening channel - use GetAppVersion in favour of channel.Version (backport #2302) #2358

Merged

chatton mentioned this pull request Nov 7, 2022

Add git cliff toml to support generation of changelog #2697

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ica handshake reopening channel - use `GetAppVersion` in favour of `channel.Version` #2302

fix: ica handshake reopening channel - use `GetAppVersion` in favour of `channel.Version` #2302

damiannolan commented Sep 19, 2022

crodriguezvega left a comment

damiannolan commented Sep 19, 2022

chatton Sep 19, 2022

colin-axner Sep 19, 2022

damiannolan Sep 19, 2022 •

edited

Loading

chatton Sep 21, 2022

colin-axner left a comment

AdityaSripal left a comment

damiannolan commented Sep 19, 2022 •

edited

Loading

codecov-commenter commented Sep 19, 2022 •

edited

Loading

jackzampolin commented Sep 19, 2022

crodriguezvega commented Sep 20, 2022

chatton left a comment

charleenfei left a comment

charleenfei Sep 21, 2022

fix: ica handshake reopening channel - use GetAppVersion in favour of channel.Version #2302

fix: ica handshake reopening channel - use GetAppVersion in favour of channel.Version #2302

Conversation

damiannolan commented Sep 19, 2022

Description

crodriguezvega left a comment

Choose a reason for hiding this comment

damiannolan commented Sep 19, 2022

chatton Sep 19, 2022

Choose a reason for hiding this comment

colin-axner Sep 19, 2022

Choose a reason for hiding this comment

damiannolan Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

chatton Sep 21, 2022

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

damiannolan commented Sep 19, 2022 • edited Loading

codecov-commenter commented Sep 19, 2022 • edited Loading

Codecov Report

jackzampolin commented Sep 19, 2022

crodriguezvega commented Sep 20, 2022

chatton left a comment

Choose a reason for hiding this comment

charleenfei left a comment

Choose a reason for hiding this comment

charleenfei Sep 21, 2022

Choose a reason for hiding this comment

fix: ica handshake reopening channel - use `GetAppVersion` in favour of `channel.Version` #2302

fix: ica handshake reopening channel - use `GetAppVersion` in favour of `channel.Version` #2302

damiannolan Sep 19, 2022 •

edited

Loading

damiannolan commented Sep 19, 2022 •

edited

Loading

codecov-commenter commented Sep 19, 2022 •

edited

Loading