refactor after reviews

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
     // ...

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.

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) {

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)

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

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"
 }
 		`,

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}},

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
 }

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,

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")
 	}