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

Set channel version at the application level #3267

Closed
wants to merge 11 commits into from

Conversation

seanchen1991
Copy link
Contributor

Closes: #3213

Description


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • 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).

@seanchen1991 seanchen1991 requested a review from romac April 24, 2023 15:24
@romac
Copy link
Member

romac commented Apr 24, 2023

Perhaps we should actually keep the counterparty version set in the OpenTry message and see if that fixes some tests.

The ordered channel test is apparently using an old version of ibc-go (v3) which seems to require the version at the open init step. We should see if we can update it to a newer version.

@romac
Copy link
Member

romac commented Apr 24, 2023

Nevermind, looks like pretty much all tests are failing, even on Gaia 8. Maybe we should just drop this PR then, or at least clarify the expected behavior with the ibc-go team.

@ljoss17
Copy link
Contributor

ljoss17 commented Apr 25, 2023

It seems that the version which is deprecated is the one in the ChannelEnd in the MsgChannelOpenTry https://github.com/cosmos/ibc-go/blob/v7.0.0/proto/ibc/core/channel/v1/tx.proto#L84.
I restored the the couterparty_version as it is still needed. This should fix the tests

@romac
Copy link
Member

romac commented Apr 25, 2023

Looks like gaia8 (and likely other chains using the same ibc-go version) still expects a version to be set at chan open init, so perhaps we should keep doing that as well.

@romac
Copy link
Member

romac commented Apr 25, 2023

Given the resulting change I wonder if we shouldn't just drop the PR altogether? Do we foresee any issues with setting the try version to its init counterpart? If not then let's drop it.

@ljoss17
Copy link
Contributor

ljoss17 commented Apr 25, 2023

I agree, the deprecated version in the Channel field doesn't seem to bring much to Hermes. I would be keen to drop this PR

@seanchen1991 seanchen1991 deleted the sean/cleanup-channel-version branch April 25, 2023 13:04
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.

Review channel version code and cleanup
3 participants