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.

Sign up for GitHub

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

Jump to bottom

feat: retry delay period as param #1308

Merged
merged 5 commits into from
Sep 18, 2023
Merged

feat: retry delay period as param #1308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3eacc5b
boilerplate
shaspitz Sep 14, 2023
33a9290
fix test
shaspitz Sep 14, 2023
5d7ab92
Update proposal_test.go
shaspitz Sep 15, 2023
2d46f4d
apply new param to keeper code
shaspitz Sep 15, 2023
9f305db
Update params_test.go
shaspitz Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions proto/interchain_security/ccv/v1/shared_consumer.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ message Params {
// Provider-originated reward denoms. These are denoms coming from the
// provider which are allowed to be used as rewards. e.g. "uatom"
repeated string provider_reward_denoms = 12;

// The period after which a consumer can retry sending a throttled packet.
google.protobuf.Duration retry_delay_period = 13
[ (gogoproto.nullable) = false, (gogoproto.stdduration) = true ];
}

// GenesisState defines the CCV consumer chain genesis state.
Expand Down
1 change: 1 addition & 0 deletions tests/difference/core/driver/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ func (b *Builder) createConsumerGenesis(client *ibctmtypes.ClientState) *ccv.Gen
"0", // disable soft opt-out
[]string{},
[]string{},
ccv.DefaultRetryDelayPeriod,
)
return ccv.NewInitialGenesisState(client, providerConsState, valUpdates, params)
}
Expand Down
7 changes: 7 additions & 0 deletions x/ccv/consumer/keeper/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ func (k Keeper) GetConsumerParams(ctx sdk.Context) ccvtypes.Params {
k.GetSoftOptOutThreshold(ctx),
k.GetRewardDenoms(ctx),
k.GetProviderRewardDenoms(ctx),
k.GetRetryDelayPeriod(ctx),
)
}

Expand Down Expand Up @@ -138,3 +139,9 @@ func (k Keeper) GetProviderRewardDenoms(ctx sdk.Context) []string {
k.paramStore.Get(ctx, ccvtypes.KeyProviderRewardDenoms, &denoms)
return denoms
}

func (k Keeper) GetRetryDelayPeriod(ctx sdk.Context) time.Duration {
var period time.Duration
k.paramStore.Get(ctx, ccvtypes.KeyRetryDelayPeriod, &period)
return period
}
3 changes: 2 additions & 1 deletion x/ccv/consumer/keeper/params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ func TestParams(t *testing.T) {
ccv.DefaultSoftOptOutThreshold,
rewardDenoms,
provideRewardDenoms,
ccv.DefaultRetryDelayPeriod,
) // these are the default params, IBC suite independently sets enabled=true

params := consumerKeeper.GetConsumerParams(ctx)
require.Equal(t, expParams, params)

newParams := ccv.NewParams(false, 1000,
"channel-2", "cosmos19pe9pg5dv9k5fzgzmsrgnw9rl9asf7ddwhu7lm",
7*24*time.Hour, 25*time.Hour, "0.5", 500, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"})
7*24*time.Hour, 25*time.Hour, "0.5", 500, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour)
consumerKeeper.SetParams(ctx, newParams)
params = consumerKeeper.GetConsumerParams(ctx)
require.Equal(t, newParams, params)
Expand Down
6 changes: 1 addition & 5 deletions x/ccv/consumer/keeper/throttle_retry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package keeper

