Skip to content

Commit

Permalink
refactor(x/gov): Audit gov changes (backport #21454) (#21567)
Browse files Browse the repository at this point in the history 
Co-authored-by: son trinh <trinhleson2000@gmail.com>
  • Loading branch information
@mergify @sontrinh16
mergify[bot] and sontrinh16 authored Sep 5, 2024
1 parent 68ec945 commit 9adce9e
Show file tree
Hide file tree
Showing 11 changed files with 357 additions and 90 deletions.
14 changes: 2 additions & 12 deletions x/gov/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2204,8 +2204,6 @@ Example Output:

The `params` endpoint allows users to query all parameters for the `gov` module.

<!-- TODO: #10197 Querying governance params outputs nil values -->

Using legacy v1beta1:

```bash
Expand All @@ -2225,16 +2223,6 @@ Example Output:
"voting_params": {
"voting_period": "172800s"
},
"deposit_params": {
"min_deposit": [
],
"max_deposit_period": "0s"
},
"tally_params": {
"quorum": "0.000000000000000000",
"threshold": "0.000000000000000000",
"veto_threshold": "0.000000000000000000"
}
}
```

Expand Down Expand Up @@ -2270,6 +2258,8 @@ Example Output:
}
```

Note: `params_type` are deprecated in v1 since all params are stored in Params.

#### deposits

The `deposits` endpoint allows users to query a deposit for a given proposal from a given depositor.
Expand Down
192 changes: 190 additions & 2 deletions x/gov/client/utils/query_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package utils_test

import (
"context"
"fmt"
"strconv"
"strings"
"testing"

"github.com/cometbft/cometbft/rpc/client/mock"
Expand All @@ -13,6 +16,7 @@ import (
"cosmossdk.io/x/gov"
"cosmossdk.io/x/gov/client/utils"
v1 "cosmossdk.io/x/gov/types/v1"
"cosmossdk.io/x/gov/types/v1beta1"

"github.com/cosmos/cosmos-sdk/client"
codectestutil "github.com/cosmos/cosmos-sdk/codec/testutil"
Expand All @@ -24,9 +28,12 @@ type TxSearchMock struct {
txConfig client.TxConfig
mock.Client
txs []cmttypes.Tx

// use for filter tx with query conditions
msgsSet [][]sdk.Msg
}

func (mock TxSearchMock) TxSearch(ctx context.Context, query string, prove bool, page, perPage *int, orderBy string) (*coretypes.ResultTxSearch, error) {
func (mock TxSearchMock) TxSearch(ctx context.Context, query string, _ bool, page, perPage *int, _ string) (*coretypes.ResultTxSearch, error) {
if page == nil {
*page = 0
}
Expand All @@ -40,8 +47,11 @@ func (mock TxSearchMock) TxSearch(ctx context.Context, query string, prove bool,
// nil result with nil error crashes utils.QueryTxsByEvents
return &coretypes.ResultTxSearch{}, nil
}
txs, err := mock.filterTxs(query, start, end)
if err != nil {
return nil, err
}

txs := mock.txs[start:end]
rst := &coretypes.ResultTxSearch{Txs: make([]*coretypes.ResultTx, len(txs)), TotalCount: len(txs)}
for i := range txs {
rst.Txs[i] = &coretypes.ResultTx{Tx: txs[i]}
Expand All @@ -54,6 +64,74 @@ func (mock TxSearchMock) Block(ctx context.Context, height *int64) (*coretypes.R
return &coretypes.ResultBlock{Block: &cmttypes.Block{}}, nil
}

// mock applying the query string in TxSearch
func (mock TxSearchMock) filterTxs(query string, start, end int) ([]cmttypes.Tx, error) {
filterTxs := []cmttypes.Tx{}
proposalIdStr, senderAddr := getQueryAttributes(query)
if senderAddr != "" {
proposalId, err := strconv.ParseUint(proposalIdStr, 10, 64)
if err != nil {
return nil, err
}

for i, msgs := range mock.msgsSet {
for _, msg := range msgs {
if voteMsg, ok := msg.(*v1beta1.MsgVote); ok {
if voteMsg.Voter == senderAddr && voteMsg.ProposalId == proposalId {
filterTxs = append(filterTxs, mock.txs[i])
continue
}
}

if voteMsg, ok := msg.(*v1.MsgVote); ok {
if voteMsg.Voter == senderAddr && voteMsg.ProposalId == proposalId {
filterTxs = append(filterTxs, mock.txs[i])
continue
}
}

if voteWeightedMsg, ok := msg.(*v1beta1.MsgVoteWeighted); ok {
if voteWeightedMsg.Voter == senderAddr && voteWeightedMsg.ProposalId == proposalId {
filterTxs = append(filterTxs, mock.txs[i])
continue
}
}

if voteWeightedMsg, ok := msg.(*v1.MsgVoteWeighted); ok {
if voteWeightedMsg.Voter == senderAddr && voteWeightedMsg.ProposalId == proposalId {
filterTxs = append(filterTxs, mock.txs[i])
continue
}
}
}
}
} else {
filterTxs = append(filterTxs, mock.txs...)
}

if len(filterTxs) < end {
return filterTxs, nil
}

return filterTxs[start:end], nil
}

// getQueryAttributes extracts value from query string
func getQueryAttributes(q string) (proposalId, senderAddr string) {
splitStr := strings.Split(q, " OR ")
if len(splitStr) >= 2 {
keySender := strings.Trim(splitStr[1], ")")
senderAddr = strings.Trim(strings.Split(keySender, "=")[1], "'")

keyProposal := strings.Split(q, " AND ")[0]
proposalId = strings.Trim(strings.Split(keyProposal, "=")[1], "'")
} else {
proposalId = strings.Trim(strings.Split(splitStr[0], "=")[1], "'")
}

return proposalId, senderAddr
}

func TestGetPaginatedVotes(t *testing.T) {
cdcOpts := codectestutil.CodecOptions{}
encCfg := moduletestutil.MakeTestEncodingConfig(cdcOpts, gov.AppModule{})
Expand Down Expand Up @@ -178,3 +256,113 @@ func TestGetPaginatedVotes(t *testing.T) {
})
}
}

func TestGetSingleVote(t *testing.T) {
cdcOpts := codectestutil.CodecOptions{}
encCfg := moduletestutil.MakeTestEncodingConfig(cdcOpts, gov.AppModule{})

type testCase struct {
description string
msgs [][]sdk.Msg
votes []v1.Vote
expErr string
}
acc1 := make(sdk.AccAddress, 20)
acc1[0] = 1
acc1Str, err := cdcOpts.GetAddressCodec().BytesToString(acc1)
require.NoError(t, err)
acc2 := make(sdk.AccAddress, 20)
acc2[0] = 2
acc2Str, err := cdcOpts.GetAddressCodec().BytesToString(acc2)
require.NoError(t, err)
acc1Msgs := []sdk.Msg{
v1.NewMsgVote(acc1Str, 0, v1.OptionYes, ""),
v1.NewMsgVote(acc1Str, 0, v1.OptionYes, ""),
v1.NewMsgDeposit(acc1Str, 0, sdk.NewCoins(sdk.NewCoin("stake", sdkmath.NewInt(10)))), // should be ignored
}
acc2Msgs := []sdk.Msg{
v1.NewMsgVote(acc2Str, 0, v1.OptionYes, ""),
v1.NewMsgVoteWeighted(acc2Str, 0, v1.NewNonSplitVoteOption(v1.OptionYes), ""),
v1beta1.NewMsgVoteWeighted(acc2Str, 0, v1beta1.NewNonSplitVoteOption(v1beta1.OptionYes)),
}
for _, tc := range []testCase{
{
description: "no vote found: no msgVote",
msgs: [][]sdk.Msg{
acc1Msgs[:1],
},
votes: []v1.Vote{},
expErr: "did not vote on proposalID",
},
{
description: "no vote found: wrong proposal ID",
msgs: [][]sdk.Msg{
acc1Msgs[:1],
},
votes: []v1.Vote{},
expErr: "did not vote on proposalID",
},
{
description: "query 2 voter vote",
msgs: [][]sdk.Msg{
acc1Msgs,
acc2Msgs[:1],
},
votes: []v1.Vote{
v1.NewVote(0, acc1Str, v1.NewNonSplitVoteOption(v1.OptionYes), ""),
v1.NewVote(0, acc2Str, v1.NewNonSplitVoteOption(v1.OptionYes), ""),
},
},
{
description: "query 2 voter vote with v1beta1",
msgs: [][]sdk.Msg{
acc1Msgs,
acc2Msgs[2:],
},
votes: []v1.Vote{
v1.NewVote(0, acc1Str, v1.NewNonSplitVoteOption(v1.OptionYes), ""),
v1.NewVote(0, acc2Str, v1.NewNonSplitVoteOption(v1.OptionYes), ""),
},
},
} {
tc := tc

t.Run(tc.description, func(t *testing.T) {
marshaled := make([]cmttypes.Tx, len(tc.msgs))
cli := TxSearchMock{txs: marshaled, txConfig: encCfg.TxConfig, msgsSet: tc.msgs}
clientCtx := client.Context{}.
WithLegacyAmino(encCfg.Amino).
WithClient(cli).
WithTxConfig(encCfg.TxConfig).
WithAddressCodec(cdcOpts.GetAddressCodec()).
WithCodec(encCfg.Codec)

for i := range tc.msgs {
txBuilder := clientCtx.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(tc.msgs[i]...)
require.NoError(t, err)

tx, err := clientCtx.TxConfig.TxEncoder()(txBuilder.GetTx())
require.NoError(t, err)
marshaled[i] = tx
}

// testing query single vote
for i, v := range tc.votes {
accAddr, err := clientCtx.AddressCodec.StringToBytes(v.Voter)
require.NoError(t, err)
voteParams := utils.QueryVoteParams{ProposalID: 0, Voter: accAddr}
voteData, err := utils.QueryVoteByTxQuery(clientCtx, voteParams)
if tc.expErr != "" {
require.Error(t, err)
require.True(t, strings.Contains(err.Error(), tc.expErr))
continue
}
require.NoError(t, err)
vote := v1.Vote{}
require.NoError(t, clientCtx.Codec.UnmarshalJSON(voteData, &vote))
require.Equal(t, v, vote, fmt.Sprintf("vote should be equal at entry %v", i))
}
})
}
}
13 changes: 0 additions & 13 deletions x/gov/common_test.go
View file

This file was deleted.

16 changes: 6 additions & 10 deletions x/gov/keeper/deposit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ func (k Keeper) AddDeposit(ctx context.Context, proposalID uint64, depositorAddr
activatedVotingPeriod = true
}

addr, err := k.authKeeper.AddressCodec().BytesToString(depositorAddr)
if err != nil {
return false, err
}

// Add or update deposit object
deposit, err := k.Deposits.Get(ctx, collections.Join(proposalID, depositorAddr))
switch {
Expand All @@ -165,10 +170,6 @@ func (k Keeper) AddDeposit(ctx context.Context, proposalID uint64, depositorAddr
deposit.Amount = sdk.NewCoins(deposit.Amount...).Add(depositAmount...)
case errors.IsOf(err, collections.ErrNotFound):
// deposit doesn't exist
addr, err := k.authKeeper.AddressCodec().BytesToString(depositorAddr)
if err != nil {
return false, err
}
deposit = v1.NewDeposit(proposalID, addr, depositAmount)
default:
// failed to get deposit
Expand All @@ -181,14 +182,9 @@ func (k Keeper) AddDeposit(ctx context.Context, proposalID uint64, depositorAddr
return false, err
}

depositorStrAddr, err := k.authKeeper.AddressCodec().BytesToString(depositorAddr)
if err != nil {
return false, err
}

if err := k.EventService.EventManager(ctx).EmitKV(
types.EventTypeProposalDeposit,
event.NewAttribute(types.AttributeKeyDepositor, depositorStrAddr),
event.NewAttribute(types.AttributeKeyDepositor, addr),
event.NewAttribute(sdk.AttributeKeyAmount, depositAmount.String()),
event.NewAttribute(types.AttributeKeyProposalID, fmt.Sprintf("%d", proposalID)),
); err != nil {
Expand Down
39 changes: 27 additions & 12 deletions x/gov/keeper/proposal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,15 @@ func (k Keeper) SubmitProposal(ctx context.Context, messages []sdk.Msg, metadata
return v1.Proposal{}, err
}

proposerAddr, err := k.authKeeper.AddressCodec().BytesToString(proposer)
if err != nil {
return v1.Proposal{}, err
}

// additional checks per proposal types
switch proposalType {
case v1.ProposalType_PROPOSAL_TYPE_OPTIMISTIC:
proposerStr, _ := k.authKeeper.AddressCodec().BytesToString(proposer)
if len(params.OptimisticAuthorizedAddresses) > 0 && !slices.Contains(params.OptimisticAuthorizedAddresses, proposerStr) {
if len(params.OptimisticAuthorizedAddresses) > 0 && !slices.Contains(params.OptimisticAuthorizedAddresses, proposerAddr) {
return v1.Proposal{}, errorsmod.Wrap(types.ErrInvalidProposer, "proposer is not authorized to submit optimistic proposal")
}
case v1.ProposalType_PROPOSAL_TYPE_MULTIPLE_CHOICE:
Expand All @@ -43,20 +47,35 @@ func (k Keeper) SubmitProposal(ctx context.Context, messages []sdk.Msg, metadata
msgs := make([]string, 0, len(messages)) // will hold a string slice of all Msg type URLs.

// Loop through all messages and confirm that each has a handler and the gov module account as the only signer
var currentMessagedBasedParams *v1.MessageBasedParams
for _, msg := range messages {
msgs = append(msgs, sdk.MsgTypeURL(msg))

// check if any of the message has message based params
hasMessagedBasedParams, err := k.MessageBasedParams.Has(ctx, sdk.MsgTypeURL(msg))
hasMessagedBasedParams := true
messagedBasedParams, err := k.MessageBasedParams.Get(ctx, sdk.MsgTypeURL(msg))
if err != nil {
return v1.Proposal{}, err
if !errorsmod.IsOf(err, collections.ErrNotFound) {
return v1.Proposal{}, err
}

hasMessagedBasedParams = false
}

if hasMessagedBasedParams {
// TODO(@julienrbrt), in the future, we can check if all messages have the same params
// and if so, we can allow the proposal.
if len(messages) > 1 {
return v1.Proposal{}, errorsmod.Wrap(types.ErrInvalidProposalMsg, "cannot submit multiple messages proposal with message based params")
// set initial value for currentMessagedBasedParams
if currentMessagedBasedParams == nil {
currentMessagedBasedParams = &messagedBasedParams
}

// check if newly fetched messagedBasedParams is different from the previous fetched params
isEqual, err := currentMessagedBasedParams.Equal(&messagedBasedParams)
if err != nil {
return v1.Proposal{}, err
}

if len(messages) > 1 && !isEqual {
return v1.Proposal{}, errorsmod.Wrap(types.ErrInvalidProposalMsg, "cannot submit multiple messages proposal with different message based params")
}

if proposalType != v1.ProposalType_PROPOSAL_TYPE_STANDARD {
Expand Down Expand Up @@ -131,10 +150,6 @@ func (k Keeper) SubmitProposal(ctx context.Context, messages []sdk.Msg, metadata
return v1.Proposal{}, err
}

proposerAddr, err := k.authKeeper.AddressCodec().BytesToString(proposer)
if err != nil {
return v1.Proposal{}, err
}
submitTime := k.HeaderService.HeaderInfo(ctx).Time
proposal, err := v1.NewProposal(messages, proposalID, submitTime, submitTime.Add(*params.MaxDepositPeriod), metadata, title, summary, proposerAddr, proposalType)
if err != nil {
Expand Down
Loading

0 comments on commit 9adce9e

Please sign in to comment.