-
Notifications
You must be signed in to change notification settings - Fork 92
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
Refactor channel handshake #217
Conversation
* add failing test * bug fix * changelog
…hannel-proof-verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks great. 👌 I noticed some inconsistencies with ibc-go (basically missing checks on our side) related to messages -> https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/types/msgs.go. I think we're missing similar checks for client and conn messages too. (Will open an issue to track them)
Let us address this in a separate PR.
} | ||
|
||
// An OPEN IBC connection running on the local (host) chain should exist. | ||
if channel_end.connection_hops().len() != 1 { | ||
if chan_end_on_b.connection_hops().len() != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move stateless checks like these into the TryFrom<RawMsg>
conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there are a bunch of checks in ibc-go that we don't have in our handlers -> https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/types/msgs.go
For e.g. ibc-go fails a MsgChanOpenInit
with whose state isn't Init
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move stateless checks like these into the TryFrom conversion.
This chan_end_on_b
comes from the context though, so no TryFrom<Raw>
.
Also in general I prefer having all the validation in the same place. Otherwise it's hard to know what are all the checks that are being applied. We can discuss in the issue #233.
* chan open init refactored * fmt * chan open init minor reorg * chan_open_try: apply naming convention * inline proof verification in chan_open_try * remove use of `Proofs` in MsgChannelOpenTry * Fix counterparty channel id unwrap (#220) * add failing test * bug fix * changelog * fix test compilation * ChanOpenAck msg update * refactor chan_open_ack * refactor mut * ChanOpenConfirm refactor msg * refactor chan_open_confirm * chan_close_init handler refactor * chan_close_confirm refactor * changelog * chan_open_init ctx_a
Closes: #166
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.