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.

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

feat!: automatically opt in validators that vote Yes on consumer addition proposals #1629

Merged
merged 6 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
53 changes: 41 additions & 12 deletions testutil/keeper/mocks.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 55 additions & 1 deletion x/ccv/provider/keeper/hooks.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package keeper

import (
"cosmossdk.io/math"
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkgov "github.com/cosmos/cosmos-sdk/x/gov/types"
v1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
Expand Down Expand Up @@ -175,7 +175,61 @@ func (h Hooks) AfterProposalVotingPeriodEnded(ctx sdk.Context, proposalID uint64
func (h Hooks) AfterProposalDeposit(ctx sdk.Context, proposalID uint64, depositorAddr sdk.AccAddress) {
}

// AfterProposalVote opts in validators that vote YES (with 100% weight) on a `ConsumerAdditionProposal`. If a
// validator votes multiple times, only the last vote would be considered on whether the validator is opted in or not.
func (h Hooks) AfterProposalVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.AccAddress) {
validator, found := h.k.stakingKeeper.GetValidator(ctx, voterAddr.Bytes())
if !found {
return
}

consAddr, err := validator.GetConsAddr()
if err != nil {
h.k.Logger(ctx).Error("could not extract validator's consensus address",
"error", err.Error(),
"validator acc addr", voterAddr,
)
return
Copy link
Contributor Author

@insumity insumity Feb 8, 2024

Choose a reason for hiding this comment

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

Note: These are cases where we could also panic instead of simply returning. We could panic because those are exceptional cases that should never trigger (e.g., a validator with no consensus address). However, I choose to simply return because a potential buggy panic call could potentially lead so that no one is able to MsgVote on such a proposal.
For this, we only log such error cases.

}

chainID, found := h.k.GetProposedConsumerChain(ctx, proposalID)
if !found {
return
}

vote, found := h.k.govKeeper.GetVote(ctx, proposalID, voterAddr)
if !found {
h.k.Logger(ctx).Error("could not find vote for validator",
"validator acc addr", voterAddr,
"proposalID", proposalID,
)
return
}

if len(vote.Options) == 1 && vote.Options[0].Option == v1.VoteOption_VOTE_OPTION_YES {
insumity marked this conversation as resolved.
Show resolved Hide resolved
// only consider votes that vote YES with 100% weight
weight, err := sdk.NewDecFromStr(vote.Options[0].Weight)
if err != nil {
h.k.Logger(ctx).Error("could not extract decimal value from vote's weight",
"vote", vote,
"error", err.Error(),
)
return
}
if !weight.Equal(math.LegacyNewDec(1)) {
h.k.Logger(ctx).Error("single vote does not have a weight of 1",
"vote", vote,
)
return
}

// in the validator is already to-be-opted in, the `SetToBeOptedIn` is a no-op
h.k.SetToBeOptedIn(ctx, chainID, providertypes.NewProviderConsAddress(consAddr))
} else {
// if vote is not a YES vote with 100% weight, we delete the validator from to-be-opted in
// if the validator is not to-be-opted in, the `DeleteToBeOptedIn` is a no-op
h.k.DeleteToBeOptedIn(ctx, chainID, providertypes.NewProviderConsAddress(consAddr))
}
}

func (h Hooks) AfterProposalFailedMinDeposit(ctx sdk.Context, proposalID uint64) {
Expand Down
92 changes: 90 additions & 2 deletions x/ccv/provider/keeper/hooks_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package keeper_test

import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"github.com/cosmos/interchain-security/v4/x/ccv/provider/types"
"testing"
"time"

Expand Down Expand Up @@ -123,11 +127,13 @@ func TestAfterPropSubmissionAndVotingPeriodEnded(t *testing.T) {

k.Hooks().AfterProposalSubmission(ctx, prop.Id)
// verify that the proposal ID is created
require.NotEmpty(t, k.GetProposedConsumerChain(ctx, prop.Id))
_, found := k.GetProposedConsumerChain(ctx, prop.Id)
require.True(t, found)

k.Hooks().AfterProposalVotingPeriodEnded(ctx, prop.Id)
// verify that the proposal ID is deleted
require.Empty(t, k.GetProposedConsumerChain(ctx, prop.Id))
_, found = k.GetProposedConsumerChain(ctx, prop.Id)
require.False(t, found)
}

func TestGetConsumerAdditionLegacyPropFromProp(t *testing.T) {
Expand Down Expand Up @@ -223,3 +229,85 @@ func TestGetConsumerAdditionLegacyPropFromProp(t *testing.T) {
})
}
}

func TestAfterProposalVoteWithYesVote(t *testing.T) {
k, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

providerConsPubKey := ed25519.GenPrivKeyFromSecret([]byte{1}).PubKey()
pkAny, _ := codectypes.NewAnyWithValue(providerConsPubKey)
providerAddr := types.NewProviderConsAddress(providerConsPubKey.Address().Bytes())

options := []*v1.WeightedVoteOption{{Option: v1.OptionYes, Weight: "1"}}
k.SetProposedConsumerChain(ctx, "chainID", 1)

gomock.InOrder(
mocks.MockStakingKeeper.EXPECT().GetValidator(ctx, gomock.Any()).Return(
stakingtypes.Validator{ConsensusPubkey: pkAny}, true),
mocks.MockGovKeeper.EXPECT().GetVote(ctx, gomock.Any(), gomock.Any()).Return(
v1.Vote{ProposalId: 1, Voter: "voter", Options: options}, true,
),
)

require.False(t, k.IsToBeOptedIn(ctx, "chainID", providerAddr))
k.Hooks().AfterProposalVote(ctx, 1, sdk.AccAddress{})
require.True(t, k.IsToBeOptedIn(ctx, "chainID", providerAddr))
}

