certusone
/
cosmos-sdk
mirror of https://github.com/certusone/cosmos-sdk.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

fix: prune Withdrawn and Aborted group proposals (#11560) 

## Description

Closes: #11531 

- loop through props during group policy update, update status field
- keep group_policy.version and group.version (not used in state machine, just for tracking)
- prune withdrawn/aborted proposals on voting_period_end (for users to be able to query)
  - in same function at voting_period_end
  - pruning should happen before tallying

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
This commit is contained in:
likhita-809 2022-04-13 14:35:05 +05:30 committed by GitHub
parent 1fe59eb22a
commit dd2b9e1110
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 208 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
simapp/helpers/test_helpers.go
View File

@ -14,7 +14,7 @@ import (
// SimAppChainID hardcoded chainID for simulation
const (
DefaultGenTxGas = 1000000
DefaultGenTxGas = 10000000
SimAppChainID = "simulation-app"
)

3
x/group/client/testutil/tx.go
View File

@ -1103,6 +1103,7 @@ func (s *IntegrationTestSuite) TestTxUpdateGroupPolicyAdmin() {
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
fmt.Sprintf("--%s=%d", flags.FlagGas, 300000),
}
testCases := []struct {
@ -1206,6 +1207,7 @@ func (s *IntegrationTestSuite) TestTxUpdateGroupPolicyDecisionPolicy() {
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
fmt.Sprintf("--%s=%d", flags.FlagGas, 300000),
}
testCases := []struct {
@ -1354,6 +1356,7 @@ func (s *IntegrationTestSuite) TestTxUpdateGroupPolicyMetadata() {
fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation),
fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
fmt.Sprintf("--%s=%d", flags.FlagGas, 300000),
}
testCases := []struct {

49
x/group/keeper/keeper.go
View File

@ -278,6 +278,32 @@ func (k Keeper) pruneProposal(ctx sdk.Context, proposalID uint64) error {
return nil
}
// updateProposalStatus iterates through all proposals by group policy index and updates proposal status
func (k Keeper) updateProposalStatus(ctx sdk.Context, groupPolicyAddr sdk.AccAddress) error {
proposalIt, err := k.proposalByGroupPolicyIndex.Get(ctx.KVStore(k.key), groupPolicyAddr.Bytes())
if err != nil {
return err
}
defer proposalIt.Close()
for {
var proposalInfo group.Proposal
_, err = proposalIt.LoadNext(&proposalInfo)
if errors.ErrORMIteratorDone.Is(err) {
break
}
if err != nil {
return err
}
proposalInfo.Status = group.PROPOSAL_STATUS_ABORTED
if err := k.proposalTable.Update(ctx.KVStore(k.key), proposalInfo.Id, &proposalInfo); err != nil {
return err
}
}
return nil
}
// pruneVotes prunes all votes for a proposal from state.
func (k Keeper) pruneVotes(ctx sdk.Context, proposalID uint64) error {
store := ctx.KVStore(k.key)
@ -340,13 +366,24 @@ func (k Keeper) TallyProposalsAtVPEnd(ctx sdk.Context) error {
return true, sdkerrors.Wrap(err, "group")
}
err = k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo)
if err != nil {
return true, sdkerrors.Wrap(err, "doTallyAndUpdate")
}
proposalId := proposal.Id
if proposal.Status == group.PROPOSAL_STATUS_ABORTED || proposal.Status == group.PROPOSAL_STATUS_WITHDRAWN {
if err := k.pruneProposal(ctx, proposalId); err != nil {
return true, err
}
if err := k.pruneVotes(ctx, proposalId); err != nil {
return true, err
}
if err := k.proposalTable.Update(ctx.KVStore(k.key), proposal.Id, &proposal); err != nil {
return true, sdkerrors.Wrap(err, "proposal update")
} else {
err = k.doTallyAndUpdate(ctx, &proposal, electorate, policyInfo)
if err != nil {
return true, sdkerrors.Wrap(err, "doTallyAndUpdate")
}
if err := k.proposalTable.Update(ctx.KVStore(k.key), proposal.Id, &proposal); err != nil {
return true, sdkerrors.Wrap(err, "proposal update")
}
}
return false, nil

3
x/group/keeper/keeper_test.go
View File

@ -2430,6 +2430,8 @@ func (s *TestSuite) TestExecPrunedProposalsAndVotes() {
s.Require().NoError(err)
return myProposalID
},
expErr: true, // since proposal status will be `aborted` when group policy is modified
expErrMsg: "not possible with proposal status",
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
},
"proposal exists when rollback all msg updates on failure": {
@ -2470,6 +2472,7 @@ func (s *TestSuite) TestExecPrunedProposalsAndVotes() {
_, err := s.keeper.Exec(ctx, &group.MsgExec{Executor: addr1.String(), ProposalId: proposalID})
if spec.expErr {
s.Require().Error(err)
s.Require().Contains(err.Error(), spec.expErrMsg)
return
}
s.Require().NoError(err)

8
x/group/keeper/msg_server.go
View File

@ -894,6 +894,11 @@ func (k Keeper) doUpdateGroupPolicy(ctx sdk.Context, groupPolicy string, admin s
return sdkerrors.Wrap(err, "load group policy")
}
groupPolicyAddr, err := sdk.AccAddressFromBech32(groupPolicy)
if err != nil {
return sdkerrors.Wrap(err, "group policy address")
}
groupPolicyAdmin, err := sdk.AccAddressFromBech32(admin)
if err != nil {
return sdkerrors.Wrap(err, "group policy admin")
@ -913,6 +918,9 @@ func (k Keeper) doUpdateGroupPolicy(ctx sdk.Context, groupPolicy string, admin s
return err
}
if err = k.updateProposalStatus(ctx, groupPolicyAddr); err != nil {
return err
}
return nil
}

166
x/group/module/abci_test.go
View File

@ -34,9 +34,16 @@ func TestEndBlockerPruning(t *testing.T) {
Admin: addr1.String(),
Members: members,
})
require.NoError(t, err)
groupRes2, err := app.GroupKeeper.CreateGroup(ctx, &group.MsgCreateGroup{
Admin: addr2.String(),
Members: members,
})
require.NoError(t, err)
groupID := groupRes.GroupId
groupID2 := groupRes2.GroupId
policy := group.NewThresholdDecisionPolicy(
"2",
@ -54,26 +61,54 @@ func TestEndBlockerPruning(t *testing.T) {
policyRes, err := app.GroupKeeper.CreateGroupPolicy(ctx, policyReq)
require.NoError(t, err)
policy2 := group.NewThresholdDecisionPolicy(
"1",
time.Second,
0,
)
policyReq2 := &group.MsgCreateGroupPolicy{
Admin: addr2.String(),
GroupId: groupID2,
}
err = policyReq2.SetDecisionPolicy(policy2)
require.NoError(t, err)
policyRes2, err := app.GroupKeeper.CreateGroupPolicy(ctx, policyReq2)
require.NoError(t, err)
groupPolicyAddr, err := sdk.AccAddressFromBech32(policyRes.Address)
require.NoError(t, err)
require.NoError(t, testutil.FundAccount(app.BankKeeper, ctx, groupPolicyAddr, sdk.Coins{sdk.NewInt64Coin("test", 10000)}))
groupPolicyAddr2, err := sdk.AccAddressFromBech32(policyRes2.Address)
require.NoError(t, err)
require.NoError(t, testutil.FundAccount(app.BankKeeper, ctx, groupPolicyAddr2, sdk.Coins{sdk.NewInt64Coin("test", 10000)}))
votingPeriod := policy.GetVotingPeriod()
msgSend1 := &banktypes.MsgSend{
FromAddress: groupPolicyAddr.String(),
ToAddress: addr2.String(),
Amount: sdk.Coins{sdk.NewInt64Coin("test", 100)},
}
msgSend2 := &banktypes.MsgSend{
FromAddress: groupPolicyAddr2.String(),
ToAddress: addr2.String(),
Amount: sdk.Coins{sdk.NewInt64Coin("test", 100)},
}
proposers := []string{addr2.String()}
specs := map[string]struct {
srcBlockTime time.Time
setupProposal func(ctx context.Context) uint64
setupProposal func(ctx sdk.Context) uint64
expErr bool
expErrMsg string
newCtx sdk.Context
expExecutorResult group.ProposalExecutorResult
expStatus group.ProposalStatus
}{
"proposal pruned after executor result success": {
setupProposal: func(ctx context.Context) uint64 {
setupProposal: func(ctx sdk.Context) uint64 {
msgs := []sdk.Msg{msgSend1}
pID, err := submitProposalAndVote(app, ctx, msgs, proposers, groupPolicyAddr, group.VOTE_OPTION_YES)
require.NoError(t, err)
@ -85,10 +120,11 @@ func TestEndBlockerPruning(t *testing.T) {
return pID
},
expErrMsg: "load proposal: not found",
newCtx: ctx,
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
},
"proposal with multiple messages pruned when executed with result success": {
setupProposal: func(ctx context.Context) uint64 {
setupProposal: func(ctx sdk.Context) uint64 {
msgs := []sdk.Msg{msgSend1, msgSend1}
pID, err := submitProposalAndVote(app, ctx, msgs, proposers, groupPolicyAddr, group.VOTE_OPTION_YES)
require.NoError(t, err)
@ -100,10 +136,11 @@ func TestEndBlockerPruning(t *testing.T) {
return pID
},
expErrMsg: "load proposal: not found",
newCtx: ctx,
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
},
"proposal not pruned when not executed and rejected": {
setupProposal: func(ctx context.Context) uint64 {
setupProposal: func(ctx sdk.Context) uint64 {
msgs := []sdk.Msg{msgSend1}
pID, err := submitProposalAndVote(app, ctx, msgs, proposers, groupPolicyAddr, group.VOTE_OPTION_NO)
require.NoError(t, err)
@ -114,10 +151,12 @@ func TestEndBlockerPruning(t *testing.T) {
return pID
},
newCtx: ctx,
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
expStatus: group.PROPOSAL_STATUS_REJECTED,
},
"open proposal is not pruned which must not fail ": {
setupProposal: func(ctx context.Context) uint64 {
setupProposal: func(ctx sdk.Context) uint64 {
pID, err := submitProposal(app, ctx, []sdk.Msg{msgSend1}, proposers, groupPolicyAddr)
require.NoError(t, err)
_, err = app.GroupKeeper.Exec(ctx, &group.MsgExec{Executor: addr3.String(), ProposalId: pID})
@ -127,10 +166,12 @@ func TestEndBlockerPruning(t *testing.T) {
return pID
},
newCtx: ctx,
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
expStatus: group.PROPOSAL_STATUS_SUBMITTED,
},
"proposal not pruned with group policy modified before tally": {
setupProposal: func(ctx context.Context) uint64 {
setupProposal: func(ctx sdk.Context) uint64 {
pID, err := submitProposal(app, ctx, []sdk.Msg{msgSend1}, proposers, groupPolicyAddr)
require.NoError(t, err)
_, err = app.GroupKeeper.UpdateGroupPolicyMetadata(ctx, &group.MsgUpdateGroupPolicyMetadata{
@ -139,16 +180,18 @@ func TestEndBlockerPruning(t *testing.T) {
})
require.NoError(t, err)
_, err = app.GroupKeeper.Exec(ctx, &group.MsgExec{Executor: addr3.String(), ProposalId: pID})
require.NoError(t, err)
require.Error(t, err) // since proposal with status Aborted cannot be executed
sdkCtx := sdk.UnwrapSDKContext(ctx)
require.NoError(t, testutil.FundAccount(app.BankKeeper, sdkCtx, groupPolicyAddr, sdk.Coins{sdk.NewInt64Coin("test", 10002)}))
return pID
},
newCtx: ctx,
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
expStatus: group.PROPOSAL_STATUS_ABORTED,
},
"pruned when proposal is executable when failed before": {
setupProposal: func(ctx context.Context) uint64 {
setupProposal: func(ctx sdk.Context) uint64 {
msgs := []sdk.Msg{msgSend1}
pID, err := submitProposalAndVote(app, ctx, msgs, proposers, groupPolicyAddr, group.VOTE_OPTION_YES)
require.NoError(t, err)
@ -156,27 +199,109 @@ func TestEndBlockerPruning(t *testing.T) {
require.NoError(t, err)
return pID
},
newCtx: ctx,
expErrMsg: "load proposal: not found",
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
},
"proposal with status withdrawn is pruned after voting period end": {
setupProposal: func(sdkCtx sdk.Context) uint64 {
pId, err := submitProposal(app, sdkCtx, []sdk.Msg{msgSend1}, proposers, groupPolicyAddr)
require.NoError(t, err)
_, err = app.GroupKeeper.WithdrawProposal(ctx, &group.MsgWithdrawProposal{
ProposalId: pId,
Address: groupPolicyAddr.String(),
})
require.NoError(t, err)
return pId
},
newCtx: ctx.WithBlockTime(ctx.BlockTime().Add(votingPeriod).Add(time.Hour)),
expErrMsg: "load proposal: not found",
expStatus: group.PROPOSAL_STATUS_WITHDRAWN,
},
"proposal with status withdrawn is not pruned (before voting period)": {
setupProposal: func(sdkCtx sdk.Context) uint64 {
pId, err := submitProposal(app, sdkCtx, []sdk.Msg{msgSend1}, proposers, groupPolicyAddr)
require.NoError(t, err)
_, err = app.GroupKeeper.WithdrawProposal(ctx, &group.MsgWithdrawProposal{
ProposalId: pId,
Address: groupPolicyAddr.String(),
})
require.NoError(t, err)
return pId
},
newCtx: ctx,
expErrMsg: "",
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
expStatus: group.PROPOSAL_STATUS_WITHDRAWN,
},
"proposal with status aborted is pruned after voting period end (due to updated group policy decision policy)": {
setupProposal: func(sdkCtx sdk.Context) uint64 {
pId, err := submitProposal(app, sdkCtx, []sdk.Msg{msgSend2}, proposers, groupPolicyAddr2)
require.NoError(t, err)
policy := group.NewThresholdDecisionPolicy("3", time.Second, 0)
msg := &group.MsgUpdateGroupPolicyDecisionPolicy{
Admin: addrs[1].String(),
GroupPolicyAddress: groupPolicyAddr2.String(),
}
err = msg.SetDecisionPolicy(policy)
require.NoError(t, err)
_, err = app.GroupKeeper.UpdateGroupPolicyDecisionPolicy(ctx, msg)
require.NoError(t, err)
return pId
},
newCtx: ctx.WithBlockTime(ctx.BlockTime().Add(votingPeriod).Add(time.Hour)),
expErrMsg: "load proposal: not found",
expStatus: group.PROPOSAL_STATUS_ABORTED,
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
},
"proposal with status aborted is not pruned before voting period end (due to updated group policy)": {
setupProposal: func(sdkCtx sdk.Context) uint64 {
pId, err := submitProposal(app, sdkCtx, []sdk.Msg{msgSend2}, proposers, groupPolicyAddr2)
require.NoError(t, err)
policy := group.NewThresholdDecisionPolicy("3", time.Second, 0)
msg := &group.MsgUpdateGroupPolicyDecisionPolicy{
Admin: addrs[1].String(),
GroupPolicyAddress: groupPolicyAddr2.String(),
}
err = msg.SetDecisionPolicy(policy)
require.NoError(t, err)
_, err = app.GroupKeeper.UpdateGroupPolicyDecisionPolicy(ctx, msg)
require.NoError(t, err)
return pId
},
newCtx: ctx,
expErrMsg: "",
expStatus: group.PROPOSAL_STATUS_ABORTED,
expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
},
}
for msg, spec := range specs {
spec := spec
t.Run(msg, func(t *testing.T) {
proposalID := spec.setupProposal(ctx)
module.EndBlocker(ctx, app.GroupKeeper)
module.EndBlocker(spec.newCtx, app.GroupKeeper)
if spec.expErrMsg != "" && spec.expExecutorResult != group.PROPOSAL_EXECUTOR_RESULT_SUCCESS {
_, err = app.GroupKeeper.Proposal(spec.newCtx, &group.QueryProposalRequest{ProposalId: proposalID})
require.Error(t, err)
require.Contains(t, err.Error(), spec.expErrMsg)
return
}
if spec.expExecutorResult == group.PROPOSAL_EXECUTOR_RESULT_SUCCESS {
// Make sure proposal is deleted from state
_, err = app.GroupKeeper.Proposal(ctx, &group.QueryProposalRequest{ProposalId: proposalID})
_, err = app.GroupKeeper.Proposal(spec.newCtx, &group.QueryProposalRequest{ProposalId: proposalID})
require.Contains(t, err.Error(), spec.expErrMsg)
res, err := app.GroupKeeper.VotesByProposal(ctx, &group.QueryVotesByProposalRequest{ProposalId: proposalID})
require.NoError(t, err)
require.Empty(t, res.GetVotes())
} else {
// Check that proposal and votes exists
res, err := app.GroupKeeper.Proposal(ctx, &group.QueryProposalRequest{ProposalId: proposalID})
res, err := app.GroupKeeper.Proposal(spec.newCtx, &group.QueryProposalRequest{ProposalId: proposalID})
require.NoError(t, err)
_, err = app.GroupKeeper.VotesByProposal(ctx, &group.QueryVotesByProposalRequest{ProposalId: res.Proposal.Id})
require.NoError(t, err)
@ -185,6 +310,8 @@ func TestEndBlockerPruning(t *testing.T) {
exp := group.ProposalExecutorResult_name[int32(spec.expExecutorResult)]
got := group.ProposalExecutorResult_name[int32(res.Proposal.ExecutorResult)]
assert.Equal(t, exp, got)
require.Equal(t, res.GetProposal().Status, spec.expStatus)
}
})
}
@ -240,13 +367,12 @@ func TestEndBlocker(t *testing.T) {
proposers := []string{addrs[2].String()}
specs := map[string]struct {
preRun func(sdkCtx sdk.Context) uint64
proposalId uint64
admin string
expErrMsg string
newCtx sdk.Context
tallyRes group.TallyResult
expStatus group.ProposalStatus
preRun func(sdkCtx sdk.Context) uint64
admin string
expErrMsg string
newCtx sdk.Context
tallyRes group.TallyResult
expStatus group.ProposalStatus
}{
"tally updated after voting power end": {
preRun: func(sdkCtx sdk.Context) uint64 {

6
x/group/spec/01_concepts.md
View File

@ -106,14 +106,16 @@ Votes are pruned:
- either after a successful tally, i.e. a tally whose result passes the decision
policy's rules, which can be trigged by a `Msg/Exec` or a
`Msg/{SubmitProposal,Vote}` with the `Exec` field,
- or on `EndBlock` right after the proposal's voting period end,
- or on `EndBlock` right after the proposal's voting period end. This applies to proposals with status `aborted` or `withdrawn` too.
whichever happens first.
Proposals are pruned:
- either after a successful proposal execution,
- on `EndBlock` whose proposal status is `withdrawn` or `aborted` on proposal's voting period end before tallying,
- and either after a successful proposal execution,
- or on `EndBlock` right after the proposal's `voting_period_end` +
`max_execution_period` (defined as an app-wide configuration) is passed,
whichever happens first.