import (
"fmt"
"time"

sdktypes "github.com/cosmos/cosmos-sdk/types"

Expand Down Expand Up @@ -44,9 +43,6 @@ import (
// This design is implemented below, and in relay.go under SendPackets() and OnAcknowledgementPacket().
//

// Retry delay period could be implemented as a param, but 1 hour is reasonable
const RetryDelayPeriod = time.Hour

// PacketSendingPermitted returns whether the consumer is allowed to send packets
// from the pending packets queue.
func (k Keeper) PacketSendingPermitted(ctx sdktypes.Context) bool {
Expand All @@ -60,7 +56,7 @@ func (k Keeper) PacketSendingPermitted(ctx sdktypes.Context) bool {
return false
}
// If retry delay period has elapsed, we can send again
return ctx.BlockTime().After(record.SendTime.Add(RetryDelayPeriod))
return ctx.BlockTime().After(record.SendTime.Add(k.GetRetryDelayPeriod(ctx)))
}

func (k Keeper) UpdateSlashRecordOnSend(ctx sdktypes.Context) {
Expand Down
7 changes: 5 additions & 2 deletions x/ccv/consumer/keeper/throttle_retry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@ import (
"github.com/stretchr/testify/require"

testutil "github.com/cosmos/interchain-security/v3/testutil/keeper"
consumerkeeper "github.com/cosmos/interchain-security/v3/x/ccv/consumer/keeper"
consumertypes "github.com/cosmos/interchain-security/v3/x/ccv/consumer/types"
ccvtypes "github.com/cosmos/interchain-security/v3/x/ccv/types"
)

func TestPacketSendingPermitted(t *testing.T) {
consumerKeeper, ctx, ctrl, _ := testutil.GetConsumerKeeperAndCtx(t, testutil.NewInMemKeeperParams(t))
defer ctrl.Finish()

consumerKeeper.SetParams(ctx, ccvtypes.DefaultParams())

ctx = ctx.WithBlockTime(time.Now())

// No slash record exists, send is permitted
Expand Down Expand Up @@ -42,7 +44,8 @@ func TestPacketSendingPermitted(t *testing.T) {
require.False(t, consumerKeeper.PacketSendingPermitted(ctx))

// Elapse retry delay period
ctx = ctx.WithBlockTime(ctx.BlockTime().Add(2 * consumerkeeper.RetryDelayPeriod))
period := consumerKeeper.GetRetryDelayPeriod(ctx)
ctx = ctx.WithBlockTime(ctx.BlockTime().Add(2 * period))

// Now packet sending is permitted again
require.True(t, consumerKeeper.PacketSendingPermitted(ctx))
Expand Down
3 changes: 3 additions & 0 deletions x/ccv/consumer/types/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ func TestValidateInitialGenesisState(t *testing.T) {
types.DefaultSoftOptOutThreshold,
[]string{},
[]string{},
types.DefaultRetryDelayPeriod,
)),
true,
},
Expand All @@ -241,6 +242,7 @@ func TestValidateInitialGenesisState(t *testing.T) {
types.DefaultSoftOptOutThreshold,
[]string{},
[]string{},
types.DefaultRetryDelayPeriod,
)),
true,
},
Expand Down Expand Up @@ -442,6 +444,7 @@ func TestValidateRestartGenesisState(t *testing.T) {
types.DefaultSoftOptOutThreshold,
[]string{},
[]string{},
types.DefaultRetryDelayPeriod,
)),
true,
},
Expand Down
36 changes: 22 additions & 14 deletions x/ccv/consumer/types/params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,59 +19,67 @@ func TestValidateParams(t *testing.T) {
{"default params", ccvtypes.DefaultParams(), true},
{
"custom valid params",
ccvtypes.NewParams(true, 5, "", "", 1004, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), true,
ccvtypes.NewParams(true, 5, "", "", 1004, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), true,
},
{
"custom invalid params, block per dist transmission",
ccvtypes.NewParams(true, -5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, -5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, dist transmission channel",
ccvtypes.NewParams(true, 5, "badchannel/", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "badchannel/", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, ccv timeout",
ccvtypes.NewParams(true, 5, "", "", -5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "", "", -5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, transfer timeout",
ccvtypes.NewParams(true, 5, "", "", 1004, -7, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "", "", 1004, -7, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, consumer redist fraction is negative",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "-0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "-0.5", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, consumer redist fraction is over 1",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "1.2", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "1.2", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, bad consumer redist fraction ",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "notFrac", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "notFrac", 1000, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, negative num historical entries",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", -100, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", -100, 24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, negative unbonding period",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, -24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, -24*21*time.Hour, "0.05", []string{"untrn"}, []string{"uatom"}, 2*time.Hour), false,
},
{
"custom invalid params, invalid soft opt out threshold",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "-0.05", []string{"u"}, []string{}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "-0.05", []string{"u"}, []string{}, 2*time.Hour), false,
},
{
"custom invalid params, invalid soft opt out threshold",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.5", []string{"u"}, []string{}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.5", []string{"u"}, []string{}, 2*time.Hour), false,
},
{
"custom invalid params, invalid reward denom",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"u"}, []string{}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{"u"}, []string{}, 2*time.Hour), false,
},
{
"custom invalid params, invalid provider reward denom",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{}, []string{"a"}), false,
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{}, []string{"a"}, 2*time.Hour), false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test with retry delay period = 0? Seems like an obvious edge case to document via test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency we'd prob want to do this for all the non-zero params, but relevant to this PR I've added the test case 9f305db

},
{
"custom invalid params, retry delay period is negative",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{}, []string{}, -2*time.Hour), false,
},
{
"custom invalid params, retry delay period is zero",
ccvtypes.NewParams(true, 5, "", "", 5, 1005, "0.5", 1000, 24*21*time.Hour, "0.05", []string{}, []string{}, 0), false,
},
}

