-
Notifications
You must be signed in to change notification settings - Fork 620
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
add protos, msgs, keeper handler to upgrade clients using v1 governance proposals #4436
Changes from 4 commits
f9ce4fd
5242278
f499db4
59514fe
a4922e7
70a431f
b2eee55
2c94f5f
5462ba6
776a7ce
efa5e43
dbbf6b9
eb99d94
77a2720
c418302
4cad828
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 |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package keeper_test | ||
|
||
import ( | ||
"errors" | ||
"math/rand" | ||
"testing" | ||
"time" | ||
|
@@ -12,7 +13,9 @@ import ( | |
"github.com/cosmos/cosmos-sdk/codec" | ||
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" | ||
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" | ||
|
||
tmbytes "github.com/cometbft/cometbft/libs/bytes" | ||
tmproto "github.com/cometbft/cometbft/proto/tendermint/types" | ||
|
@@ -483,3 +486,114 @@ func (suite *KeeperTestSuite) TestUnsetParams() { | |
suite.chainA.GetSimApp().IBCKeeper.ClientKeeper.GetParams(ctx) | ||
}) | ||
} | ||
|
||
// TestScheduleIBCClientUpgrade tests that an IBC client upgrade has been properly scheduled | ||
func (suite *KeeperTestSuite) TestScheduleIBCClientUpgrade() { | ||
var ( | ||
upgradedClientState *ibctm.ClientState | ||
oldPlan, plan upgradetypes.Plan | ||
expError error | ||
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. is it possible to |
||
) | ||
|
||
testCases := []struct { | ||
name string | ||
malleate func() | ||
expPass bool | ||
}{ | ||
{ | ||
"valid upgrade proposal", | ||
func() {}, | ||
true, | ||
}, | ||
{ | ||
"valid upgrade proposal with previous IBC state", func() { | ||
oldPlan = upgradetypes.Plan{ | ||
Name: "upgrade IBC clients", | ||
Height: 100, | ||
} | ||
}, true, | ||
}, | ||
{ | ||
"fail: scheduling upgrade with plan height 0", | ||
func() { | ||
plan.Height = 0 | ||
expError = sdkerrors.ErrInvalidRequest | ||
damiannolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
false, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
tc := tc | ||
|
||
suite.Run(tc.name, func() { | ||
suite.SetupTest() // reset | ||
oldPlan.Height = 0 // reset | ||
|
||
path := ibctesting.NewPath(suite.chainA, suite.chainB) | ||
suite.coordinator.SetupClients(path) | ||
upgradedClientState = suite.chainA.GetClientState(path.EndpointA.ClientID).ZeroCustomFields().(*ibctm.ClientState) | ||
|
||
// use height 1000 to distinguish from old plan | ||
plan = upgradetypes.Plan{ | ||
Name: "upgrade IBC clients", | ||
Height: 1000, | ||
} | ||
|
||
tc.malleate() | ||
|
||
// set the old plan if it is not empty | ||
if oldPlan.Height != 0 { | ||
// set upgrade plan in the upgrade store | ||
store := suite.chainA.GetContext().KVStore(suite.chainA.GetSimApp().GetKey(upgradetypes.StoreKey)) | ||
bz := suite.chainA.App.AppCodec().MustMarshal(&oldPlan) | ||
store.Set(upgradetypes.PlanKey(), bz) | ||
|
||
bz, err := types.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClientState) | ||
suite.Require().NoError(err) | ||
|
||
suite.chainA.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainA.GetContext(), oldPlan.Height, bz) //nolint:errcheck | ||
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. may as well check the error instead of having the |
||
} | ||
|
||
err := suite.chainA.App.GetIBCKeeper().ClientKeeper.ScheduleIBCClientUpgrade(suite.chainA.GetContext(), plan, upgradedClientState) | ||
|
||
if tc.expPass { | ||
suite.Require().NoError(err) | ||
|
||
// check that the correct plan is returned | ||
storedPlan, found := suite.chainA.GetSimApp().UpgradeKeeper.GetUpgradePlan(suite.chainA.GetContext()) | ||
suite.Require().True(found) | ||
suite.Require().Equal(plan, storedPlan) | ||
|
||
// check that old upgraded client state is cleared | ||
_, found = suite.chainA.GetSimApp().UpgradeKeeper.GetUpgradedClient(suite.chainA.GetContext(), oldPlan.Height) | ||
suite.Require().False(found) | ||
|
||
// check that client state was set | ||
storedClientState, found := suite.chainA.GetSimApp().UpgradeKeeper.GetUpgradedClient(suite.chainA.GetContext(), plan.Height) | ||
suite.Require().True(found) | ||
clientState, err := types.UnmarshalClientState(suite.chainA.App.AppCodec(), storedClientState) | ||
suite.Require().NoError(err) | ||
suite.Require().Equal(upgradedClientState, clientState) | ||
} else { | ||
suite.Require().True(errors.Is(err, expError)) | ||
|
||
// check that the new plan wasn't stored | ||
storedPlan, found := suite.chainA.GetSimApp().UpgradeKeeper.GetUpgradePlan(suite.chainA.GetContext()) | ||
if oldPlan.Height != 0 { | ||
// NOTE: this is only true if the ScheduleUpgrade function | ||
// returns an error before clearing the old plan | ||
suite.Require().True(found) | ||
suite.Require().Equal(oldPlan, storedPlan) | ||
} else { | ||
suite.Require().False(found) | ||
suite.Require().Empty(storedPlan) | ||
} | ||
|
||
// check that client state was not set | ||
_, found = suite.chainA.GetSimApp().UpgradeKeeper.GetUpgradedClient(suite.chainA.GetContext(), plan.Height) | ||
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. nit: I have a preference for not discarding args as much as possible. We can do an assert empty or assert default struct of this value I think, right? |
||
suite.Require().False(found) | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,23 @@ | ||
package types_test | ||
|
||
import ( | ||
"errors" | ||
"testing" | ||
"time" | ||
|
||
"github.com/golang/protobuf/proto" //nolint:staticcheck | ||
"github.com/stretchr/testify/require" | ||
testifysuite "github.com/stretchr/testify/suite" | ||
|
||
codectypes "github.com/cosmos/cosmos-sdk/codec/types" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
damiannolan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" | ||
|
||
"github.com/cosmos/ibc-go/v7/modules/core/02-client/types" | ||
commitmenttypes "github.com/cosmos/ibc-go/v7/modules/core/23-commitment/types" | ||
ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors" | ||
"github.com/cosmos/ibc-go/v7/modules/core/exported" | ||
solomachine "github.com/cosmos/ibc-go/v7/modules/light-clients/06-solomachine" | ||
ibctm "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint" | ||
ibctesting "github.com/cosmos/ibc-go/v7/testing" | ||
|
@@ -678,3 +684,155 @@ func TestMsgUpdateParamsGetSigners(t *testing.T) { | |
} | ||
} | ||
} | ||
|
||
// TestMsgScheduleIBCClientUpgrade_NewMsgScheduleIBCClientUpgrade tests NewMsgScheduleIBCClientUpgrade | ||
func (suite *TypesTestSuite) TestMsgScheduleIBCClientUpgrade_NewMsgScheduleIBCClientUpgrade() { | ||
testCases := []struct { | ||
name string | ||
upgradedClientState exported.ClientState | ||
expPass bool | ||
}{ | ||
{ | ||
"success", | ||
ibctm.NewClientState(suite.chainA.ChainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), | ||
true, | ||
}, | ||
{ | ||
"fail: failed to pack ClientState", | ||
nil, | ||
false, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
plan := upgradetypes.Plan{ | ||
Name: "upgrade IBC clients", | ||
Height: 1000, | ||
} | ||
_, err := types.NewMsgScheduleIBCClientUpgrade( | ||
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 we could not discard this and make some assertions on expected value in expPass conditional |
||
ibctesting.TestAccAddress, | ||
plan, | ||
tc.upgradedClientState, | ||
) | ||
|
||
if tc.expPass { | ||
suite.Require().NoError(err) | ||
} else { | ||
suite.Require().True(errors.Is(err, ibcerrors.ErrPackAny)) | ||
} | ||
} | ||
} | ||
|
||
// TestMsgScheduleIBCClientUpgrade_GetSigners tests GetSigners for MsgScheduleIBCClientUpgrade | ||
func (suite *TypesTestSuite) TestMsgScheduleIBCClientUpgrade_GetSigners() { | ||
testCases := []struct { | ||
name string | ||
address sdk.AccAddress | ||
expPass bool | ||
}{ | ||
{ | ||
"success: valid address", | ||
sdk.AccAddress(ibctesting.TestAccAddress), | ||
true, | ||
}, | ||
{ | ||
"failure: nil address", | ||
nil, | ||
false, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
clientState := ibctm.NewClientState(suite.chainA.ChainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) | ||
plan := upgradetypes.Plan{ | ||
Name: "upgrade IBC clients", | ||
Height: 1000, | ||
} | ||
msg, err := types.NewMsgScheduleIBCClientUpgrade( | ||
tc.address.String(), | ||
plan, | ||
clientState, | ||
) | ||
suite.Require().NoError(err) | ||
|
||
if tc.expPass { | ||
suite.Require().Equal([]sdk.AccAddress{tc.address}, msg.GetSigners()) | ||
} else { | ||
suite.Require().Panics(func() { msg.GetSigners() }) | ||
} | ||
} | ||
} | ||
|
||
// TestMsgScheduleIBCClientUpgrade_ValidateBasic tests ValidateBasic for MsgScheduleIBCClientUpgrade | ||
func (suite *TypesTestSuite) TestMsgScheduleIBCClientUpgrade_ValidateBasic() { | ||
var ( | ||
authority string | ||
plan upgradetypes.Plan | ||
anyClient *codectypes.Any | ||
expError error | ||
) | ||
testCases := []struct { | ||
name string | ||
malleate func() | ||
expPass bool | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}{ | ||
{ | ||
"success", | ||
func() {}, | ||
true, | ||
}, | ||
{ | ||
"failure: invalid authority address", | ||
func() { | ||
authority = "invalid" | ||
expError = ibcerrors.ErrInvalidAddress | ||
}, | ||
false, | ||
}, | ||
{ | ||
"failure: error unpacking client state", | ||
func() { | ||
anyClient = &codectypes.Any{} | ||
expError = ibcerrors.ErrUnpackAny | ||
}, | ||
false, | ||
}, | ||
{ | ||
"failure: error validating upgrade plan, height is not greater than zero", | ||
func() { | ||
plan.Height = 0 | ||
expError = sdkerrors.ErrInvalidRequest | ||
}, | ||
false, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
authority = ibctesting.TestAccAddress | ||
plan = upgradetypes.Plan{ | ||
Name: "upgrade IBC clients", | ||
Height: 1000, | ||
} | ||
upgradedClientState := ibctm.NewClientState(suite.chainA.ChainID, ibctesting.DefaultTrustLevel, ibctesting.TrustingPeriod, ibctesting.UnbondingPeriod, ibctesting.MaxClockDrift, clientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath) | ||
var err error | ||
anyClient, err = types.PackClientState(upgradedClientState) | ||
suite.Require().NoError(err) | ||
|
||
tc.malleate() | ||
|
||
msg := types.MsgScheduleIBCClientUpgrade{ | ||
authority, | ||
plan, | ||
anyClient, | ||
} | ||
|
||
err = msg.ValidateBasic() | ||
|
||
if tc.expPass { | ||
suite.Require().NoError(err) | ||
} | ||
if expError != nil { | ||
suite.Require().True(errors.Is(err, expError)) | ||
} | ||
} | ||
} |
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.
do we want a separate event for this?
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 would lean towards not for this function, as this function calls the upgradeKeeper to schedule a client upgrade so i think the event should remain the same.
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.
potentially could be different for recover client tho
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.
Yes we should, opened #4507