diff --git a/CHANGELOG.md b/CHANGELOG.md index 817a82908812..0cc22b659bf8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -164,7 +164,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/gov) [#13160](https://github.com/cosmos/cosmos-sdk/pull/13160) Remove custom marshaling of proposl and voteoption. * (types) [#13430](https://github.com/cosmos/cosmos-sdk/pull/13430) Remove unused code `ResponseCheckTx` and `ResponseDeliverTx` * (store) [#13529](https://github.com/cosmos/cosmos-sdk/pull/13529) Add method `LatestVersion` to `MultiStore` interface, add method `SetQueryMultiStore` to baesapp to support alternative `MultiStore` implementation for query service. -* (pruning) [#13609]](https://github.com/cosmos/cosmos-sdk/pull/13609) Move pruning package to be under store package +* (pruning) [#13609](https://github.com/cosmos/cosmos-sdk/pull/13609) Move pruning package to be under store package * [#13794](https://github.com/cosmos/cosmos-sdk/pull/13794) Most methods on `types/module.AppModule` have been moved to extension interfaces. `module.Manager.Modules` is now of type `map[string]interface{}` to support in parallel the new `cosmossdk.io/core/appmodule.AppModule` API. @@ -176,9 +176,11 @@ extension interfaces. `module.Manager.Modules` is now of type `map[string]interf ### Bug Fixes +* (x/group) [#13869](https://github.com/cosmos/cosmos-sdk/pull/13869) Fix issue when tallying group proposal votes. +* (x/group) [#13869](https://github.com/cosmos/cosmos-sdk/pull/13869) Group members weight must be positive and a finite number. * (x/auth) [#13838](https://github.com/cosmos/cosmos-sdk/pull/13838) Fix calling `String()` and `MarshalYAML` panics when pubkey is set on a `BaseAccount`. * (rosetta) [#13583](https://github.com/cosmos/cosmos-sdk/pull/13583) Misc fixes for cosmos-rosetta. -* (x/evidence) [#13740](https://github.com/cosmos/cosmos-sdk/pull/13740) Fix evidence query API to decode the hash properly. +* (x/evidence) [#13740](https://github.com/cosmos/cosmos-sdk/pull/13740) Fix evidence query API to decode the hash properly. * (bank) [#13691](https://github.com/cosmos/cosmos-sdk/issues/13691) Fix unhandled error for vesting account transfers, when total vesting amount exceeds total balance. * [#13553](https://github.com/cosmos/cosmos-sdk/pull/13553) Ensure all parameter validation for decimal types handles nil decimal values. * [#13145](https://github.com/cosmos/cosmos-sdk/pull/13145) Fix panic when calling `String()` to a Record struct type. diff --git a/x/group/internal/math/dec.go b/x/group/internal/math/dec.go index 07d789e4cb46..3755304ccacd 100644 --- a/x/group/internal/math/dec.go +++ b/x/group/internal/math/dec.go @@ -1,4 +1,4 @@ -// Package math provides helper functions for doing mathematical calculations and parsing for the ecocredit module. +// Package math provides helper functions for doing mathematical calculations and parsing for the group module. package math import ( @@ -46,11 +46,22 @@ func (x Dec) IsPositive() bool { return !x.dec.Negative && !x.dec.IsZero() } +func (x Dec) IsFinite() bool { + return x.dec.Form != apd.Finite +} + +// NewDecFromString returns a new Dec from a string +// It only support finite numbers, not NaN, +Inf, -Inf func NewDecFromString(s string) (Dec, error) { d, _, err := apd.NewFromString(s) if err != nil { return Dec{}, errors.ErrInvalidDecString.Wrap(err.Error()) } + + if d.Form != apd.Finite { + return Dec{}, errors.ErrInvalidDecString.Wrapf("expected a finite decimal, got %s", s) + } + return Dec{*d}, nil } diff --git a/x/group/internal/math/dec_test.go b/x/group/internal/math/dec_test.go index 4c9fcc6cc656..c524716934ca 100644 --- a/x/group/internal/math/dec_test.go +++ b/x/group/internal/math/dec_test.go @@ -54,6 +54,14 @@ func TestDec(t *testing.T) { require.NoError(t, err) minusFivePointZero, err := NewDecFromString("-5.0") require.NoError(t, err) + _, err = NewDecFromString("inf") + require.Error(t, err) + _, err = NewDecFromString("Infinite") + require.Error(t, err) + _, err = NewDecFromString("foo") + require.Error(t, err) + _, err = NewDecFromString("NaN") + require.Error(t, err) res, err := two.Add(zero) require.NoError(t, err) diff --git a/x/group/keeper/keeper.go b/x/group/keeper/keeper.go index 234a0ba359c7..633edf0be580 100644 --- a/x/group/keeper/keeper.go +++ b/x/group/keeper/keeper.go @@ -382,7 +382,6 @@ func (k Keeper) PruneProposals(ctx sdk.Context) error { // TallyProposalsAtVPEnd iterates over all proposals whose voting period // has ended, tallies their votes, prunes them, and updates the proposal's // `FinalTallyResult` field. - func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error { proposals, err := k.proposalsByVPEnd(ctx, ctx.BlockTime()) if err != nil { @@ -400,6 +399,16 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error { return sdkerrors.Wrap(err, "group") } + // skip the invalid decision policies that could have been created before + // https://github.com/cosmos/cosmos-sdk/pull/13869 + decisionPolicy, err := policyInfo.GetDecisionPolicy() + if err != nil { + return sdkerrors.Wrap(err, "decision policy") + } + if err := decisionPolicy.Validate(electorate, k.config); err != nil { + continue + } + proposalID := proposal.Id if proposal.Status == group.PROPOSAL_STATUS_ABORTED || proposal.Status == group.PROPOSAL_STATUS_WITHDRAWN { if err := k.pruneProposal(ctx, proposalID); err != nil { @@ -409,8 +418,7 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error { return err } } else { - err = k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo) - if err != nil { + if err := k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo); err != nil { return sdkerrors.Wrap(err, "doTallyAndUpdate") } diff --git a/x/group/keeper/keeper_test.go b/x/group/keeper/keeper_test.go index a67390083793..6b23824c8a26 100644 --- a/x/group/keeper/keeper_test.go +++ b/x/group/keeper/keeper_test.go @@ -253,6 +253,26 @@ func (s *TestSuite) TestCreateGroup() { }, expErr: true, }, + "invalid member weight - Inf": { + req: &group.MsgCreateGroup{ + Admin: addr1.String(), + Members: []group.MemberRequest{{ + Address: addr3.String(), + Weight: "inf", + }}, + }, + expErr: true, + }, + "invalid member weight - NaN": { + req: &group.MsgCreateGroup{ + Admin: addr1.String(), + Members: []group.MemberRequest{{ + Address: addr3.String(), + Weight: "NaN", + }}, + }, + expErr: true, + }, } var seq uint32 = 1 @@ -899,6 +919,19 @@ func (s *TestSuite) TestCreateGroupWithPolicy() { ), expErr: false, }, + "minExecutionPeriod = votingPeriod + MaxExecutionPeriod": { + req: &group.MsgCreateGroupWithPolicy{ + Admin: addr1.String(), + Members: members, + GroupPolicyAsAdmin: false, + }, + policy: group.NewThresholdDecisionPolicy( + "1", + time.Second, + time.Second+group.DefaultConfig().MaxExecutionPeriod, + ), + expErr: true, + }, } for msg, spec := range specs { @@ -1862,6 +1895,67 @@ func (s *TestSuite) TestWithdrawProposal() { } } +func (s *TestSuite) TestTallyProposalsAtVPEnd() { + // panics before https://github.com/cosmos/cosmos-sdk/pull/13869 fixes + // we need to skip the test because extra validation was added making the invalid threshold policy + // impossible to create. However, we still want to make sure that the panic will not be triggered for the already existing invalid policies. + s.T().Skip() + + addrs := s.addrs + addr1 := addrs[0] + addr2 := addrs[1] + votingPeriod := time.Duration(4 * time.Minute) + minExecutionPeriod := votingPeriod + group.DefaultConfig().MaxExecutionPeriod + + groupMsg := &group.MsgCreateGroupWithPolicy{ + Admin: addr1.String(), + Members: []group.MemberRequest{ + {Address: addr1.String(), Weight: "1"}, + {Address: addr2.String(), Weight: "1"}, + }, + } + policy := group.NewThresholdDecisionPolicy( + "1", + votingPeriod, + minExecutionPeriod, + ) + s.Require().NoError(groupMsg.SetDecisionPolicy(policy)) + + s.setNextAccount() + groupRes, err := s.groupKeeper.CreateGroupWithPolicy(s.ctx, groupMsg) + s.Require().NoError(err) + accountAddr := groupRes.GetGroupPolicyAddress() + groupPolicy, err := sdk.AccAddressFromBech32(accountAddr) + s.Require().NoError(err) + s.Require().NotNil(groupPolicy) + + proposalRes, err := s.groupKeeper.SubmitProposal(s.ctx, &group.MsgSubmitProposal{ + GroupPolicyAddress: accountAddr, + Proposers: []string{addr1.String()}, + Messages: nil, + }) + s.Require().NoError(err) + + _, err = s.groupKeeper.Vote(s.ctx, &group.MsgVote{ + ProposalId: proposalRes.ProposalId, + Voter: addr1.String(), + Option: group.VOTE_OPTION_YES, + }) + s.Require().NoError(err) + + // move forward in time + ctx := s.sdkCtx.WithBlockTime(s.sdkCtx.BlockTime().Add(votingPeriod + 1)) + + result, err := s.groupKeeper.TallyResult(ctx, &group.QueryTallyResultRequest{ + ProposalId: proposalRes.ProposalId, + }) + s.Require().Equal("1", result.Tally.YesCount) + s.Require().NoError(err) + + s.Require().NoError(s.groupKeeper.TallyProposalsAtVPEnd(ctx)) + s.NotPanics(func() { module.EndBlocker(ctx, s.groupKeeper) }) +} + func (s *TestSuite) TestVote() { addrs := s.addrs addr1 := addrs[0] diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go index fd144525cc34..2320b417ca4c 100644 --- a/x/group/keeper/msg_server.go +++ b/x/group/keeper/msg_server.go @@ -687,15 +687,13 @@ func (k Keeper) doTallyAndUpdate(ctx sdk.Context, p *group.Proposal, electorate sinceSubmission := ctx.BlockTime().Sub(p.SubmitTime) // duration passed since proposal submission. result, err := policy.Allow(tallyResult, electorate.TotalWeight, sinceSubmission) - // If the result was final (i.e. enough votes to pass) or if the voting - // period ended, then we consider the proposal as final. - isFinal := result.Final || ctx.BlockTime().After(p.VotingPeriodEnd) - - switch { - case err != nil: + if err != nil { return sdkerrors.Wrap(err, "policy allow") + } - case isFinal: + // If the result was final (i.e. enough votes to pass) or if the voting + // period ended, then we consider the proposal as final. + if isFinal := result.Final || ctx.BlockTime().After(p.VotingPeriodEnd); isFinal { if err := k.pruneVotes(ctx, p.Id); err != nil { return err } diff --git a/x/group/module/abci.go b/x/group/module/abci.go index 4e3817f804b1..22809b61d60e 100644 --- a/x/group/module/abci.go +++ b/x/group/module/abci.go @@ -9,12 +9,8 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) { if err := k.TallyProposalsAtVPEnd(ctx); err != nil { panic(err) } - pruneProposals(ctx, k) -} -func pruneProposals(ctx sdk.Context, k keeper.Keeper) { - err := k.PruneProposals(ctx) - if err != nil { + if err := k.PruneProposals(ctx); err != nil { panic(err) } } diff --git a/x/group/msgs.go b/x/group/msgs.go index 4261b44a2478..02e4a6102681 100644 --- a/x/group/msgs.go +++ b/x/group/msgs.go @@ -294,10 +294,7 @@ func (m MsgCreateGroupPolicy) GetSignBytes() []byte { // GetSigners returns the expected signers for a MsgCreateGroupPolicy. func (m MsgCreateGroupPolicy) GetSigners() []sdk.AccAddress { - admin, err := sdk.AccAddressFromBech32(m.Admin) - if err != nil { - panic(err) - } + admin := sdk.MustAccAddressFromBech32(m.Admin) return []sdk.AccAddress{admin} } diff --git a/x/group/types.go b/x/group/types.go index ce01ff7b267d..7ad22a654e3d 100644 --- a/x/group/types.go +++ b/x/group/types.go @@ -136,8 +136,8 @@ func (p *ThresholdDecisionPolicy) Validate(g GroupInfo, config Config) error { return sdkerrors.Wrap(err, "group total weight") } - if p.Windows.MinExecutionPeriod > p.Windows.VotingPeriod+config.MaxExecutionPeriod { - return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be smaller than voting_period + max_execution_period") + if p.Windows.MinExecutionPeriod >= p.Windows.VotingPeriod+config.MaxExecutionPeriod { + return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be strictly smaller than voting_period + max_execution_period") } return nil } @@ -171,8 +171,8 @@ func (p PercentageDecisionPolicy) ValidateBasic() error { } func (p *PercentageDecisionPolicy) Validate(g GroupInfo, config Config) error { - if p.Windows.MinExecutionPeriod > p.Windows.VotingPeriod+config.MaxExecutionPeriod { - return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be smaller than voting_period + max_execution_period") + if p.Windows.MinExecutionPeriod >= p.Windows.VotingPeriod+config.MaxExecutionPeriod { + return sdkerrors.Wrap(errors.ErrInvalid, "min_execution_period should be strictly smaller than voting_period + max_execution_period") } return nil }