Expand Down
1 change: 1 addition & 0 deletions x/ccv/provider/keeper/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ func (k Keeper) MakeConsumerGenesis(
"0.05",
[]string{},
[]string{},
ccv.DefaultRetryDelayPeriod,
)

gen = *ccv.NewInitialGenesisState(
Expand Down
3 changes: 2 additions & 1 deletion x/ccv/provider/keeper/proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,7 +813,8 @@ func TestMakeConsumerGenesis(t *testing.T) {
"unbonding_period": 1728000000000000,
"soft_opt_out_threshold": "0.05",
"reward_denoms": [],
"provider_reward_denoms": []
"provider_reward_denoms": [],
"retry_delay_period": 3600000000000
},
"new_chain": true,
"provider_client_state": {
Expand Down
14 changes: 13 additions & 1 deletion x/ccv/types/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ const (

// By default, the bottom 5% of the validator set can opt out of validating consumer chains
DefaultSoftOptOutThreshold = "0.05"

// Default retry delay period is 1 hour.
DefaultRetryDelayPeriod = time.Hour
)

// Reflection based keys for params subspace
Expand All @@ -54,6 +57,7 @@ var (
KeySoftOptOutThreshold = []byte("SoftOptOutThreshold")
KeyRewardDenoms = []byte("RewardDenoms")
KeyProviderRewardDenoms = []byte("ProviderRewardDenoms")
KeyRetryDelayPeriod = []byte("RetryDelayPeriod")
)

// ParamKeyTable type declaration for parameters
Expand All @@ -66,7 +70,8 @@ func NewParams(enabled bool, blocksPerDistributionTransmission int64,
distributionTransmissionChannel, providerFeePoolAddrStr string,
ccvTimeoutPeriod, transferTimeoutPeriod time.Duration,
consumerRedistributionFraction string, historicalEntries int64,
consumerUnbondingPeriod time.Duration, softOptOutThreshold string, rewardDenoms, providerRewardDenoms []string,
consumerUnbondingPeriod time.Duration, softOptOutThreshold string,
rewardDenoms, providerRewardDenoms []string, retryDelayPeriod time.Duration,
) Params {
return Params{
Enabled: enabled,
Expand All @@ -81,6 +86,7 @@ func NewParams(enabled bool, blocksPerDistributionTransmission int64,
SoftOptOutThreshold: softOptOutThreshold,
RewardDenoms: rewardDenoms,
ProviderRewardDenoms: providerRewardDenoms,
RetryDelayPeriod: retryDelayPeriod,
}
}

Expand All @@ -101,6 +107,7 @@ func DefaultParams() Params {
DefaultSoftOptOutThreshold,
rewardDenoms,
provideRewardDenoms,
DefaultRetryDelayPeriod,
)
}

Expand Down Expand Up @@ -142,6 +149,9 @@ func (p Params) Validate() error {
if err := ValidateDenoms(p.ProviderRewardDenoms); err != nil {
return err
}
if err := ValidateDuration(p.RetryDelayPeriod); err != nil {
return err
}
return nil
}

Expand Down Expand Up @@ -171,6 +181,8 @@ func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
p.RewardDenoms, ValidateDenoms),
paramtypes.NewParamSetPair(KeyProviderRewardDenoms,
p.ProviderRewardDenoms, ValidateDenoms),
paramtypes.NewParamSetPair(KeyRetryDelayPeriod,
p.RetryDelayPeriod, ValidateDuration),
}
}

Expand Down
Loading
Loading