Upgrade Client #7367
Upgrade Client #7367
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7367 +/- ##
==========================================
+ Coverage 54.81% 54.96% +0.14%
==========================================
Files 587 588 +1
Lines 36512 36630 +118
==========================================
+ Hits 20014 20133 +119
- Misses 14406 14408 +2
+ Partials 2092 2089 -3 |
AdityaSripal
requested review from
aaronc,
alessio and
alexanderbez
as code owners
| var height = clienttypes.NewHeight(0, 4) | ||
| var ( | ||
| height = clienttypes.NewHeight(0, 4) | ||
| upgradeHeight = clienttypes.NewHeight(1, 1) |
There was a problem hiding this comment.
can we define these on ibctesting instead?
Co-authored-by: Federico Kunze <[email protected]>
…into aditya-upgrade-client
| Name: "alt-good", | ||
| Info: "new text here", | ||
| Height: 543210000, | ||
| UpgradedClientState: altCs, |
There was a problem hiding this comment.
this should overwrite with non IBC plan so the upgrade client state should be nil right?
There was a problem hiding this comment.
Correct, as mentioned in test case name
There was a problem hiding this comment.
I'm confused? Why is UpgradedClientState: altCs set then?
| func (p Plan) String() string { | ||
| due := p.DueAt() | ||
| dueUp := strings.ToUpper(due[0:1]) + due[1:] | ||
| var upgradedClientStr string | ||
| upgradedClient, err := clienttypes.UnpackClientState(p.UpgradedClientState) |
There was a problem hiding this comment.
I'd suggest checking if p.UpgradedClientState != nil instead of allowing unpack with a nil client state. The err on unpacking could be return instead of no upgraded client provided
There was a problem hiding this comment.
Can't return an error because that would break fmt.Stringer interface
There was a problem hiding this comment.
why can't you set upgradedClientStr = err.Error()?
| // UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces | ||
| func (p Plan) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error { | ||
| // UpgradedClientState may be nil | ||
| if p.UpgradedClientState == nil { |
There was a problem hiding this comment.
see above comment, this seems unnecessary. I'm a little confused why unpack doesn't succeed if this returns nil as well
…into aditya-upgrade-client
|
|
||
| // counterparty chain must commit the upgraded client with all client-customizable fields zeroed out | ||
| // at the upgrade path specified by current client | ||
| committedClient := upgradedClient.ZeroCustomFields() |
There was a problem hiding this comment.
Where do we check that the non-zeroed-out fields correspond to the old (current) fields? The relayer shouldn't be choosing these
There was a problem hiding this comment.
It's not generally possible because the client state structure may have changed. I just check that the chosen fields are consistent with unchosen fields.
There was a problem hiding this comment.
Can't the relayer change the trusting ratio or something then? Are we preventing that somewhere?
There was a problem hiding this comment.
I think the issue here is the client state could change to something radically different (a solo machine client is used in testing) which may not have a trusting ratio?
There was a problem hiding this comment.
hmm, that would be alright, but we still can't let the relayer change these fields in e.g. a Tendermint to Tendermint upgrade
Description
closes: #6378
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.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes