refactor: TrustingPeriodFraction should be a fraction. (#593)

* WIP

* Refactor TrustingPeriodFraction

* Update default TrustingPeriodFraction to 2/3 or 66%

Co-authored-by: Jehan <[email protected]>

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -80,7 +80,7 @@ message ConsumerAdditionProposal {
 message Params {
   ibc.lightclients.tendermint.v1.ClientState template_client = 1;
   // TrustingPeriodFraction is used to compute the consumer and provider IBC client's TrustingPeriod from the chain defined UnbondingPeriod
-  int64 trusting_period_fraction = 2;
+  string trusting_period_fraction = 2;
   // Sent IBC packets will timeout after this duration
   google.protobuf.Duration ccv_timeout_period = 3
       [(gogoproto.nullable) = false, (gogoproto.stdduration) = true];

tests/e2e/common.go

@@ -436,7 +436,10 @@ func (suite *CCVTestSuite) CreateCustomClient(endpoint *ibctesting.Endpoint, unb
 	tmConfig, ok := endpoint.ClientConfig.(*ibctesting.TendermintConfig)
 	require.True(endpoint.Chain.T, ok)
 	tmConfig.UnbondingPeriod = unbondingPeriod
-	tmConfig.TrustingPeriod = unbondingPeriod / providertypes.DefaultTrustingPeriodFraction
+
+	trustPeriod, err := ccv.CalculateTrustPeriod(unbondingPeriod, providertypes.DefaultTrustingPeriodFraction)
+	require.NoError(endpoint.Chain.T, err)
+	tmConfig.TrustingPeriod = trustPeriod
 
 	height := endpoint.Counterparty.Chain.LastHeader.GetHeight().(clienttypes.Height)
 	UpgradePath := []string{"upgrade", "upgradedIBCState"}

x/ccv/provider/keeper/params.go

@@ -20,10 +20,10 @@ func (k Keeper) GetTemplateClient(ctx sdk.Context) *ibctmtypes.ClientState {
 
 // GetTrustingPeriodFraction returns a TrustingPeriodFraction
 // used to compute the provider IBC client's TrustingPeriod as UnbondingPeriod / TrustingPeriodFraction
-func (k Keeper) GetTrustingPeriodFraction(ctx sdk.Context) int64 {
-	var i int64
-	k.paramSpace.Get(ctx, types.KeyTrustingPeriodFraction, &i)
-	return i
+func (k Keeper) GetTrustingPeriodFraction(ctx sdk.Context) string {
+	var f string
+	k.paramSpace.Get(ctx, types.KeyTrustingPeriodFraction, &f)
+	return f
 }
 
 // GetCCVTimeoutPeriod returns the timeout period for sent ibc packets

x/ccv/provider/keeper/params_test.go

@@ -38,7 +38,7 @@ func TestParams(t *testing.T) {
 			true,
 			false,
 		),
-		4,
+		"0.25",
 		7*24*time.Hour,
 		5*time.Hour,
 		10*time.Minute,

x/ccv/provider/keeper/proposal.go

@@ -64,7 +64,12 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditi
 	clientState := k.GetTemplateClient(ctx)
 	clientState.ChainId = chainID
 	clientState.LatestHeight = prop.InitialHeight
-	clientState.TrustingPeriod = consumerUnbondingPeriod / time.Duration(k.GetTrustingPeriodFraction(ctx))
+
+	trustPeriod, err := ccv.CalculateTrustPeriod(consumerUnbondingPeriod, k.GetTrustingPeriodFraction(ctx))
+	if err != nil {
+		return err
+	}
+	clientState.TrustingPeriod = trustPeriod
 	clientState.UnbondingPeriod = consumerUnbondingPeriod
 
 	consumerGen, validatorSetHash, err := k.MakeConsumerGenesis(ctx, prop)
@@ -223,7 +228,11 @@ func (k Keeper) MakeConsumerGenesis(ctx sdk.Context, prop *types.ConsumerAdditio
 	// this is the latest height the client was updated at, i.e.,
 	// the height of the latest consensus state (see below)
 	clientState.LatestHeight = height
-	clientState.TrustingPeriod = providerUnbondingPeriod / time.Duration(k.GetTrustingPeriodFraction(ctx))
+	trustPeriod, err := ccv.CalculateTrustPeriod(providerUnbondingPeriod, k.GetTrustingPeriodFraction(ctx))
+	if err != nil {
+		return gen, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "error %s calculating trusting_period for: %s", err, height)
+	}
+	clientState.TrustingPeriod = trustPeriod
 	clientState.UnbondingPeriod = providerUnbondingPeriod
 
 	consState, err := k.clientKeeper.GetSelfConsensusState(ctx, height)

x/ccv/provider/keeper/proposal_test.go

@@ -715,12 +715,12 @@ func TestMakeConsumerGenesis(t *testing.T) {
 	actualGenesis, _, err := providerKeeper.MakeConsumerGenesis(ctx, &prop)
 	require.NoError(t, err)
 
-	jsonString := `{"params":{"enabled":true, "blocks_per_distribution_transmission":1000, "ccv_timeout_period":2419200000000000, "transfer_timeout_period": 3600000000000, "consumer_redistribution_fraction":"0.75", "historical_entries":10000, "unbonding_period": 1728000000000000},"new_chain":true,"provider_client_state":{"chain_id":"testchain1","trust_level":{"numerator":1,"denominator":3},"trusting_period":907200000000000,"unbonding_period":1814400000000000,"max_clock_drift":10000000000,"frozen_height":{},"latest_height":{"revision_height":5},"proof_specs":[{"leaf_spec":{"hash":1,"prehash_value":1,"length":1,"prefix":"AA=="},"inner_spec":{"child_order":[0,1],"child_size":33,"min_prefix_length":4,"max_prefix_length":12,"hash":1}},{"leaf_spec":{"hash":1,"prehash_value":1,"length":1,"prefix":"AA=="},"inner_spec":{"child_order":[0,1],"child_size":32,"min_prefix_length":1,"max_prefix_length":1,"hash":1}}],"upgrade_path":["upgrade","upgradedIBCState"],"allow_update_after_expiry":true,"allow_update_after_misbehaviour":true},"provider_consensus_state":{"timestamp":"2020-01-02T00:00:10Z","root":{"hash":"LpGpeyQVLUo9HpdsgJr12NP2eCICspcULiWa5u9udOA="},"next_validators_hash":"E30CE736441FB9101FADDAF7E578ABBE6DFDB67207112350A9A904D554E1F5BE"},"unbonding_sequences":null,"initial_val_set":[{"pub_key":{"type":"tendermint/PubKeyEd25519","value":"dcASx5/LIKZqagJWN0frOlFtcvz91frYmj/zmoZRWro="},"power":1}]}`
+	jsonString := `{"params":{"enabled":true, "blocks_per_distribution_transmission":1000, "ccv_timeout_period":2419200000000000, "transfer_timeout_period": 3600000000000, "consumer_redistribution_fraction":"0.75", "historical_entries":10000, "unbonding_period": 1728000000000000},"new_chain":true,"provider_client_state":{"chain_id":"testchain1","trust_level":{"numerator":1,"denominator":3},"trusting_period":1197504000000000,"unbonding_period":1814400000000000,"max_clock_drift":10000000000,"frozen_height":{},"latest_height":{"revision_height":5},"proof_specs":[{"leaf_spec":{"hash":1,"prehash_value":1,"length":1,"prefix":"AA=="},"inner_spec":{"child_order":[0,1],"child_size":33,"min_prefix_length":4,"max_prefix_length":12,"hash":1}},{"leaf_spec":{"hash":1,"prehash_value":1,"length":1,"prefix":"AA=="},"inner_spec":{"child_order":[0,1],"child_size":32,"min_prefix_length":1,"max_prefix_length":1,"hash":1}}],"upgrade_path":["upgrade","upgradedIBCState"],"allow_update_after_expiry":true,"allow_update_after_misbehaviour":true},"provider_consensus_state":{"timestamp":"2020-01-02T00:00:10Z","root":{"hash":"LpGpeyQVLUo9HpdsgJr12NP2eCICspcULiWa5u9udOA="},"next_validators_hash":"E30CE736441FB9101FADDAF7E578ABBE6DFDB67207112350A9A904D554E1F5BE"},"unbonding_sequences":null,"initial_val_set":[{"pub_key":{"type":"tendermint/PubKeyEd25519","value":"dcASx5/LIKZqagJWN0frOlFtcvz91frYmj/zmoZRWro="},"power":1}]}`
 
 	var expectedGenesis consumertypes.GenesisState
 	err = json.Unmarshal([]byte(jsonString), &expectedGenesis)
 	require.NoError(t, err)
-
+	
 	// Zeroing out different fields that are challenging to mock
 	actualGenesis.InitialValSet = []abci.ValidatorUpdate{}
 	expectedGenesis.InitialValSet = []abci.ValidatorUpdate{}

x/ccv/provider/types/genesis_test.go

@@ -75,7 +75,7 @@ func TestValidateGenesisState(t *testing.T) {
 				nil,
 				types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 					time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-					3, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
+					types.DefaultTrustingPeriodFraction, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
 				nil,
 				nil,
 				nil,
@@ -94,7 +94,7 @@ func TestValidateGenesisState(t *testing.T) {
 				nil,
 				types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 					time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-					3, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
+					types.DefaultTrustingPeriodFraction, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
 				nil,
 				nil,
 				nil,
@@ -113,7 +113,7 @@ func TestValidateGenesisState(t *testing.T) {
 				nil,
 				types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 					time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-					3, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
+					types.DefaultTrustingPeriodFraction, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
 				nil,
 				nil,
 				nil,
@@ -132,7 +132,7 @@ func TestValidateGenesisState(t *testing.T) {
 				nil,
 				types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 					time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-					3, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
+					types.DefaultTrustingPeriodFraction, time.Hour, time.Hour, 30*time.Minute, time.Hour, "0.1", 400),
 				nil,
 				nil,
 				nil,
@@ -176,7 +176,7 @@ func TestValidateGenesisState(t *testing.T) {
 				nil,
 				types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 					time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-					0, // 0 trusting period fraction here
+					"0.0", // 0 trusting period fraction here
 					ccv.DefaultCCVTimeoutPeriod,
 					types.DefaultInitTimeoutPeriod,
 					types.DefaultVscTimeoutPeriod,

x/ccv/provider/types/params.go

@@ -18,8 +18,8 @@ const (
 	DefaultMaxClockDrift = 10 * time.Second
 
 	// DefaultTrustingPeriodFraction is the default fraction used to compute TrustingPeriod
-	// as UnbondingPeriod / TrustingPeriodFraction
-	DefaultTrustingPeriodFraction = 2
+	// as UnbondingPeriod * TrustingPeriodFraction
+	DefaultTrustingPeriodFraction = "0.66"
 
 	// DefaultInitTimeoutPeriod defines the init timeout period
 	DefaultInitTimeoutPeriod = 7 * 24 * time.Hour
@@ -59,7 +59,7 @@ func ParamKeyTable() paramtypes.KeyTable {
 // NewParams creates new provider parameters with provided arguments
 func NewParams(
 	cs *ibctmtypes.ClientState,
-	trustingPeriodFraction int64,
+	trustingPeriodFraction string,
 	ccvTimeoutPeriod time.Duration,
 	initTimeoutPeriod time.Duration,
 	vscTimeoutPeriod time.Duration,
@@ -114,7 +114,7 @@ func (p Params) Validate() error {
 	if err := validateTemplateClient(*p.TemplateClient); err != nil {
 		return err
 	}
-	if err := ccvtypes.ValidatePositiveInt64(p.TrustingPeriodFraction); err != nil {
+	if err := ccvtypes.ValidateStringFraction(p.TrustingPeriodFraction); err != nil {
 		return fmt.Errorf("trusting period fraction is invalid: %s", err)
 	}
 	if err := ccvtypes.ValidateDuration(p.CcvTimeoutPeriod); err != nil {
@@ -142,7 +142,7 @@ func (p Params) Validate() error {
 func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
 	return paramtypes.ParamSetPairs{
 		paramtypes.NewParamSetPair(KeyTemplateClient, p.TemplateClient, validateTemplateClient),
-		paramtypes.NewParamSetPair(KeyTrustingPeriodFraction, p.TrustingPeriodFraction, ccvtypes.ValidatePositiveInt64),
+		paramtypes.NewParamSetPair(KeyTrustingPeriodFraction, p.TrustingPeriodFraction, ccvtypes.ValidateStringFraction),
 		paramtypes.NewParamSetPair(ccvtypes.KeyCCVTimeoutPeriod, p.CcvTimeoutPeriod, ccvtypes.ValidateDuration),
 		paramtypes.NewParamSetPair(KeyInitTimeoutPeriod, p.InitTimeoutPeriod, ccvtypes.ValidateDuration),
 		paramtypes.NewParamSetPair(KeyVscTimeoutPeriod, p.VscTimeoutPeriod, ccvtypes.ValidateDuration),
@@ -163,7 +163,13 @@ func validateTemplateClient(i interface{}) error {
 
 	// populate zeroed fields with valid fields
 	copiedClient.ChainId = "chainid"
-	copiedClient.TrustingPeriod = consumertypes.DefaultConsumerUnbondingPeriod / DefaultTrustingPeriodFraction
+
+	trustPeriod, err := ccvtypes.CalculateTrustPeriod(consumertypes.DefaultConsumerUnbondingPeriod, DefaultTrustingPeriodFraction)
+	if err != nil {
+		return fmt.Errorf("invalid TrustPeriodFraction: %T", err)
+	}
+	copiedClient.TrustingPeriod = trustPeriod
+
 	copiedClient.UnbondingPeriod = consumertypes.DefaultConsumerUnbondingPeriod
 	copiedClient.LatestHeight = clienttypes.NewHeight(0, 1)
 

x/ccv/provider/types/params_test.go

@@ -13,7 +13,6 @@ import (
 )
 
 func TestValidateParams(t *testing.T) {
-
 	testCases := []struct {
 		name    string
 		params  types.Params
@@ -22,34 +21,35 @@ func TestValidateParams(t *testing.T) {
 		{"default params", types.DefaultParams(), true},
 		{"custom valid params", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			3, time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), true},
+			"0.33", time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), true},
 		{"custom invalid params", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			0, clienttypes.Height{}, nil, []string{"ibc", "upgradedIBCState"}, true, false),
-			3, time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
+			"0.33", time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
 		{"blank client", types.NewParams(&ibctmtypes.ClientState{},
-			3, time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
-		{"nil client", types.NewParams(nil, 3, time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
-		{"0 trusting period fraction (denominator)", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
+			"0.33", time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
+		{"nil client", types.NewParams(nil, "0.33", time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
+		// Check if "0.00" is valid or if a zero dec TrustFraction needs to return an error
+		{"0 trusting period fraction", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			0, time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
+			"0.00", time.Hour, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), true},
 		{"0 ccv timeout period", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			3, 0, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
+			"0.33", 0, time.Hour, time.Hour, 30*time.Minute, "0.1", 100), false},
 		{"0 init timeout period", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			3, time.Hour, 0, time.Hour, 30*time.Minute, "0.1", 100), false},
+			"0.33", time.Hour, 0, time.Hour, 30*time.Minute, "0.1", 100), false},
 		{"0 vsc timeout period", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			3, time.Hour, time.Hour, 0, 30*time.Minute, "0.1", 100), false},
+			"0.33", time.Hour, time.Hour, 0, 30*time.Minute, "0.1", 100), false},
 		{"0 slash meter replenish period", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			3, time.Hour, time.Hour, 24*time.Hour, 0, "0.1", 100), false},
+			"0.33", time.Hour, time.Hour, 24*time.Hour, 0, "0.1", 100), false},
 		{"slash meter replenish fraction over 1", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			3, time.Hour, time.Hour, 24*time.Hour, time.Hour, "1.5", 100), false},
+			"0.33", time.Hour, time.Hour, 24*time.Hour, time.Hour, "1.5", 100), false},
 		{"negative max pending slash packets", types.NewParams(ibctmtypes.NewClientState("", ibctmtypes.DefaultTrustLevel, 0, 0,
 			time.Second*40, clienttypes.Height{}, commitmenttypes.GetSDKSpecs(), []string{"ibc", "upgradedIBCState"}, true, false),
-			3, time.Hour, time.Hour, 24*time.Hour, time.Hour, "0.1", -100), false},
+			"0.33", time.Hour, time.Hour, 24*time.Hour, time.Hour, "0.1", -100), false},
 	}
 
 	for _, tc := range testCases {