From 85dc2807e1251bf7a481aa2debc7b7a2b475b64a Mon Sep 17 00:00:00 2001 From: insumity Date: Fri, 27 Sep 2024 15:56:17 +0200 Subject: [PATCH] took into account comments --- x/ccv/provider/ibc_middleware.go | 2 ++ x/ccv/provider/keeper/distribution.go | 34 +++++++++------------- x/ccv/provider/keeper/distribution_test.go | 8 ++--- x/ccv/provider/keeper/msg_server.go | 10 +++---- x/ccv/provider/types/keys.go | 4 +++ 5 files changed, 26 insertions(+), 32 deletions(-) diff --git a/x/ccv/provider/ibc_middleware.go b/x/ccv/provider/ibc_middleware.go index 796ced5cb1..4454f5ed2e 100644 --- a/x/ccv/provider/ibc_middleware.go +++ b/x/ccv/provider/ibc_middleware.go @@ -206,6 +206,7 @@ func (im IBCMiddleware) OnRecvPacket( "denom", coinDenom, "error", err.Error(), ) + return ack } alloc.Rewards = alloc.Rewards.Add( @@ -223,6 +224,7 @@ func (im IBCMiddleware) OnRecvPacket( "denom", coinDenom, "error", err.Error(), ) + return ack } logger.Info( diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index dbfdadfb5b..b20c07c15e 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -81,15 +81,11 @@ func (k Keeper) GetAllowlistedRewardDenoms(ctx sdk.Context, consumerId string) ( return denoms.Denoms, nil } -// SetAllowlistedRewardDenom sets the allowlisted reward denom for the given consumer id. -func (k Keeper) SetAllowlistedRewardDenom(ctx sdk.Context, consumerId string, denom string) error { - currentDenoms, err := k.GetAllowlistedRewardDenoms(ctx, consumerId) - if err != nil { - return err - } +// SetAllowlistedRewardDenoms sets the allowlisted reward denoms for the given consumer id. +func (k Keeper) SetAllowlistedRewardDenoms(ctx sdk.Context, consumerId string, rewardDenoms []string) error { store := ctx.KVStore(k.storeKey) - updatedDenoms := types.AllowlistedRewardDenoms{Denoms: append(currentDenoms, denom)} - bz, err := updatedDenoms.Marshal() + allowlistedUpdatedDenoms := types.AllowlistedRewardDenoms{Denoms: rewardDenoms} + bz, err := allowlistedUpdatedDenoms.Marshal() if err != nil { return err } @@ -97,22 +93,16 @@ func (k Keeper) SetAllowlistedRewardDenom(ctx sdk.Context, consumerId string, de return nil } -// DeleteAllowlistedRewardDenom deletes the allowlisted reward denom for the given consumer id. -func (k Keeper) DeleteAllowlistedRewardDenom(ctx sdk.Context, consumerId string) { +// DeleteAllowlistedRewardDenoms deletes the allowlisted reward denom for the given consumer id. +func (k Keeper) DeleteAllowlistedRewardDenoms(ctx sdk.Context, consumerId string) { store := ctx.KVStore(k.storeKey) store.Delete(types.ConsumerIdToAllowlistedRewardDenomKey(consumerId)) } // UpdateAllowlistedRewardDenoms updates the allowlisted reward denoms for this consumer chain with the provided `rewardDenoms` func (k Keeper) UpdateAllowlistedRewardDenoms(ctx sdk.Context, consumerId string, rewardDenoms []string) error { - k.DeleteAllowlistedRewardDenom(ctx, consumerId) - for _, denom := range rewardDenoms { - err := k.SetAllowlistedRewardDenom(ctx, consumerId, denom) - if err != nil { - return err - } - } - return nil + k.DeleteAllowlistedRewardDenoms(ctx, consumerId) + return k.SetAllowlistedRewardDenoms(ctx, consumerId, rewardDenoms) } // GetConsumerRewardsAllocationByDenom returns the consumer rewards allocation for the given consumer id and denom @@ -251,7 +241,7 @@ func (k Keeper) AllocateConsumerRewards(ctx sdk.Context, consumerId string, allo "consumerId", consumerId, "chainId", chainId, "total-rewards", consumerRewards.String(), - "sent-to-distribution", validatorsRewardsTrunc.String(), + "sent-to-validators", validatorsRewardsTrunc.String(), "sent-to-CP", remainingRewards.String(), ) @@ -289,6 +279,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { "fail to retrieve the allowlisted reward denoms for consumer chain", "consumer id", consumerId, "error", err.Error()) + continue } allAllowlistedDenoms := append(allConsumerRewardDenoms, consumerAllowlistedRewardDenoms...) @@ -304,8 +295,9 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { "denom", denom, "error", err.Error(), ) + continue } - allocatedRewards, err := k.AllocateConsumerRewards(cachedCtx, consumerId, consumerRewards) + remainingRewardDec, err := k.AllocateConsumerRewards(cachedCtx, consumerId, consumerRewards) if err != nil { k.Logger(ctx).Error( "fail to allocate rewards for consumer chain", @@ -314,7 +306,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { ) continue } - err = k.SetConsumerRewardsAllocationByDenom(cachedCtx, consumerId, denom, allocatedRewards) + err = k.SetConsumerRewardsAllocationByDenom(cachedCtx, consumerId, denom, remainingRewardDec) if err != nil { k.Logger(ctx).Error( "fail to set rewards for consumer chain", diff --git a/x/ccv/provider/keeper/distribution_test.go b/x/ccv/provider/keeper/distribution_test.go index 446bc94ce8..ee5afd8a2b 100644 --- a/x/ccv/provider/keeper/distribution_test.go +++ b/x/ccv/provider/keeper/distribution_test.go @@ -379,7 +379,7 @@ func TestHandleSetConsumerCommissionRate(t *testing.T) { } // TestAllowlistedRewardDenoms tests the `GetAllowlistedRewardDenoms`, `SetAllowlistedRewardDenom`, -// `UpdateAllowlistedRewardDenoms` and `DeleteAllowlistedRewardDenom` methods. +// `UpdateAllowlistedRewardDenoms` and `DeleteAllowlistedRewardDenoms` methods. func TestAllowlistedRewardDenoms(t *testing.T) { providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) defer ctrl.Finish() @@ -390,9 +390,7 @@ func TestAllowlistedRewardDenoms(t *testing.T) { require.NoError(t, err) denomsToSet := []string{"denom1", "denom2", "denom3"} - providerKeeper.SetAllowlistedRewardDenom(ctx, consumerId, "denom1") - providerKeeper.SetAllowlistedRewardDenom(ctx, consumerId, "denom2") - providerKeeper.SetAllowlistedRewardDenom(ctx, consumerId, "denom3") + providerKeeper.SetAllowlistedRewardDenoms(ctx, consumerId, denomsToSet) denoms, err = providerKeeper.GetAllowlistedRewardDenoms(ctx, consumerId) require.Equal(t, denomsToSet, denoms) @@ -405,7 +403,7 @@ func TestAllowlistedRewardDenoms(t *testing.T) { require.Equal(t, updatedDenoms, denoms) require.NoError(t, err) - providerKeeper.DeleteAllowlistedRewardDenom(ctx, consumerId) + providerKeeper.DeleteAllowlistedRewardDenoms(ctx, consumerId) denoms, err = providerKeeper.GetAllowlistedRewardDenoms(ctx, consumerId) require.Empty(t, denoms) require.NoError(t, err) diff --git a/x/ccv/provider/keeper/msg_server.go b/x/ccv/provider/keeper/msg_server.go index 301fa83c39..fbef100bb9 100644 --- a/x/ccv/provider/keeper/msg_server.go +++ b/x/ccv/provider/keeper/msg_server.go @@ -19,8 +19,6 @@ import ( ccvtypes "github.com/cosmos/interchain-security/v6/x/ccv/types" ) -const MaxAllowlistedRewardDenomsPerChain = 3 - type msgServer struct { *Keeper } @@ -417,9 +415,9 @@ func (k msgServer) CreateConsumer(goCtx context.Context, msg *types.MsgCreateCon } if msg.AllowlistedRewardDenoms != nil { - if len(msg.AllowlistedRewardDenoms.Denoms) > MaxAllowlistedRewardDenomsPerChain { + if len(msg.AllowlistedRewardDenoms.Denoms) > types.MaxAllowlistedRewardDenomsPerChain { return &resp, errorsmod.Wrapf(types.ErrInvalidAllowlistedRewardDenoms, - fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", MaxAllowlistedRewardDenomsPerChain)) + fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", types.MaxAllowlistedRewardDenomsPerChain)) } err := k.UpdateAllowlistedRewardDenoms(ctx, consumerId, msg.AllowlistedRewardDenoms.Denoms) @@ -605,9 +603,9 @@ func (k msgServer) UpdateConsumer(goCtx context.Context, msg *types.MsgUpdateCon } if msg.AllowlistedRewardDenoms != nil { - if len(msg.AllowlistedRewardDenoms.Denoms) > MaxAllowlistedRewardDenomsPerChain { + if len(msg.AllowlistedRewardDenoms.Denoms) > types.MaxAllowlistedRewardDenomsPerChain { return &resp, errorsmod.Wrapf(types.ErrInvalidAllowlistedRewardDenoms, - fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", MaxAllowlistedRewardDenomsPerChain)) + fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", types.MaxAllowlistedRewardDenomsPerChain)) } if err := k.UpdateAllowlistedRewardDenoms(ctx, consumerId, msg.AllowlistedRewardDenoms.Denoms); err != nil { diff --git a/x/ccv/provider/types/keys.go b/x/ccv/provider/types/keys.go index b07a5e84e7..345a26e252 100644 --- a/x/ccv/provider/types/keys.go +++ b/x/ccv/provider/types/keys.go @@ -33,6 +33,10 @@ const ( // This address receives rewards from consumer chains ConsumerRewardsPool = "consumer_rewards_pool" + // MaxAllowlistedRewardDenomsPerChain corresponds to the maximum number of reward denoms + // a consumer chain can allowlist + MaxAllowlistedRewardDenomsPerChain = 3 + // Names for the store keys. // Used for storing the byte prefixes in the constant map. // See getKeyPrefixes().