imp: remove kv generator and construct expected channel for both single and multi hop cases #45
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| connectionHops, kvGenerator); err != nil { | ||
| // get the last hop connection on the other side of the multihop channel | ||
| // the last hop connection is the connection end on the chain before the counterparty multihop chain | ||
| lastHopConnectionEnd, err := multihopProof.GetLastHopConnectionEnd(k.cdc, connectionEnd) |
There was a problem hiding this comment.
There is still some leakage of the multi hop abstraction at 04-channel, but I couldn't see any way around it. Open to ideas.
There was a problem hiding this comment.
I took a deeper look at this today and I think I see a path forward, but it is annoying due to import cycles. I am working on a branch to merge multi-hop and single hop logic into the existing VerifyXXX() functions by changing the single hop proof into a multi-hop proof. The multi-hop proof logic then degenerates into standard single hop proof logic.
However, I need to to some moderate refactoring to get this approach to work. I'll share a draft PR so you can see the approach hopefully next week. The main changes are to:
- move multihop message definitions under commitment types (at least not under channel/tx types)
- remove dependency on tmclient from the multihop verification code (need to break import cycle and shouldn't really be hardcoded to tm anyway...)
- need to add connectionHops as arg to connection keeper interface
- add a multihop membership verification method to client state interface and implement for tendermint
Hopefully I can do this in a way where I can pick just one verification function and show how it would look. This would also remove the key generator functions as well.
| @@ -394,43 +370,38 @@ func (k Keeper) ChanOpenConfirm( | |||
| ) | |||
| } | |||
|
|
|||
| // verify multihop proof or standard proof | |||
| var counterpartyHops []string | |||
There was a problem hiding this comment.
The code below setting the counterparty hops is pretty much repeated in all handlers, so we could rug it under some common function.
Description
This PR builds up on @AdityaSripal's PR #43. It tries to address this comment by @dshiell so that the expected channel is constructed with the right counterparty hops in both the single and multi hop cases. In the process I have removed the KV generator from the channel open handshake handlers. If this considered good to proceed with, I can look into doing a similar refactor in the packet and timeout handlers.
Test suite for multi hop is failing, since it required changes from #43.
closes: #XXXX
Commit Message / Changelog Entry
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.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.