From 5738abe0ada331186a88634c81f54bee7949346f Mon Sep 17 00:00:00 2001 From: insumity Date: Wed, 25 Sep 2024 13:53:25 +0200 Subject: [PATCH] fixed integration tests --- .../ccv/provider/v1/tx.proto | 1 + tests/integration/distribution.go | 97 +++++++------- x/ccv/provider/ibc_middleware.go | 16 +-- x/ccv/provider/keeper/distribution.go | 122 ++++++++++++++---- x/ccv/provider/keeper/distribution_test.go | 59 +++++++++ x/ccv/provider/keeper/keeper.go | 61 --------- x/ccv/provider/keeper/keeper_test.go | 15 +++ x/ccv/provider/keeper/msg_server.go | 27 ++-- x/ccv/provider/types/errors.go | 1 + 9 files changed, 237 insertions(+), 162 deletions(-) diff --git a/proto/interchain_security/ccv/provider/v1/tx.proto b/proto/interchain_security/ccv/provider/v1/tx.proto index 2ceedbf710..9e89332aef 100644 --- a/proto/interchain_security/ccv/provider/v1/tx.proto +++ b/proto/interchain_security/ccv/provider/v1/tx.proto @@ -234,6 +234,7 @@ message MsgChangeRewardDenoms { repeated string denoms_to_remove = 2; // authority is the address of the governance account string authority = 3 [(cosmos_proto.scalar) = "cosmos.AddressString"]; + } // MsgChangeRewardDenomsResponse defines response type for MsgChangeRewardDenoms messages diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 71908e134b..453ddd4c27 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -107,21 +107,35 @@ func (s *CCVTestSuite) TestRewardsDistribution() { rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) s.Require().Equal(rewardCoins.AmountOf(rewardsIBCdenom), providerExpRewardsAmount) - // Set the consumer reward denom. This would be done by a governance proposal in prod. + // increase the block height so validators are eligible for consumer rewards (see `IsEligibleForConsumerRewards`) + currentProviderBlockHeight := s.providerCtx().BlockHeight() + numberOfBlocksToStartReceivingRewards := providerKeeper.GetNumberOfEpochsToStartReceivingRewards(s.providerCtx()) * providerKeeper.GetBlocksPerEpoch(s.providerCtx()) + for s.providerCtx().BlockHeight() <= currentProviderBlockHeight+numberOfBlocksToStartReceivingRewards { + s.providerChain.NextBlock() + } + + // Allowlist the consumer reward denom. This would be done by a governance proposal in prod. providerKeeper.SetConsumerRewardDenom(s.providerCtx(), rewardsIBCdenom) - // Refill the consumer fee pool - err = consumerBankKeeper.SendCoinsFromAccountToModule( - s.consumerCtx(), - s.consumerChain.SenderAccount.GetAddress(), - authtypes.FeeCollectorName, - fees, + // Even though the reward denom is allowlisted, the rewards are not yet distributed because `BeginBlockRD` has not executed. + // and hence the distribution has not taken place. Because of this, all the rewards are still stored in consumer rewards allocation by denom. + rewardsAlloc := providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, rewardsIBCdenom) + remainingAlloc := rewardsAlloc.Rewards.AmountOf(rewardsIBCdenom) + s.Require().Equal( + math.LegacyNewDecFromInt(providerExpRewardsAmount), + rewardsAlloc.Rewards.AmountOf(rewardsIBCdenom), ) - s.Require().NoError(err) + s.Require().False(remainingAlloc.LTE(math.LegacyOneDec())) - // Pass two blocks - s.consumerChain.NextBlock() - s.consumerChain.NextBlock() + // Move by one block to execute `BeginBlockRD`. + s.providerChain.NextBlock() + + // Consumer allocations are now distributed between the validators and the community pool. + // The decimals resulting from the distribution are expected to remain in the consumer allocations + // and hence the check that remainingAlloc is less than one. + rewardsAlloc = providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, rewardsIBCdenom) + remainingAlloc = rewardsAlloc.Rewards.AmountOf(rewardsIBCdenom) + s.Require().True(remainingAlloc.LTE(math.LegacyOneDec())) // Save the consumer validators total outstanding rewards on the provider consumerValsOutstandingRewardsFunc := func(ctx sdk.Context) sdk.DecCoins { @@ -137,45 +151,17 @@ func (s *CCVTestSuite) TestRewardsDistribution() { valReward, _ := providerDistributionKeeper.GetValidatorOutstandingRewards(ctx, valAddr) totalRewards = totalRewards.Add(valReward.Rewards...) } - return totalRewards - } - consuValsRewards := consumerValsOutstandingRewardsFunc(s.providerCtx()) - // increase the block height so validators are eligible for consumer rewards (see `IsEligibleForConsumerRewards`) - numberOfBlocksToStartReceivingRewards := providerKeeper.GetNumberOfEpochsToStartReceivingRewards(s.providerCtx()) * providerKeeper.GetBlocksPerEpoch(s.providerCtx()) - - for s.providerCtx().BlockHeight() <= numberOfBlocksToStartReceivingRewards { - s.providerChain.NextBlock() + return totalRewards } - // Transfer rewards from consumer to provider and distribute rewards to - // validators and community pool by calling BeginBlockRD - relayAllCommittedPackets( - s, - s.consumerChain, - s.transferPath, - transfertypes.PortID, - s.transferPath.EndpointA.ChannelID, - 1, - ) - - // Consumer allocations are distributed between the validators and the community pool. - // The decimals resulting from the distribution are expected to remain in the consumer allocations. - rewardsAlloc := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.getFirstBundle().ConsumerId) - remainingAlloc := rewardsAlloc.Rewards.AmountOf(rewardsIBCdenom) - s.Require().True(remainingAlloc.LTE(math.LegacyOneDec())) - - // Check that the reward pool still holds the coins from the first transfer - // which were never allocated since they were not whitelisted - // plus the remaining decimals from the second transfer. + // Check that the reward pool does not hold the coins from the transfer because + // after the coin got allowlisted, all coins of this denom were distributed. rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - s.Require().Equal( - math.LegacyNewDecFromInt(rewardCoins.AmountOf(rewardsIBCdenom)), - math.LegacyNewDecFromInt(providerExpRewardsAmount).Add(remainingAlloc), - ) + s.Require().True(rewardCoins.AmountOf(rewardsIBCdenom).LTE(math.NewInt(1))) // Check that the distribution module account balance is equal to the consumer rewards - consuValsRewardsReceived := consumerValsOutstandingRewardsFunc(s.providerCtx()).Sub(consuValsRewards) + consuValsRewardsReceived := consumerValsOutstandingRewardsFunc(s.providerCtx()) distrAcct := providerDistributionKeeper.GetDistributionAccount(s.providerCtx()) distrAcctBalance := providerBankKeeper.GetAllBalances(s.providerCtx(), distrAcct.GetAddress()) @@ -259,7 +245,7 @@ func (s *CCVTestSuite) TestSendRewardsRetries() { } // TestEndBlockRD tests that the last transmission block height (LTBH) is correctly updated after the expected -// number of block have passed. It also checks that the IBC transfer transfer states are discarded if +// number of block have passed. It also checks that the IBC transfer states are discarded if // the reward distribution to the provider has failed. // // Note: this method is effectively a unit test for EndBLockRD(), but is written as an integration test to avoid excessive mocking. @@ -570,7 +556,7 @@ func (s *CCVTestSuite) TestIBCTransferMiddleware() { { "IBC Transfer coin denom isn't registered", func(ctx sdk.Context, keeper *providerkeeper.Keeper, bankKeeper icstestingutils.TestBankKeeper) {}, - false, + true, // even if the denom is not registered/allowlisted, the rewards are still allocated by denom false, }, { @@ -600,9 +586,10 @@ func (s *CCVTestSuite) TestIBCTransferMiddleware() { sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100_000))), ) // update consumer allocation - keeper.SetConsumerRewardsAllocation( + keeper.SetConsumerRewardsAllocationByDenom( ctx, s.getFirstBundle().ConsumerId, + getIBCDenom(packet.DestinationPort, packet.DestinationChannel), providertypes.ConsumerRewardsAllocation{ Rewards: sdk.NewDecCoins(sdk.NewDecCoin(sdk.DefaultBondDenom, math.NewInt(100_000))), }, @@ -669,7 +656,8 @@ func (s *CCVTestSuite) TestIBCTransferMiddleware() { rewardsPoolBalance := bankKeeper.GetAllBalances(s.providerCtx(), sdk.MustAccAddressFromBech32(data.Receiver)) // save the consumer's rewards allocated - consumerRewardsAllocations := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.getFirstBundle().ConsumerId) + ibcDenom := getIBCDenom(packet.DestinationPort, packet.DestinationChannel) + consumerRewardsAllocations := providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, ibcDenom) // execute middleware OnRecvPacket logic ack := cbs.OnRecvPacket(s.providerCtx(), packet, sdk.AccAddress{}) @@ -677,13 +665,13 @@ func (s *CCVTestSuite) TestIBCTransferMiddleware() { // compute expected rewards with provider denom expRewards := sdk.Coin{ Amount: amount, - Denom: getIBCDenom(packet.DestinationPort, packet.DestinationChannel), + Denom: ibcDenom, } // compute the balance and allocation difference rewardsTransferred := bankKeeper.GetAllBalances(s.providerCtx(), sdk.MustAccAddressFromBech32(data.Receiver)). Sub(rewardsPoolBalance...) - rewardsAllocated := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.getFirstBundle().ConsumerId). + rewardsAllocated := providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, ibcDenom). Rewards.Sub(consumerRewardsAllocations.Rewards) if !tc.expErr { @@ -738,13 +726,18 @@ func (s *CCVTestSuite) TestAllocateTokens() { totalRewards, ) + // Allowlist a reward denom that the allocated rewards have + ibcDenom := "ibc/somedenom" + providerKeeper.SetConsumerRewardDenom(providerCtx, ibcDenom) + // Allocate rewards evenly between consumers rewardsPerChain := totalRewards.QuoInt(math.NewInt(int64(len(s.consumerBundles)))) for consumerId := range s.consumerBundles { // update consumer allocation - providerKeeper.SetConsumerRewardsAllocation( + providerKeeper.SetConsumerRewardsAllocationByDenom( providerCtx, consumerId, + ibcDenom, providertypes.ConsumerRewardsAllocation{ Rewards: sdk.NewDecCoinsFromCoins(rewardsPerChain...), }, @@ -798,7 +791,7 @@ func (s *CCVTestSuite) TestAllocateTokens() { // check that the total expected rewards are transferred to the distribution module account // store the decimal remainders in the consumer reward allocations - allocRemainderPerChain := providerKeeper.GetConsumerRewardsAllocation(providerCtx, s.getFirstBundle().ConsumerId).Rewards + allocRemainderPerChain := providerKeeper.GetConsumerRewardsAllocationByDenom(providerCtx, s.getFirstBundle().ConsumerId, ibcDenom).Rewards // compute the total rewards distributed to the distribution module balance (validator outstanding rewards + community pool tax), totalRewardsDistributed := sdk.NewDecCoinsFromCoins(totalRewards...).Sub(allocRemainderPerChain.MulDec(math.LegacyNewDec(int64(consNum)))) diff --git a/x/ccv/provider/ibc_middleware.go b/x/ccv/provider/ibc_middleware.go index 966f31e06e..1f3d0bc36c 100644 --- a/x/ccv/provider/ibc_middleware.go +++ b/x/ccv/provider/ibc_middleware.go @@ -196,23 +196,13 @@ func (im IBCMiddleware) OnRecvPacket( sdk.NewAttribute(types.AttributeRewardAmount, data.Amount), }...) - alloc2 := im.keeper.GetConsumerRewardsAllocationByDenom(ctx, consumerId, coinDenom) - alloc2.Rewards = alloc2.Rewards.Add( + alloc := im.keeper.GetConsumerRewardsAllocationByDenom(ctx, consumerId, coinDenom) + alloc.Rewards = alloc.Rewards.Add( sdk.NewDecCoinFromCoin(sdk.Coin{ Denom: coinDenom, Amount: coinAmt, })) - im.keeper.SetConsumerRewardsAllocationByDenom(ctx, consumerId, coinDenom, alloc2) - - if im.keeper.ConsumerRewardDenomExists(ctx, coinDenom) { - alloc := im.keeper.GetConsumerRewardsAllocation(ctx, consumerId) - alloc.Rewards = alloc.Rewards.Add( - sdk.NewDecCoinsFromCoins(sdk.Coin{ - Denom: coinDenom, - Amount: coinAmt, - })...) - im.keeper.SetConsumerRewardsAllocation(ctx, consumerId, alloc) - } + im.keeper.SetConsumerRewardsAllocationByDenom(ctx, consumerId, coinDenom, alloc) logger.Info( "scheduled ICS rewards to be distributed", diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 12f9dc1a24..78d4c82cfe 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -66,6 +66,86 @@ func (k Keeper) GetAllConsumerRewardDenoms(ctx sdk.Context) (consumerRewardDenom return consumerRewardDenoms } +// GetAllowlistedRewardDenoms returns the allowlisted reward denom for the given consumer id. +func (k Keeper) GetAllowlistedRewardDenoms(ctx sdk.Context, consumerId string) ([]string, error) { + store := ctx.KVStore(k.storeKey) + bz := store.Get(types.ConsumerIdToAllowlistedRewardDenomKey(consumerId)) + if bz == nil { + return []string{}, nil + } + + var denoms types.AllowlistedRewardDenoms + if err := denoms.Unmarshal(bz); err != nil { + return []string{}, errorsmod.Wrapf(err, "cannot unmarshal allowlisted reward denoms for consumer id: %s", consumerId) + } + 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 + } + store := ctx.KVStore(k.storeKey) + updatedDenoms := types.AllowlistedRewardDenoms{Denoms: append(currentDenoms, denom)} + bz, err := updatedDenoms.Marshal() + if err != nil { + return err + } + store.Set(types.ConsumerIdToAllowlistedRewardDenomKey(consumerId), bz) + return nil +} + +// DeleteAllowlistedRewardDenom deletes the allowlisted reward denom for the given consumer id. +func (k Keeper) DeleteAllowlistedRewardDenom(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 +} + +// GetConsumerRewardsAllocationByDenom returns the consumer rewards allocation for the given consumer id and denom +func (k Keeper) GetConsumerRewardsAllocationByDenom(ctx sdk.Context, consumerId string, denom string) (types.ConsumerRewardsAllocation, error) { + store := ctx.KVStore(k.storeKey) + bz := store.Get(types.ConsumerRewardsAllocationByDenomKey(consumerId, denom)) + + var rewardsAllocation types.ConsumerRewardsAllocation + err := rewardsAllocation.Unmarshal(bz) + if err != nil { + return types.ConsumerRewardsAllocation{}, err + } + + return rewardsAllocation, nil +} + +// SetConsumerRewardsAllocationByDenom sets the consumer rewards allocation for the given consumer id and denom +func (k Keeper) SetConsumerRewardsAllocationByDenom(ctx sdk.Context, consumerId string, denom string, rewardsAllocation types.ConsumerRewardsAllocation) error { + store := ctx.KVStore(k.storeKey) + bz, err := rewardsAllocation.Marshal() + if err != nil { + return err + } + store.Set(types.ConsumerRewardsAllocationByDenomKey(consumerId, denom), bz) + return nil +} + +// DeleteConsumerRewardsAllocationByDenom deletes the consumer rewards allocation for the given consumer id and denom +func (k Keeper) DeleteConsumerRewardsAllocationByDenom(ctx sdk.Context, consumerId string, denom string) { + store := ctx.KVStore(k.storeKey) + store.Delete(types.ConsumerRewardsAllocationByDenomKey(consumerId, denom)) +} + // AllocateConsumerRewards allocates the given rewards to provider consumer chain with the given consumer id func (k Keeper) AllocateConsumerRewards(ctx sdk.Context, consumerId string, alloc types.ConsumerRewardsAllocation) (types.ConsumerRewardsAllocation, error) { if alloc.Rewards.IsZero() { @@ -105,7 +185,6 @@ func (k Keeper) AllocateConsumerRewards(ctx sdk.Context, consumerId string, allo // set the consumer allocation to the remaining reward decimals alloc.Rewards = rewardsChange - // WE DO NOT RETURN AN ERROR HERE because we want to update ... return alloc, nil } @@ -166,7 +245,6 @@ func (k Keeper) AllocateConsumerRewards(ctx sdk.Context, consumerId string, allo // set consumer allocations to the remaining rewards decimals alloc.Rewards = validatorsRewardsChange.Add(remainingChanges...) - //k.SetConsumerRewardsAllocation(ctx, consumerId, alloc) k.Logger(ctx).Info( "distributed ICS rewards successfully", @@ -202,23 +280,31 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { // Iterate over all launched consumer chains. // To avoid large iterations over all the consumer IDs, iterate only over // chains with an IBC client created. + allConsumerRewardDenoms := k.GetAllConsumerRewardDenoms(ctx) // corresponds to allowlisted denoms that were allowlisted through governance for _, consumerId := range k.GetAllConsumersWithIBCClients(ctx) { - oldRewards := k.GetConsumerRewardsAllocation(ctx, consumerId) - returnedRewards, err := k.AllocateConsumerRewards(ctx, consumerId, oldRewards) + // also consider this chain's allowlisted reward denoms + consumerAllowlistedRewardDenoms, err := k.GetAllowlistedRewardDenoms(ctx, consumerId) if err != nil { k.Logger(ctx).Error( - "fail to allocate rewards for consumer chain", + "fail to retrieve the allowlisted reward denoms for consumer chain", "consumer id", consumerId, - "error", err.Error(), - ) - } else { - k.SetConsumerRewardsAllocation(ctx, consumerId, returnedRewards) + "error", err.Error()) } - allAllowlistedDenoms := append(k.GetAllConsumerRewardDenoms(ctx), k.GetAllowlistedRewardDenoms(ctx, consumerId)...) + allAllowlistedDenoms := append(allConsumerRewardDenoms, consumerAllowlistedRewardDenoms...) for _, denom := range allAllowlistedDenoms { + // uUe a cached context to verify that the call to `AllocateConsumerRewards` is atomic, and hence + // all transfers in `AllocateConsumerRewards` happen all together or not at all. cachedCtx, writeCache := ctx.CacheContext() - consumerRewards := k.GetConsumerRewardsAllocationByDenom(cachedCtx, consumerId, denom) + consumerRewards, err := k.GetConsumerRewardsAllocationByDenom(cachedCtx, consumerId, denom) + if err != nil { + k.Logger(ctx).Error( + "failed to get the consumer rewards allocation for this denom", + "consumer id", consumerId, + "denom", denom, + "error", err.Error(), + ) + } allocatedRewards, err := k.AllocateConsumerRewards(cachedCtx, consumerId, consumerRewards) if err != nil { k.Logger(ctx).Error( @@ -229,19 +315,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { continue } k.SetConsumerRewardsAllocationByDenom(cachedCtx, consumerId, denom, allocatedRewards) - // TODO: fix for the tests - _ = writeCache - //writeCache() - } - - // note that it's possible that no rewards are collected even though the - // reward pool isn't empty. This can happen if the reward pool holds some tokens - // of non-whitelisted denominations. - rewardsAlloc := k.GetConsumerRewardsAllocation(ctx, consumerId) - remainingRewardsAlloc, err := k.AllocateConsumerRewards(ctx, consumerId, rewardsAlloc) - - if err == nil { - k.SetConsumerRewardsAllocation(ctx, consumerId, remainingRewardsAlloc) + writeCache() } } } diff --git a/x/ccv/provider/keeper/distribution_test.go b/x/ccv/provider/keeper/distribution_test.go index ba48758c21..446bc94ce8 100644 --- a/x/ccv/provider/keeper/distribution_test.go +++ b/x/ccv/provider/keeper/distribution_test.go @@ -377,3 +377,62 @@ func TestHandleSetConsumerCommissionRate(t *testing.T) { require.Equal(t, commissionRate, cr) require.True(t, found) } + +// TestAllowlistedRewardDenoms tests the `GetAllowlistedRewardDenoms`, `SetAllowlistedRewardDenom`, +// `UpdateAllowlistedRewardDenoms` and `DeleteAllowlistedRewardDenom` methods. +func TestAllowlistedRewardDenoms(t *testing.T) { + providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + + consumerId := "0" + denoms, err := providerKeeper.GetAllowlistedRewardDenoms(ctx, consumerId) + require.Empty(t, denoms) + require.NoError(t, err) + + denomsToSet := []string{"denom1", "denom2", "denom3"} + providerKeeper.SetAllowlistedRewardDenom(ctx, consumerId, "denom1") + providerKeeper.SetAllowlistedRewardDenom(ctx, consumerId, "denom2") + providerKeeper.SetAllowlistedRewardDenom(ctx, consumerId, "denom3") + + denoms, err = providerKeeper.GetAllowlistedRewardDenoms(ctx, consumerId) + require.Equal(t, denomsToSet, denoms) + require.NoError(t, err) + + updatedDenoms := []string{"updatedDenom1", "updatedDenom2"} + err = providerKeeper.UpdateAllowlistedRewardDenoms(ctx, consumerId, updatedDenoms) + require.NoError(t, err) + denoms, err = providerKeeper.GetAllowlistedRewardDenoms(ctx, consumerId) + require.Equal(t, updatedDenoms, denoms) + require.NoError(t, err) + + providerKeeper.DeleteAllowlistedRewardDenom(ctx, consumerId) + denoms, err = providerKeeper.GetAllowlistedRewardDenoms(ctx, consumerId) + require.Empty(t, denoms) + require.NoError(t, err) +} + +// TestConsumerRewardsAllocationByDenom tests the `*ConsumerRewardsAllocationByDenom* methods +func TestConsumerRewardsAllocationByDenom(t *testing.T) { + providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + + consumerId := "0" + denom := "denom" + rewards, err := providerKeeper.GetConsumerRewardsAllocationByDenom(ctx, consumerId, denom) + require.Empty(t, rewards.Rewards) + require.NoError(t, err) + + rewardAllocation := providertypes.ConsumerRewardsAllocation{ + Rewards: sdk.NewDecCoins(sdk.NewDecCoin("uatom", math.NewInt(1000))), + } + + err = providerKeeper.SetConsumerRewardsAllocationByDenom(ctx, consumerId, denom, rewardAllocation) + rewards, err = providerKeeper.GetConsumerRewardsAllocationByDenom(ctx, consumerId, denom) + require.Equal(t, rewardAllocation, rewards) + require.NoError(t, err) + + providerKeeper.DeleteConsumerRewardsAllocationByDenom(ctx, consumerId, denom) + rewards, err = providerKeeper.GetConsumerRewardsAllocationByDenom(ctx, consumerId, denom) + require.Empty(t, rewards.Rewards) + require.NoError(t, err) +} diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go index 0fb6de9790..55ddcf413f 100644 --- a/x/ccv/provider/keeper/keeper.go +++ b/x/ccv/provider/keeper/keeper.go @@ -681,67 +681,6 @@ func (k Keeper) DeleteConsumerClientId(ctx sdk.Context, consumerId string) { store.Delete(types.ConsumerIdToClientIdKey(consumerId)) } -// GetAllowlistedRewardDenoms returns the allowlisted reward denom for the given consumer id. -func (k Keeper) GetAllowlistedRewardDenoms(ctx sdk.Context, consumerId string) []string { - store := ctx.KVStore(k.storeKey) - bz := store.Get(types.ConsumerIdToAllowlistedRewardDenomKey(consumerId)) - if bz == nil { - return []string{} - } - - var denoms types.AllowlistedRewardDenoms - if err := denoms.Unmarshal(bz); err != nil { - // An error here would indicate something is very wrong, - // the PendingVSCPackets are assumed to be correctly serialized in AppendPendingVSCPackets. - panic(fmt.Errorf("cannot unmarshal pending validator set changes: %w", err)) - } - return denoms.Denoms -} - -// SetAllowlistedRewardDenom sets the allowlisted reward denom for the given consumer id. -func (k Keeper) SetAllowlistedRewardDenom(ctx sdk.Context, consumerId string, denom string) error { - denoms := k.GetAllowlistedRewardDenoms(ctx, consumerId) - // TODO: check nil and so on - denoms = append(denoms, denom) - store := ctx.KVStore(k.storeKey) - dDenoms := types.AllowlistedRewardDenoms{Denoms: denoms} - bz, err := dDenoms.Marshal() - if err != nil { - return err - } - store.Set(types.ConsumerIdToAllowlistedRewardDenomKey(consumerId), bz) - return nil -} - -// DeleteAllowlistedRewardDenom deletes the allowlisted reward denom for the given consumer id. -func (k Keeper) DeleteAllowlistedRewardDenom(ctx sdk.Context, consumerId string) { - store := ctx.KVStore(k.storeKey) - store.Delete(types.ConsumerIdToAllowlistedRewardDenomKey(consumerId)) -} - -// GetConsumerRewardsAllocationByDenom returns the consumer rewards allocation for the given consumer id and denom -func (k Keeper) GetConsumerRewardsAllocationByDenom(ctx sdk.Context, consumerId string, denom string) (pool types.ConsumerRewardsAllocation) { - store := ctx.KVStore(k.storeKey) - b := store.Get(types.ConsumerRewardsAllocationByDenomKey(consumerId, denom)) - k.cdc.MustUnmarshal(b, &pool) - return -} - -// SetConsumerRewardsAllocationByDenom sets the consumer rewards allocation for the given consumer id and denom -func (k Keeper) SetConsumerRewardsAllocationByDenom(ctx sdk.Context, consumerId string, denom string, pool types.ConsumerRewardsAllocation) { - store := ctx.KVStore(k.storeKey) - b := k.cdc.MustMarshal(&pool) // why is this MUST MARSHALLL?? - store.Set(types.ConsumerRewardsAllocationByDenomKey(consumerId, denom), b) -} - -// DeleteConsumerRewardsAllocationByDenom deletes the consumer rewards allocation for the given consumer id and denom -func (k Keeper) DeleteConsumerRewardsAllocationByDenom(ctx sdk.Context, consumerId string, denom string) { - store := ctx.KVStore(k.storeKey) - store.Delete(types.ConsumerRewardsAllocationByDenomKey(consumerId, denom)) -} - -/// - // SetSlashLog updates validator's slash log for a consumer chain // If an entry exists for a given validator address, at least one // double signing slash packet was received by the provider from at least one consumer chain diff --git a/x/ccv/provider/keeper/keeper_test.go b/x/ccv/provider/keeper/keeper_test.go index 6b3e9ba8a3..60cc5110bd 100644 --- a/x/ccv/provider/keeper/keeper_test.go +++ b/x/ccv/provider/keeper/keeper_test.go @@ -372,3 +372,18 @@ func TestConsumerClientId(t *testing.T) { _, found = providerKeeper.GetClientIdToConsumerId(ctx, clientIds[1]) require.False(t, found) } + +func TestFoo(t *testing.T) { + providerKeeper, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) + defer ctrl.Finish() + + providerKeeper.SetConsumerClientId(ctx, "3", "a") + providerKeeper.SetConsumerClientId(ctx, "0000", "0") + providerKeeper.SetConsumerClientId(ctx, "100", "client-2") + providerKeeper.SetConsumerClientId(ctx, "0001", "client-1") + providerKeeper.SetConsumerClientId(ctx, "1", "client-4") + providerKeeper.SetConsumerClientId(ctx, "0", "client-3") + providerKeeper.SetConsumerClientId(ctx, "2", "client-5") + + fmt.Println(providerKeeper.GetAllConsumersWithIBCClients(ctx)) +} diff --git a/x/ccv/provider/keeper/msg_server.go b/x/ccv/provider/keeper/msg_server.go index 85c3ff52bc..301fa83c39 100644 --- a/x/ccv/provider/keeper/msg_server.go +++ b/x/ccv/provider/keeper/msg_server.go @@ -19,6 +19,8 @@ import ( ccvtypes "github.com/cosmos/interchain-security/v6/x/ccv/types" ) +const MaxAllowlistedRewardDenomsPerChain = 3 + type msgServer struct { *Keeper } @@ -415,14 +417,15 @@ func (k msgServer) CreateConsumer(goCtx context.Context, msg *types.MsgCreateCon } if msg.AllowlistedRewardDenoms != nil { - // if allowlisted denoms are provided, they overwrite the previously written ones - k.DeleteAllowlistedRewardDenom(ctx, consumerId) - for _, denom := range msg.AllowlistedRewardDenoms.Denoms { - k.SetAllowlistedRewardDenom(ctx, consumerId, denom) + if len(msg.AllowlistedRewardDenoms.Denoms) > MaxAllowlistedRewardDenomsPerChain { + return &resp, errorsmod.Wrapf(types.ErrInvalidAllowlistedRewardDenoms, + fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", MaxAllowlistedRewardDenomsPerChain)) } - if len(k.GetAllowlistedRewardDenoms(ctx, consumerId)) > 3 { - return &resp, errorsmod.Wrapf(types.ErrInvalidMsgCreateConsumer, "a consumer chain cannot allowlist more than 3 denom") + err := k.UpdateAllowlistedRewardDenoms(ctx, consumerId, msg.AllowlistedRewardDenoms.Denoms) + if err != nil { + return &resp, errorsmod.Wrapf(types.ErrInvalidAllowlistedRewardDenoms, + "cannot update allowlisted reward denoms: %s", err.Error()) } } @@ -602,14 +605,14 @@ func (k msgServer) UpdateConsumer(goCtx context.Context, msg *types.MsgUpdateCon } if msg.AllowlistedRewardDenoms != nil { - // if allowlisted denoms are provided, they overwrite the previously written ones - k.DeleteAllowlistedRewardDenom(ctx, consumerId) - for _, denom := range msg.AllowlistedRewardDenoms.Denoms { - k.SetAllowlistedRewardDenom(ctx, consumerId, denom) + if len(msg.AllowlistedRewardDenoms.Denoms) > MaxAllowlistedRewardDenomsPerChain { + return &resp, errorsmod.Wrapf(types.ErrInvalidAllowlistedRewardDenoms, + fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", MaxAllowlistedRewardDenomsPerChain)) } - if len(k.GetAllowlistedRewardDenoms(ctx, consumerId)) > 3 { - return &resp, errorsmod.Wrapf(types.ErrInvalidMsgCreateConsumer, "a consumer chain cannot allowlist more than 3 denom") + if err := k.UpdateAllowlistedRewardDenoms(ctx, consumerId, msg.AllowlistedRewardDenoms.Denoms); err != nil { + return &resp, errorsmod.Wrapf(types.ErrInvalidAllowlistedRewardDenoms, + "cannot update allowlisted reward denoms: %s", err.Error()) } } diff --git a/x/ccv/provider/types/errors.go b/x/ccv/provider/types/errors.go index dd2a6ea4c6..f15913d6f6 100644 --- a/x/ccv/provider/types/errors.go +++ b/x/ccv/provider/types/errors.go @@ -40,4 +40,5 @@ var ( ErrInvalidMsgOptOut = errorsmod.Register(ModuleName, 50, "invalid opt out message") ErrInvalidMsgSetConsumerCommissionRate = errorsmod.Register(ModuleName, 51, "invalid set consumer commission rate message") ErrInvalidMsgChangeRewardDenoms = errorsmod.Register(ModuleName, 52, "invalid change reward denoms message") + ErrInvalidAllowlistedRewardDenoms = errorsmod.Register(ModuleName, 53, "invalid allowlisted reward denoms") )