Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add handshake logic to ics29 #307
feat: Add handshake logic to ics29 #307
Changes from 18 commits
7b51ebd
150211d
ae319ea
b8f2a7a
fb8522b
1f62964
4711774
dac74bc
d190a19
2f73fd4
f49f0b4
4a73249
d622916
094fef9
763159a
f60897b
a167c49
bc90365
b9028a2
d37a7f7
3deee69
131e95a
40783bf
24d2a20
6b45015
56d80fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Separated IBCModule struct from AppModule
AppModule should only be instantiated once and will hook up to msg server, genesis, etc
IBCModule may be instantiated any number of times with different underlying applications
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'm pretty sure crossing hello's are not possible on INIT with auto-generated IDs. So I think I can remove this case. @colin-axner
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.
Crossing hello's are still possible. I'm not sure I understand why they wouldn't be? The counterparty channel identifiers are not specified until OpenTry
so in this case:
chainA init channel-1 for fees
chainB init channel-2 for fees (or even without fee version)
chainA attempts try for pairing to channel-2 with a non fee version
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.
Turns out this is irrelevant. We don't have to check here because core IBC checks that in case of crossing hellos,
the channel params in current TRY are exact same as the params in previous INIT: https://github.com/cosmos/ibc-go/blob/main/modules/core/04-channel/keeper/handshake.go#L131
which is exactly the behaviour we want out of core IBC, so we don't have to handle that case in the application.
I fixed the incorrect godoc in this PR.
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.
It is possible the version changed. If INIT started with non fee version and Ack returns with fee version, this would perform incorrect behavior
Since OpenTry chooses the version, my preference is still to set fee enabled on try/ack. Let the relayer decide how the channels will be used. I don't see the benefit of deciding on INIT. From my point of view, it reduces the code complexity
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.
No because in that case the entire counterparty version
fee29-1:ics20-1
would be passed down to transfer which would error.Though it's a good thing to test. I added a test case to prove it.
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.
We don't want future relayers to override the initial decisions of past relayers.
Future relayers should only complete the handshake as initial relayers intended it
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.
That makes sense. Can you add a inline comment to the code?
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.
middleware must also wrap ChannelKeeper and PortKeeper since middleware is a "2-way" wrapper between IBC and app