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

fix(group)!: Don't re-tally proposals after VP end #14071

Merged
merged 10 commits into from Nov 30, 2022
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -101,6 +101,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
### State Machine Breaking

* (x/group) [#13742](https://github.com/cosmos/cosmos-sdk/pull/13742) Migrate group policy account from module accounts to base account.
* (x/group) [#14071](https://github.com/cosmos/cosmos-sdk/pull/14071) Don't re-tally proposal after voting period end if they have been marked as ACCEPTED or REJECTED.
* (codec) [#13307](https://github.com/cosmos/cosmos-sdk/pull/13307) Register all modules' `Msg`s with group's ModuleCdc so that Amino sign bytes are correctly generated.
* (codec) [#13196](https://github.com/cosmos/cosmos-sdk/pull/13196) Register all modules' `Msg`s with gov's ModuleCdc so that Amino sign bytes are correctly generated.
* (group) [#13592](https://github.com/cosmos/cosmos-sdk/pull/13592) Fix group types registration with Amino.
Expand Down
4 changes: 3 additions & 1 deletion x/group/keeper/keeper.go
Expand Up @@ -407,7 +407,7 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
if err := k.pruneVotes(ctx, proposalID); err != nil {
return err
}
} else {
} else if proposal.Status == group.PROPOSAL_STATUS_SUBMITTED {
if err := k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo); err != nil {
return sdkerrors.Wrap(err, "doTallyAndUpdate")
}
Expand All @@ -416,6 +416,8 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
return sdkerrors.Wrap(err, "proposal update")
}
}
// Note: We do nothing if the proposal has been marked as ACCEPTED or
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about just logging so that in the future we can know if it was accepted or rejected given this can easily slip the mind since we aren't doing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By logging you mean a ctx.Logger().Log? Isn't it too much to show one additional line in the CLI on each block?

// REJECTED.
}
return nil
}
72 changes: 72 additions & 0 deletions x/group/keeper/keeper_test.go
Expand Up @@ -3263,3 +3263,75 @@ func (s *TestSuite) TestTallyProposalsAtVPEnd() {
s.Require().NoError(s.groupKeeper.TallyProposalsAtVPEnd(ctx))
s.NotPanics(func() { module.EndBlocker(ctx, s.groupKeeper) })
}

// TestTallyProposalsAtVPEnd_GroupMemberLeaving test that the node doesn't
// panic if a member leaves after the voting period end.
func (s *TestSuite) TestTallyProposalsAtVPEnd_GroupMemberLeaving() {
addrs := s.addrs
addr1 := addrs[0]
addr2 := addrs[1]
addr3 := addrs[2]
votingPeriod := time.Duration(4 * time.Minute)
minExecutionPeriod := votingPeriod + group.DefaultConfig().MaxExecutionPeriod

groupMsg := &group.MsgCreateGroupWithPolicy{
Admin: addr1.String(),
Members: []group.MemberRequest{
{Address: addr1.String(), Weight: "0.3"},
{Address: addr2.String(), Weight: "7"},
{Address: addr3.String(), Weight: "0.6"},
},
}
policy := group.NewThresholdDecisionPolicy(
"3",
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)

// group members vote
_, err = s.groupKeeper.Vote(s.ctx, &group.MsgVote{
ProposalId: proposalRes.ProposalId,
Voter: addr1.String(),
Option: group.VOTE_OPTION_NO,
})
s.Require().NoError(err)
_, err = s.groupKeeper.Vote(s.ctx, &group.MsgVote{
ProposalId: proposalRes.ProposalId,
Voter: addr2.String(),
Option: group.VOTE_OPTION_NO,
})
s.Require().NoError(err)

// move forward in time
ctx := s.sdkCtx.WithBlockTime(s.sdkCtx.BlockTime().Add(votingPeriod + 1))

// Tally the result. This saves the tally result to state.
s.Require().NoError(s.groupKeeper.TallyProposalsAtVPEnd(ctx))
s.NotPanics(func() { module.EndBlocker(ctx, s.groupKeeper) })

// member 2 (high weight) leaves group.
_, err = s.groupKeeper.LeaveGroup(ctx, &group.MsgLeaveGroup{
Address: addr2.String(),
GroupId: groupRes.GroupId,
})
s.Require().NoError(err)

s.Require().NoError(s.groupKeeper.TallyProposalsAtVPEnd(ctx))
s.NotPanics(func() { module.EndBlocker(ctx, s.groupKeeper) })
}