Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore!: fix PSS mem-tests (SDK v50 upgrade) #1933

Merged
merged 5 commits into from
Jun 5, 2024
Merged

chore!: fix PSS mem-tests (SDK v50 upgrade) #1933

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a39d2a9
fix mem-tests
sainoe Jun 3, 2024
1784aa2
Merge branch 'feat/pss-upgrade-v50' into sainoe/pss-upgrade
sainoe Jun 3, 2024
736294d
nits
sainoe Jun 4, 2024
f9bb81d
nits
sainoe Jun 4, 2024
a6c76e1
other nits
sainoe Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
286 changes: 133 additions & 153 deletions tests/integration/distribution.go
View file
Open in desktop

Large diffs are not rendered by default.

30 changes: 16 additions & 14 deletions testutil/ibc_testing/generic_setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,26 +142,28 @@ func AddConsumer[Tp testutil.ProviderApp, Tc testutil.ConsumerApp](
prop.ChainId = chainID
prop.Top_N = consumerTopNParams[index] // isn't used in CreateConsumerClient

// opt-in all validators
for _, v := range providerApp.GetTestStakingKeeper().GetLastValidators(providerChain.GetContext()) {
consAddr, _ := v.GetConsAddr()
providerKeeper.SetOptedIn(providerChain.GetContext(), chainID, providertypes.NewProviderConsAddress(consAddr))
}

// NOTE: we cannot use the time.Now() because the coordinator chooses a hardcoded start time
// using time.Now() could set the spawn time to be too far in the past or too far in the future
prop.SpawnTime = coordinator.CurrentTime
// NOTE: the initial height passed to CreateConsumerClient
// must be the height on the consumer when InitGenesis is called
prop.InitialHeight = clienttypes.Height{RevisionNumber: 0, RevisionHeight: 3}
err := providerKeeper.CreateConsumerClient(
providerChain.GetContext(),
prop,
)
prop.InitialHeight = clienttypes.Height{RevisionNumber: 0, RevisionHeight: 2}

providerKeeper.SetPendingConsumerAdditionProp(providerChain.GetContext(), prop)
props := providerKeeper.GetAllPendingConsumerAdditionProps(providerChain.GetContext())
s.Require().Len(props, 1, "unexpected len consumer addition proposals in AddConsumer")

// opt-in all validators
lastVals, err := providerApp.GetTestStakingKeeper().GetLastValidators(providerChain.GetContext())
s.Require().NoError(err)

// set the consumer TopN here since the test suite setup only used the consumer addition prop
// to create the consumer genesis, see BeginBlockInit in /x/ccv/provider/keeper/proposal.go.
providerKeeper.SetTopN(providerChain.GetContext(), chainID, prop.Top_N)
for _, v := range lastVals {
consAddr, _ := v.GetConsAddr()
providerKeeper.SetOptedIn(providerChain.GetContext(), chainID, providertypes.NewProviderConsAddress(consAddr))
}

// commit the state on the provider chain
// and create the client and genesis of consumer
coordinator.CommitBlock(providerChain)

// get genesis state created by the provider
Expand Down
2 changes: 1 addition & 1 deletion testutil/keeper/expectations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ func GetMocksForMakeConsumerGenesis(ctx sdk.Context, mocks *MockedKeepers,
) []*gomock.Call {
return []*gomock.Call{
mocks.MockStakingKeeper.EXPECT().UnbondingTime(gomock.Any()).Return(unbondingTimeToInject, nil).Times(1),

mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(gomock.Any(),
clienttypes.GetSelfHeight(ctx)).Return(&ibctmtypes.ConsensusState{}, nil).Times(1),
// mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove if not needed

}
}

Expand Down
4 changes: 2 additions & 2 deletions x/ccv/provider/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,11 @@ func TestAssignConsensusKeyMsgHandling(t *testing.T) {
mocks.MockStakingKeeper.EXPECT().GetValidator(
ctx, providerCryptoId.SDKValOpAddress(),
// validator should not be missing
).Return(providerCryptoId.SDKStakingValidator(), true).Times(1),
).Return(providerCryptoId.SDKStakingValidator(), nil).Times(1),
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx,
consumerConsAddr.ToSdkConsAddr(),
// return false - no other validator uses the consumer key to validate *on the provider*
).Return(stakingtypes.Validator{}, false),
).Return(stakingtypes.Validator{}, nil),
)
},
expError: true,
Expand Down
30 changes: 12 additions & 18 deletions x/ccv/provider/keeper/distribution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,31 +76,25 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {

// Iterate over all registered consumer chains
for _, consumer := range k.GetAllConsumerChains(ctx) {
// transfer the consumer rewards to the distribution module account
// note that the rewards transferred are only consumer whitelisted denoms
// Get coins of the consumer rewards allocation
allocation := k.GetConsumerRewardsAllocation(ctx, consumer.ChainId)
if allocation.Rewards.IsZero() {
rewardsCollected, err := k.TransferConsumerRewardsToDistributionModule(ctx, consumer.ChainId)
bermuell marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
k.Logger(ctx).Error(
"fail to transfer rewards to distribution module for chain %s: %s",
consumer.ChainId,
err,
)
Comment on lines +79 to +87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refining the error handling in AllocateTokens.

- k.Logger(ctx).Error(
-   "fail to transfer rewards to distribution module for chain %s: %s",
-   consumer.ChainId,
-   err,
- )
+ if errors.Is(err, ExpectedErrorType) {
+   k.Logger(ctx).Error(
+     "expected error occurred while transferring rewards for chain %s: %s",
+     consumer.ChainId,
+     err,
+   )
+ } else {
+   k.Logger(ctx).Error(
+     "unexpected error occurred while transferring rewards for chain %s: %s",
+     consumer.ChainId,
+     err,
+   )
+ }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
// transfer the consumer rewards to the distribution module account
// note that the rewards transferred are only consumer whitelisted denoms
// Get coins of the consumer rewards allocation
allocation := k.GetConsumerRewardsAllocation(ctx, consumer.ChainId)
if allocation.Rewards.IsZero() {
rewardsCollected, err := k.TransferConsumerRewardsToDistributionModule(ctx, consumer.ChainId)
if err != nil {
k.Logger(ctx).Error(
"fail to transfer rewards to distribution module for chain %s: %s",
consumer.ChainId,
err,
)
// transfer the consumer rewards to the distribution module account
// note that the rewards transferred are only consumer whitelisted denoms
rewardsCollected, err := k.TransferConsumerRewardsToDistributionModule(ctx, consumer.ChainId)
if err != nil {
if errors.Is(err, ExpectedErrorType) {
k.Logger(ctx).Error(
"expected error occurred while transferring rewards for chain %s: %s",
consumer.ChainId,
err,
)
} else {
k.Logger(ctx).Error(
"unexpected error occurred while transferring rewards for chain %s: %s",
consumer.ChainId,
err,
)
}

continue
}

// Truncate coin rewards
rewardsCollected := sdk.Coins{}
rewardsCollected, _ = 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 FundCommunityPool call.

// Update consumer rewards allocation with the remaining decimal coins
allocation.Rewards = allocation.Rewards.Sub(sdk.NewDecCoinsFromCoins(rewardsCollected...))
k.SetConsumerRewardsAllocation(ctx, consumer.ChainId, allocation)

// 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
}
rewardsCollectedDec := sdk.NewDecCoinsFromCoins(rewardsCollected...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for readablity I suggest to move this down to L117 where it is actually used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to line 107 where it used to initiate the remaining variable.
remaining := rewardsCollectedDec


// temporary workaround to keep CanWithdrawInvariant happy
// general discussions here: https://github.com/cosmos/cosmos-sdk/issues/2906#issuecomment-441867634
Expand All @@ -110,7 +104,6 @@ func (k Keeper) AllocateTokens(ctx sdk.Context) {
}

// calculate the reward allocations
rewardsCollectedDec := sdk.NewDecCoinsFromCoins(rewardsCollected...)
remaining := rewardsCollectedDec
communityTax, err := k.distributionKeeper.GetCommunityTax(ctx)
if err != nil {
Expand Down Expand Up @@ -192,8 +185,9 @@ func (k Keeper) AllocateTokensToConsumerValidators(
return allocated
}

// GetConsumerRewardsCollected get the consumer rewards pool to a the distribution module
func (k Keeper) GetConsumerRewardsCollected(
// TransferConsumerRewardsToDistributionModule transfers the rewards allocation of the given consumer chain
// from the consumer rewards pool to a the distribution module
func (k Keeper) TransferConsumerRewardsToDistributionModule(
sainoe marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The TransferConsumerRewardsToDistributionModule function is now visible. It includes error handling for the SendCoinsFromModuleToModule call, returning an error if the transfer fails. However, it does not handle other potential edge cases explicitly, such as the scenario where allocation.Rewards is zero, which is handled by returning an empty sdk.Coins slice.

  • Ensure that the function handles all potential edge cases and errors adequately.
  • Consider adding more comprehensive error handling and logging for other potential failure points.
Analysis chain

Ensure that the TransferConsumerRewardsToDistributionModule function handles edge cases and potential errors adequately.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all edge cases and potential errors are handled in the `TransferConsumerRewardsToDistributionModule` function.

# Test: Search for error handling patterns in the function implementation.
ast-grep --lang go --pattern $'func (k Keeper) TransferConsumerRewardsToDistributionModule($_, $_) {
  $$$
  if $_ {
    $$$
  }
  $$$
}'

Length of output: 138

Script:

#!/bin/bash
# Locate the `TransferConsumerRewardsToDistributionModule` function definition and print its implementation.
ast-grep --lang go --pattern 'func (k Keeper) TransferConsumerRewardsToDistributionModule($_, $_) { $$$ }'

Length of output: 108

Script:

#!/bin/bash
# Search for the `TransferConsumerRewardsToDistributionModule` function definition in the codebase.
rg 'func (k Keeper) TransferConsumerRewardsToDistributionModule'

Length of output: 66

Script:

#!/bin/bash
# List all Go files and search for the `TransferConsumerRewardsToDistributionModule` function definition within those files.
fd -e go -x rg 'func (k Keeper) TransferConsumerRewardsToDistributionModule' {}

Length of output: 83

Script:

#!/bin/bash
# Search for the `TransferConsumerRewardsToDistributionModule` function definition within the specified file.
rg 'func (k Keeper) TransferConsumerRewardsToDistributionModule' x/ccv/provider/keeper/distribution.go

Length of output: 104

Script:

#!/bin/bash
# List all functions in the specified file to confirm the presence of the `TransferConsumerRewardsToDistributionModule` function.
ast-grep --lang go --pattern 'func $_($_, $_) { $$$ }' x/ccv/provider/keeper/distribution.go

Length of output: 94

Script:

#!/bin/bash
# Search for the `TransferConsumerRewardsToDistributionModule` function definition within the specified file using a more general pattern.
rg 'TransferConsumerRewardsToDistributionModule' x/ccv/provider/keeper/distribution.go

Length of output: 351

Script:

#!/bin/bash
# Extract the full implementation of the `TransferConsumerRewardsToDistributionModule` function.
rg -A 20 'func (k Keeper) TransferConsumerRewardsToDistributionModule' x/ccv/provider/keeper/distribution.go

Length of output: 110

Script:

#!/bin/bash
# Extract the full implementation of the `TransferConsumerRewardsToDistributionModule` function using a larger context window.
rg -A 50 'func (k Keeper) TransferConsumerRewardsToDistributionModule' x/ccv/provider/keeper/distribution.go

Length of output: 110

Script:

#!/bin/bash
# Display the contents of the specified file to manually locate and inspect the function.
cat x/ccv/provider/keeper/distribution.go

Length of output: 10788

Tools
golangci-lint

190-190: undefined: Keeper (typecheck)

ctx sdk.Context,
chainID string,
) (sdk.Coins, error) {
Expand Down
8 changes: 4 additions & 4 deletions x/ccv/provider/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,11 @@ func TestQueryConsumerChainsValidatorHasToValidate(t *testing.T) {
pk, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

val := createStakingValidator(ctx, mocks, 1, 1)
val := createStakingValidator(ctx, mocks, 1, 1, 1)
valConsAddr, _ := val.GetConsAddr()
providerAddr := types.NewProviderConsAddress(valConsAddr)
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valConsAddr).Return(val, true).AnyTimes()
mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{val}).AnyTimes()
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valConsAddr).Return(val, nil).AnyTimes()
mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{val}, nil).AnyTimes()
sainoe marked this conversation as resolved.
Show resolved Hide resolved

req := types.QueryConsumerChainsValidatorHasToValidateRequest{
ProviderAddress: providerAddr.String(),
Expand Down Expand Up @@ -246,7 +246,7 @@ func TestQueryValidatorConsumerCommissionRate(t *testing.T) {
// because no consumer commission rate is set, the validator's set commission rate on the provider is used
val := stakingtypes.Validator{Commission: stakingtypes.Commission{CommissionRates: stakingtypes.CommissionRates{Rate: expectedCommissionRate}}}
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(
ctx, providerAddr.ToSdkConsAddr()).Return(val, true).Times(1)
ctx, providerAddr.ToSdkConsAddr()).Return(val, nil).Times(1)
p-offtermatt marked this conversation as resolved.
Show resolved Hide resolved
res, _ = pk.QueryValidatorConsumerCommissionRate(ctx, &req)
require.Equal(t, expectedCommissionRate, res.Rate)
}
7 changes: 5 additions & 2 deletions x/ccv/provider/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/stretchr/testify/require"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"

abci "github.com/cometbft/cometbft/abci/types"
tmprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto"
Expand Down Expand Up @@ -411,10 +412,12 @@ func TestGetAllConsumerChains(t *testing.T) {
{OperatorAddress: "cosmosvaloper1tflk30mq5vgqjdly92kkhhq3raev2hnz6eete3"}, // 500 power
}
powers := []int64{50, 150, 300, 500} // sum = 1000
mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Return(vals).AnyTimes()
mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Return(vals, nil).AnyTimes()

for i, val := range vals {
mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), val.GetOperator()).Return(powers[i]).AnyTimes()
valAddr, err := sdk.ValAddressFromBech32(val.GetOperator())
require.NoError(t, err)
mocks.MockStakingKeeper.EXPECT().GetLastValidatorPower(gomock.Any(), valAddr).Return(powers[i], nil).AnyTimes()
}

// set Top N parameters, client ids and expected result
Expand Down
4 changes: 2 additions & 2 deletions x/ccv/provider/keeper/legacy_proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ func TestHandleLegacyConsumerAdditionProposal(t *testing.T) {

if tc.expAppendProp {
// Mock calls are only asserted if we expect a client to be created.
mocks.MockStakingKeeper.EXPECT().GetLastValidators(gomock.Any()).Times(1)
gomock.InOrder(
testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, tc.prop.ChainId, clienttypes.NewHeight(2, 3))...,
)
Expand Down Expand Up @@ -233,8 +234,7 @@ func TestHandleLegacyConsumerRemovalProposal(t *testing.T) {
// meaning no external keeper methods are allowed to be called.
if tc.expAppendProp {
testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks)

// assert mocks for expected calls to `StopConsumerChain` when closing the underlying channel
// Valid client creation is asserted with mock expectations here
gomock.InOrder(testkeeper.GetMocksForStopConsumerChainWithCloseChannel(ctx, &mocks)...)
}

Expand Down
59 changes: 30 additions & 29 deletions x/ccv/provider/keeper/partial_set_security_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ func TestHandleOptInWithConsumerKey(t *testing.T) {
calls := []*gomock.Call{
mocks.MockStakingKeeper.EXPECT().
GetValidatorByConsAddr(gomock.Any(), gomock.Any()).
DoAndReturn(func(ctx sdk.Context, addr sdk.ConsAddress) (stakingtypes.Validator, bool) {
DoAndReturn(func(ctx sdk.Context, addr sdk.ConsAddress) (stakingtypes.Validator, error) {
if addr.Equals(providerAddr.Address) {
// Given `providerAddr`, `GetValidatorByConsAddr` returns a validator with the
// exact same `ConsensusPubkey`
pkAny, _ := codectypes.NewAnyWithValue(providerConsPubKey)
return stakingtypes.Validator{ConsensusPubkey: pkAny}, true
return stakingtypes.Validator{ConsensusPubkey: pkAny}, nil
} else {
// for any other consensus address, we cannot find a validator
return stakingtypes.Validator{}, false
return stakingtypes.Validator{}, stakingtypes.ErrNoValidatorFound
}
}).Times(2),
}
Expand All @@ -72,9 +72,10 @@ func TestHandleOptInWithConsumerKey(t *testing.T) {
// create a sample consumer key to assign to the `providerAddr` validator
// on the consumer chain with id `chainID`
consumerKey := "{\"@type\":\"/cosmos.crypto.ed25519.PubKey\",\"key\":\"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is=\"}"
expectedConsumerPubKey, _ := providerKeeper.ParseConsumerKey(consumerKey)
expectedConsumerPubKey, err := providerKeeper.ParseConsumerKey(consumerKey)
require.NoError(t, err)

err := providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, &consumerKey)
err = providerKeeper.HandleOptIn(ctx, "chainID", providerAddr, &consumerKey)
require.NoError(t, err)

// assert that the consumeKey was assigned to `providerAddr` validator on chain with id `chainID`
Expand Down Expand Up @@ -119,20 +120,20 @@ func TestHandleOptOutFromTopNChain(t *testing.T) {
// set the chain as Top 50 and create 4 validators with 10%, 20%, 30%, and 40% of the total voting power
// respectively
providerKeeper.SetTopN(ctx, "chainID", 50)
valA := createStakingValidator(ctx, mocks, 1, 1) // 10% of the total voting power (can opt out)
valA := createStakingValidator(ctx, mocks, 1, 1, 1) // 10% of the total voting power (can opt out)
valAConsAddr, _ := valA.GetConsAddr()
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valAConsAddr).Return(valA, true).AnyTimes()
valB := createStakingValidator(ctx, mocks, 2, 2) // 20% of the total voting power (can opt out)
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valAConsAddr).Return(valA, nil).AnyTimes()
valB := createStakingValidator(ctx, mocks, 2, 2, 2) // 20% of the total voting power (can opt out)
valBConsAddr, _ := valB.GetConsAddr()
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valBConsAddr).Return(valB, true).AnyTimes()
valC := createStakingValidator(ctx, mocks, 3, 3) // 30% of the total voting power (cannot opt out)
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valBConsAddr).Return(valB, nil).AnyTimes()
valC := createStakingValidator(ctx, mocks, 3, 3, 3) // 30% of the total voting power (cannot opt out)
valCConsAddr, _ := valC.GetConsAddr()
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valCConsAddr).Return(valC, true).AnyTimes()
valD := createStakingValidator(ctx, mocks, 4, 4) // 40% of the total voting power (cannot opt out)
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valCConsAddr).Return(valC, nil).AnyTimes()
valD := createStakingValidator(ctx, mocks, 4, 4, 4) // 40% of the total voting power (cannot opt out)
valDConsAddr, _ := valD.GetConsAddr()
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valDConsAddr).Return(valD, true).AnyTimes()
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, valDConsAddr).Return(valD, nil).AnyTimes()

mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{valA, valB, valC, valD}).AnyTimes()
mocks.MockStakingKeeper.EXPECT().GetLastValidators(ctx).Return([]stakingtypes.Validator{valA, valB, valC, valD}, nil).AnyTimes()

// opt in all validators
providerKeeper.SetOptedIn(ctx, chainID, types.NewProviderConsAddress(valAConsAddr))
Expand All @@ -150,10 +151,10 @@ func TestHandleOptOutFromTopNChain(t *testing.T) {
require.Error(t, providerKeeper.HandleOptOut(ctx, chainID, types.NewProviderConsAddress(valDConsAddr)))

// opting out a validator that cannot be found from a Top N chain should also return an error
notFoundValidator := createStakingValidator(ctx, mocks, 5, 5)
notFoundValidator := createStakingValidator(ctx, mocks, 5, 5, 5)
notFoundValidatorConsAddr, _ := notFoundValidator.GetConsAddr()
mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, notFoundValidatorConsAddr).
Return(stakingtypes.Validator{}, false)
Return(stakingtypes.Validator{}, stakingtypes.ErrNoValidatorFound)
require.Error(t, providerKeeper.HandleOptOut(ctx, chainID, types.NewProviderConsAddress(notFoundValidatorConsAddr)))
}

Expand All @@ -174,7 +175,7 @@ func TestHandleSetConsumerCommissionRate(t *testing.T) {
_, found := providerKeeper.GetConsumerCommissionRate(ctx, chainID, providerAddr)
require.False(t, found)

mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(math.LegacyZeroDec()).Times(1)
mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(math.LegacyZeroDec(), nil).Times(1)
require.NoError(t, providerKeeper.HandleSetConsumerCommissionRate(ctx, chainID, providerAddr, math.LegacyOneDec()))

// check that the commission rate is now set
Expand All @@ -184,7 +185,7 @@ func TestHandleSetConsumerCommissionRate(t *testing.T) {

// set minimum rate of 1/2
commissionRate := math.LegacyNewDec(1).Quo(math.LegacyNewDec(2))
mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(commissionRate).AnyTimes()
mocks.MockStakingKeeper.EXPECT().MinCommissionRate(ctx).Return(commissionRate, nil).AnyTimes()

// try to set a rate slightly below the minimum
require.Error(t, providerKeeper.HandleSetConsumerCommissionRate(
Expand All @@ -208,13 +209,13 @@ func TestOptInTopNValidators(t *testing.T) {
defer ctrl.Finish()

// create 4 validators with powers 1, 2, 3, and 1 respectively
valA := createStakingValidator(ctx, mocks, 1, 1)
valA := createStakingValidator(ctx, mocks, 1, 1, 1)
valAConsAddr, _ := valA.GetConsAddr()
valB := createStakingValidator(ctx, mocks, 2, 2)
valB := createStakingValidator(ctx, mocks, 2, 2, 2)
valBConsAddr, _ := valB.GetConsAddr()
valC := createStakingValidator(ctx, mocks, 3, 3)
valC := createStakingValidator(ctx, mocks, 3, 3, 3)
valCConsAddr, _ := valC.GetConsAddr()
valD := createStakingValidator(ctx, mocks, 4, 1)
valD := createStakingValidator(ctx, mocks, 4, 1, 4)
valDConsAddr, _ := valD.GetConsAddr()

// Start Test 1: opt in all validators with power >= 0
Expand Down Expand Up @@ -295,11 +296,11 @@ func TestComputeMinPowerToOptIn(t *testing.T) {
// 1 => 100%

bondedValidators := []stakingtypes.Validator{
createStakingValidator(ctx, mocks, 1, 5),
createStakingValidator(ctx, mocks, 2, 10),
createStakingValidator(ctx, mocks, 3, 3),
createStakingValidator(ctx, mocks, 4, 1),
createStakingValidator(ctx, mocks, 5, 6),
createStakingValidator(ctx, mocks, 1, 5, 1),
createStakingValidator(ctx, mocks, 2, 10, 2),
createStakingValidator(ctx, mocks, 3, 3, 3),
createStakingValidator(ctx, mocks, 4, 1, 4),
createStakingValidator(ctx, mocks, 5, 6, 5),
}

m, err := providerKeeper.ComputeMinPowerToOptIn(ctx, "chainID", bondedValidators, 100)
Expand Down Expand Up @@ -355,7 +356,7 @@ func TestFilterOptedInAndAllowAndDenylistedPredicate(t *testing.T) {
providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

validator := createStakingValidator(ctx, mocks, 0, 1)
validator := createStakingValidator(ctx, mocks, 0, 1, 1)
consAddr, _ := validator.GetConsAddr()
providerAddr := types.NewProviderConsAddress(consAddr)

Expand All @@ -365,7 +366,7 @@ func TestFilterOptedInAndAllowAndDenylistedPredicate(t *testing.T) {
require.True(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))

// create an allow list but do not add the validator `providerAddr` to it
validatorA := createStakingValidator(ctx, mocks, 1, 1)
validatorA := createStakingValidator(ctx, mocks, 1, 1, 2)
consAddrA, _ := validatorA.GetConsAddr()
providerKeeper.SetAllowlist(ctx, "chainID", types.NewProviderConsAddress(consAddrA))
require.False(t, providerKeeper.FilterOptedInAndAllowAndDenylistedPredicate(ctx, "chainID", providerAddr))
Expand Down
9 changes: 6 additions & 3 deletions x/ccv/provider/keeper/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"strconv"
"time"

stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"

clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types"
Expand Down Expand Up @@ -39,8 +41,8 @@ func (k Keeper) HandleConsumerAdditionProposal(ctx sdk.Context, proposal *types.
HistoricalEntries: proposal.HistoricalEntries,
DistributionTransmissionChannel: proposal.DistributionTransmissionChannel,
}
return k.HandleLegacyConsumerAdditionProposal(ctx, &p)

return k.HandleLegacyConsumerAdditionProposal(ctx, &p)
}

// Wrapper for the new proposal message MsgConsumerRemoval
Expand All @@ -50,8 +52,8 @@ func (k Keeper) HandleConsumerRemovalProposal(ctx sdk.Context, proposal *types.M
ChainId: proposal.ChainId,
StopTime: proposal.StopTime,
}
return k.HandleLegacyConsumerRemovalProposal(ctx, &p)

return k.HandleLegacyConsumerRemovalProposal(ctx, &p)
}

// Wrapper for the new proposal message MsgChangeRewardDenoms
Expand All @@ -61,6 +63,7 @@ func (k Keeper) HandleConsumerRewardDenomProposal(ctx sdk.Context, proposal *typ
DenomsToAdd: proposal.DenomsToAdd,
DenomsToRemove: proposal.DenomsToRemove,
}

return k.HandleLegacyConsumerRewardDenomProposal(ctx, &p)
}

Expand Down Expand Up @@ -260,7 +263,7 @@ func (k Keeper) MakeConsumerGenesis(
// get the bonded validators from the staking module
bondedValidators, err := k.stakingKeeper.GetLastValidators(ctx)
if err != nil {
return gen, nil, err
return gen, nil, errorsmod.Wrapf(stakingtypes.ErrNoValidatorFound, "error getting last bonded validators: %s", err)
}

if topN, found := k.GetTopN(ctx, chainID); found && topN > 0 {
Expand Down
Loading
Loading