From b2a272f6e4b86e3cb254c3f1817ea0e8074c9059 Mon Sep 17 00:00:00 2001 From: Marius Poke Date: Tue, 9 Aug 2022 21:14:00 +0200 Subject: [PATCH] Drop duplicate spawn/stop proposals (#254) * drop duplicate create / stop proposals * small test fix for #254 (#258) two different chain ids * return nil on duplicate proposal * minor changes to UTs Co-authored-by: Shawn Marshall-Spitzbart <44221603+smarshall-spitzbart@users.noreply.github.com> --- x/ccv/provider/keeper/proposal.go | 11 +++++++++++ x/ccv/provider/keeper/proposal_test.go | 14 +++++++------- x/ccv/provider/proposal_handler_test.go | 4 +++- x/ccv/provider/types/errors.go | 7 ++++--- x/ccv/provider/types/proposal.go | 14 +++++++------- 5 files changed, 32 insertions(+), 18 deletions(-) diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 44cabbe53c..b601ba4f72 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -50,6 +50,11 @@ func (k Keeper) StopConsumerChainProposal(ctx sdk.Context, p *types.StopConsumer // StopConsumerChain cleans up the states for the given consumer chain ID and, if the given lockUbd is false, // it completes the outstanding unbonding operations lock by the consumer chain. func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, closeChan bool) (err error) { + // check that a client for chainID exists + if _, found := k.GetConsumerClientId(ctx, chainID); !found { + // drop the proposal + return nil + } // clean up states k.DeleteConsumerClientId(ctx, chainID) @@ -113,6 +118,12 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, clos // CreateConsumerClient will create the CCV client for the given consumer chain. The CCV channel must be built // on top of the CCV client to ensure connection with the right consumer chain. func (k Keeper) CreateConsumerClient(ctx sdk.Context, chainID string, initialHeight clienttypes.Height, lockUbdOnTimeout bool) error { + // check that a client for this chain does not exist + if _, found := k.GetConsumerClientId(ctx, chainID); found { + // drop the proposal + return nil + } + // Use the unbonding period on the provider to // compute the unbonding period on the consumer unbondingTime := utils.ComputeConsumerUnbondingPeriod(k.stakingKeeper.UnbondingTime(ctx)) diff --git a/x/ccv/provider/keeper/proposal_test.go b/x/ccv/provider/keeper/proposal_test.go index 3949a67dc9..c605d36f63 100644 --- a/x/ccv/provider/keeper/proposal_test.go +++ b/x/ccv/provider/keeper/proposal_test.go @@ -66,6 +66,7 @@ func (suite *KeeperTestSuite) TestCreateConsumerChainProposal() { suite.Require().NoError(err) proposal, ok = content.(*types.CreateConsumerChainProposal) suite.Require().True(ok) + proposal.LockUnbondingOnTimeout = lockUbdOnTimeout }, true, true, }, { @@ -76,6 +77,7 @@ func (suite *KeeperTestSuite) TestCreateConsumerChainProposal() { suite.Require().NoError(err) proposal, ok = content.(*types.CreateConsumerChainProposal) suite.Require().True(ok) + proposal.LockUnbondingOnTimeout = lockUbdOnTimeout }, true, false, }, } @@ -103,9 +105,9 @@ func (suite *KeeperTestSuite) TestCreateConsumerChainProposal() { suite.Require().Equal(expectedGenesis, consumerGenesis) suite.Require().NotEqual("", clientId, "consumer client was not created after spawn time reached") } else { - gotClient := suite.providerChain.App.(*appProvider.App).ProviderKeeper.GetPendingCreateProposal(ctx, proposal.SpawnTime, chainID) - suite.Require().Equal(initialHeight, gotClient.InitialHeight, "pending client not equal to clientstate in proposal") - suite.Require().Equal(lockUbdOnTimeout, gotClient.LockUnbondingOnTimeout, "pending client not equal to clientstate in proposal") + gotProposal := suite.providerChain.App.(*appProvider.App).ProviderKeeper.GetPendingCreateProposal(ctx, proposal.SpawnTime, chainID) + suite.Require().Equal(initialHeight, gotProposal.InitialHeight, "unexpected pending proposal (InitialHeight)") + suite.Require().Equal(lockUbdOnTimeout, gotProposal.LockUnbondingOnTimeout, "unexpected pending proposal (LockUnbondingOnTimeout)") } } else { suite.Require().Error(err, "did not return error on invalid case") @@ -148,18 +150,16 @@ func (suite *KeeperTestSuite) TestIteratePendingStopProposal() { func (suite *KeeperTestSuite) TestIteratePendingClientInfo() { - chainID := suite.consumerChain.ChainID - testCases := []struct { types.CreateConsumerChainProposal ExpDeleted bool }{ { - CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: chainID, SpawnTime: time.Now().UTC()}, + CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: "0", SpawnTime: time.Now().UTC()}, ExpDeleted: true, }, { - CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: chainID, SpawnTime: time.Now().UTC().Add(time.Hour)}, + CreateConsumerChainProposal: types.CreateConsumerChainProposal{ChainId: "1", SpawnTime: time.Now().UTC().Add(time.Hour)}, ExpDeleted: false, }, } diff --git a/x/ccv/provider/proposal_handler_test.go b/x/ccv/provider/proposal_handler_test.go index e60e1082d1..cfce155545 100644 --- a/x/ccv/provider/proposal_handler_test.go +++ b/x/ccv/provider/proposal_handler_test.go @@ -13,7 +13,9 @@ import ( "github.com/cosmos/interchain-security/x/ccv/provider/types" ) -func (suite *ProviderTestSuite) TestCreateConsumerChainProposalHandler() { +// TestConsumerChainProposalHandler tests the handler for consumer chain proposals +// for both CreateConsumerChainProposal and StopConsumerChainProposal +func (suite *ProviderTestSuite) TestConsumerChainProposalHandler() { var ( ctx sdk.Context content govtypes.Content diff --git a/x/ccv/provider/types/errors.go b/x/ccv/provider/types/errors.go index 4ec917b377..d438f0262c 100644 --- a/x/ccv/provider/types/errors.go +++ b/x/ccv/provider/types/errors.go @@ -6,7 +6,8 @@ import ( // Provider sentinel errors var ( - ErrInvalidProposal = sdkerrors.Register(ModuleName, 1, "invalid create consumer chain proposal") - ErrUnknownConsumerChainId = sdkerrors.Register(ModuleName, 2, "no consumer chain with this chain id") - ErrUnknownConsumerChannelId = sdkerrors.Register(ModuleName, 3, "no consumer chain with this channel id") + ErrInvalidCreateProposal = sdkerrors.Register(ModuleName, 1, "invalid create consumer chain proposal") + ErrInvalidStopProposal = sdkerrors.Register(ModuleName, 2, "invalid stop consumer chain proposal") + ErrUnknownConsumerChainId = sdkerrors.Register(ModuleName, 3, "no consumer chain with this chain id") + ErrUnknownConsumerChannelId = sdkerrors.Register(ModuleName, 4, "no consumer chain with this channel id") ) diff --git a/x/ccv/provider/types/proposal.go b/x/ccv/provider/types/proposal.go index bc5a11f3c7..bce81eae32 100644 --- a/x/ccv/provider/types/proposal.go +++ b/x/ccv/provider/types/proposal.go @@ -57,22 +57,22 @@ func (cccp *CreateConsumerChainProposal) ValidateBasic() error { } if strings.TrimSpace(cccp.ChainId) == "" { - return sdkerrors.Wrap(ErrInvalidProposal, "consumer chain id must not be blank") + return sdkerrors.Wrap(ErrInvalidCreateProposal, "consumer chain id must not be blank") } if cccp.InitialHeight.IsZero() { - return sdkerrors.Wrap(ErrInvalidProposal, "initial height cannot be zero") + return sdkerrors.Wrap(ErrInvalidCreateProposal, "initial height cannot be zero") } if len(cccp.GenesisHash) == 0 { - return sdkerrors.Wrap(ErrInvalidProposal, "genesis hash cannot be empty") + return sdkerrors.Wrap(ErrInvalidCreateProposal, "genesis hash cannot be empty") } if len(cccp.BinaryHash) == 0 { - return sdkerrors.Wrap(ErrInvalidProposal, "binary hash cannot be empty") + return sdkerrors.Wrap(ErrInvalidCreateProposal, "binary hash cannot be empty") } if cccp.SpawnTime.IsZero() { - return sdkerrors.Wrap(ErrInvalidProposal, "spawn time cannot be zero") + return sdkerrors.Wrap(ErrInvalidCreateProposal, "spawn time cannot be zero") } return nil } @@ -112,11 +112,11 @@ func (sccp *StopConsumerChainProposal) ValidateBasic() error { } if strings.TrimSpace(sccp.ChainId) == "" { - return sdkerrors.Wrap(ErrInvalidProposal, "consumer chain id must not be blank") + return sdkerrors.Wrap(ErrInvalidStopProposal, "consumer chain id must not be blank") } if sccp.StopTime.IsZero() { - return sdkerrors.Wrap(ErrInvalidProposal, "spawn time cannot be zero") + return sdkerrors.Wrap(ErrInvalidStopProposal, "spawn time cannot be zero") } return nil }