imp: add a check for the client status in CreateClient (#3514)

* imp: add a check for the client status in CreateClient --------- Co-authored-by: colin axnér <[email protected]>
cosmos · May 3, 2023 · c3b12b1 · c3b12b1
1 parent 6894016
commit c3b12b1
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 21 deletions.
diff --git a/e2e/tests/core/client_test.go b/e2e/tests/core/client_test.go
@@ -67,7 +67,8 @@ func (s *ClientTestSuite) TestClientUpdateProposal_Succeeds() {
 		relayer            ibc.Relayer
 		subjectClientID    string
 		substituteClientID string
-		badTrustingPeriod  = time.Duration(time.Second)
+		// set the trusting period to a value which will still be valid upon client creation, but invalid before the first update
+		badTrustingPeriod = time.Duration(time.Second * 10)
 	)
 
 	t.Run("create substitute client with correct trusting period", func(t *testing.T) {

diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
@@ -35,6 +35,10 @@ func (k Keeper) CreateClient(
 		return "", err
 	}
 
+	if status := k.GetClientStatus(ctx, clientState, clientID); status != exported.Active {
+		return "", errorsmod.Wrapf(types.ErrClientNotActive, "cannot create client (%s) with status %s", clientID, status)
+	}
+
 	k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String())
 
 	defer telemetry.IncrCounterWithLabels(

diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go
@@ -18,43 +18,71 @@ import (
 )
 
 func (suite *KeeperTestSuite) TestCreateClient() {
-	cases := []struct {
-		msg            string
+	var (
 		clientState    exported.ClientState
 		consensusState exported.ConsensusState
-		expPass        bool
+	)
+
+	testCases := []struct {
+		msg      string
+		malleate func()
+		expPass  bool
 	}{
 		{
 			"success: 07-tendermint client type supported",
-			ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath),
-			suite.consensusState,
+			func() {
+				clientState = ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath)
+				consensusState = suite.consensusState
+			},
 			true,
 		},
+		{
+			"failure: 07-tendermint client status is not active",
+			func() {
+				clientState = ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath)
+				tmcs, ok := clientState.(*ibctm.ClientState)
+				suite.Require().True(ok)
+				tmcs.FrozenHeight = ibctm.FrozenHeight
+				consensusState = suite.consensusState
+			},
+			false,
+		},
 		{
 			"success: 06-solomachine client type supported",
-			solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}),
-			&solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time},
+			func() {
+				clientState = solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time})
+				consensusState = &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}
+			},
 			true,
 		},
 		{
 			"failure: 09-localhost client type not supported",
-			localhost.NewClientState(clienttypes.GetSelfHeight(suite.ctx)),
-			nil,
+			func() {
+				clientState = localhost.NewClientState(clienttypes.GetSelfHeight(suite.ctx))
+				consensusState = nil
+			},
 			false,
 		},
 	}
 
-	for i, tc := range cases {
-		clientID, err := suite.keeper.CreateClient(suite.ctx, tc.clientState, tc.consensusState)
-		if tc.expPass {
-			suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg)
-			suite.Require().NotNil(clientID, "valid test case %d failed: %s", i, tc.msg)
-			suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey()))
-		} else {
-			suite.Require().Error(err, "invalid test case %d passed: %s", i, tc.msg)
-			suite.Require().Equal("", clientID, "invalid test case %d passed: %s", i, tc.msg)
-			suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey()))
-		}
+	for _, tc := range testCases {
+		tc := tc
+
+		suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
+			suite.SetupTest() // reset
+			tc.malleate()
+
+			clientID, err := suite.keeper.CreateClient(suite.ctx, clientState, consensusState)
+			if tc.expPass {
+				suite.Require().NoError(err)
+				suite.Require().NotEmpty(clientID)
+				suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey()))
+			} else {
+				suite.Require().Error(err)
+				suite.Require().Empty(clientID)
+				suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey()))
+			}
+		})
 	}
 }