multihop: some improvements (naming, documentation, errors) #41
Conversation
| @@ -152,57 +152,34 @@ func (k Keeper) ChanOpenTry( | |||
| ) | |||
| } | |||
|
|
|||
| // check version support | |||
| versionCheckFunc := func(connection *connectiontypes.ConnectionEnd) error { | |||
There was a problem hiding this comment.
I turned this into a function in line 732 below.
| return nil, err | ||
| } | ||
| } else { | ||
| panic("") // TODO: is this reachable? |
There was a problem hiding this comment.
I added this, but it should not reach here, right? So would it be fine to panic?
There was a problem hiding this comment.
Yes, this looks correct to me.
The alternative would be to pass in and return the known counterparty consensus state if there are no intermediate connection proofs? Possibly could help merge the single-hop vs multi-hop logic?
| var connectionEnd ConnectionEnd | ||
| if len(m.ConnectionProofs) > 0 { | ||
| // connection proofs are ordered from executing chain to counterparty, | ||
| // so the last hop connection end is the value of last connection proof | ||
| if err := cdc.Unmarshal(m.ConnectionProofs[len(m.ConnectionProofs)-1].Value, &connectionEnd); err != nil { | ||
| return nil, err | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Is the else statement reachable? If it is, I don't understand how returning the same connection end as the function received as a parameter would be correct...
There was a problem hiding this comment.
Yeah, it's been a bit since I wrote this, but I believe I designed the multi-hop code so that it could also work for the case of a single-hop. In that case, no extra connection proofs are required because we already store the IBC connection for the counterparty. i.e. this is handling the degenerate case when multi-hop works like single-hop.
I was hoping that somehow the single-hop and multi-hop verification code could merge one day.
There was a problem hiding this comment.
I was hoping that somehow the single-hop and multi-hop verification code could merge one day.
Yes, that would be great! I was also thinking about it the other day. It would make the code easier to reason about. I will try to see if I can come up with something, or ask later on the rest of the team if they would have any ideas.
There was a problem hiding this comment.
Currently the connectionKeeper interface has verification methods for each separate proof type (channel try/ack, recv packer, ack packet, etc). But really they are only needed because the derived key/values are passed in and need to be computed differently for each case. The multihop verification method allows passing in a key/value generation function which could generalize this.
The next issue is the proof format being different, but if verification could work similarly given a single-hop or multi-hop proof method its seems possible to collapse all the methods down to: VerifyMembership and VerifyNonmembership.
Anyway, these were a few thoughts I had a while back.
| // parse the client ID from the the consensusState key path | ||
| clientID, err := parseClientIDFromKey(consensusProof.PrefixedKey.KeyPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // check that the client ID matches the client ID in the connectionEnd on the same hop. | ||
| if connectionEnd.ClientId != clientID { | ||
| return fmt.Errorf("consensus state client id (%s) does not match expected client id (%s)", | ||
| connectionEnd.ClientId, | ||
| clientID, | ||
| ) | ||
| } |
There was a problem hiding this comment.
I moved this validation here from verifyIntermediateStates because it's not verifying any proofs.
| // from the executing chain (but excluding it) to the counterparty. | ||
| // - connectionHops contains the connection IDs for the hops from the executing chain to the counterparty, but it does not include the | ||
| // connection ID for the first hop on the executing chain. | ||
| func validateIntermediateStates( |
There was a problem hiding this comment.
I renamed the function to use the word validate instead of verify because it does not do proof verification. It's a distinction that we like to make in ibc-go. :)
| prefix := lastHopConnectionEnd.GetCounterparty().GetPrefix() | ||
| path, err := commitmenttypes.ApplyPrefix(prefix, commitmenttypes.NewMerklePath(key)) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Instead of passing prefix and key to VerifyMultihopMembership and VerifyMultihopNonMembership I decided to construct the path here and pass only one parameter to those two functions.
| var connectionEnd ConnectionEnd | ||
| if len(m.ConnectionProofs) > 0 { | ||
| // connection proofs are ordered from executing chain to counterparty, | ||
| // so the last hop connection end is the value of last connection proof | ||
| if err := cdc.Unmarshal(m.ConnectionProofs[len(m.ConnectionProofs)-1].Value, &connectionEnd); err != nil { | ||
| return nil, err | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Yeah, it's been a bit since I wrote this, but I believe I designed the multi-hop code so that it could also work for the case of a single-hop. In that case, no extra connection proofs are required because we already store the IBC connection for the counterparty. i.e. this is handling the degenerate case when multi-hop works like single-hop.
I was hoping that somehow the single-hop and multi-hop verification code could merge one day.
| return nil, err | ||
| } | ||
| } else { | ||
| panic("") // TODO: is this reachable? |
There was a problem hiding this comment.
Yes, this looks correct to me.
The alternative would be to pass in and return the known counterparty consensus state if there are no intermediate connection proofs? Possibly could help merge the single-hop vs multi-hop logic?
dshiell
merged commit b5d5877
into
polymerdao:polymer/multihop-main
Description
Some (hopefully) improvements to align with idc-go's code guidelines (e.g. regarding error reporting).
Renamed some variables, functions to (hopefully) more clarifying names (at least for me 😄).
Added extra comments and godocs.
I will open more PRs as I continue to go through the code.
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.