self review bug and test fixes

CHANGELOG.md

@@ -50,7 +50,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (x/distribution) [\#8473](https://github.com/cosmos/cosmos-sdk/pull/8473) On genesis init, if the distribution module account balance, coming from bank module state, does not match the one in distribution module state, the initialization will panic.
 * (client/keys) [\#8500](https://github.com/cosmos/cosmos-sdk/pull/8500) `InfoImporter` interface is removed from legacy keybase.
 * [\#8629](https://github.com/cosmos/cosmos-sdk/pull/8629) Deprecated `SetFullFundraiserPath` from `Config` in favor of `SetPurpose` and `SetCoinType`.
-* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove x/ibc imports from x/upgrade by replacing plan.UpgradedClient with a `[]byte` instead of using an `Any`. IBC upgrade begin blocker logic moved to the IBC module.
+* (x/upgrade) [\#8673](https://github.com/cosmos/cosmos-sdk/pull/8673) Remove IBC logic from x/upgrade. Deprecates IBC fields in an Upgrade Plan. IBC upgrade logic moved to 02-client and an IBC UpgradeProposal is added. 
 
 ### State Machine Breaking
 

x/ibc/core/02-client/keeper/proposal.go

@@ -79,7 +79,7 @@ func (k Keeper) HandleUpgradeProposal(ctx sdk.Context, p *types.UpgradeProposal)
 	// clear any old IBC state stored by previous plan
 	oldPlan, found := k.GetUpgradePlan(ctx)
 	if found {
-		k.upgradeKeeper.ClearIBCState(ctx, oldPlan.Height-1)
+		k.upgradeKeeper.ClearIBCState(ctx, oldPlan.Height)
 	}
 
 	clientState, err := types.UnpackClientState(p.UpgradedClientState)

x/ibc/core/02-client/keeper/proposal_test.go

@@ -211,6 +211,10 @@ func (suite *KeeperTestSuite) TestHandleUpgradeProposal() {
 				store := suite.chainA.GetContext().KVStore(suite.chainA.App.GetKey(upgradetypes.StoreKey))
 				bz := suite.chainA.App.AppCodec().MustMarshalBinaryBare(&oldPlan)
 				store.Set(upgradetypes.PlanKey(), bz)
+
+				bz, err := types.MarshalClientState(suite.chainA.App.AppCodec(), upgradedClientState)
+				suite.Require().NoError(err)
+				suite.chainA.App.UpgradeKeeper.SetUpgradedClient(suite.chainA.GetContext(), oldPlan.Height, bz)
 			}
 
 			upgradeProp, ok := content.(*types.UpgradeProposal)
@@ -225,6 +229,10 @@ func (suite *KeeperTestSuite) TestHandleUpgradeProposal() {
 				suite.Require().True(found)
 				suite.Require().Equal(plan, storedPlan)
 
+				// check that old upgraded client state is cleared
+				_, found = suite.chainA.App.UpgradeKeeper.GetUpgradedClient(suite.chainA.GetContext(), oldPlan.Height)
+				suite.Require().False(found)
+
 				// check that client state was set
 				storedClientState, found := suite.chainA.App.UpgradeKeeper.GetUpgradedClient(suite.chainA.GetContext(), plan.Height)
 				suite.Require().True(found)

x/ibc/core/02-client/types/proposal.go

@@ -89,13 +89,13 @@ func NewUpgradeProposal(title, description string, plan upgradetypes.Plan, upgra
 	}, nil
 }
 
-// GetTitle returns the title of a client update proposal.
+// GetTitle returns the title of a upgrade proposal.
 func (up *UpgradeProposal) GetTitle() string { return up.Title }
 
-// GetDescription returns the description of a client update proposal.
+// GetDescription returns the description of a upgrade proposal.
 func (up *UpgradeProposal) GetDescription() string { return up.Description }
 
-// ProposalRoute returns the routing key of a client update proposal.
+// ProposalRoute returns the routing key of a upgrade proposal.
 func (up *UpgradeProposal) ProposalRoute() string { return RouterKey }
 
 // ProposalType returns the upgrade proposal type.
@@ -115,6 +115,10 @@ func (up *UpgradeProposal) ValidateBasic() error {
 		return sdkerrors.Wrap(ErrInvalidUpgradeProposal, "IBC chain upgrades must only set height")
 	}
 
+	if up.UpgradedClientState == nil {
+		return sdkerrors.Wrap(ErrInvalidUpgradeProposal, "upgraded client state cannot be nil")
+	}
+
 	_, err := UnpackClientState(up.UpgradedClientState)
 	if err != nil {
 		return sdkerrors.Wrap(err, "failed to unpack upgraded client state")
@@ -133,12 +137,11 @@ func (up UpgradeProposal) String() string {
 		upgradedClientStr = upgradedClient.String()
 	}
 
-	return fmt.Sprintf(`IBC Upgrade Proposal:
-  Title:               %s
-  Description:         %s
-  Plan:				   %s
-  UpgradedClientState: %s
-`, up.Title, up.Description, up.Plan, upgradedClientStr)
+	return fmt.Sprintf(`IBC Upgrade Proposal
+  Title: %s
+  Description: %s
+  %s
+  Upgraded IBC Client: %s`, up.Title, up.Description, up.Plan, upgradedClientStr)
 }
 
 // UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces

x/ibc/core/02-client/types/proposal_test.go

@@ -81,7 +81,8 @@ func (suite *TypesTestSuite) TestMarshalClientUpdateProposalProposal() {
 	cdc := codec.NewProtoCodec(ir)
 
 	// marshal message
-	bz, err := cdc.MarshalJSON(proposal)
+	content := proposal.(*types.ClientUpdateProposal)
+	bz, err := cdc.MarshalJSON(content)
 	suite.Require().NoError(err)
 
 	// unmarshal proposal
@@ -131,11 +132,21 @@ func (suite *TypesTestSuite) TestUpgradeProposalValidateBasic() {
 		{
 			"plan time is not set to 0", func() {
 				invalidPlan := upgradetypes.Plan{Name: "ibc upgrade", Time: time.Now()}
-				types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, invalidPlan, cs)
+				proposal, err = types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, invalidPlan, cs)
 				suite.Require().NoError(err)
 
 			}, false,
 		},
+		{
+			"client state is nil", func() {
+				proposal = &types.UpgradeProposal{
+					Title:               ibctesting.Title,
+					Description:         ibctesting.Description,
+					Plan:                plan,
+					UpgradedClientState: nil,
+				}
+			}, false,
+		},
 		{
 			"failed to unpack client state", func() {
 				any, err := types.PackConsensusState(&ibctmtypes.ConsensusState{})
@@ -153,6 +164,8 @@ func (suite *TypesTestSuite) TestUpgradeProposalValidateBasic() {
 
 	for _, tc := range testCases {
 
+		tc.malleate()
+
 		err := proposal.ValidateBasic()
 
 		if tc.expPass {
@@ -209,7 +222,7 @@ func (suite *TypesTestSuite) TestUpgradeString() {
 	proposal, err := types.NewUpgradeProposal(ibctesting.Title, ibctesting.Description, plan, &ibctmtypes.ClientState{})
 	suite.Require().NoError(err)
 
-	expect := fmt.Sprintf("Upgrade Plan\n  Name: ibc upgrade\n  Height: 1000\n  Info: https://foo.bar/baz.\n  Upgraded IBC Client: %s", &ibctmtypes.ClientState{})
+	expect := fmt.Sprintf("IBC Upgrade Proposal\n  Title: title\n  Description: description\n  Upgrade Plan\n  Name: ibc upgrade\n  Height: 1000\n  Info: https://foo.bar/baz\n  Upgraded IBC Client: %s", &ibctmtypes.ClientState{})
 
 	suite.Require().Equal(expect, proposal.String())
 }

x/upgrade/keeper/keeper.go

@@ -183,7 +183,7 @@ func (k Keeper) ApplyUpgrade(ctx sdk.Context, plan types.Plan) {
 
 	// Must clear IBC state after upgrade is applied as it is stored separately from the upgrade plan.
 	// This will prevent resubmission of upgrade msg after upgrade is already completed.
-	k.ClearIBCState(ctx, plan.Height-1)
+	k.ClearIBCState(ctx, plan.Height)
 	k.ClearUpgradePlan(ctx)
 	k.setDone(ctx, plan.Name)
 }