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

@@ -2,6 +2,7 @@ package keeper
 
 import (
 	"context"
+	"fmt"
 	channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
 
 	errorsmod "cosmossdk.io/errors"
@@ -74,6 +75,7 @@ func (k Keeper) AllocateConsumerRewards(ctx sdk.Context, consumerId string, allo
 
 	chainId, err := k.GetConsumerChainId(ctx, consumerId)
 	if err != nil {
+		fmt.Println("mpika edo!!!")
 		k.Logger(ctx).Error(
 			"cannot get consumer chain id in AllocateConsumerRewards",
 			"consumerId", consumerId,
@@ -203,23 +205,24 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 	// To avoid large iterations over all the consumer IDs, iterate only over
 	// chains with an IBC client created.
 	for _, consumerId := range k.GetAllConsumersWithIBCClients(ctx) {
-		oldRewards := k.GetConsumerRewardsAllocation(ctx, consumerId)
-		returnedRewards, err := k.AllocateConsumerRewards(ctx, consumerId, oldRewards)
-		if err != nil {
-			k.Logger(ctx).Error(
-				"fail to allocate rewards for consumer chain",
-				"consumer id", consumerId,
-				"error", err.Error(),
-			)
-		} else {
-			k.SetConsumerRewardsAllocation(ctx, consumerId, returnedRewards)
-		}
+		//oldRewards := k.GetConsumerRewardsAllocation(ctx, consumerId)
+		//returnedRewards, err := k.AllocateConsumerRewards(ctx, consumerId, oldRewards)
+		//if err != nil {
+		//	k.Logger(ctx).Error(
+		//		"fail to allocate rewards for consumer chain",
+		//		"consumer id", consumerId,
+		//		"error", err.Error(),
+		//	)
+		//} else {
+		//	k.SetConsumerRewardsAllocation(ctx, consumerId, returnedRewards)
+		//}
 
 		allAllowlistedDenoms := append(k.GetAllConsumerRewardDenoms(ctx), k.GetAllowlistedRewardDenoms(ctx, consumerId)...)
 		for _, denom := range allAllowlistedDenoms {
 			cachedCtx, writeCache := ctx.CacheContext()
 			consumerRewards := k.GetConsumerRewardsAllocationByDenom(cachedCtx, consumerId, denom)
 			allocatedRewards, err := k.AllocateConsumerRewards(cachedCtx, consumerId, consumerRewards)
+
 			if err != nil {
 				k.Logger(ctx).Error(
 					"fail to allocate rewards for consumer chain",
@@ -228,21 +231,22 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
 				)
 				continue
 			}
+			//_ = allocatedRewards
 			k.SetConsumerRewardsAllocationByDenom(cachedCtx, consumerId, denom, allocatedRewards)
 			// TODO: fix for the tests
-			_ = writeCache
-			//writeCache()
+			//_ = 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)
-		}
+		//// 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)
+		//}
 	}
 }
 

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))
+}

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
 }
@@ -418,11 +420,15 @@ func (k msgServer) CreateConsumer(goCtx context.Context, msg *types.MsgCreateCon
 		// 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)
+			err := k.SetAllowlistedRewardDenom(ctx, consumerId, denom)
+			if err != nil {
+				//TODO FIX ABOVE AS WELL
+			}
 		}
 
-		if len(k.GetAllowlistedRewardDenoms(ctx, consumerId)) > 3 {
-			return &resp, errorsmod.Wrapf(types.ErrInvalidMsgCreateConsumer, "a consumer chain cannot allowlist more than 3 denom")
+		if len(k.GetAllowlistedRewardDenoms(ctx, consumerId)) > MaxAllowlistedRewardDenomsPerChain {
+			return &resp, errorsmod.Wrapf(types.ErrInvalidMsgCreateConsumer,
+				fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", MaxAllowlistedRewardDenomsPerChain))
 		}
 	}
 
@@ -605,11 +611,15 @@ func (k msgServer) UpdateConsumer(goCtx context.Context, msg *types.MsgUpdateCon
 		// 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)
+			err := k.SetAllowlistedRewardDenom(ctx, consumerId, denom)
+			if err != nil {
+				//TODO FIX ABOVE AS WELL
+			}
 		}
 
-		if len(k.GetAllowlistedRewardDenoms(ctx, consumerId)) > 3 {
-			return &resp, errorsmod.Wrapf(types.ErrInvalidMsgCreateConsumer, "a consumer chain cannot allowlist more than 3 denom")
+		if len(k.GetAllowlistedRewardDenoms(ctx, consumerId)) > MaxAllowlistedRewardDenomsPerChain {
+			return &resp, errorsmod.Wrapf(types.ErrInvalidMsgCreateConsumer,
+				fmt.Sprintf("a consumer chain cannot allowlist more than %d reward denoms", MaxAllowlistedRewardDenomsPerChain))
 		}
 	}