From 7676773e72cbdf8672fbb535ab1e1aa5a9e6d435 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Mon, 24 Jun 2024 13:33:43 +0200 Subject: [PATCH 1/2] chore: align to LSM hook signature to SDK v0.50.0 (#1984) fix LSM hook --- x/ccv/provider/keeper/hooks.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x/ccv/provider/keeper/hooks.go b/x/ccv/provider/keeper/hooks.go index 4c4672ce4c..81226a2bdf 100644 --- a/x/ccv/provider/keeper/hooks.go +++ b/x/ccv/provider/keeper/hooks.go @@ -198,6 +198,10 @@ func (h Hooks) BeforeDelegationRemoved(_ context.Context, _ sdk.AccAddress, _ sd return nil } +func (h Hooks) BeforeTokenizeShareRecordRemoved(_ context.Context, _ uint64) error { + return nil +} + // // gov hooks // @@ -271,7 +275,3 @@ func (h Hooks) GetConsumerAdditionLegacyPropFromProp( } return providertypes.ConsumerAdditionProposal{}, false } - -func (h Hooks) BeforeTokenizeShareRecordRemoved(_ sdk.Context, _ uint64) error { - return nil -} From 3e63d54cd5a11ef438803727d99561ef5d0aaf66 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 25 Jun 2024 09:11:32 +0200 Subject: [PATCH 2/2] fix!: update PSS reward distribution to work with SDK v0.50.0 changes (#1982) * fix commented distribution test * doc * Update tests/integration/distribution.go Co-authored-by: insumity * Update tests/integration/distribution.go Co-authored-by: insumity * Update tests/integration/distribution.go Co-authored-by: insumity * add godoc * fix renaming issue * address comments * Update tests/integration/distribution.go Co-authored-by: insumity * make naming more consistent with godoc * Update tests/integration/distribution.go Co-authored-by: insumity --------- Co-authored-by: insumity --- tests/integration/distribution.go | 277 +++++++++----------------- x/ccv/provider/keeper/distribution.go | 96 ++++----- 2 files changed, 131 insertions(+), 242 deletions(-) diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index f3488d6139..acff226828 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -137,7 +137,8 @@ func (s *CCVTestSuite) TestRewardsDistribution() { } consuValsRewards := consumerValsOutstandingRewardsFunc(s.providerCtx()) - // Transfer rewards from consumer to provider + // Transfer rewards from consumer to provider and distribute rewards to + // validators and community pool by calling BeginBlockRD relayAllCommittedPackets( s, s.consumerChain, @@ -147,15 +148,20 @@ func (s *CCVTestSuite) TestRewardsDistribution() { 1, ) - // Check that the consumer rewards allocation are empty since relayAllCommittedPackets calls BeginBlockRD, - // which in turns calls AllocateTokens. + // 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.consumerChain.ChainID) - s.Require().Empty(rewardsAlloc.Rewards) + 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, + // 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. rewardCoins = providerBankKeeper.GetAllBalances(s.providerCtx(), rewardPool) - s.Require().Equal(rewardCoins.AmountOf(rewardsIBCdenom), providerExpRewardsAmount) + s.Require().Equal( + math.LegacyNewDecFromInt(rewardCoins.AmountOf(rewardsIBCdenom)), + math.LegacyNewDecFromInt(providerExpRewardsAmount).Add(remainingAlloc), + ) // Check that the distribution module account balance is equal to the consumer rewards consuValsRewardsReceived := consumerValsOutstandingRewardsFunc(s.providerCtx()).Sub(consuValsRewards) @@ -690,197 +696,96 @@ func (s *CCVTestSuite) TestIBCTransferMiddleware() { // TestAllocateTokens is a happy-path test of the consumer rewards pool allocation // to opted-in validators and the community pool -// func (s *CCVTestSuite) TestAllocateTokens() { -// // set up channel and delegate some tokens in order for validator set update to be sent to the consumer chain -// s.SetupAllCCVChannels() -// providerKeeper := s.providerApp.GetProviderKeeper() -// bankKeeper := s.providerApp.GetTestBankKeeper() -// distributionKeeper := s.providerApp.GetTestDistributionKeeper() - -// totalRewards := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100))} - -// // fund consumer rewards pool -// bankKeeper.SendCoinsFromAccountToModule( -// s.providerCtx(), -// s.providerChain.SenderAccount.GetAddress(), -// providertypes.ConsumerRewardsPool, -// totalRewards, -// ) - -// // Allocate rewards evenly between consumers -// rewardsPerConsumer := totalRewards.QuoInt(math.NewInt(int64(len(s.consumerBundles)))) -// for chainID := range s.consumerBundles { -// // update consumer allocation -// providerKeeper.SetConsumerRewardsAllocation( -// s.providerCtx(), -// chainID, -// providertypes.ConsumerRewardsAllocation{ -// Rewards: sdk.NewDecCoinsFromCoins(rewardsPerConsumer...), -// }, -// ) -// } - -// // Iterate over the validators and -// // store their current voting power and outstanding rewards -// lastValOutRewards := map[string]sdk.DecCoins{} -// for _, val := range s.providerChain.Vals.Validators { -// valRewards := distributionKeeper.GetValidatorOutstandingRewards(s.providerCtx(), sdk.ValAddress(val.Address)) -// lastValOutRewards[sdk.ValAddress(val.Address).String()] = valRewards.Rewards -// } - -// // store community pool balance -// lastCommPool := distributionKeeper.GetFeePoolCommunityCoins(s.providerCtx()) - -// // execute BeginBlock to trigger the token allocation -// providerKeeper.BeginBlockRD(s.providerCtx()) - -// valNum := len(s.providerChain.Vals.Validators) -// consuNum := len(s.consumerBundles) - -// // compute the expected validators token allocation by subtracting the community tax -// rewardsPerConsumerDec := sdk.NewDecCoinsFromCoins(rewardsPerConsumer...) -// communityTax := distributionKeeper.GetCommunityTax(s.providerCtx()) -// validatorsExpRewards := rewardsPerConsumerDec. -// MulDecTruncate(math.LegacyOneDec().Sub(communityTax)). -// // multiply by the number of consumers since all the validators opted in -// MulDec(sdk.NewDec(int64(consuNum))) -// perValExpReward := validatorsExpRewards.QuoDec(sdk.NewDec(int64(valNum))) - -// // verify the validator tokens allocation -// // note that the validators have the same voting power to keep things simple -// for _, val := range s.providerChain.Vals.Validators { -// valRewards := distributionKeeper.GetValidatorOutstandingRewards(s.providerCtx(), sdk.ValAddress(val.Address)) -// s.Require().Equal( -// valRewards.Rewards, -// lastValOutRewards[sdk.ValAddress(val.Address).String()].Add(perValExpReward...), -// ) -// } - -// commPoolExpRewards := sdk.NewDecCoinsFromCoins(totalRewards...).Sub(validatorsExpRewards) -// currCommPool := distributionKeeper.GetFeePoolCommunityCoins(s.providerCtx()) - -// s.Require().Equal(currCommPool, (lastCommPool.Add(commPoolExpRewards...))) -// } - -// TestAllocateTokens is a unit-test for TransferConsumerRewardsToDistributionModule() -// but is written as an integration test to avoid excessive mocking. -func (s *CCVTestSuite) TransferConsumerRewardsToDistributionModule() { - testCases := []struct { - name string - rewardsPool sdk.Coins - rewardsAlloc sdk.DecCoins - expErr bool - }{ - { - "empty consumer rewards pool", - sdk.Coins{}, - sdk.DecCoins{}, - false, - }, - { - "empty consumer allocation", - sdk.Coins{ - sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100)), - }, - sdk.DecCoins{}, - false, - }, - { - "equal consumer rewards pool and allocation", - sdk.Coins{ - sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100)), - }, - sdk.DecCoins{ - sdk.NewDecCoin(sdk.DefaultBondDenom, math.NewInt(100)), - }, - false, - }, - { - "less consumer rewards than allocation", - sdk.Coins{ - sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(90)), - }, - sdk.DecCoins{ - sdk.NewDecCoin(sdk.DefaultBondDenom, math.NewInt(100)), - }, - true, - }, - { - "remaining consumer rewards allocation", - sdk.Coins{ - sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100)), - }, - sdk.DecCoins{ - sdk.DecCoin{ - Denom: sdk.DefaultBondDenom, - Amount: math.LegacyNewDecWithPrec(995, 1), - }, - }, - false, - }, - } - +func (s *CCVTestSuite) TestAllocateTokens() { + // set up channel and delegate some tokens in order for validator set update to be sent to the consumer chain + s.SetupAllCCVChannels() providerKeeper := s.providerApp.GetProviderKeeper() bankKeeper := s.providerApp.GetTestBankKeeper() distributionKeeper := s.providerApp.GetTestDistributionKeeper() + accountKeeper := s.providerApp.GetTestAccountKeeper() - chainID := s.consumerChain.ChainID + getDistrAcctBalFn := func(ctx sdk.Context) sdk.DecCoins { + bal := bankKeeper.GetAllBalances(ctx, accountKeeper.GetModuleAccount(ctx, distrtypes.ModuleName).GetAddress()) + return sdk.NewDecCoinsFromCoins(bal...) + } - for _, tc := range testCases { - s.Run(tc.name, func() { - ctx, _ := s.providerCtx().CacheContext() - // fund consumer rewards pool - bankKeeper.SendCoinsFromAccountToModule( - ctx, - s.providerChain.SenderAccount.GetAddress(), - providertypes.ConsumerRewardsPool, - tc.rewardsPool, - ) + totalRewards := sdk.Coins{sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100))} - // update consumer rewars allocation - providerKeeper.SetConsumerRewardsAllocation( - ctx, - chainID, - providertypes.ConsumerRewardsAllocation{ - Rewards: tc.rewardsAlloc, - }, - ) + // fund consumer rewards pool + bankKeeper.SendCoinsFromAccountToModule( + s.providerCtx(), + s.providerChain.SenderAccount.GetAddress(), + providertypes.ConsumerRewardsPool, + totalRewards, + ) - // store pool balance - oldPool := bankKeeper.GetAllBalances( - ctx, - distributionKeeper.GetDistributionAccount(ctx).GetAddress(), - ) + // Allocate rewards evenly between consumers + rewardsPerChain := totalRewards.QuoInt(math.NewInt(int64(len(s.consumerBundles)))) + for chainID := range s.consumerBundles { + // update consumer allocation + providerKeeper.SetConsumerRewardsAllocation( + s.providerCtx(), + chainID, + providertypes.ConsumerRewardsAllocation{ + Rewards: sdk.NewDecCoinsFromCoins(rewardsPerChain...), + }, + ) + } - coinsTransferred, _ := providerKeeper.GetConsumerRewardsAllocation(ctx, chainID).Rewards.TruncateDecimal() - // // transfer consumer rewards to distribution module - // coinsTransferred, err := providerKeeper.TransferConsumerRewardsToDistributionModule( - // ctx, - // chainID, - // ) - // if tc.expErr { - // s.Require().Error(err) - // return - // } - - // check remaining consumer rewards allocation - expCoinTransferred, expRemaining := tc.rewardsAlloc.TruncateDecimal() - s.Require().Equal(expCoinTransferred, coinsTransferred) - - s.Require().Equal( - expRemaining, - providerKeeper.GetConsumerRewardsAllocation(ctx, chainID).Rewards, - ) + // iterate over the validators and verify that no validator has outstanding rewards + totalValsRewards := sdk.DecCoins{} + for _, val := range s.providerChain.Vals.Validators { + valRewards, err := distributionKeeper.GetValidatorOutstandingRewards(s.providerCtx(), sdk.ValAddress(val.Address)) + s.Require().NoError(err) + totalValsRewards = totalValsRewards.Add(valRewards.Rewards...) + } - // check updated consuemer rewards pool balance - newPool := bankKeeper.GetAllBalances( - ctx, - distributionKeeper.GetDistributionAccount(ctx).GetAddress(), - ) + s.Require().True(totalValsRewards.IsZero()) - s.Require().Equal(newPool.Sub(oldPool...), coinsTransferred) - }) + // At this point the distribution module account + // only holds the community pool's tokens + // since there are no validators with outstanding rewards + lastCommPool := getDistrAcctBalFn(s.providerCtx()) + + // execute BeginBlock to trigger the token allocation + providerKeeper.BeginBlockRD(s.providerCtx()) + + valNum := len(s.providerChain.Vals.Validators) + consNum := len(s.consumerBundles) + + // compute the expected validators token allocation by subtracting the community tax + rewardsPerChainDec := sdk.NewDecCoinsFromCoins(rewardsPerChain...) + communityTax, err := distributionKeeper.GetCommunityTax(s.providerCtx()) + s.Require().NoError(err) + + rewardsPerChainTrunc, _ := rewardsPerChainDec. + MulDecTruncate(math.LegacyOneDec().Sub(communityTax)).TruncateDecimal() + validatorsExpRewardsPerChain := sdk.NewDecCoinsFromCoins(rewardsPerChainTrunc...).QuoDec(math.LegacyNewDec(int64(valNum))) + // multiply by the number of consumers + validatorsExpRewards := validatorsExpRewardsPerChain.MulDec(math.LegacyNewDec(int64(consNum))) + + // verify the validator tokens allocation + // note that the validators have the same voting power to keep things simple + for _, val := range s.providerChain.Vals.Validators { + valRewards, err := distributionKeeper.GetValidatorOutstandingRewards(s.providerCtx(), sdk.ValAddress(val.Address)) + s.Require().NoError(err) + + s.Require().Equal( + valRewards.Rewards, + validatorsExpRewards, + ) } + + // 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(s.providerCtx(), s.consumerChain.ChainID).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)))) + + // compare the expected total rewards against the distribution module balance + s.Require().Equal(lastCommPool.Add(totalRewardsDistributed...), getDistrAcctBalFn(s.providerCtx())) } // getEscrowBalance gets the current balances in the escrow account holding the transferred tokens to the provider diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 37571be6f0..9ac016abbc 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -76,29 +76,20 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { // Iterate over all registered consumer chains for _, consumerChainID := range k.GetAllRegisteredConsumerChainIDs(ctx) { - // transfer the consumer rewards to the distribution module account - // note that the rewards transferred are only consumer whitelisted denoms - rewardsCollected, err := k.TransferConsumerRewardsToDistributionModule(ctx, consumerChainID) - if err != nil { - k.Logger(ctx).Error( - "fail to transfer rewards to distribution module for chain %s: %s", - consumerChainID, - err, - ) - 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() { + alloc := k.GetConsumerRewardsAllocation(ctx, consumerChainID) + if alloc.Rewards.IsZero() { continue } // temporary workaround to keep CanWithdrawInvariant happy // general discussions here: https://github.com/cosmos/cosmos-sdk/issues/2906#issuecomment-441867634 if k.ComputeConsumerTotalVotingPower(ctx, consumerChainID) == 0 { - err := k.distributionKeeper.FundCommunityPool(context.Context(ctx), rewardsCollected, k.accountKeeper.GetModuleAccount(ctx, types.ConsumerRewardsPool).GetAddress()) + rewardsToSend, rewardsChange := alloc.Rewards.TruncateDecimal() + err := k.distributionKeeper.FundCommunityPool(context.Context(ctx), rewardsToSend, k.accountKeeper.GetModuleAccount(ctx, types.ConsumerRewardsPool).GetAddress()) if err != nil { k.Logger(ctx).Error( "fail to allocate rewards from consumer chain %s to community pool: %s", @@ -106,12 +97,17 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { err, ) } + + // set the consumer allocation to the remaining reward decimals + alloc.Rewards = rewardsChange + k.SetConsumerRewardsAllocation(ctx, consumerChainID, alloc) + return } - // calculate the reward allocations - rewardsCollectedDec := sdk.NewDecCoinsFromCoins(rewardsCollected...) - remaining := rewardsCollectedDec + // Consumer rewards are distributed between the validators and the community pool. + // The decimals resulting from the distribution are expected to remain in the consumer reward allocations. + communityTax, err := k.distributionKeeper.GetCommunityTax(ctx) if err != nil { k.Logger(ctx).Error( @@ -121,20 +117,37 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { ) continue } + + // compute rewards for validators + consumerRewards := alloc.Rewards voteMultiplier := math.LegacyOneDec().Sub(communityTax) - feeMultiplier := rewardsCollectedDec.MulDecTruncate(voteMultiplier) + validatorsRewards := consumerRewards.MulDecTruncate(voteMultiplier) + + // compute remaining rewards for the community pool + remaining := consumerRewards.Sub(validatorsRewards) + + // transfer validators rewards to distribution module account + validatorsRewardsTrunc, validatorsRewardsChange := validatorsRewards.TruncateDecimal() + err = k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ConsumerRewardsPool, distrtypes.ModuleName, validatorsRewardsTrunc) + if err != nil { + k.Logger(ctx).Error( + "cannot send rewards to distribution module account %s: %s", + consumerChainID, + err, + ) + continue + } // allocate tokens to consumer validators - feeAllocated := k.AllocateTokensToConsumerValidators( + k.AllocateTokensToConsumerValidators( ctx, consumerChainID, - feeMultiplier, + sdk.NewDecCoinsFromCoins(validatorsRewardsTrunc...), ) - remaining = remaining.Sub(feeAllocated) - // allocate community funding - remainingCoins, _ := remaining.TruncateDecimal() - err = k.distributionKeeper.FundCommunityPool(context.Context(ctx), remainingCoins, k.accountKeeper.GetModuleAccount(ctx, types.ConsumerRewardsPool).GetAddress()) + // allocate remaining rewards to the community pool + remainingRewards, remainingChanges := remaining.TruncateDecimal() + err = k.distributionKeeper.FundCommunityPool(context.Context(ctx), remainingRewards, k.accountKeeper.GetModuleAccount(ctx, types.ConsumerRewardsPool).GetAddress()) if err != nil { k.Logger(ctx).Error( "fail to allocate rewards from consumer chain %s to community pool: %s", @@ -143,6 +156,10 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) { ) continue } + + // set consumer allocations to the remaining rewards decimals + alloc.Rewards = validatorsRewardsChange.Add(remainingChanges...) + k.SetConsumerRewardsAllocation(ctx, consumerChainID, alloc) } } @@ -205,39 +222,6 @@ func (k Keeper) AllocateTokensToConsumerValidators( return allocated } -// TransferConsumerRewardsToDistributionModule transfers the rewards allocation of the given consumer chain -// from the consumer rewards pool to the distribution module -func (k Keeper) TransferConsumerRewardsToDistributionModule( - ctx sdk.Context, - chainID string, -) (sdk.Coins, error) { - // Get coins of the consumer rewards allocation - allocation := k.GetConsumerRewardsAllocation(ctx, chainID) - - if allocation.Rewards.IsZero() { - return sdk.Coins{}, nil - } - - // Truncate coin rewards - rewardsToSend, remRewards := allocation.Rewards.TruncateDecimal() - - // NOTE the consumer rewards allocation isn't a module account, however its coins - // are held in the consumer reward pool module account. Thus the consumer - // rewards allocation must be reduced separately from the SendCoinsFromModuleToAccount call. - - // Update consumer rewards allocation with the remaining decimal coins - allocation.Rewards = remRewards - - // Send coins to distribution module account - err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ConsumerRewardsPool, distrtypes.ModuleName, rewardsToSend) - if err != nil { - return sdk.Coins{}, err - } - - k.SetConsumerRewardsAllocation(ctx, chainID, allocation) - return rewardsToSend, nil -} - // consumer reward pools getter and setter // GetConsumerRewardsAllocation returns the consumer rewards allocation for the given chain ID