imp(ica/host): removed previous version validation check

[srdtrk]

Description

closes: #5605

Commit Message / Changelog Entry

imp(ica/host): removed previous version validation check

see the guidelines for commit messages. (view raw markdown for examples)

---

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 the 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 and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/) or specification (x/<module>/spec/).
• Added relevant godoc comments.
• Provide a commit message to be used for the changelog entry in the PR description for review.
• Re-reviewed Files changed in the Github PR explorer.
• Review Codecov Report in the comment section below once CI passes.

[srdtrk]

Review this carefully and consider security implications before approving

[colin-axner]

just for my own reasoning. The previous metadata has:
version, controller/host connection id's, address, encoding, tx type

Only the host connection id and address cannot change? The host connection id check above in ValidateHostMetadata, but is the address? What happens if the controller reopens a channel modifying the interchain acc address to be different than what it is. It looks like it'll be overwritten later on. I feel like we should document these cases or make them more explicit

[damiannolan]

If I'm understanding correctly, the address is looked up with a key built from host connection id and counterparty port id, right? And counterparty port ID needs to proven by core before getting here (correction: is used for the key in proving, not value) and if host connection id cannot change then it should be all fine.

[colin-axner]

It's just a bit odd to me because the relayer can pass in:
metadata.Address = invalid-address
and our code won't error
it'll return
metadata.Address = valid-address

just feels a bit odd if a middleware relies on the value before the callback

[srdtrk]

That's right. Moreover, the removed code (IsPreviousMetadataEqual) never actually checked if the addresses were the same... It only checked other fields

[srdtrk]

So this was always possible

[srdtrk]

But this damages the backwards compatibility if we check the address since the users are used to being able to pass invalid stuff there (it is common to pass ""! This is because a normal channel handshake must start with this field empty, so users just usually reuse the same version string, including my hackathon project. There are other reasons it is convenient to pass "").

Try step allows the app to propose a new version anyway, so any middleware should use the result of the Try step, and not what the counterparty proposes. Also, #5533 is going to allow host connection id to be fully empty anyway.

So I suggest we don't add these checks, but if you still want the check, then I can add, I suspect it will break some stuff for our users initially.

[colin-axner]

ah okay, thanks for the context. Should we add a comment in case someone has the same question as me?

maybe something like:

// if a channel is being reopened, we allow the controller to propose new fields
// which are not exactly the same as the previous. The provided address will 
// be overwritten with the correct one before the metadata is returned

[srdtrk]

Added the comment

[crodriguezvega]

Should we check at least that version is ics27-1?

[srdtrk]

This is still being checked here @crodriguezvega

[damiannolan]

Spent a few mins to reason through flows with this check being removed! I think it should be all good and makes sense. Most of the main checks are still done in ValidateHostMetadata

LGTM! 🙏🏻

[damiannolan]

And then set the new encoding to be protobuf.

This is because path.EndpointA.ChannelConfig.Version is passed as counterpartyVersion on L312, correct?
Maybe we could make that more explicit in the comment?

[colin-axner]

Thanks @srdtrk, appreciate the followup on my comment!