feat!: enable chains to allowlist reward denoms permissionlessly (#2309)

* move big chunk of AllocateTokens to new AllocateConsumerRewards method

* update protos to included allowlisted denoms

* added state and main functionality

* fixed integration tests

* add changelogs

* read the returned error

* took into account comments

* remove unused state

Author: insumity

.changelog/unreleased/features/2309-allow-permissionless-whitelisting.md

@@ -0,0 +1,2 @@
+- `[x/provider]` Enable permissionless allowlisting of reward denoms (at most 3) per consumer chain. 
+  ([\#2309](https://github.com/cosmos/interchain-security/pull/2309))

.changelog/unreleased/state-breaking/2309-allow-permissionless-whitelisting.md

@@ -0,0 +1,2 @@
+- `[x/provider]` Enable permissionless allowlisting of reward denoms (at most 3) per consumer chain.
+  ([\#2309](https://github.com/cosmos/interchain-security/pull/2309))

proto/interchain_security/ccv/provider/v1/provider.proto

@@ -515,3 +515,6 @@ enum ConsumerPhase {
   // DELETED defines the phase in which the state of a stopped chain has been deleted.
   CONSUMER_PHASE_DELETED = 5;
 }
+
+// AllowlistedRewardDenoms corresponds to the denoms allowlisted by a specific consumer id
+message AllowlistedRewardDenoms { repeated string denoms = 1; }

proto/interchain_security/ccv/provider/v1/tx.proto

@@ -360,6 +360,9 @@ message MsgCreateConsumer {
   ConsumerInitializationParameters initialization_parameters = 4;
 
   PowerShapingParameters power_shaping_parameters = 5;
+
+  // allowlisted reward denoms of the consumer
+  AllowlistedRewardDenoms allowlisted_reward_denoms = 7;
 }
 
 // MsgCreateConsumerResponse defines response type for MsgCreateConsumer
@@ -388,6 +391,9 @@ message MsgUpdateConsumer {
 
   // the power-shaping parameters of the consumer when updated
   PowerShapingParameters power_shaping_parameters = 6;
+
+  // allowlisted reward denoms of the consumer (if provided they overwrite previously set reward denoms)
+  AllowlistedRewardDenoms allowlisted_reward_denoms = 7;
 }
 
 // MsgUpdateConsumerResponse defines response type for MsgUpdateConsumer messages

tests/integration/distribution.go

@@ -107,21 +107,37 @@ 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, err := providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, rewardsIBCdenom)
 	s.Require().NoError(err)
+	remainingAlloc := rewardsAlloc.Rewards.AmountOf(rewardsIBCdenom)
+	s.Require().Equal(
+		math.LegacyNewDecFromInt(providerExpRewardsAmount),
+		rewardsAlloc.Rewards.AmountOf(rewardsIBCdenom),
+	)
+	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, err = providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, rewardsIBCdenom)
+	s.Require().NoError(err)
+	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 +153,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 +247,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 +558,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 +588,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,22 +658,25 @@ 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, err := providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, ibcDenom)
+			s.Require().NoError(err)
 
 			// 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).
-				Rewards.Sub(consumerRewardsAllocations.Rewards)
+			rewardsAllocatedByDenom, err := providerKeeper.GetConsumerRewardsAllocationByDenom(s.providerCtx(), s.getFirstBundle().ConsumerId, ibcDenom)
+			s.Require().NoError(err)
+			rewardsAllocated := rewardsAllocatedByDenom.Rewards.Sub(consumerRewardsAllocations.Rewards)
 
 			if !tc.expErr {
 				s.Require().True(ack.Success())
@@ -738,13 +730,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 +795,9 @@ 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
+	allocRemainderPerChainRes, err := providerKeeper.GetConsumerRewardsAllocationByDenom(providerCtx, s.getFirstBundle().ConsumerId, ibcDenom)
+	s.Require().NoError(err)
+	allocRemainderPerChain := allocRemainderPerChainRes.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

@@ -192,30 +192,48 @@ func (im IBCMiddleware) OnRecvPacket(
 			sdk.NewAttribute(types.AttributeRewardAmount, data.Amount),
 		}...)
 
-		// verify that the coin's denom is a whitelisted consumer denom,
-		// and if so, adds it to the consumer chain rewards allocation,
-		// otherwise the prohibited coin just stays in the pool forever.
-		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)
-
-			logger.Info(
-				"scheduled ICS rewards to be distributed",
+		alloc, err := im.keeper.GetConsumerRewardsAllocationByDenom(ctx, consumerId, coinDenom)
+		if err != nil {
+			logger.Error(
+				"cannot get consumer rewards by denom",
 				"consumerId", consumerId,
-				"chainId", chainId,
+				"packet", packet.String(),
+				"fungibleTokenPacketData", data.String(),
 				"denom", coinDenom,
-				"amount", data.Amount,
+				"error", err.Error(),
 			)
+			return ack
+		}
 
-			// add RewardDistribution event attribute
-			eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeRewardDistribution, "scheduled"))
+		alloc.Rewards = alloc.Rewards.Add(
+			sdk.NewDecCoinFromCoin(sdk.Coin{
+				Denom:  coinDenom,
+				Amount: coinAmt,
+			}))
+		err = im.keeper.SetConsumerRewardsAllocationByDenom(ctx, consumerId, coinDenom, alloc)
+		if err != nil {
+			logger.Error(
+				"cannot set consumer rewards by denom",
+				"consumerId", consumerId,
+				"packet", packet.String(),
+				"fungibleTokenPacketData", data.String(),
+				"denom", coinDenom,
+				"error", err.Error(),
+			)
+			return ack
 		}
 
+		logger.Info(
+			"scheduled ICS rewards to be distributed",
+			"consumerId", consumerId,
+			"chainId", chainId,
+			"denom", coinDenom,
+			"amount", data.Amount,
+		)
+
+		// add RewardDistribution event attribute
+		eventAttributes = append(eventAttributes, sdk.NewAttribute(types.AttributeRewardDistribution, "scheduled"))
+
 		ctx.EventManager().EmitEvent(
 			sdk.NewEvent(
 				types.EventTypeUpdateConsumer,

x/ccv/provider/keeper/consumer_lifecycle.go

@@ -487,7 +487,6 @@ func (k Keeper) DeleteConsumerChain(ctx sdk.Context, consumerId string) (err err
 	k.DeleteAllOptedIn(ctx, consumerId)
 	k.DeleteConsumerValSet(ctx, consumerId)
 
-	k.DeleteConsumerRewardsAllocation(ctx, consumerId)
 	k.DeleteConsumerRemovalTime(ctx, consumerId)
 
 	// TODO (PERMISSIONLESS) add newly-added state to be deleted