func TestAfterProposalVoteWithNoVote(t *testing.T) {
testCases := []struct {
name string
options []*v1.WeightedVoteOption
setup func(sdk.Context, []*v1.WeightedVoteOption, testkeeper.MockedKeepers, *codectypes.Any)
}{
{
"Weighted vote with 100% NO",
[]*v1.WeightedVoteOption{{Option: v1.OptionNo, Weight: "1"}},
func(ctx sdk.Context, options []*v1.WeightedVoteOption,
mocks testkeeper.MockedKeepers, pubKey *codectypes.Any) {
gomock.InOrder(
mocks.MockStakingKeeper.EXPECT().GetValidator(ctx, gomock.Any()).Return(
stakingtypes.Validator{ConsensusPubkey: pubKey}, true),
mocks.MockGovKeeper.EXPECT().GetVote(ctx, gomock.Any(), gomock.Any()).Return(
v1.Vote{ProposalId: 1, Voter: "voter", Options: options}, true,
),
)
},
},
{
"Weighted vote with 99.9% YES and 0.1% NO",
[]*v1.WeightedVoteOption{{Option: v1.OptionYes, Weight: "0.999"}, {Option: v1.OptionNo, Weight: "0.001"}},
func(ctx sdk.Context, options []*v1.WeightedVoteOption,
mocks testkeeper.MockedKeepers, pubKey *codectypes.Any) {
gomock.InOrder(
mocks.MockStakingKeeper.EXPECT().GetValidator(ctx, gomock.Any()).Return(
stakingtypes.Validator{ConsensusPubkey: pubKey}, true),
mocks.MockGovKeeper.EXPECT().GetVote(ctx, gomock.Any(), gomock.Any()).Return(
v1.Vote{ProposalId: 1, Voter: "voter", Options: options}, true,
),
)
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
k, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
defer ctrl.Finish()

providerConsPubKey := ed25519.GenPrivKeyFromSecret([]byte{1}).PubKey()
pkAny, _ := codectypes.NewAnyWithValue(providerConsPubKey)
providerAddr := types.NewProviderConsAddress(providerConsPubKey.Address().Bytes())

k.SetProposedConsumerChain(ctx, "chainID", 1)

tc.setup(ctx, tc.options, mocks, pkAny)

// set the validator to-be-opted in first to assert that a NO vote removes the validator from to-be-opted in
k.SetToBeOptedIn(ctx, "chainID", providerAddr)
require.True(t, k.IsToBeOptedIn(ctx, "chainID", providerAddr))
k.Hooks().AfterProposalVote(ctx, 1, sdk.AccAddress{})
require.False(t, k.IsToBeOptedIn(ctx, "chainID", providerAddr))
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a test with a weighted vote, just to safeguard against potential future regressions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, in case you think https://github.com/cosmos/interchain-security/pull/1629/files#r1482790758 should be taken into account, we also should add test cases for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one more test case and made the "No" vote test tabular.

5 changes: 3 additions & 2 deletions x/ccv/provider/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,10 @@ func (k Keeper) SetProposedConsumerChain(ctx sdk.Context, chainID string, propos
}

// GetProposedConsumerChain returns the proposed chainID for the given consumerAddition proposal ID.
func (k Keeper) GetProposedConsumerChain(ctx sdk.Context, proposalID uint64) string {
func (k Keeper) GetProposedConsumerChain(ctx sdk.Context, proposalID uint64) (string, bool) {
store := ctx.KVStore(k.storeKey)
return string(store.Get(types.ProposedConsumerChainKey(proposalID)))
consumerChain := store.Get(types.ProposedConsumerChainKey(proposalID))
return string(consumerChain), consumerChain != nil
}

// DeleteProposedConsumerChainInStore deletes the consumer chainID from store
Expand Down
6 changes: 3 additions & 3 deletions x/ccv/provider/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ func TestSetProposedConsumerChains(t *testing.T) {

for _, test := range tests {
providerKeeper.SetProposedConsumerChain(ctx, test.chainID, test.proposalID)
cID := providerKeeper.GetProposedConsumerChain(ctx, test.proposalID)
cID, _ := providerKeeper.GetProposedConsumerChain(ctx, test.proposalID)
require.Equal(t, cID, test.chainID)
}
}
Expand All @@ -574,9 +574,9 @@ func TestDeleteProposedConsumerChainInStore(t *testing.T) {
for _, test := range tests {
providerKeeper.SetProposedConsumerChain(ctx, test.chainID, test.proposalID)
providerKeeper.DeleteProposedConsumerChainInStore(ctx, test.deleteProposalID)
cID := providerKeeper.GetProposedConsumerChain(ctx, test.proposalID)
cID, found := providerKeeper.GetProposedConsumerChain(ctx, test.proposalID)
if test.ok {
require.Equal(t, cID, "")
require.False(t, found)
} else {
require.Equal(t, cID, test.chainID)
}
Expand Down
2 changes: 2 additions & 0 deletions x/ccv/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,4 +151,6 @@ type ScopedKeeper interface {

type GovKeeper interface {
GetProposal(ctx sdk.Context, proposalID uint64) (v1.Proposal, bool)
GetProposals(ctx sdk.Context) (proposals v1.Proposals)
GetVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.AccAddress) (vote v1.Vote, found bool)
}
Loading