From 724d0fcfecf1ddd4d6dfe18ca83088adbd864cec Mon Sep 17 00:00:00 2001 From: Matija Salopek Date: Wed, 7 Dec 2022 18:23:22 +0100 Subject: [PATCH] refactor after reviews --- docs/architecture/adr-001-key-assignment.md | 2 +- .../ccv/provider/v1/provider.proto | 21 +++++------ tests/e2e/channel_init.go | 2 +- tests/e2e/stop_consumer.go | 35 +++++++++---------- tests/e2e/unbonding.go | 2 +- x/ccv/provider/client/proposal_handler.go | 12 +++---- x/ccv/provider/keeper/genesis_test.go | 2 -- x/ccv/provider/keeper/proposal.go | 33 ++++++++--------- x/ccv/provider/types/consumer.go | 1 - x/ccv/provider/types/proposal.go | 15 ++++---- 10 files changed, 55 insertions(+), 70 deletions(-) diff --git a/docs/architecture/adr-001-key-assignment.md b/docs/architecture/adr-001-key-assignment.md index 114d91cae6..66d983132b 100644 --- a/docs/architecture/adr-001-key-assignment.md +++ b/docs/architecture/adr-001-key-assignment.md @@ -240,7 +240,7 @@ func OnRecvVSCMaturedPacket(packet channeltypes.Packet, data ccv.VSCMaturedPacke On stopping a consumer chain: ```golang -func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, closeChan bool) (err error) { +func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, closeChan bool) (err error) { // ... // deletes all the state needed for key assignments on this consumer chain // ... diff --git a/proto/interchain_security/ccv/provider/v1/provider.proto b/proto/interchain_security/ccv/provider/v1/provider.proto index ec98c3bb27..87a0058b4c 100644 --- a/proto/interchain_security/ccv/provider/v1/provider.proto +++ b/proto/interchain_security/ccv/provider/v1/provider.proto @@ -37,33 +37,28 @@ message ConsumerAdditionProposal { // will be responsible for starting their consumer chain validator node. google.protobuf.Timestamp spawn_time = 7 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false]; - - // BlocksPerDistributionTransmission is the number of blocks between ibc-token-transfers from the consumer chain to the provider chain. - // Note that at this transmission event a fraction of the accumulated tokens are divided and sent consumer redistribution address. - int64 blocks_per_distribution_transmission = 8; + // Unbonding period for the consumer, + // which should be smaller than that of the provider in general. + google.protobuf.Duration unbonding_period = 8 + [(gogoproto.nullable) = false, (gogoproto.stdduration) = true]; // Sent CCV related IBC packets will timeout after this duration google.protobuf.Duration ccv_timeout_period = 9 [(gogoproto.nullable) = false, (gogoproto.stdduration) = true]; - // Sent transfer related IBC packets will timeout after this duration google.protobuf.Duration transfer_timeout_period = 10 [(gogoproto.nullable) = false, (gogoproto.stdduration) = true]; - // The fraction of tokens allocated to the consumer redistribution address // during distribution events. The fraction is a string representing a // decimal number. For example "0.75" would represent 75%. string consumer_redistribution_fraction = 11; - + // BlocksPerDistributionTransmission is the number of blocks between ibc-token-transfers from the consumer chain to the provider chain. + // On sending transmission event, `consumer_redistribution_fraction` of the accumulated tokens are sent to the consumer redistribution address. + int64 blocks_per_distribution_transmission = 12; // The number of historical info entries to persist in store. // This param is a part of the cosmos sdk staking module. In the case of // a ccv enabled consumer chain, the ccv module acts as the staking module. - int64 historical_entries = 12; - - // Unbonding period for the consumer, - // which should be smaller than that of the provider in general. - google.protobuf.Duration unbonding_period = 13 - [(gogoproto.nullable) = false, (gogoproto.stdduration) = true]; + int64 historical_entries = 13; } // ConsumerRemovalProposal is a governance proposal on the provider chain to remove (and stop) a consumer chain. diff --git a/tests/e2e/channel_init.go b/tests/e2e/channel_init.go index 055cf4d800..8cfc6c0100 100644 --- a/tests/e2e/channel_init.go +++ b/tests/e2e/channel_init.go @@ -85,7 +85,7 @@ func (suite *CCVTestSuite) TestInitTimeout() { if tc.removed { // check if the chain was properly removed - suite.checkConsumerChainIsRemoved(chainID, false, false) + suite.checkConsumerChainIsRemoved(chainID, false) } if i+1 < len(testCases) { diff --git a/tests/e2e/stop_consumer.go b/tests/e2e/stop_consumer.go index a16d848c57..e35f892e97 100644 --- a/tests/e2e/stop_consumer.go +++ b/tests/e2e/stop_consumer.go @@ -92,7 +92,7 @@ func (s *CCVTestSuite) TestStopConsumerChain() { s.Require().NoError(err) // check all states are removed and the unbonding operation released - s.checkConsumerChainIsRemoved(consumerChainID, false, true) + s.checkConsumerChainIsRemoved(consumerChainID, true) } // TODO Simon: implement OnChanCloseConfirm in IBC-GO testing to close the consumer chain's channel end @@ -126,7 +126,7 @@ func (s *CCVTestSuite) TestStopConsumerOnChannelClosed() { // s.Require().False(found) } -func (s *CCVTestSuite) checkConsumerChainIsRemoved(chainID string, lockUbd bool, checkChannel bool) { +func (s *CCVTestSuite) checkConsumerChainIsRemoved(chainID string, checkChannel bool) { channelID := s.path.EndpointB.ChannelID providerKeeper := s.providerApp.GetProviderKeeper() providerStakingKeeper := s.providerApp.GetE2eStakingKeeper() @@ -137,24 +137,21 @@ func (s *CCVTestSuite) checkConsumerChainIsRemoved(chainID string, lockUbd bool, } // check UnbondingOps were deleted and undelegation entries aren't onHold - if !lockUbd { - providerKeeper.IterateOverUnbondingOpIndex( - s.providerCtx(), - chainID, - func(vscID uint64, ubdIndex []uint64) (stop bool) { - _, found := providerKeeper.GetUnbondingOpIndex(s.providerCtx(), chainID, uint64(vscID)) + providerKeeper.IterateOverUnbondingOpIndex( + s.providerCtx(), + chainID, + func(vscID uint64, ubdIndex []uint64) (stop bool) { + _, found := providerKeeper.GetUnbondingOpIndex(s.providerCtx(), chainID, uint64(vscID)) + s.Require().False(found) + for _, ubdID := range ubdIndex { + _, found = providerKeeper.GetUnbondingOp(s.providerCtx(), ubdIndex[ubdID]) s.Require().False(found) - for _, ubdID := range ubdIndex { - _, found = providerKeeper.GetUnbondingOp(s.providerCtx(), ubdIndex[ubdID]) - s.Require().False(found) - ubd, _ := providerStakingKeeper.GetUnbondingDelegationByUnbondingID(s.providerCtx(), ubdIndex[ubdID]) - s.Require().Zero(ubd.Entries[ubdID].UnbondingOnHoldRefCount) - } - return false // do not stop the iteration - }, - ) - - } + ubd, _ := providerStakingKeeper.GetUnbondingDelegationByUnbondingID(s.providerCtx(), ubdIndex[ubdID]) + s.Require().Zero(ubd.Entries[ubdID].UnbondingOnHoldRefCount) + } + return false // do not stop the iteration + }, + ) // verify consumer chain's states are removed _, found := providerKeeper.GetConsumerGenesis(s.providerCtx(), chainID) diff --git a/tests/e2e/unbonding.go b/tests/e2e/unbonding.go index 2a3b3199eb..00f25a509f 100644 --- a/tests/e2e/unbonding.go +++ b/tests/e2e/unbonding.go @@ -167,7 +167,7 @@ func (s *CCVTestSuite) TestUndelegationVscTimeout() { s.Require().Equal(false, found, "consumer chain was not removed") // check if the chain was properly removed - s.checkConsumerChainIsRemoved(chainID, false, true) + s.checkConsumerChainIsRemoved(chainID, true) // check that the unbonding operation completed // - check that ccv unbonding op has been deleted diff --git a/x/ccv/provider/client/proposal_handler.go b/x/ccv/provider/client/proposal_handler.go index 751ac3a305..bfccec5c8b 100644 --- a/x/ccv/provider/client/proposal_handler.go +++ b/x/ccv/provider/client/proposal_handler.go @@ -53,12 +53,12 @@ Where proposal.json contains: "genesis_hash": "Z2VuZXNpcyBoYXNo", "binary_hash": "YmluYXJ5IGhhc2g=", "spawn_time": "2022-01-27T15:59:50.121607-08:00", - "blocks_per_distribution_transmission": 1000, - "ccv_timeout_period": 2419200000000000, - "transfer_timeout_period": 3600000000000, - "consumer_redistribution_fraction": "0.75, - "historical_entries": 10000, - "unbonding_period": 1728000000000000, + "blocks_per_distribution_transmission": 1000, + "consumer_redistribution_fraction": "0.75", + "historical_entries": 10000, + "transfer_timeout_period": 3600000000000, + "ccv_timeout_period": 2419200000000000, + "unbonding_period": 1728000000000000, "deposit": "10000stake" } `, diff --git a/x/ccv/provider/keeper/genesis_test.go b/x/ccv/provider/keeper/genesis_test.go index df9f9d6ab8..45ccc1fc2f 100644 --- a/x/ccv/provider/keeper/genesis_test.go +++ b/x/ccv/provider/keeper/genesis_test.go @@ -44,7 +44,6 @@ func TestInitAndExportGenesis(t *testing.T) { expClientID, "channel", initHeight, - true, *consumertypes.DefaultGenesisState(), []providertypes.UnbondingOpIndex{ {ValsetUpdateId: vscID, UnbondingOpIndex: ubdIndex}, @@ -57,7 +56,6 @@ func TestInitAndExportGenesis(t *testing.T) { expClientID, "", 0, - false, *consumertypes.DefaultGenesisState(), nil, []ccv.ValidatorSetChangePacketData{{ValsetUpdateId: vscID}}, diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 57e7f451b4..cab6bd5e94 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -123,8 +123,8 @@ func (k Keeper) HandleConsumerRemovalProposal(ctx sdk.Context, p *types.Consumer return nil } -// 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. +// StopConsumerChain cleans up the states for the given consumer chain ID and +// completes the outstanding unbonding operations on the consumer chain. // // This method implements StopConsumerChain from spec. // See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-stcc1 @@ -211,21 +211,6 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, closeChan boo // MakeConsumerGenesis constructs the consumer CCV module part of the genesis state. func (k Keeper) MakeConsumerGenesis(ctx sdk.Context, prop *types.ConsumerAdditionProposal) (gen consumertypes.GenesisState, nextValidatorsHash []byte, err error) { chainID := prop.ChainId - consumerParams := consumertypes.NewParams( - true, - prop.BlocksPerDistributionTransmission, - "", // distributionTransmissionChannel - "", // providerFeePoolAddrStr, - prop.CcvTimeoutPeriod, - prop.TransferTimeoutPeriod, - prop.ConsumerRedistributionFraction, - prop.HistoricalEntries, - prop.UnbondingPeriod, - ) - if err := consumerParams.Validate(); err != nil { - return gen, nil, sdkerrors.Wrapf(types.ErrInvalidConsumerParams, "error validating consumer params for chain-id '%s': %s", chainID, err) - } - providerUnbondingPeriod := k.stakingKeeper.UnbondingTime(ctx) height := clienttypes.GetSelfHeight(ctx) @@ -286,11 +271,23 @@ func (k Keeper) MakeConsumerGenesis(ctx sdk.Context, prop *types.ConsumerAdditio } hash := tmtypes.NewValidatorSet(updatesAsValSet).Hash() + consumerGenesisParams := consumertypes.NewParams( + true, + prop.BlocksPerDistributionTransmission, + "", // distributionTransmissionChannel + "", // providerFeePoolAddrStr, + prop.CcvTimeoutPeriod, + prop.TransferTimeoutPeriod, + prop.ConsumerRedistributionFraction, + prop.HistoricalEntries, + prop.UnbondingPeriod, + ) + gen = *consumertypes.NewInitialGenesisState( clientState, consState.(*ibctmtypes.ConsensusState), initialUpdatesWithConsumerKeys, - consumerParams, + consumerGenesisParams, ) return gen, hash, nil } diff --git a/x/ccv/provider/types/consumer.go b/x/ccv/provider/types/consumer.go index 93a4363e84..dfbd862cfe 100644 --- a/x/ccv/provider/types/consumer.go +++ b/x/ccv/provider/types/consumer.go @@ -10,7 +10,6 @@ func NewConsumerStates( clientID, channelID string, initialHeight uint64, - lockUbdTimeout bool, genesis consumertypes.GenesisState, unbondingOpsIndexes []UnbondingOpIndex, pendingValsetChanges []ccv.ValidatorSetChangePacketData, diff --git a/x/ccv/provider/types/proposal.go b/x/ccv/provider/types/proposal.go index eb2b08bb12..9b7b928b51 100644 --- a/x/ccv/provider/types/proposal.go +++ b/x/ccv/provider/types/proposal.go @@ -5,10 +5,10 @@ import ( "strings" time "time" - sdktypes "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" + ccvtypes "github.com/cosmos/interchain-security/x/ccv/types" ) const ( @@ -93,28 +93,27 @@ func (cccp *ConsumerAdditionProposal) ValidateBasic() error { return sdkerrors.Wrap(ErrInvalidConsumerAdditionProposal, "spawn time cannot be zero") } - if _, err := sdktypes.NewDecFromStr(cccp.ConsumerRedistributionFraction); err != nil { + if err := ccvtypes.ValidateStringFraction(cccp.ConsumerRedistributionFraction); err != nil { return sdkerrors.Wrapf(ErrInvalidConsumerAdditionProposal, "consumer redistribution fraction is invalid: %s", err) } - if cccp.BlocksPerDistributionTransmission < 1 { + if err := ccvtypes.ValidatePositiveInt64(cccp.BlocksPerDistributionTransmission); err != nil { return sdkerrors.Wrap(ErrInvalidConsumerAdditionProposal, "blocks per distribution transmission cannot be < 1") } - if cccp.HistoricalEntries < 1 { + if err := ccvtypes.ValidatePositiveInt64(cccp.HistoricalEntries); err != nil { return sdkerrors.Wrap(ErrInvalidConsumerAdditionProposal, "historical entries cannot be < 1") } - zeroDuration := time.Duration(0) - if cccp.CcvTimeoutPeriod == zeroDuration { + if err := ccvtypes.ValidateDuration(cccp.CcvTimeoutPeriod); err != nil { return sdkerrors.Wrap(ErrInvalidConsumerAdditionProposal, "ccv timeout period cannot be zero") } - if cccp.TransferTimeoutPeriod == zeroDuration { + if err := ccvtypes.ValidateDuration(cccp.TransferTimeoutPeriod); err != nil { return sdkerrors.Wrap(ErrInvalidConsumerAdditionProposal, "transfer timeout period cannot be zero") } - if cccp.UnbondingPeriod == zeroDuration { + if err := ccvtypes.ValidateDuration(cccp.UnbondingPeriod); err != nil { return sdkerrors.Wrap(ErrInvalidConsumerAdditionProposal, "unbonding period cannot be zero") }