fix(group)!: Fix group min execution period (backport #13876) (#13885)

* fix(group)!: Fix group min execution period (#13876) * fix: don't check MinExecutionPeriod in `Allow` * Check MinExecutionPeriod on doExecuteMsgs * Fix TestExec * Fix TestExec * test exec pruned * Fix submitproposal * Add changelog * typo * revert some changes * add minExecutionPeriod * Add docs and specs (cherry picked from commit 7661f62) # Conflicts: # CHANGELOG.md # x/group/README.md # x/group/keeper/keeper_test.go * fix docs * fix other conflicts * fix test Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
cosmos · Nov 16, 2022 · 6b633ef · 6b633ef
1 parent 0b81939
commit 6b633ef
Show file tree

Hide file tree

Showing 8 changed files with 190 additions and 91 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -44,6 +44,14 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * [#13826](https://github.com/cosmos/cosmos-sdk/pull/13826) Support custom `GasConfig` configuration for applications.
 * (deps) Bump Tendermint version to [v0.34.23](https://github.com/tendermint/tendermint/releases/tag/v0.34.23).
 
+### State Machine Breaking
+
+* (x/group) [#13876](https://github.com/cosmos/cosmos-sdk/pull/13876) Fix group MinExecutionPeriod that is checked on execution now, instead of voting period end.
+
+### API Breaking Changes
+
+* (x/group) [#13876](https://github.com/cosmos/cosmos-sdk/pull/13876) Add `GetMinExecutionPeriod` method on DecisionPolicy interface.
+
 ### Bug Fixes
 
 * (x/group) [#13869](https://github.com/cosmos/cosmos-sdk/pull/13869) Group members weight must be positive and a finite number.

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
@@ -1,14 +1,17 @@
 # Cosmos SDK v0.46.5 Release Notes
 
-This release introduces a number of bug fixes and improvements.  
-Notably, an upgrade to Tendermint [v0.34.23](https://github.com/tendermint/tendermint/releases/tag/v0.34.23).
+This release introduces a number of bug fixes and improvements. Notably, an upgrade to Tendermint [v0.34.23](https://github.com/tendermint/tendermint/releases/tag/v0.34.23).
 
-If your chain's state has coin metadata, an issue has been discovered in the coin metadata migration. This issue is fixed in `v0.46.5`.  
+If your chain's state has coin metadata, an issue has been discovered in the bank module coin metadata migration. This issue is fixed in `v0.46.5`.  
 
 * If you are planning to migrate to v0.46, please use `v0.46.5`. All releases prior to `v0.46.5`, **must NOT be used**. All previous version of `v0.46` are retracted.
 * If your chain is already on v0.46 using `<= v0.46.4` and has coin metadata, a **coordinated upgrade** to `v0.46.5` is required.
 * If your chain is already on v0.46 using `<= v0.46.4` but has no coin metadata, this release is **non-breaking**.
 
+Moreover, an issue have been found in the group module. This issue is fixed in `v0.46.5`.
+
+* If you use the group module, upgrade to `v0.46.5` **immediately**. A **coordinated upgrade** to `v0.46.5` is required.
+
 Please see the [CHANGELOG](https://github.com/cosmos/cosmos-sdk/blob/release/v0.46.x/CHANGELOG.md) for an exhaustive list of changes.
 
 Full Commit History: https://github.com/cosmos/cosmos-sdk/compare/v0.46.4...v0.46.5

diff --git a/x/group/keeper/keeper_test.go b/x/group/keeper/keeper_test.go
@@ -23,6 +23,8 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/group/module"
 )
 
+const minExecutionPeriod = 5 * time.Second
+
 type TestSuite struct {
 	suite.Suite
 
@@ -64,7 +66,7 @@ func (s *TestSuite) SetupTest() {
 	policy := group.NewThresholdDecisionPolicy(
 		"2",
 		time.Second,
-		0,
+		minExecutionPeriod, // Must wait 5 seconds before executing proposal
 	)
 	policyReq := &group.MsgCreateGroupPolicy{
 		Admin:   s.addrs[0].String(),
@@ -1465,34 +1467,47 @@ func (s *TestSuite) TestGroupPoliciesByAdminOrGroup() {
 func (s *TestSuite) TestSubmitProposal() {
 	addrs := s.addrs
 	addr1 := addrs[0]
-	addr2 := addrs[1]
+	addr2 := addrs[1] // Has weight 2
 	addr4 := addrs[3]
-	addr5 := addrs[4]
+	addr5 := addrs[4] // Has weight 1
 
 	myGroupID := s.groupID
 	accountAddr := s.groupPolicyAddr
 
-	msgSend := &banktypes.MsgSend{
-		FromAddress: s.groupPolicyAddr.String(),
-		ToAddress:   addr2.String(),
-		Amount:      sdk.Coins{sdk.NewInt64Coin("test", 100)},
-	}
-
+	// Create a new group policy to test TRY_EXEC
 	policyReq := &group.MsgCreateGroupPolicy{
 		Admin:   addr1.String(),
 		GroupId: myGroupID,
 	}
-	policy := group.NewThresholdDecisionPolicy(
+	noMinExecPeriodPolicy := group.NewThresholdDecisionPolicy(
+		"2",
+		time.Second,
+		0, // no MinExecutionPeriod to test TRY_EXEC
+	)
+	err := policyReq.SetDecisionPolicy(noMinExecPeriodPolicy)
+	s.Require().NoError(err)
+	res, err := s.app.GroupKeeper.CreateGroupPolicy(s.ctx, policyReq)
+	s.Require().NoError(err)
+	noMinExecPeriodPolicyAddr := sdk.MustAccAddressFromBech32(res.Address)
+	s.Require().NoError(testutil.FundAccount(s.app.BankKeeper, s.sdkCtx, noMinExecPeriodPolicyAddr, sdk.Coins{sdk.NewInt64Coin("test", 10000)}))
+
+	// Create a new group policy with super high threshold
+	bigThresholdPolicy := group.NewThresholdDecisionPolicy(
 		"100",
 		time.Second,
-		0,
+		minExecutionPeriod,
 	)
-	err := policyReq.SetDecisionPolicy(policy)
+	err = policyReq.SetDecisionPolicy(bigThresholdPolicy)
 	s.Require().NoError(err)
-	bigThresholdRes, err := s.keeper.CreateGroupPolicy(s.ctx, policyReq)
+	bigThresholdRes, err := s.app.GroupKeeper.CreateGroupPolicy(s.ctx, policyReq)
 	s.Require().NoError(err)
 	bigThresholdAddr := bigThresholdRes.Address
 
+	msgSend := &banktypes.MsgSend{
+		FromAddress: noMinExecPeriodPolicyAddr.String(),
+		ToAddress:   addr2.String(),
+		Amount:      sdk.Coins{sdk.NewInt64Coin("test", 100)},
+	}
 	defaultProposal := group.Proposal{
 		GroupPolicyAddress: accountAddr.String(),
 		Status:             group.PROPOSAL_STATUS_SUBMITTED,
@@ -1605,13 +1620,13 @@ func (s *TestSuite) TestSubmitProposal() {
 		},
 		"with try exec": {
 			req: &group.MsgSubmitProposal{
-				GroupPolicyAddress: accountAddr.String(),
+				GroupPolicyAddress: noMinExecPeriodPolicyAddr.String(),
 				Proposers:          []string{addr2.String()},
 				Exec:               group.Exec_EXEC_TRY,
 			},
 			msgs: []sdk.Msg{msgSend},
 			expProposal: group.Proposal{
-				GroupPolicyAddress: accountAddr.String(),
+				GroupPolicyAddress: noMinExecPeriodPolicyAddr.String(),
 				Status:             group.PROPOSAL_STATUS_ACCEPTED,
 				FinalTallyResult: group.TallyResult{
 					YesCount:        "2",
@@ -1622,21 +1637,21 @@ func (s *TestSuite) TestSubmitProposal() {
 				ExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
 			},
 			postRun: func(sdkCtx sdk.Context) {
-				fromBalances := s.app.BankKeeper.GetAllBalances(sdkCtx, accountAddr)
+				fromBalances := s.app.BankKeeper.GetAllBalances(sdkCtx, noMinExecPeriodPolicyAddr)
 				s.Require().Contains(fromBalances, sdk.NewInt64Coin("test", 9900))
 				toBalances := s.app.BankKeeper.GetAllBalances(sdkCtx, addr2)
 				s.Require().Contains(toBalances, sdk.NewInt64Coin("test", 100))
 			},
 		},
 		"with try exec, not enough yes votes for proposal to pass": {
 			req: &group.MsgSubmitProposal{
-				GroupPolicyAddress: accountAddr.String(),
+				GroupPolicyAddress: noMinExecPeriodPolicyAddr.String(),
 				Proposers:          []string{addr5.String()},
 				Exec:               group.Exec_EXEC_TRY,
 			},
 			msgs: []sdk.Msg{msgSend},
 			expProposal: group.Proposal{
-				GroupPolicyAddress: accountAddr.String(),
+				GroupPolicyAddress: noMinExecPeriodPolicyAddr.String(),
 				Status:             group.PROPOSAL_STATUS_SUBMITTED,
 				FinalTallyResult: group.TallyResult{
 					YesCount:        "0", // Since tally doesn't pass Allow(), we consider the proposal not final
@@ -1689,6 +1704,7 @@ func (s *TestSuite) TestSubmitProposal() {
 				}
 			}
 
+			s.sdkCtx.WithBlockTime(s.blockTime)
 			spec.postRun(s.sdkCtx)
 		})
 	}
@@ -2284,6 +2300,7 @@ func (s *TestSuite) TestExecProposal() {
 				msgs := []sdk.Msg{msgSend1}
 				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_YES)
 			},
+			srcBlockTime:      s.blockTime.Add(minExecutionPeriod), // After min execution period end
 			expProposalStatus: group.PROPOSAL_STATUS_ACCEPTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
 			expBalance:        true,
@@ -2295,6 +2312,7 @@ func (s *TestSuite) TestExecProposal() {
 				msgs := []sdk.Msg{msgSend1, msgSend1}
 				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_YES)
 			},
+			srcBlockTime:      s.blockTime.Add(minExecutionPeriod), // After min execution period end
 			expProposalStatus: group.PROPOSAL_STATUS_ACCEPTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
 			expBalance:        true,
@@ -2306,6 +2324,7 @@ func (s *TestSuite) TestExecProposal() {
 				msgs := []sdk.Msg{msgSend1}
 				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_NO)
 			},
+			srcBlockTime:      s.blockTime.Add(minExecutionPeriod), // After min execution period end
 			expProposalStatus: group.PROPOSAL_STATUS_REJECTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
 		},
@@ -2322,33 +2341,57 @@ func (s *TestSuite) TestExecProposal() {
 			},
 			expErr: true,
 		},
-		"Decision policy also applied on timeout": {
+		"Decision policy also applied on exactly voting period end": {
 			setupProposal: func(ctx context.Context) uint64 {
 				msgs := []sdk.Msg{msgSend1}
 				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_NO)
 			},
-			srcBlockTime:      s.blockTime.Add(time.Second),
+			srcBlockTime:      s.blockTime.Add(time.Second), // Voting period is 1s
 			expProposalStatus: group.PROPOSAL_STATUS_REJECTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
 		},
-		"Decision policy also applied after timeout": {
+		"Decision policy also applied after voting period end": {
 			setupProposal: func(ctx context.Context) uint64 {
 				msgs := []sdk.Msg{msgSend1}
 				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_NO)
 			},
-			srcBlockTime:      s.blockTime.Add(time.Second).Add(time.Millisecond),
+			srcBlockTime:      s.blockTime.Add(time.Second).Add(time.Millisecond), // Voting period is 1s
 			expProposalStatus: group.PROPOSAL_STATUS_REJECTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_NOT_RUN,
 		},
+		"exec proposal before MinExecutionPeriod should fail": {
+			setupProposal: func(ctx context.Context) uint64 {
+				msgs := []sdk.Msg{msgSend1}
+				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_YES)
+			},
+			srcBlockTime:      s.blockTime.Add(4 * time.Second), // min execution date is 5s later after s.blockTime
+			expProposalStatus: group.PROPOSAL_STATUS_ACCEPTED,
+			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_FAILURE, // Because MinExecutionPeriod has not passed
+		},
+		"exec proposal at exactly MinExecutionPeriod should pass": {
+			setupProposal: func(ctx context.Context) uint64 {
+				msgs := []sdk.Msg{msgSend1}
+				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_YES)
+			},
+			srcBlockTime:      s.blockTime.Add(5 * time.Second), // min execution date is 5s later after s.blockTime
+			expProposalStatus: group.PROPOSAL_STATUS_ACCEPTED,
+			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
+		},
 		"prevent double execution when successful": {
 			setupProposal: func(ctx context.Context) uint64 {
 				myProposalID := submitProposalAndVote(ctx, s, []sdk.Msg{msgSend1}, proposers, group.VOTE_OPTION_YES)
 
-				_, err := s.keeper.Exec(ctx, &group.MsgExec{Executor: addr1.String(), ProposalId: myProposalID})
+				// Wait after min execution period end before Exec
+				sdkCtx := sdk.UnwrapSDKContext(ctx)
+				sdkCtx = sdkCtx.WithBlockTime(sdkCtx.BlockTime().Add(minExecutionPeriod)) // MinExecutionPeriod is 5s
+				ctx = sdk.WrapSDKContext(sdkCtx)
+
+				_, err := s.app.GroupKeeper.Exec(ctx, &group.MsgExec{Executor: addr1.String(), ProposalId: myProposalID})
 				s.Require().NoError(err)
 				return myProposalID
 			},
-			expErr:            true, // since proposal is pruned after a successful MsgExec
+			srcBlockTime:      s.blockTime.Add(minExecutionPeriod), // After min execution period end
+			expErr:            true,                                // since proposal is pruned after a successful MsgExec
 			expProposalStatus: group.PROPOSAL_STATUS_ACCEPTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
 			expBalance:        true,
@@ -2360,6 +2403,7 @@ func (s *TestSuite) TestExecProposal() {
 				msgs := []sdk.Msg{msgSend1, msgSend2}
 				return submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_YES)
 			},
+			srcBlockTime:      s.blockTime.Add(minExecutionPeriod), // After min execution period end
 			expProposalStatus: group.PROPOSAL_STATUS_ACCEPTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_FAILURE,
 		},
@@ -2368,13 +2412,14 @@ func (s *TestSuite) TestExecProposal() {
 				msgs := []sdk.Msg{msgSend2}
 				myProposalID := submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_YES)
 
-				_, err := s.keeper.Exec(ctx, &group.MsgExec{Executor: addr1.String(), ProposalId: myProposalID})
-				s.Require().NoError(err)
+				// Wait after min execution period end before Exec
 				sdkCtx := sdk.UnwrapSDKContext(ctx)
+				sdkCtx = sdkCtx.WithBlockTime(sdkCtx.BlockTime().Add(minExecutionPeriod)) // MinExecutionPeriod is 5s
 				s.Require().NoError(testutil.FundAccount(s.app.BankKeeper, sdkCtx, s.groupPolicyAddr, sdk.Coins{sdk.NewInt64Coin("test", 10002)}))
 
 				return myProposalID
 			},
+			srcBlockTime:      s.blockTime.Add(minExecutionPeriod), // After min execution period end
 			expProposalStatus: group.PROPOSAL_STATUS_ACCEPTED,
 			expExecutorResult: group.PROPOSAL_EXECUTOR_RESULT_SUCCESS,
 		},
@@ -2516,9 +2561,15 @@ func (s *TestSuite) TestExecPrunedProposalsAndVotes() {
 				msgs := []sdk.Msg{msgSend2}
 				myProposalID := submitProposalAndVote(ctx, s, msgs, proposers, group.VOTE_OPTION_YES)
 
-				_, err := s.keeper.Exec(ctx, &group.MsgExec{Executor: addr1.String(), ProposalId: myProposalID})
-				s.Require().NoError(err)
+				// Wait for min execution period end
 				sdkCtx := sdk.UnwrapSDKContext(ctx)
+				sdkCtx = sdkCtx.WithBlockTime(sdkCtx.BlockTime().Add(minExecutionPeriod))
+				ctx = sdk.WrapSDKContext(sdkCtx)
+
+				_, err := s.app.GroupKeeper.Exec(ctx, &group.MsgExec{Executor: addr1.String(), ProposalId: myProposalID})
+				s.Require().NoError(err)
+
+				sdkCtx = sdk.UnwrapSDKContext(ctx)
 				s.Require().NoError(testutil.FundAccount(s.app.BankKeeper, sdkCtx, s.groupPolicyAddr, sdk.Coins{sdk.NewInt64Coin("test", 10002)}))
 
 				return myProposalID
@@ -2538,6 +2589,8 @@ func (s *TestSuite) TestExecPrunedProposalsAndVotes() {
 				sdkCtx = sdkCtx.WithBlockTime(spec.srcBlockTime)
 			}
 
+			// Wait for min execution period end
+			sdkCtx = sdkCtx.WithBlockTime(sdkCtx.BlockTime().Add(minExecutionPeriod))
 			ctx = sdk.WrapSDKContext(sdkCtx)
 			_, err := s.keeper.Exec(ctx, &group.MsgExec{Executor: addr1.String(), ProposalId: proposalID})
 			if spec.expErr {
@@ -2990,3 +3043,58 @@ func (s *TestSuite) createGroupAndGroupPolicy(
 
 	return policyAddr, groupID
 }
+
+func (s *TestSuite) TestTallyProposalsAtVPEnd() {
+	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))
+
+	groupRes, err := s.app.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.app.GroupKeeper.SubmitProposal(s.ctx, &group.MsgSubmitProposal{
+		GroupPolicyAddress: accountAddr,
+		Proposers:          []string{addr1.String()},
+		Messages:           nil,
+	})
+	s.Require().NoError(err)
+
+	_, err = s.app.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.app.GroupKeeper.TallyResult(ctx, &group.QueryTallyResultRequest{
+		ProposalId: proposalRes.ProposalId,
+	})
+	s.Require().Equal("1", result.Tally.YesCount)
+	s.Require().NoError(err)
+
+	s.Require().NoError(s.app.GroupKeeper.TallyProposalsAtVPEnd(ctx))
+	s.NotPanics(func() { module.EndBlocker(ctx, s.app.GroupKeeper) })
+}
diff --git a/x/group/keeper/msg_server.go b/x/group/keeper/msg_server.go
@@ -685,8 +685,7 @@ func (k Keeper) doTallyAndUpdate(ctx sdk.Context, p *group.Proposal, electorate
 		return err
 	}
 
-	sinceSubmission := ctx.BlockTime().Sub(p.SubmitTime) // duration passed since proposal submission.
-	result, err := policy.Allow(tallyResult, electorate.TotalWeight, sinceSubmission)
+	result, err := policy.Allow(tallyResult, electorate.TotalWeight)
 	if err != nil {
 		return sdkerrors.Wrap(err, "policy allow")
 	}
@@ -752,7 +751,8 @@ func (k Keeper) Exec(goCtx context.Context, req *group.MsgExec) (*group.MsgExecR
 			return nil, err
 		}
 
-		if results, err := k.doExecuteMsgs(cacheCtx, k.router, proposal, addr); err != nil {
+		decisionPolicy := policyInfo.DecisionPolicy.GetCachedValue().(group.DecisionPolicy)
+		if results, err := k.doExecuteMsgs(cacheCtx, k.router, proposal, addr, decisionPolicy); err != nil {
 			proposal.ExecutorResult = group.PROPOSAL_EXECUTOR_RESULT_FAILURE
 			logs = fmt.Sprintf("proposal execution failed on proposal %d, because of error %s", id, err.Error())
 			k.Logger(ctx).Info("proposal execution failed", "cause", err, "proposalID", id)

diff --git a/x/group/keeper/proposal_executor.go b/x/group/keeper/proposal_executor.go
@@ -12,7 +12,13 @@ import (
 
 // doExecuteMsgs routes the messages to the registered handlers. Messages are limited to those that require no authZ or
 // by the account of group policy only. Otherwise this gives access to other peoples accounts as the sdk middlewares are bypassed
-func (s Keeper) doExecuteMsgs(ctx sdk.Context, router *baseapp.MsgServiceRouter, proposal group.Proposal, groupPolicyAcc sdk.AccAddress) ([]sdk.Result, error) {
+func (s Keeper) doExecuteMsgs(ctx sdk.Context, router *baseapp.MsgServiceRouter, proposal group.Proposal, groupPolicyAcc sdk.AccAddress, decisionPolicy group.DecisionPolicy) ([]sdk.Result, error) {
+	// Ensure it's not too early to execute the messages.
+	minExecutionDate := proposal.SubmitTime.Add(decisionPolicy.GetMinExecutionPeriod())
+	if ctx.BlockTime().Before(minExecutionDate) {
+		return nil, grouperrors.ErrInvalid.Wrapf("must wait until %s to execute proposal %d", minExecutionDate, proposal.Id)
+	}
+
 	// Ensure it's not too late to execute the messages.
 	// After https://github.com/cosmos/cosmos-sdk/issues/11245, proposals should
 	// be pruned automatically, so this function should not even be called, as