From 51a70bb2d09721e55e2bae55b32078e3938b50eb Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Thu, 14 Mar 2024 10:15:51 +0100 Subject: [PATCH 1/8] update compute consumer total power for reward distribution --- tests/integration/distribution.go | 5 ++- x/ccv/provider/keeper/distribution.go | 25 +++++---------- x/ccv/provider/keeper/distribution_test.go | 37 +++++++++------------- 3 files changed, 27 insertions(+), 40 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 9f85b29f28..06cad0a855 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -906,7 +906,7 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() { distributionKeeper := s.providerApp.GetTestDistributionKeeper() bankKeeper := s.providerApp.GetTestBankKeeper() - chainID := "consumer" + chainID := s.consumerChain.ChainID validators := []bytes.HexBytes{ s.providerChain.Vals.Validators[0].Address, s.providerChain.Vals.Validators[1].Address, @@ -965,6 +965,9 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() { ) } + consuVals := providerKeeper.GetConsumerValSet(s.providerCtx(), chainID) + _ = consuVals + // TODO: opt validators in and verify // that rewards are only allocated to them ctx, _ := s.providerCtx().CacheContext() diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index aa7195cf4b..4ba8ab5452 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -104,7 +104,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded // temporary workaround to keep CanWithdrawInvariant happy // general discussions here: https://github.com/cosmos/cosmos-sdk/issues/2906#issuecomment-441867634 feePool := k.distributionKeeper.GetFeePool(ctx) - if k.ComputeConsumerTotalVotingPower(ctx, consumer.ChainId, bondedVotes) == 0 { + if k.ComputeConsumerTotalVotingPower(ctx, consumer.ChainId) == 0 { feePool.CommunityPool = feePool.CommunityPool.Add(rewardsCollectedDec...) k.distributionKeeper.SetFeePool(ctx, feePool) return @@ -131,9 +131,6 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded } } -// TODO: allocate tokens to validators that opted-in and for long enough e.g. 1000 blocks -// once the opt-in logic is integrated QueueVSCPackets() -// // AllocateTokensToConsumerValidators allocates the given tokens from the // from consumer rewards pool to validator according to their voting power func (k Keeper) AllocateTokensToConsumerValidators( @@ -148,7 +145,7 @@ func (k Keeper) AllocateTokensToConsumerValidators( } // get the consumer total voting power from the votes - totalPower := k.ComputeConsumerTotalVotingPower(ctx, chainID, bondedVotes) + totalPower := k.ComputeConsumerTotalVotingPower(ctx, chainID) if totalPower == 0 { return allocated } @@ -242,21 +239,15 @@ func (k Keeper) GetConsumerRewardsPool(ctx sdk.Context) sdk.Coins { ) } -// ComputeConsumerTotalVotingPower returns the total voting power for a given consumer chain -// by summing its opted-in validators votes -func (k Keeper) ComputeConsumerTotalVotingPower(ctx sdk.Context, chainID string, votes []abci.VoteInfo) int64 { - // TODO: create a optedIn set from the OptedIn validators - // and sum their validator power - var totalPower int64 - +// ComputeConsumerTotalVotingPower returns the total voting power of the given consumer chain +// validator set +func (k Keeper) ComputeConsumerTotalVotingPower(ctx sdk.Context, chainID string) (totalPower int64) { // sum the opted-in validators set voting powers - for _, vote := range votes { - // TODO: check that val is in the optedIn set - - totalPower += vote.Validator.Power + for _, v := range k.GetConsumerValSet(ctx, chainID) { + totalPower += v.Power } - return totalPower + return } // IdentifyConsumerChainIDFromIBCPacket checks if the packet destination matches a registered consumer chain. diff --git a/x/ccv/provider/keeper/distribution_test.go b/x/ccv/provider/keeper/distribution_test.go index d12b2c910a..68e5eab829 100644 --- a/x/ccv/provider/keeper/distribution_test.go +++ b/x/ccv/provider/keeper/distribution_test.go @@ -12,7 +12,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" - abci "github.com/cometbft/cometbft/abci/types" tmtypes "github.com/cometbft/cometbft/types" testkeeper "github.com/cosmos/interchain-security/v4/testutil/keeper" @@ -30,44 +29,38 @@ func TestComputeConsumerTotalVotingPower(t *testing.T) { } chainID := "consumer" - validatorsVotes := make([]abci.VoteInfo, 5) - expTotalPower := int64(0) - // create validators, opt them in and use them - // to create block votes + // verify that the total power returned is equal to zero + // when the consumer doesn't exist or has no validators. + require.Zero(t, keeper.ComputeConsumerTotalVotingPower( + ctx, + chainID, + )) + + // create validators and block votes for i := 0; i < 5; i++ { val := createVal(int64(i)) - keeper.SetOptedIn( + keeper.SetConsumerValidator( ctx, chainID, - types.OptedInValidator{ - ProviderAddr: val.Address, - BlockHeight: ctx.BlockHeight(), - Power: val.VotingPower, - PublicKey: val.PubKey.Bytes(), - }, - ) - - validatorsVotes = append( - validatorsVotes, - abci.VoteInfo{ - Validator: abci.Validator{ - Address: val.Address, - Power: val.VotingPower, - }, + types.ConsumerValidator{ + ProviderConsAddr: val.Address, + Power: val.VotingPower, }, ) expTotalPower += val.VotingPower } + // compute the total power of opted-in validators res := keeper.ComputeConsumerTotalVotingPower( ctx, chainID, - validatorsVotes, ) + // check that the total power returned corresponds + // to the total power of opted-in validators only require.Equal(t, expTotalPower, res) } From 76bcf98783278a9747c17d996c32a301483c04f6 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Thu, 14 Mar 2024 14:43:13 +0100 Subject: [PATCH 2/8] update distribution logic to work with epochcs --- tests/integration/distribution.go | 50 +++++++++------------- x/ccv/provider/keeper/distribution.go | 8 ++-- x/ccv/provider/keeper/distribution_test.go | 5 +-- 3 files changed, 25 insertions(+), 38 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 06cad0a855..30c07e278d 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -14,7 +14,6 @@ import ( distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types" abci "github.com/cometbft/cometbft/abci/types" - "github.com/cometbft/cometbft/libs/bytes" icstestingutils "github.com/cosmos/interchain-security/v4/testutil/integration" consumerkeeper "github.com/cosmos/interchain-security/v4/x/ccv/consumer/keeper" @@ -907,18 +906,10 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() { bankKeeper := s.providerApp.GetTestBankKeeper() chainID := s.consumerChain.ChainID - validators := []bytes.HexBytes{ - s.providerChain.Vals.Validators[0].Address, - s.providerChain.Vals.Validators[1].Address, - } - votes := []abci.VoteInfo{ - {Validator: abci.Validator{Address: validators[0], Power: 1}}, - {Validator: abci.Validator{Address: validators[1], Power: 1}}, - } testCases := []struct { name string - votes []abci.VoteInfo + consuValLen int tokens sdk.DecCoins rate sdk.Dec expAllocated sdk.DecCoins @@ -930,21 +921,21 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() { expAllocated: nil, }, { - name: "total voting power is zero", + name: "consumer valset is empty - total voting power is zero", tokens: sdk.DecCoins{sdk.NewDecCoin(sdk.DefaultBondDenom, math.NewInt(100_000))}, rate: sdk.ZeroDec(), expAllocated: nil, }, { name: "expect all tokens to be allocated to a single validator", - votes: []abci.VoteInfo{votes[0]}, + consuValLen: 1, tokens: sdk.DecCoins{sdk.NewDecCoin(sdk.DefaultBondDenom, math.NewInt(999))}, rate: sdk.NewDecWithPrec(5, 1), expAllocated: sdk.DecCoins{sdk.NewDecCoin(sdk.DefaultBondDenom, math.NewInt(999))}, }, { name: "expect tokens to be allocated evenly between validators", - votes: []abci.VoteInfo{votes[0], votes[1]}, + consuValLen: 2, tokens: sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, math.LegacyNewDecFromIntWithPrec(math.NewInt(999), 2))}, rate: sdk.OneDec(), expAllocated: sdk.DecCoins{sdk.NewDecCoinFromDec(sdk.DefaultBondDenom, math.LegacyNewDecFromIntWithPrec(math.NewInt(999), 2))}, @@ -953,30 +944,29 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() { for _, tc := range testCases { s.Run(tc.name, func() { - // set the same consumer commission rate for all validators - for _, v := range s.providerChain.Vals.Validators { - provAddr := providertypes.NewProviderConsAddress(sdk.ConsAddress(v.Address)) + ctx, _ := s.providerCtx().CacheContext() + // change the consumer valset + consuVals := providerKeeper.GetConsumerValSet(ctx, chainID) + providerKeeper.DeleteConsumerValSet(ctx, chainID) + providerKeeper.SetConsumerValSet(ctx, chainID, consuVals[0:tc.consuValLen]) + consuVals = providerKeeper.GetConsumerValSet(ctx, chainID) + + // set the same consumer commission rate for all consumer validators + for _, v := range consuVals { + provAddr := providertypes.NewProviderConsAddress(sdk.ConsAddress(v.ProviderConsAddr)) providerKeeper.SetConsumerCommissionRate( - s.providerCtx(), + ctx, chainID, provAddr, tc.rate, ) } - consuVals := providerKeeper.GetConsumerValSet(s.providerCtx(), chainID) - _ = consuVals - - // TODO: opt validators in and verify - // that rewards are only allocated to them - ctx, _ := s.providerCtx().CacheContext() - // allocate tokens res := providerKeeper.AllocateTokensToConsumerValidators( ctx, chainID, - tc.votes, tc.tokens, ) @@ -985,11 +975,11 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() { if !tc.expAllocated.Empty() { // rewards are expected to be allocated evenly between validators - rewardsPerVal := tc.expAllocated.QuoDec(sdk.NewDec(int64(len(tc.votes)))) + rewardsPerVal := tc.expAllocated.QuoDec(sdk.NewDec(int64(len(consuVals)))) // check that the rewards are allocated to validators - for _, v := range tc.votes { - valAddr := sdk.ValAddress(v.Validator.Address) + for _, v := range consuVals { + valAddr := sdk.ValAddress(v.ProviderConsAddr) rewards := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards( ctx, valAddr, @@ -1022,8 +1012,8 @@ func (s *CCVTestSuite) TestAllocateTokensToValidator() { s.Require().Equal(withdrawnCoins, bankKeeper.GetAllBalances(ctx, sdk.AccAddress(valAddr))) } } else { - for _, v := range tc.votes { - valAddr := sdk.ValAddress(v.Validator.Address) + for _, v := range consuVals { + valAddr := sdk.ValAddress(v.ProviderConsAddr) rewards := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards( ctx, valAddr, diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 4ba8ab5452..7972b82a38 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -120,7 +120,6 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded feeAllocated := k.AllocateTokensToConsumerValidators( ctx, consumer.ChainId, - bondedVotes, feeMultiplier, ) remaining = remaining.Sub(feeAllocated) @@ -136,7 +135,6 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded func (k Keeper) AllocateTokensToConsumerValidators( ctx sdk.Context, chainID string, - bondedVotes []abci.VoteInfo, tokens sdk.DecCoins, ) (allocated sdk.DecCoins) { // return early if the tokens are empty @@ -150,11 +148,11 @@ func (k Keeper) AllocateTokensToConsumerValidators( return allocated } - for _, vote := range bondedVotes { + for _, val := range k.GetConsumerValSet(ctx, chainID) { // TODO: should check if validator IsOptIn or continue here - consAddr := sdk.ConsAddress(vote.Validator.Address) + consAddr := sdk.ConsAddress(val.ProviderConsAddr) - powerFraction := math.LegacyNewDec(vote.Validator.Power).QuoTruncate(math.LegacyNewDec(totalPower)) + powerFraction := math.LegacyNewDec(val.Power).QuoTruncate(math.LegacyNewDec(totalPower)) tokensFraction := tokens.MulDecTruncate(powerFraction) // get the validator type struct for the consensus address diff --git a/x/ccv/provider/keeper/distribution_test.go b/x/ccv/provider/keeper/distribution_test.go index 68e5eab829..1e37b1112e 100644 --- a/x/ccv/provider/keeper/distribution_test.go +++ b/x/ccv/provider/keeper/distribution_test.go @@ -38,7 +38,7 @@ func TestComputeConsumerTotalVotingPower(t *testing.T) { chainID, )) - // create validators and block votes + // set 5 validators to the consumer chain for i := 0; i < 5; i++ { val := createVal(int64(i)) keeper.SetConsumerValidator( @@ -59,8 +59,7 @@ func TestComputeConsumerTotalVotingPower(t *testing.T) { chainID, ) - // check that the total power returned corresponds - // to the total power of opted-in validators only + // check the total power returned require.Equal(t, expTotalPower, res) } From ab0906ab25388940e7cf2c6ae0eaf774eef9f330 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 15 Mar 2024 09:36:56 +0100 Subject: [PATCH 3/8] Adapt reward distribution mem test to epochs --- tests/integration/distribution.go | 67 +++++++++++++++++++++------ x/ccv/provider/keeper/distribution.go | 14 +++--- 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 30c07e278d..89fb01c410 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -78,12 +78,18 @@ func (s *CCVTestSuite) TestRewardsDistribution() { s.Require().Equal(providerExpectedRewards.AmountOf(sdk.DefaultBondDenom), providerTokens.AmountOf(sdk.DefaultBondDenom)) // send the reward to provider chain after 2 blocks - s.consumerChain.NextBlock() providerTokens = consumerBankKeeper.GetAllBalances(s.consumerCtx(), providerRedistributeAddr) s.Require().Equal(0, len(providerTokens)) - relayAllCommittedPackets(s, s.consumerChain, s.transferPath, transfertypes.PortID, s.transferPath.EndpointA.ChannelID, 1) + relayAllCommittedPackets( + s, + s.consumerChain, + s.transferPath, + transfertypes.PortID, + s.transferPath.EndpointA.ChannelID, + 1, + ) s.providerChain.NextBlock() // Since consumer reward denom is not yet registered, the coins never get into the fee pool, staying in the ConsumerRewardsPool @@ -101,41 +107,72 @@ func (s *CCVTestSuite) TestRewardsDistribution() { s.Require().Greater(ibcCoinIndex, -1) // Check that the coins got into the ConsumerRewardsPool - s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, (providerExpectedRewards[0].Amount)) + providerExpRewardsAmount := providerExpectedRewards.AmountOf(sdk.DefaultBondDenom) + s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, (providerExpRewardsAmount)) // Advance a block and check that the coins are still in the ConsumerRewardsPool s.providerChain.NextBlock() rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, (providerExpectedRewards[0].Amount)) + s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, (providerExpRewardsAmount)) // Set the consumer reward denom. This would be done by a governance proposal in prod s.providerApp.GetProviderKeeper().SetConsumerRewardDenom(s.providerCtx(), rewardCoins[ibcCoinIndex].Denom) // Refill the consumer fee pool - err = consumerBankKeeper.SendCoinsFromAccountToModule(s.consumerCtx(), s.consumerChain.SenderAccount.GetAddress(), authtypes.FeeCollectorName, fees) + err = consumerBankKeeper.SendCoinsFromAccountToModule( + s.consumerCtx(), + s.consumerChain.SenderAccount.GetAddress(), + authtypes.FeeCollectorName, + fees, + ) s.Require().NoError(err) // pass two blocks s.consumerChain.NextBlock() s.consumerChain.NextBlock() + // save the consumer validators total outstanding rewards on the provider, + // before transferring the rewards from the consumer + consumerValsOutstandingRewardsFunc := func(ctx sdk.Context) sdk.DecCoins { + totalRewards := sdk.DecCoins{} + for _, v := range s.providerApp.GetProviderKeeper().GetConsumerValSet(ctx, s.consumerChain.ChainID) { + val, ok := s.providerApp.GetTestStakingKeeper().GetValidatorByConsAddr(ctx, sdk.ConsAddress(v.ProviderConsAddr)) + s.Require().True(ok) + valReward := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards(ctx, val.GetOperator()) + totalRewards = totalRewards.Add(valReward.Rewards...) + } + return totalRewards + } + consuValsRewards := consumerValsOutstandingRewardsFunc(s.providerCtx()) + + // save community pool balance + communityPool := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()) + // transfer rewards from consumer to provider - relayAllCommittedPackets(s, s.consumerChain, s.transferPath, transfertypes.PortID, s.transferPath.EndpointA.ChannelID, 1) + relayAllCommittedPackets( + s, + s.consumerChain, + s.transferPath, + transfertypes.PortID, + s.transferPath.EndpointA.ChannelID, + 1, + ) // check that the consumer rewards allocation are empty since relayAllCommittedPackets call BeginBlock rewardsAlloc := s.providerApp.GetProviderKeeper().GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID) s.Require().Empty(rewardsAlloc.Rewards) - // Check that the reward pool still has the first coins transferred that were never allocated + // Check that the reward pool still holds the first coins transferred that were never allocated rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, (providerExpectedRewards[0].Amount)) - - // check that the fee pool has the expected amount of coins - // Note that all rewards are allocated to the community pool since - // BeginBlock is called without the validators' votes in ibctesting. - // See NextBlock() in https://github.com/cosmos/ibc-go/blob/release/v7.3.x/testing/chain.go#L281 - communityCoins := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()) - s.Require().Equal(communityCoins[ibcCoinIndex].Amount, (sdk.NewDecCoinFromCoin(providerExpectedRewards[0]).Amount)) + s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, providerExpRewardsAmount) + + // check that the rewards are transferred to both the consumer validators and the community pool + consuValsRewardsReceived := consumerValsOutstandingRewardsFunc(s.providerCtx()).Sub(consuValsRewards) + s.Require().NotEmpty(consuValsRewardsReceived) + commPoolDelta := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()).Sub(communityPool) + s.Require().NotEmpty(commPoolDelta) + + s.Require().Equal(sdk.NewDecFromInt(providerExpRewardsAmount), consuValsRewardsReceived[0].Amount.Add(commPoolDelta[0].Amount)) } // TestSendRewardsRetries tests that failed reward transmissions are retried every BlocksPerDistributionTransmission blocks diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 7972b82a38..21f63d3f17 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -17,16 +17,11 @@ import ( // BeginBlockRD executes BeginBlock logic for the Reward Distribution sub-protocol. func (k Keeper) BeginBlockRD(ctx sdk.Context, req abci.RequestBeginBlock) { - // determine the total power signing the block - var previousTotalPower int64 - for _, voteInfo := range req.LastCommitInfo.GetVotes() { - previousTotalPower += voteInfo.Validator.Power - } // TODO this is Tendermint-dependent // ref https://github.com/cosmos/cosmos-sdk/issues/3095 if ctx.BlockHeight() > 1 { - k.AllocateTokens(ctx, previousTotalPower, req.LastCommitInfo.GetVotes()) + k.AllocateTokens(ctx) } } @@ -75,7 +70,7 @@ func (k Keeper) GetAllConsumerRewardDenoms(ctx sdk.Context) (consumerRewardDenom // AllocateTokens performs rewards distribution to the community pool and validators // based on the Partial Set Security distribution specification. -func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bondedVotes []abci.VoteInfo) { +func (k Keeper) AllocateTokens(ctx sdk.Context) { // return if there is no coins in the consumer rewards pool if k.GetConsumerRewardsPool(ctx).IsZero() { return @@ -95,6 +90,9 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded continue } + // 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. if rewardsCollected.IsZero() { continue } @@ -110,7 +108,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded return } - // Calculate the reward allocations + // calculate the reward allocations remaining := rewardsCollectedDec communityTax := k.distributionKeeper.GetCommunityTax(ctx) voteMultiplier := math.LegacyOneDec().Sub(communityTax) From dcb569c089ec11d997ffd321a531d9cd43ec5c56 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 15 Mar 2024 09:54:49 +0100 Subject: [PATCH 4/8] doc --- tests/integration/distribution.go | 56 +++++++++++++++---------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 89fb01c410..68d2702dad 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -50,6 +50,7 @@ func (s *CCVTestSuite) TestRewardsDistribution() { providerAccountKeeper := s.providerApp.GetTestAccountKeeper() consumerBankKeeper := s.consumerApp.GetTestBankKeeper() providerBankKeeper := s.providerApp.GetTestBankKeeper() + providerKeeper := s.providerApp.GetProviderKeeper() // send coins to the fee pool which is used for reward distribution consumerFeePoolAddr := consumerAccountKeeper.GetModuleAccount(s.consumerCtx(), authtypes.FeeCollectorName).GetAddress() @@ -82,41 +83,35 @@ func (s *CCVTestSuite) TestRewardsDistribution() { providerTokens = consumerBankKeeper.GetAllBalances(s.consumerCtx(), providerRedistributeAddr) s.Require().Equal(0, len(providerTokens)) - relayAllCommittedPackets( - s, - s.consumerChain, - s.transferPath, - transfertypes.PortID, - s.transferPath.EndpointA.ChannelID, - 1, - ) + relayAllCommittedPackets(s, s.consumerChain, s.transferPath, transfertypes.PortID, s.transferPath.EndpointA.ChannelID, 1) s.providerChain.NextBlock() // Since consumer reward denom is not yet registered, the coins never get into the fee pool, staying in the ConsumerRewardsPool rewardPool := providerAccountKeeper.GetModuleAccount(s.providerCtx(), providertypes.ConsumerRewardsPool).GetAddress() rewardCoins := providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - ibcCoinIndex := -1 - for i, coin := range rewardCoins { + // Find the reward denom with an IBC prefix + rewardsIBCdenom := "" + for _, coin := range rewardCoins { if strings.HasPrefix(coin.Denom, "ibc") { - ibcCoinIndex = i + rewardsIBCdenom = coin.Denom } } // Check that we found an ibc denom in the reward pool - s.Require().Greater(ibcCoinIndex, -1) + s.Require().NotZero(rewardsIBCdenom) // Check that the coins got into the ConsumerRewardsPool providerExpRewardsAmount := providerExpectedRewards.AmountOf(sdk.DefaultBondDenom) - s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, (providerExpRewardsAmount)) + s.Require().Equal(rewardCoins.AmountOf(rewardsIBCdenom), providerExpRewardsAmount) // Advance a block and check that the coins are still in the ConsumerRewardsPool s.providerChain.NextBlock() rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, (providerExpRewardsAmount)) + s.Require().Equal(rewardCoins.AmountOf(rewardsIBCdenom), providerExpRewardsAmount) // Set the consumer reward denom. This would be done by a governance proposal in prod - s.providerApp.GetProviderKeeper().SetConsumerRewardDenom(s.providerCtx(), rewardCoins[ibcCoinIndex].Denom) + providerKeeper.SetConsumerRewardDenom(s.providerCtx(), rewardsIBCdenom) // Refill the consumer fee pool err = consumerBankKeeper.SendCoinsFromAccountToModule( @@ -127,15 +122,15 @@ func (s *CCVTestSuite) TestRewardsDistribution() { ) s.Require().NoError(err) - // pass two blocks + // Pass two blocks s.consumerChain.NextBlock() s.consumerChain.NextBlock() - // save the consumer validators total outstanding rewards on the provider, + // Save the consumer validators total outstanding rewards on the provider, // before transferring the rewards from the consumer consumerValsOutstandingRewardsFunc := func(ctx sdk.Context) sdk.DecCoins { totalRewards := sdk.DecCoins{} - for _, v := range s.providerApp.GetProviderKeeper().GetConsumerValSet(ctx, s.consumerChain.ChainID) { + for _, v := range providerKeeper.GetConsumerValSet(ctx, s.consumerChain.ChainID) { val, ok := s.providerApp.GetTestStakingKeeper().GetValidatorByConsAddr(ctx, sdk.ConsAddress(v.ProviderConsAddr)) s.Require().True(ok) valReward := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards(ctx, val.GetOperator()) @@ -145,10 +140,10 @@ func (s *CCVTestSuite) TestRewardsDistribution() { } consuValsRewards := consumerValsOutstandingRewardsFunc(s.providerCtx()) - // save community pool balance + // Save community pool balance communityPool := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()) - // transfer rewards from consumer to provider + // Transfer rewards from consumer to provider relayAllCommittedPackets( s, s.consumerChain, @@ -158,21 +153,24 @@ func (s *CCVTestSuite) TestRewardsDistribution() { 1, ) - // check that the consumer rewards allocation are empty since relayAllCommittedPackets call BeginBlock - rewardsAlloc := s.providerApp.GetProviderKeeper().GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID) + // Check that the consumer rewards allocation are empty since relayAllCommittedPackets call BeginBlock + rewardsAlloc := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID) s.Require().Empty(rewardsAlloc.Rewards) - // Check that the reward pool still holds the first coins transferred that were never allocated + // Check that the reward pool still holds the coins from the first transfer, + // which were never allocated since they were not whitelisted rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - s.Require().Equal(rewardCoins[ibcCoinIndex].Amount, providerExpRewardsAmount) + s.Require().Equal(rewardCoins.AmountOf(rewardsIBCdenom), providerExpRewardsAmount) - // check that the rewards are transferred to both the consumer validators and the community pool + // Check that summing the rewards received by the consumer validators and the community pool + // is equal to the expected provider rewards consuValsRewardsReceived := consumerValsOutstandingRewardsFunc(s.providerCtx()).Sub(consuValsRewards) - s.Require().NotEmpty(consuValsRewardsReceived) - commPoolDelta := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()).Sub(communityPool) - s.Require().NotEmpty(commPoolDelta) + communityPoolDelta := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()).Sub(communityPool) - s.Require().Equal(sdk.NewDecFromInt(providerExpRewardsAmount), consuValsRewardsReceived[0].Amount.Add(commPoolDelta[0].Amount)) + s.Require().Equal( + sdk.NewDecFromInt(providerExpRewardsAmount), + consuValsRewardsReceived.AmountOf(rewardsIBCdenom).Add(communityPoolDelta.AmountOf(rewardsIBCdenom)), + ) } // TestSendRewardsRetries tests that failed reward transmissions are retried every BlocksPerDistributionTransmission blocks From 4d51ae6e0a839c374399b83f0c0bb1d0f56e3cf1 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 15 Mar 2024 10:11:10 +0100 Subject: [PATCH 5/8] nits --- tests/integration/distribution.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 68d2702dad..bf7e5dced8 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -51,6 +51,7 @@ func (s *CCVTestSuite) TestRewardsDistribution() { consumerBankKeeper := s.consumerApp.GetTestBankKeeper() providerBankKeeper := s.providerApp.GetTestBankKeeper() providerKeeper := s.providerApp.GetProviderKeeper() + providerDistributionKeeper := s.providerApp.GetTestDistributionKeeper() // send coins to the fee pool which is used for reward distribution consumerFeePoolAddr := consumerAccountKeeper.GetModuleAccount(s.consumerCtx(), authtypes.FeeCollectorName).GetAddress() @@ -90,15 +91,13 @@ func (s *CCVTestSuite) TestRewardsDistribution() { rewardPool := providerAccountKeeper.GetModuleAccount(s.providerCtx(), providertypes.ConsumerRewardsPool).GetAddress() rewardCoins := providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - // Find the reward denom with an IBC prefix + // Check that the reward pool contains a coin with a IBC denom rewardsIBCdenom := "" for _, coin := range rewardCoins { if strings.HasPrefix(coin.Denom, "ibc") { rewardsIBCdenom = coin.Denom } } - - // Check that we found an ibc denom in the reward pool s.Require().NotZero(rewardsIBCdenom) // Check that the coins got into the ConsumerRewardsPool @@ -110,7 +109,7 @@ 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 + // Set the consumer reward denom. This would be done by a governance proposal in prod. providerKeeper.SetConsumerRewardDenom(s.providerCtx(), rewardsIBCdenom) // Refill the consumer fee pool @@ -126,14 +125,13 @@ func (s *CCVTestSuite) TestRewardsDistribution() { s.consumerChain.NextBlock() s.consumerChain.NextBlock() - // Save the consumer validators total outstanding rewards on the provider, - // before transferring the rewards from the consumer + // Save the consumer validators total outstanding rewards on the provider consumerValsOutstandingRewardsFunc := func(ctx sdk.Context) sdk.DecCoins { totalRewards := sdk.DecCoins{} for _, v := range providerKeeper.GetConsumerValSet(ctx, s.consumerChain.ChainID) { val, ok := s.providerApp.GetTestStakingKeeper().GetValidatorByConsAddr(ctx, sdk.ConsAddress(v.ProviderConsAddr)) s.Require().True(ok) - valReward := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards(ctx, val.GetOperator()) + valReward := providerDistributionKeeper.GetValidatorOutstandingRewards(ctx, val.GetOperator()) totalRewards = totalRewards.Add(valReward.Rewards...) } return totalRewards @@ -141,7 +139,7 @@ func (s *CCVTestSuite) TestRewardsDistribution() { consuValsRewards := consumerValsOutstandingRewardsFunc(s.providerCtx()) // Save community pool balance - communityPool := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()) + communityPool := providerDistributionKeeper.GetFeePoolCommunityCoins(s.providerCtx()) // Transfer rewards from consumer to provider relayAllCommittedPackets( @@ -165,7 +163,7 @@ func (s *CCVTestSuite) TestRewardsDistribution() { // Check that summing the rewards received by the consumer validators and the community pool // is equal to the expected provider rewards consuValsRewardsReceived := consumerValsOutstandingRewardsFunc(s.providerCtx()).Sub(consuValsRewards) - communityPoolDelta := s.providerApp.GetTestDistributionKeeper().GetFeePoolCommunityCoins(s.providerCtx()).Sub(communityPool) + communityPoolDelta := providerDistributionKeeper.GetFeePoolCommunityCoins(s.providerCtx()).Sub(communityPool) s.Require().Equal( sdk.NewDecFromInt(providerExpRewardsAmount), From 3252dd71e377fb44bd5ccbb82f56cc9155c0e39a Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 15 Mar 2024 10:46:31 +0100 Subject: [PATCH 6/8] other nits --- tests/integration/distribution.go | 2 +- x/ccv/provider/keeper/distribution.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index bf7e5dced8..347151f59c 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -151,7 +151,7 @@ func (s *CCVTestSuite) TestRewardsDistribution() { 1, ) - // Check that the consumer rewards allocation are empty since relayAllCommittedPackets call BeginBlock + // Check that the consumer rewards allocation are empty since relayAllCommittedPackets call BeginBlockRD rewardsAlloc := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID) s.Require().Empty(rewardsAlloc.Rewards) diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 21f63d3f17..55ea9fd30e 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -147,7 +147,6 @@ func (k Keeper) AllocateTokensToConsumerValidators( } for _, val := range k.GetConsumerValSet(ctx, chainID) { - // TODO: should check if validator IsOptIn or continue here consAddr := sdk.ConsAddress(val.ProviderConsAddr) powerFraction := math.LegacyNewDec(val.Power).QuoTruncate(math.LegacyNewDec(totalPower)) From d06c17ccf4554c59e2f67960f43ebdae421d5878 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Mon, 18 Mar 2024 12:07:58 +0100 Subject: [PATCH 7/8] nits --- tests/integration/distribution.go | 5 +++-- x/ccv/provider/keeper/distribution.go | 18 ++++++++++-------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 347151f59c..331b1a397b 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -91,7 +91,7 @@ func (s *CCVTestSuite) TestRewardsDistribution() { rewardPool := providerAccountKeeper.GetModuleAccount(s.providerCtx(), providertypes.ConsumerRewardsPool).GetAddress() rewardCoins := providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - // Check that the reward pool contains a coin with a IBC denom + // Check that the reward pool contains a coin with an IBC denom rewardsIBCdenom := "" for _, coin := range rewardCoins { if strings.HasPrefix(coin.Denom, "ibc") { @@ -151,7 +151,8 @@ func (s *CCVTestSuite) TestRewardsDistribution() { 1, ) - // Check that the consumer rewards allocation are empty since relayAllCommittedPackets call BeginBlockRD + // Check that the consumer rewards allocation are empty since relayAllCommittedPackets calls BeginBlockRD, + // which in turns call AllocateTokens. rewardsAlloc := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID) s.Require().Empty(rewardsAlloc.Rewards) diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 55ea9fd30e..cd7755ac3e 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -128,8 +128,8 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { } } -// AllocateTokensToConsumerValidators allocates the given tokens from the -// from consumer rewards pool to validator according to their voting power +// AllocateTokensToConsumerValidators allocates tokens +// to the given consumer chain's validator set func (k Keeper) AllocateTokensToConsumerValidators( ctx sdk.Context, chainID string, @@ -140,16 +140,18 @@ func (k Keeper) AllocateTokensToConsumerValidators( return allocated } - // get the consumer total voting power from the votes + // get the total voting power of the consumer valset totalPower := k.ComputeConsumerTotalVotingPower(ctx, chainID) if totalPower == 0 { return allocated } - for _, val := range k.GetConsumerValSet(ctx, chainID) { - consAddr := sdk.ConsAddress(val.ProviderConsAddr) + // Allocate tokens by iterating over the consumer validators + for _, consumerVal := range k.GetConsumerValSet(ctx, chainID) { + consAddr := sdk.ConsAddress(consumerVal.ProviderConsAddr) - powerFraction := math.LegacyNewDec(val.Power).QuoTruncate(math.LegacyNewDec(totalPower)) + // get the validator tokens fraction using its voting power + powerFraction := math.LegacyNewDec(consumerVal.Power).QuoTruncate(math.LegacyNewDec(totalPower)) tokensFraction := tokens.MulDecTruncate(powerFraction) // get the validator type struct for the consensus address @@ -234,8 +236,8 @@ func (k Keeper) GetConsumerRewardsPool(ctx sdk.Context) sdk.Coins { ) } -// ComputeConsumerTotalVotingPower returns the total voting power of the given consumer chain -// validator set +// ComputeConsumerTotalVotingPower returns the validator set total voting power +// for the given consumer chain func (k Keeper) ComputeConsumerTotalVotingPower(ctx sdk.Context, chainID string) (totalPower int64) { // sum the opted-in validators set voting powers for _, v := range k.GetConsumerValSet(ctx, chainID) { From e949f66137cfbd2873c0b4fd8eaa4dceda94b74f Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Mon, 18 Mar 2024 17:46:41 +0100 Subject: [PATCH 8/8] Update tests/integration/distribution.go --- tests/integration/distribution.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 331b1a397b..d3111a39a5 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -152,7 +152,7 @@ func (s *CCVTestSuite) TestRewardsDistribution() { ) // Check that the consumer rewards allocation are empty since relayAllCommittedPackets calls BeginBlockRD, - // which in turns call AllocateTokens. + // which in turns calls AllocateTokens. rewardsAlloc := providerKeeper.GetConsumerRewardsAllocation(s.providerCtx(), s.consumerChain.ChainID) s.Require().Empty(rewardsAlloc.Rewards)