-
Notifications
You must be signed in to change notification settings - Fork 627
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 9 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 |
---|---|---|
|
@@ -12,7 +12,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 +485,113 @@ func (suite *KeeperTestSuite) TestUnsetParams() { | |
suite.chainA.GetSimApp().IBCKeeper.ClientKeeper.GetParams(ctx) | ||
}) | ||
} | ||
|
||
// TestIBCSoftwareUpgrade tests that an IBC client upgrade has been properly scheduled | ||
func (suite *KeeperTestSuite) TestIBCSoftwareUpgrade() { | ||
var ( | ||
upgradedClientState *ibctm.ClientState | ||
oldPlan, plan upgradetypes.Plan | ||
) | ||
|
||
testCases := []struct { | ||
name string | ||
malleate func() | ||
expError error | ||
}{ | ||
{ | ||
"valid upgrade proposal", | ||
func() {}, | ||
nil, | ||
}, | ||
{ | ||
"valid upgrade proposal with previous IBC state", func() { | ||
oldPlan = upgradetypes.Plan{ | ||
Name: "upgrade IBC clients", | ||
Height: 100, | ||
} | ||
}, | ||
nil, | ||
}, | ||
{ | ||
"fail: scheduling upgrade with plan height 0", | ||
func() { | ||
plan.Height = 0 | ||
}, | ||
sdkerrors.ErrInvalidRequest, | ||
}, | ||
} | ||
|
||
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.Require().NoError(suite.chainA.GetSimApp().UpgradeKeeper.SetUpgradedClient(suite.chainA.GetContext(), oldPlan.Height, bz)) | ||
} | ||
Comment on lines
+543
to
+554
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. There's probably some code magic we could do here by adding the logic to setting an old plan and checking the expected result only in the test cases that use it. Maybe we can open as a followup issue (I know this comes from the existing code). I'd expect only the test cases needing an existing plan to execute that logic (maybe by calling |
||
|
||
err := suite.chainA.App.GetIBCKeeper().ClientKeeper.ScheduleIBCSoftwareUpgrade(suite.chainA.GetContext(), plan, upgradedClientState) | ||
|
||
if tc.expError == nil { | ||
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 | ||
cs, found := suite.chainA.GetSimApp().UpgradeKeeper.GetUpgradedClient(suite.chainA.GetContext(), oldPlan.Height) | ||
suite.Require().False(found) | ||
suite.Require().Empty(cs) | ||
|
||
// 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 { | ||
// 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 | ||
cs, found := suite.chainA.GetSimApp().UpgradeKeeper.GetUpgradedClient(suite.chainA.GetContext(), plan.Height) | ||
suite.Require().Empty(cs) | ||
suite.Require().False(found) | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
|
||
codectypes "github.com/cosmos/cosmos-sdk/codec/types" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types" | ||
|
||
host "github.com/cosmos/ibc-go/v7/modules/core/24-host" | ||
ibcerrors "github.com/cosmos/ibc-go/v7/modules/core/errors" | ||
|
@@ -283,3 +284,39 @@ func (msg *MsgUpdateParams) ValidateBasic() error { | |
} | ||
return msg.Params.Validate() | ||
} | ||
|
||
// NewMsgIBCSoftwareUpgrade creates a new MsgIBCSoftwareUpgrade instance | ||
func NewMsgIBCSoftwareUpgrade(authority string, plan upgradetypes.Plan, upgradedClientState exported.ClientState) (*MsgIBCSoftwareUpgrade, error) { | ||
charleenfei marked this conversation as resolved.
Show resolved
Hide resolved
|
||
anyClient, err := PackClientState(upgradedClientState) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return &MsgIBCSoftwareUpgrade{ | ||
Authority: authority, | ||
Plan: plan, | ||
UpgradedClientState: anyClient, | ||
}, nil | ||
} | ||
|
||
// GetSigners returns the expected signers for a MsgIBCSoftwareUpgrade message. | ||
func (msg *MsgIBCSoftwareUpgrade) GetSigners() []sdk.AccAddress { | ||
accAddr, err := sdk.AccAddressFromBech32(msg.Authority) | ||
if err != nil { | ||
panic(err) | ||
} | ||
return []sdk.AccAddress{accAddr} | ||
} | ||
|
||
// ValidateBasic performs basic checks on a MsgIBCSoftwareUpgrade. | ||
func (msg *MsgIBCSoftwareUpgrade) ValidateBasic() 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. Considering that for the time being we're implicitly on a tendermint chain when using ibc-go should we add a check on the provided client state that cc. @colin-axner 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. Fine by me |
||
if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { | ||
return errorsmod.Wrapf(ibcerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) | ||
} | ||
|
||
if _, err := UnpackClientState(msg.UpgradedClientState); err != nil { | ||
return err | ||
} | ||
|
||
return msg.Plan.ValidateBasic() | ||
} |
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