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

Review channel version code and cleanup #3213

Closed
5 tasks
Tracked by #3217
ancazamfir opened this issue Apr 2, 2023 · 4 comments
Closed
5 tasks
Tracked by #3217

Review channel version code and cleanup #3213

ancazamfir opened this issue Apr 2, 2023 · 4 comments
Assignees
Milestone

Comments

@ancazamfir
Copy link
Collaborator

Summary of Bug

We should cleanup the code around channel version according to the changes in cosmos/ibc#628

Version

Steps to Reproduce

Acceptance Criteria


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@github-project-automation github-project-automation bot moved this to 🩹 Triage in Hermes Apr 2, 2023
@seanchen1991 seanchen1991 moved this from 🩹 Triage to 📥 Todo in Hermes Apr 4, 2023
@seanchen1991 seanchen1991 self-assigned this Apr 4, 2023
@seanchen1991 seanchen1991 added this to the v1.5 milestone Apr 11, 2023
@seanchen1991
Copy link
Contributor

@ancazamfir could you clarify what changes need to occur in Hermes in order to satisfy the changes proposed in cosmos/ibc#628?

@romac
Copy link
Member

romac commented Apr 14, 2023

@seanchen1991 For open init, I think it's just a matter of turning this

// If the user supplied a version, use that.
// Otherwise, either use the version defined for the `transfer`
// or an empty version if the port is non-standard.
let version = self
.dst_version()
.cloned()
.or_else(|| version::default_by_port(self.dst_port_id()))
.unwrap_or_else(|| {
warn!(
chain = %self.dst_chain().id(),
channel = ?self.dst_channel_id(),
port = %self.dst_port_id(),
"no version specified for the channel, falling back on empty version"
);
Version::empty()
});

into something like this

        // If the user supplied a version, use that.
        // Otherwise, use the empty version and let the app decide on it.
        let version = self.dst_version().cloned().unwrap_or_else(Version::empty);

And remove default_by_port.

@romac
Copy link
Member

romac commented Apr 14, 2023

For open try, we already use the version that the app decided on, so there is nothing to do here.

// Re-use the version that was either set on ChanOpenInit or overwritten by the application.
let version = src_channel.version().clone();

We could pass the empty version here since the app is going to ignore it anyway, but I don't believe it's necessary.

@seanchen1991 seanchen1991 moved this from 📥 Todo to 🏗 In progress in Hermes Apr 24, 2023
@seanchen1991
Copy link
Contributor

Closing this as it seems like most chains have not incorporated these changes yet, so they aren't quite relevant for now. We can revisit if/when these changes become necessary.

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Hermes Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants