[R4R] update gov strategy #73

yutianwu · 2019-03-04T08:33:55Z

Description

Ref: https://github.com/binance-chain/docs-internal/wiki/Gov-strategy

Voting period

As for now, voting period of every proposal is the same, like 4 hours in testnet.

The drawback is the fixed voting period is not flexible. We can specify voting period in proposal, but we still need to control the duration, it should not be too long.

So we can set MaxVotingPeriod and specify voting period in proposal.

Tally strategy

There are four types of votes:

OptionYes
OptionAbstain
OptionNo
OptionNoWithVeto

Tally rules:

If there is no staked coins, the proposal fails

If there is no validator bond tokens, the proposal fails.
If there is not enough quorum of votes, the proposal fails

quorum is minimum percentage of total stake needed to vote for a result to be considered valid
If no one votes (everyone abstains), proposal fails
If more than 1/3 of voters veto, proposal fails
If more than 1/2 of non-abstaining voters vote Yes, proposal passes
If more than 1/2 of non-abstaining voters vote No, proposal fails

Disposition of deposits

Rejected

If votes reach the quorum, the deposits will be distributed to validator.

If votes do not reach the quorum, the deposits will be refunded.

Passed

Deposits will be returned.

Rationale

tell us why we need these changes...

Example

add an example CLI or API response...

Changes

Notable changes:

add voting period for every proposal
add quorum for tally procedure
add limit to title and description
refund deposits if quorum is not reached
deposit funds to a specific address

Preflight checks

build passed (make build)
tests passed (make test)
integration tests passed (make integration_test)
manual transaction test passed (cli invoke)

Already reviewed by

...

Related issues

... reference related issue #'s here ...

x/gov/keeper.go

x/gov/handler.go

HaoyangLiu · 2019-03-21T10:15:36Z

There are some failed unit tests:

x/stake/handler_test.go:1034:40: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1048:40: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1100:39: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1122:38: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1134:38: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)

yutianwu · 2019-03-21T11:01:19Z

There are some failed unit tests:

x/stake/handler_test.go:1034:40: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1048:40: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1100:39: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1122:38: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)
x/stake/handler_test.go:1134:38: not enough arguments in call to govKeeper.NewTextProposal
        have ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind)
        want ("github.com/cosmos/cosmos-sdk/types".Context, string, string, gov.ProposalKind, time.Duration)

you are right, I just ran the build command.

HaoyangLiu · 2019-03-21T12:07:51Z

func handleMsgVote(ctx sdk.Context, keeper Keeper, msg MsgVote) sdk.Result {
	isValidator := false
	keeper.vs.IterateValidatorsBonded(ctx, func(index int64, validator sdk.Validator) (stop bool) {
		if sdk.ValAddress(msg.Voter).Equals(validator.GetOperator()) {
			isValidator = true
			return true
		}
		return false
	})

How about replace the validator iterator with the following code:

func handleMsgVote(ctx sdk.Context, keeper Keeper, msg MsgVote) sdk.Result {
	validator := keeper.vs.Validator(ctx, sdk.ValAddress(msg.Voter))

	if validator == nil {
		return sdk.ErrUnauthorized("Vote is not from a validator operator").Result()
	}
	
	if validator.GetPower().IsZero() {
		return sdk.ErrUnauthorized("Validator is not bonded").Result()
	}

yutianwu · 2019-03-21T12:09:11Z

func handleMsgVote(ctx sdk.Context, keeper Keeper, msg MsgVote) sdk.Result {
	isValidator := false
	keeper.vs.IterateValidatorsBonded(ctx, func(index int64, validator sdk.Validator) (stop bool) {
		if sdk.ValAddress(msg.Voter).Equals(validator.GetOperator()) {
			isValidator = true
			return true
		}
		return false
	})

How about replace the validator iterator with the following code:

func handleMsgVote(ctx sdk.Context, keeper Keeper, msg MsgVote) sdk.Result {
	validator := keeper.vs.Validator(ctx, sdk.ValAddress(msg.Voter))

	if validator == nil {
		return sdk.ErrUnauthorized("Vote is not from a validator operator").Result()
	}
	
	if validator.GetPower().IsZero() {
		return sdk.ErrUnauthorized("Validator is not bonded").Result()
	}

yeah, good catch

HaoyangLiu · 2019-03-28T09:31:16Z

As you lowered minimum deposit amount, there are some failed unit tests. Please fix them

--- FAIL: TestTickExpiredDepositPeriod (0.00s)
        Error Trace:    endblocker_test.go:38
        Error:          Expected value not to be nil.
        Test:           TestTickExpiredDepositPeriod
D[2019-03-28|17:28:20.022] Commit synced                                module=sdk/app commit="CommitID{[120 203 47 234 101 159 18 99 131 193 198 251 23 59 203 219 103 181 217 136 139 213 127 32 252 105 74 119 99 147 24 248]:1}"
--- FAIL: TestTickMultipleExpiredDepositPeriod (0.00s)
        Error Trace:    endblocker_test.go:87
        Error:          Expected value not to be nil.
        Test:           TestTickMultipleExpiredDepositPeriod
D[2019-03-28|17:28:20.024] Commit synced                                module=sdk/app commit="CommitID{[119 142 7 158 245 83 135 53 231 204 138 130 110 48 19 51 100 169 75 24 202 150 116 184 13 95 168 113 44 84 232 42]:1}"
--- FAIL: TestTickPassedDepositPeriod (0.00s)
        Error Trace:    endblocker_test.go:142
        Error:          Expected value not to be nil.
        Test:           TestTickPassedDepositPeriod

yutianwu · 2019-03-28T10:38:08Z

As you lowered minimum deposit amount, there are some failed unit tests. Please fix them

--- FAIL: TestTickExpiredDepositPeriod (0.00s)
        Error Trace:    endblocker_test.go:38
        Error:          Expected value not to be nil.
        Test:           TestTickExpiredDepositPeriod
D[2019-03-28|17:28:20.022] Commit synced                                module=sdk/app commit="CommitID{[120 203 47 234 101 159 18 99 131 193 198 251 23 59 203 219 103 181 217 136 139 213 127 32 252 105 74 119 99 147 24 248]:1}"
--- FAIL: TestTickMultipleExpiredDepositPeriod (0.00s)
        Error Trace:    endblocker_test.go:87
        Error:          Expected value not to be nil.
        Test:           TestTickMultipleExpiredDepositPeriod
D[2019-03-28|17:28:20.024] Commit synced                                module=sdk/app commit="CommitID{[119 142 7 158 245 83 135 53 231 204 138 130 110 48 19 51 100 169 75 24 202 150 116 184 13 95 168 113 44 84 232 42]:1}"
--- FAIL: TestTickPassedDepositPeriod (0.00s)
        Error Trace:    endblocker_test.go:142
        Error:          Expected value not to be nil.
        Test:           TestTickPassedDepositPeriod

My bad, just bring back the change, I will replace it in node repo as we did before.

darren-liu · 2019-03-28T09:14:03Z

x/gov/client/cli/tx.go

+
+			votingPeriod := time.Duration(proposal.VotingPeriod) * time.Second
+			if votingPeriod > gov.MaxVotingPeriod {
+				return errors.New(fmt.Sprintf("voting period should less than %d seconds", gov.MaxVotingPeriod/time.Second))


should be less

darren-liu · 2019-03-28T09:25:14Z

x/gov/msgs.go

@@ -67,7 +70,7 @@ func (msg MsgSubmitProposal) ValidateBasic() sdk.Error {
 	if !validProposalType(msg.ProposalType) {
 		return ErrInvalidProposalType(DefaultCodespace, msg.ProposalType)
 	}
-	if len(msg.Proposer) == 0 {
+	if msg.Proposer.Empty() {


need to ensure the Proposer is exactly sdk.AddrLen

no need to check whether Empty() or not

darren-liu · 2019-03-28T09:25:38Z

x/gov/msgs.go

@@ -129,7 +135,7 @@ func (msg MsgDeposit) Type() string  { return "deposit" }

 // Implements Msg.
 func (msg MsgDeposit) ValidateBasic() sdk.Error {
-	if len(msg.Depositer) == 0 {
+	if msg.Depositer.Empty() {


x/gov/msgs.go

x/gov/tally.go

darren-liu · 2019-03-29T00:44:30Z

x/gov/msgs.go

@@ -67,7 +70,7 @@ func (msg MsgSubmitProposal) ValidateBasic() sdk.Error {
 	if !validProposalType(msg.ProposalType) {
 		return ErrInvalidProposalType(DefaultCodespace, msg.ProposalType)
 	}
-	if len(msg.Proposer) == 0 {
+	if msg.Proposer.Empty() {


no need to check whether Empty() or not

* R4R: Add interface to remove validator (#91) * Add validator remove interface * add unit test * Fix failed unit test in gov and stake * Add proposal id checking in msg validate * Add validator consensus addr into proposal decription * Add remove validator command line interface * Add new proposal type to NormalizeProposalType * add validator consensus address checking * Rename consensus address flag name * refactor handler test import * Use json marshal to replace cdc marshal * Add automatic gov proposal when proposal id is zero * add deposit flag * Add routerCallRecord * Resolve comment, a tx only contains one msg * improve the validate logic for remove validator * refactor go import * remove necessary checking * stake: add fee addr for each validator * [R4R] update gov strategy (#73) * update gov strategy * refactor * revert change * [R4R] Add deposit address and do some refactor (#101) * add address for deposits * refactor * refactor * revert change * refactor * refactor * change proposals to publish * update deposit address

yutianwu requested review from rickyyangz, unclezoro, darren-liu and ackratos and removed request for rickyyangz and unclezoro March 4, 2019 08:33

yutianwu force-pushed the feature/update_gov branch 2 times, most recently from 45aa7c7 to b6dcc27 Compare March 4, 2019 11:29

darren-liu requested changes Mar 6, 2019

View reviewed changes

x/gov/keeper.go Show resolved Hide resolved

x/gov/handler.go Show resolved Hide resolved

yutianwu force-pushed the feature/update_gov branch from f836568 to f1d50bd Compare March 6, 2019 09:31

yutianwu changed the title ~~update gov strategy~~ [R4R] update gov strategy Mar 20, 2019

HaoyangLiu reviewed Mar 20, 2019

View reviewed changes

x/gov/handler.go Outdated Show resolved Hide resolved

yutianwu force-pushed the feature/update_gov branch from f1d50bd to f0e758d Compare March 21, 2019 08:01

HaoyangLiu approved these changes Mar 21, 2019

View reviewed changes

yutianwu force-pushed the feature/update_gov branch from cee7a70 to d47acdd Compare March 25, 2019 08:04

HaoyangLiu approved these changes Mar 25, 2019

View reviewed changes

yutianwu force-pushed the feature/update_gov branch from cee7a70 to 1afab6b Compare March 28, 2019 09:01

darren-liu requested changes Mar 28, 2019

View reviewed changes

update gov strategy

f7125dc

yutianwu force-pushed the feature/update_gov branch from 570ce59 to f7125dc Compare March 28, 2019 13:04

HaoyangLiu approved these changes Mar 28, 2019

View reviewed changes

darren-liu approved these changes Mar 29, 2019

View reviewed changes

yutianwu added 2 commits March 29, 2019 10:42

refactor

f865b4b

revert change

f58e532

yutianwu merged commit f3c94ce into develop Mar 29, 2019

yutianwu mentioned this pull request Mar 29, 2019

[R4R] Update gov strategy bnb-chain/node#518

Merged

4 tasks

forcodedancing deleted the feature/update_gov branch May 18, 2022 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R4R] update gov strategy #73

[R4R] update gov strategy #73

yutianwu commented Mar 4, 2019 •

edited

Loading

HaoyangLiu commented Mar 21, 2019

yutianwu commented Mar 21, 2019

HaoyangLiu commented Mar 21, 2019

yutianwu commented Mar 21, 2019

HaoyangLiu commented Mar 28, 2019

yutianwu commented Mar 28, 2019

darren-liu Mar 28, 2019

darren-liu Mar 28, 2019

darren-liu Mar 29, 2019

darren-liu Mar 28, 2019

darren-liu Mar 29, 2019

[R4R] update gov strategy #73

[R4R] update gov strategy #73

Conversation

yutianwu commented Mar 4, 2019 • edited Loading

Description

Voting period

Tally strategy

Disposition of deposits

Rejected

Passed

Rationale

Example

Changes

Preflight checks

Already reviewed by

Related issues

HaoyangLiu commented Mar 21, 2019

yutianwu commented Mar 21, 2019

HaoyangLiu commented Mar 21, 2019

yutianwu commented Mar 21, 2019

HaoyangLiu commented Mar 28, 2019

yutianwu commented Mar 28, 2019

darren-liu Mar 28, 2019

Choose a reason for hiding this comment

darren-liu Mar 28, 2019

Choose a reason for hiding this comment

darren-liu Mar 29, 2019

Choose a reason for hiding this comment

darren-liu Mar 28, 2019

Choose a reason for hiding this comment

darren-liu Mar 29, 2019

Choose a reason for hiding this comment

yutianwu commented Mar 4, 2019 •

edited

Loading