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..c2dfedec68 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/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) + //} } } 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..436ae08773 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 } @@ -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)) } } diff --git a/x/ccv/provider/types/keys_test.go b/x/ccv/provider/types/keys_test.go index f97f6d2115..38136142ec 100644 --- a/x/ccv/provider/types/keys_test.go +++ b/x/ccv/provider/types/keys_test.go @@ -1,6 +1,8 @@ package types_test import ( + "cosmossdk.io/math" + "fmt" "strings" "testing" "time" @@ -353,3 +355,26 @@ func TestKeysWithUint64Payload(t *testing.T) { } } } + +func TestFoo(t *testing.T) { + legacyDec, err := math.LegacyNewDecFromStr("1.23") + fmt.Println(err) + decCoin := sdk.DecCoin{Denom: "uatom", Amount: legacyDec} + decCoins := []sdk.DecCoin{decCoin} + rewards := providertypes.ConsumerRewardsAllocation{Rewards: decCoins} + fmt.Println(rewards) + + rewardsToSend, rewardsChange := rewards.Rewards.TruncateDecimal() + fmt.Println(rewardsToSend) + fmt.Println(rewardsChange) + + rewardsToSend, rewardsChange = rewardsChange.TruncateDecimal() + fmt.Println(rewardsToSend) + fmt.Println(rewardsChange) + + rewardsToSend, rewardsChange = rewardsChange.TruncateDecimal() + fmt.Println(rewardsToSend) + fmt.Println(rewardsChange) + + fmt.Println(rewardsChange.IsZero()) +}