Skip to content
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

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

Merged
merged 9 commits into from
May 3, 2023
2 changes: 1 addition & 1 deletion e2e/tests/core/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func (s *ClientTestSuite) TestClientUpdateProposal_Succeeds() {
relayer ibc.Relayer
subjectClientID string
substituteClientID string
badTrustingPeriod = time.Duration(time.Second)
badTrustingPeriod = time.Duration(time.Second * 10)
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
)

t.Run("create substitute client with correct trusting period", func(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions modules/core/02-client/keeper/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
68 changes: 48 additions & 20 deletions modules/core/02-client/keeper/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
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)
DimitrisJim marked this conversation as resolved.
Show resolved Hide resolved
if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotNil(clientID)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
suite.Require().True(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey()))
} else {
suite.Require().Error(err)
suite.Require().Equal("", clientID)
suite.Require().False(suite.keeper.ClientStore(suite.ctx, clientID).Has(host.ClientStateKey()))
}
})
}
}

Expand Down