-
Notifications
You must be signed in to change notification settings - Fork 679
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: allow scaling of trusting period for client upgrades #8185
base: main
Are you sure you want to change the base?
Changes from all commits
f0bc826
d1a65a2
0c5f489
a033427
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package tendermint_test | |
|
||
import ( | ||
"errors" | ||
"time" | ||
|
||
upgradetypes "cosmossdk.io/x/upgrade/types" | ||
|
||
|
@@ -86,6 +87,38 @@ func (suite *TendermintTestSuite) TestVerifyUpgrade() { | |
}, | ||
expErr: nil, | ||
}, | ||
{ | ||
name: "successful upgrade scales trusting period with unbonding period decrease", | ||
setup: func() { | ||
newUnbondingPeriod := time.Hour * 24 * 7 * 2 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lemme know if you want a dedicated test func to perform assertions on new trusting period. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to assert the new trusting period is what we expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, I'll add a separate test func for this. Will try to get around to it tomorrow or so |
||
upgradedClient = ibctm.NewClientState(suite.chainB.ChainID, ibctm.DefaultTrustLevel, trustingPeriod, newUnbondingPeriod, maxClockDrift, clienttypes.NewHeight(clienttypes.ParseChainID(suite.chainB.ChainID), upgradedClient.(*ibctm.ClientState).LatestHeight.GetRevisionHeight()+10), commitmenttypes.GetSDKSpecs(), upgradePath) | ||
upgradedClient = upgradedClient.(*ibctm.ClientState).ZeroCustomFields() | ||
upgradedClientBz, err = clienttypes.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClient) | ||
suite.Require().NoError(err) | ||
|
||
// upgrade Height is at next block | ||
lastHeight = clienttypes.NewHeight(0, uint64(suite.chainB.GetContext().BlockHeight()+1)) | ||
|
||
// zero custom fields and store in upgrade store | ||
suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedClientBz) //nolint:errcheck // ignore error for test | ||
suite.chainB.GetSimApp().UpgradeKeeper.SetUpgradedConsensusState(suite.chainB.GetContext(), int64(lastHeight.GetRevisionHeight()), upgradedConsStateBz) //nolint:errcheck // ignore error for test | ||
|
||
// commit upgrade store changes and update clients | ||
|
||
suite.coordinator.CommitBlock(suite.chainB) | ||
err := path.EndpointA.UpdateClient() | ||
suite.Require().NoError(err) | ||
|
||
cs, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientState(suite.chainA.GetContext(), path.EndpointA.ClientID) | ||
suite.Require().True(found) | ||
tmCs, ok := cs.(*ibctm.ClientState) | ||
suite.Require().True(ok) | ||
|
||
upgradedClientProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedClientKey(int64(lastHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight()) | ||
upgradedConsensusStateProof, _ = suite.chainB.QueryUpgradeProof(upgradetypes.UpgradedConsStateKey(int64(lastHeight.GetRevisionHeight())), tmCs.LatestHeight.GetRevisionHeight()) | ||
}, | ||
expErr: nil, | ||
}, | ||
{ | ||
name: "unsuccessful upgrade: upgrade path not set", | ||
setup: func() { | ||
|
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.
Isn't it much simpler to just do:
This is equivalent to what you have if I'm not mistaken
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.
lol, yes, I think they are equivalent. Happy to use this if you find it more readable. I can update!