fixed integration tests

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

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,21 +656,22 @@ 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{})
 
 			// 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))))

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

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{}, err
+	}
+	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 {
+			// use 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()
 		}
 	}
 }