From e7a18f5da576111b8f71923a4a733dea8a65406c Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 16 Feb 2024 09:37:56 +0100 Subject: [PATCH] add comments --- tests/integration/distribution.go | 88 +++++++++++++++++++++++++++ testutil/integration/debug_test.go | 4 ++ x/ccv/provider/ibc_middleware.go | 8 +-- x/ccv/provider/ibc_middleware_test.go | 86 ++++++++++++++++++++++++++ x/ccv/provider/keeper/distribution.go | 49 +++++++++------ 5 files changed, 212 insertions(+), 23 deletions(-) create mode 100644 x/ccv/provider/ibc_middleware_test.go diff --git a/tests/integration/distribution.go b/tests/integration/distribution.go index 43965889bb..a524a11e35 100644 --- a/tests/integration/distribution.go +++ b/tests/integration/distribution.go @@ -6,6 +6,7 @@ import ( "cosmossdk.io/math" abci "github.com/cometbft/cometbft/abci/types" + "github.com/cometbft/cometbft/libs/bytes" "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" @@ -901,3 +902,90 @@ func (s *CCVTestSuite) prepareRewardDist() { blocksToGo := bpdt - blocksSinceLastTrans s.coordinator.CommitNBlocks(s.consumerChain, uint64(blocksToGo)) } + +func (s *CCVTestSuite) TestAllocateTokensToValidator() { + + providerkeepr := s.providerApp.GetProviderKeeper() + + chainID := "consumer" + + 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 + tokens sdk.DecCoins + expCoinTransferred sdk.DecCoins + }{ + { + name: "reward tokens are empty", + }, + { + name: "total voting power is zero", + tokens: sdk.DecCoins{sdk.NewDecCoin("uatom", math.NewInt(100_000))}, + }, + { + name: "expect all tokens to be allocated to a single validator", + votes: []abci.VoteInfo{votes[0]}, + tokens: sdk.DecCoins{sdk.NewDecCoin("uatom", math.NewInt(100_000))}, + expCoinTransferred: sdk.DecCoins{sdk.NewDecCoin("uatom", math.NewInt(100_000))}, + }, + { + name: "expect tokens to be allocated evenly between validators", + votes: []abci.VoteInfo{votes[0], votes[1]}, + tokens: sdk.DecCoins{sdk.NewDecCoin("uatom", math.NewInt(555_555))}, + expCoinTransferred: sdk.DecCoins{sdk.NewDecCoin("uatom", math.NewInt(555_555))}, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + + ctx, _ := s.providerCtx().CacheContext() + + // opt validators in + // for _, v := range tc.optedIn { + // keeper.SetOptedIn( + // ctx, + // chainID, + // types.NewProviderConsAddress(sdk.ConsAddress(v)), + // 0, + // ) + // } + + // allocate tokens + res := providerkeepr.AllocateTokensToConsumerValidators( + ctx, + chainID, + tc.votes, + tc.tokens, + ) + + // check that the expect result is returned + s.Require().True(tc.expCoinTransferred.IsEqual(res)) + + if !tc.expCoinTransferred.Empty() { + // rewards are expected to be allocated evenly between validators + rewardsPerVal := tc.expCoinTransferred.QuoDec(sdk.NewDec(int64(len(tc.votes)))) + + // check that the rewards are allocated to validators as expected + for _, v := range tc.votes { + rewards := s.providerApp.GetTestDistributionKeeper().GetValidatorOutstandingRewards( + ctx, + sdk.ValAddress(v.Validator.Address), + ) + s.Require().True(rewardsPerVal.IsEqual(rewards.Rewards)) + } + } + + }) + } +} diff --git a/testutil/integration/debug_test.go b/testutil/integration/debug_test.go index 83301343be..c370e8e701 100644 --- a/testutil/integration/debug_test.go +++ b/testutil/integration/debug_test.go @@ -295,3 +295,7 @@ func TestAllocateTokens(t *testing.T) { func TestTransferConsumerRewardsToDistributionModule(t *testing.T) { runCCVTestByName(t, "TransferConsumerRewardsToDistributionModule") } + +func TestAllocateTokensToValidator(t *testing.T) { + runCCVTestByName(t, "TestAllocateTokensToValidator") +} diff --git a/x/ccv/provider/ibc_middleware.go b/x/ccv/provider/ibc_middleware.go index ac46ae5132..fc5a25c37b 100644 --- a/x/ccv/provider/ibc_middleware.go +++ b/x/ccv/provider/ibc_middleware.go @@ -131,7 +131,7 @@ func (im IBCMiddleware) OnRecvPacket( return ack } - coinDenom := getProviderDenom(data.Denom, packet) + coinDenom := GetProviderDenom(data.Denom, packet) coinAmt, _ := math.NewIntFromString(data.Amount) // Iterate over the whitelisted consumer denoms @@ -205,11 +205,11 @@ func (im IBCMiddleware) GetAppVersion(ctx sdk.Context, portID, channelID string) return "", false } -// getProviderDenom returns the updated given denom according to the given IBC packet +// GetProviderDenom returns the updated given denom according to the given IBC packet // It follows the same logic than the OnRecvPacket method of the IBC transfer module // see https://github.com/cosmos/ibc-go/blob/v7.3.2/modules/apps/transfer/keeper/relay.go#L162 -func getProviderDenom(denom string, packet channeltypes.Packet) (providerDenom string) { - // If the the prefix denom corresponds to the provider transfer channel info, +func GetProviderDenom(denom string, packet channeltypes.Packet) (providerDenom string) { + // If the the prefix denom corresponds to the packet's source port and channel, // returns the base denom if ibctransfertypes.ReceiverChainIsSource(packet.GetSourcePort(), packet.GetSourceChannel(), denom) { voucherPrefix := ibctransfertypes.GetDenomPrefix(packet.GetSourcePort(), packet.GetSourceChannel()) diff --git a/x/ccv/provider/ibc_middleware_test.go b/x/ccv/provider/ibc_middleware_test.go new file mode 100644 index 0000000000..25ff8b06ca --- /dev/null +++ b/x/ccv/provider/ibc_middleware_test.go @@ -0,0 +1,86 @@ +package provider_test + +import ( + "testing" + + clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" + channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" + "github.com/cosmos/interchain-security/v4/x/ccv/provider" + "github.com/stretchr/testify/require" +) + +func TestGetProviderDenom(t *testing.T) { + testCases := []struct { + name string + denom string + packet channeltypes.Packet + expProviderDenom string + }{ + { + name: "returns base denom with destination port and channel as prefix", + denom: "stake", + packet: channeltypes.NewPacket( + []byte{}, + 0, + "srcPort", + "srcChannel", + "dstPort", + "dstChannel", + clienttypes.NewHeight(1, 1), + 0, + ), + expProviderDenom: "dstPort/dstChannel/stake", + }, + { + name: "returns base denom if the prefix denom corresponds to the packet's port and channel source", + denom: "srcPort/srcChannel/stake", + packet: channeltypes.NewPacket( + []byte{}, + 0, + "srcPort", + "srcChannel", + "dstPort", + "dstChannel", + clienttypes.NewHeight(1, 1), + 0, + ), + expProviderDenom: "stake", + }, + { + name: "returns prefixed denom updated with destination port and channel as prefix", + denom: "dstPort/dstChannel/stake", + packet: channeltypes.NewPacket( + []byte{}, + 0, + "srcPort", + "srcChannel", + "dstPort", + "dstChannel", + clienttypes.NewHeight(1, 1), + 0, + ), + expProviderDenom: "dstPort/dstChannel/dstPort/dstChannel/stake", + }, + } + + channeltypes.NewPacket( + []byte{}, + 0, + "srcPort", + "srcChannel", + "dstPort", + "dstChannel", + clienttypes.NewHeight(1, 1), + 0, + ) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + res := provider.GetProviderDenom( + tc.denom, + tc.packet, + ) + + require.Equal(t, tc.expProviderDenom, res) + }) + } +} diff --git a/x/ccv/provider/keeper/distribution.go b/x/ccv/provider/keeper/distribution.go index 0c5f61f8b3..61c19fe320 100644 --- a/x/ccv/provider/keeper/distribution.go +++ b/x/ccv/provider/keeper/distribution.go @@ -83,7 +83,7 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded // note that the rewards transferred are only consumer whitelisted denoms rewardsCollected, err := k.TransferConsumerRewardsToDistributionModule(ctx, consumer.ChainId) if err != nil { - panic(err) + k.Logger(ctx).Error("fail to transfer consumer rewards to distribution module: %s", err) } if rewardsCollected.IsZero() { @@ -111,8 +111,6 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, totalPreviousPower int64, bonded feeAllocated := k.AllocateTokensToConsumerValidators( ctx, consumer.ChainId, - // pass consumer opted-in vals total power - k.ComputeConsumerTotalVotingPower(ctx, consumer.ChainId, bondedVotes), bondedVotes, feeMultiplier, ) @@ -127,16 +125,25 @@ 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 consumer rewards pool to validator -// according to their voting power +// AllocateTokensToConsumerValidators allocates the given tokens from the +// from consumer rewards pool to validator according to their voting power func (k Keeper) AllocateTokensToConsumerValidators( ctx sdk.Context, chainID string, - totalPreviousPower int64, bondedVotes []abci.VoteInfo, tokens sdk.DecCoins, -) sdk.DecCoins { - totalReward := sdk.DecCoins{} +) (totalReward sdk.DecCoins) { + // return early if the tokens are empty + if tokens.Empty() { + return totalReward + } + + // get the consumer total voting power from the votes + totalPower := k.ComputeConsumerTotalVotingPower(ctx, chainID, bondedVotes) + if totalPower == 0 { + return totalReward + } + for _, vote := range bondedVotes { // TODO: should check if validator IsOptIn or continue here consAddr := sdk.ConsAddress(vote.Validator.Address) @@ -144,7 +151,7 @@ func (k Keeper) AllocateTokensToConsumerValidators( // TODO: Consider micro-slashing for missing votes. // // Ref: https://github.com/cosmos/cosmos-sdk/issues/2525#issuecomment-430838701 - powerFraction := math.LegacyNewDec(vote.Validator.Power).QuoTruncate(math.LegacyNewDec(totalPreviousPower)) + powerFraction := math.LegacyNewDec(vote.Validator.Power).QuoTruncate(math.LegacyNewDec(totalPower)) tokensFraction := tokens.MulDecTruncate(powerFraction) k.distributionKeeper.AllocateTokensToValidator( @@ -166,6 +173,9 @@ func (k Keeper) TransferConsumerRewardsToDistributionModule( ) (sdk.Coins, error) { // Get coins of the consumer rewards allocation allocation := k.GetConsumerRewardsAllocation(ctx, chainID) + + // TODO: check if this condition doesn't break invariant + // CanWithdrawInvariant if allocation.Rewards.IsZero() { return sdk.Coins{}, nil } @@ -216,23 +226,24 @@ func (k Keeper) GetConsumerRewardsPool(ctx sdk.Context) sdk.Coins { ) } -// ComputeConsumerTotalVotingPower returns the total voting power -// for the given consumer chain opted-in validators +// 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 { - optedIn := map[string]struct{}{} + // optedIn := map[string]struct{}{} - // create set with opted-in validators - for _, v := range k.GetOptedIn(ctx, chainID) { - optedIn[v.ProviderAddr.ToSdkConsAddr().String()] = struct{}{} - } + // // create set with opted-in validators + // for _, v := range k.GetOptedIn(ctx, chainID) { + // optedIn[v.ProviderAddr.ToSdkConsAddr().String()] = struct{}{} + // } var totalPower int64 // sum the opted-in validators set voting powers for _, vote := range votes { - if _, ok := optedIn[sdk.ConsAddress(vote.Validator.Address).String()]; ok { - totalPower += vote.Validator.Power - } + // if _, ok := optedIn[sdk.ConsAddress(vote.Validator.Address).String()]; ok { + // totalPower += vote.Validator.Power + // } + totalPower += vote.Validator.Power } return totalPower