feat(cli): cancel gov proposal by proposer before voting period ends

[gsk967]

Description

This pull request contains the new cli tx to cancel the gov proposal by the proposer

Closes: #11554

1. Proposer can cancel the proposal before the voting period ends.
2. Added proposal_cancel_burn_rate param to gov.
3. When proposal cancel by proposer i. (deposits * proposal_cancel_burn_rate) amount will be burned. ii. deposits * (1 - proposal_cancel_burn_rate) amount will be move to the community pool.

Proposal migrations with proposer

package simapp

import (
	sdk "github.com/cosmos/cosmos-sdk/types"
	"github.com/cosmos/cosmos-sdk/types/module"
	v4 "github.com/cosmos/cosmos-sdk/x/gov/migrations/v4"
	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
)

// UpgradeName defines the on-chain upgrade name for the sample simapp upgrade from v045 to v046.
//
// NOTE: This upgrade defines a reference implementation of what an upgrade could look like
// when an application is migrating from Cosmos SDK version v0.45.x to v0.46.x.
const UpgradeName = "v046-to-v047"

func (app SimApp) RegisterUpgradeHandlers() {
	app.UpgradeKeeper.SetUpgradeHandler(UpgradeName,
		func(ctx sdk.Context, plan upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) {
			// add proposal ids with proposers
			proposals := make(map[uint64]string)
			// proposals[1] = "cosmos1luyncewxk4lm24k6gqy8y5dxkj0klr4tu0lmnj" ...
			v4.AddProposerAddressToProposal(ctx, sdk.NewKVStoreKey(v4.ModuleName), app.appCodec, proposals)
			return app.ModuleManager.RunMigrations(ctx, app.Configurator(), fromVM)
		})
}

---

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 in the PR title
• [ ] added ! to the type prefix if API or client breaking change
• [ ] targeted the correct branch (see PR Targeting)
• [ ] provided a link to the relevant issue or specification
• [ ] followed the guidelines for building modules
• [ ] included the necessary unit and integration tests
• [ ] added a changelog entry to CHANGELOG.md
• [ ] included comments for documenting Go code
• [ ] 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 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)

[codecov[bot]]

Codecov Report

Merging #13010 (479e71c) into main (2739f83) will decrease coverage by 2.08%. The diff coverage is 36.59%.

:exclamation: Current head 479e71c differs from pull request most recent head 127caff. Consider uploading reports for the commit 127caff to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13010      +/-   ##
==========================================
- Coverage   56.25%   54.18%   -2.08%     
==========================================
  Files         667      654      -13     
  Lines       56576    55910     -666     
==========================================
- Hits        31829    30294    -1535     
- Misses      22165    23205    +1040     
+ Partials     2582     2411     -171     

Impacted Files	Coverage Δ
x/gov/client/cli/tx.go	12.82% <0.00%> (-57.70%)	:arrow_down:
x/gov/keeper/deposit.go	71.76% <0.00%> (-17.95%)	:arrow_down:
x/gov/migrations/v4/store.go	50.98% <0.00%> (-30.66%)	:arrow_down:
x/gov/types/expected_keepers.go	0.00% <ø> (ø)
x/gov/types/keys.go	88.88% <0.00%> (ø)
x/gov/types/v1/msgs.go	53.44% <0.00%> (-5.05%)	:arrow_down:
x/gov/types/v1/proposal.go	16.94% <0.00%> (-0.30%)	:arrow_down:
x/gov/keeper/msg_server.go	76.67% <7.14%> (-6.40%)	:arrow_down:
x/gov/keeper/proposal.go	77.31% <53.84%> (-4.50%)	:arrow_down:
x/gov/keeper/keeper.go	68.18% <54.54%> (-5.23%)	:arrow_down:
... and 309 more

[hxrts]

could you add an example command here? https://github.com/cosmos/cosmos-sdk/tree/main/x/gov#proposals-2

[tac0turtle]

@hxrts would love your approval here when you believe its ready for code review

[gsk967]

@anilCSE @robert-zaremba @hxrts @alexanderbez @atheeshp @tac0turtle can anyone look into this pull request, I have update with proposal_cancel_burn_rate and fix the test cases

[gsk967]

Going to block this for now, until I understand why we need a CanceledProposalQueue at all @amaurym I thought like if we do all the process inside msg execution then proposer has to wait for result, because we need do take care deposits and votes inside the process

if you think this will not take that much time, I can remove the StatusCanceled and move the entire process inside msg execution

[gsk967]

I think what @amaurym is pointing out makes sense -- why are we putting things into a queue just to execute them at the same block at EndBlock? Doesn't make sense. Is there a strong reason for this @gsk967? Instead, why not just cancel the proposal right as the message is executed?

Actually I don't have any strong reason for that, I just though why we can't do all process inside the EndBlock same as other proposal operations (like proposal status checking...) doing,

@alexanderbez @amaurym I will move the EndBlock process inside the msg execution.

[gsk967]

@amaurym @alexanderbez @robert-zaremba @anilCSE I move the execution flow of cancel proposal into the msg execution , Please look into changes

[julienrbrt]

@gsk967 are you on discord? We can sync with the concerns mentioned here.

[gsk967]

@gsk967 are you on discord? We can sync with the concerns mentioned here.

Yes , I am on discord, My discord handle is Sai Kumar#9563

[robert-zaremba]

and then we were mixing burning with sending to community pool.

So the idea is to add a new parameter: proposal_cancel_dest: address.

• When empty, then the we burn the portion of deposit.
• When not empty, we send a portion to that address. Suggested default should be community pool (in a documentation).

[robert-zaremba]

also, maybe we should rename proposal_cancel_burn_rate to proposal_cancel_rate, wdyt?

[johnletey]

also, maybe we should rename proposal_cancel_burn_rate to proposal_cancel_rate, wdyt?

@robert-zaremba I would be in favour of proposal_cancel_ratio (so it's in line with min_initial_deposit_ratio).

[robert-zaremba]

I would be in favour of proposal_cancel_ratio (so it's in line with min_initial_deposit_ratio).

OK, makes sense!

[julienrbrt]

@gsk967 is it R4R again?

[gsk967]

@gsk967 is it R4R again?

Yes @julienrbrt , I refactored based on this Robert comment https://github.com/cosmos/cosmos-sdk/pull/13010#discussion_r1027755380

1. `(deposits * proposal_cancel_ratio)` will sent to `proposal_cancel_dest` if there otherwise it will burned.
2. `deposits * (1-proposal_cancel_ratio)` will sent back to depositors.

[gsk967]

@alexanderbez @robert-zaremba @atheeshp I updated docs and address the pr comments

[tac0turtle]

could we get a synopsis update of the current standing of this pr? I'd like to see it merged

[gsk967]

@robert-zaremba @amaurym @atheeshp @tac0turtle I address the pr comments, Please look into the changes

[tac0turtle]

seems there is a failing test, could we get it fixed

--- FAIL: TestAppStateDeterminism (846.24s)
[10035](https://github.com/cosmos/cosmos-sdk/actions/runs/3824520202/jobs/6506732847#step:5:10036)
panic: invalid argument to Intn [recovered]
[10036](https://github.com/cosmos/cosmos-sdk/actions/runs/3824520202/jobs/6506732847#step:5:10037)
	panic: invalid argument to Intn [recovered]
[10037](https://github.com/cosmos/cosmos-sdk/actions/runs/3824520202/jobs/6506732847#step:5:10038)
	panic: invalid argument to Intn

[gsk967]

utACK. I'm approving, the state machine logic LGTM, but I think the PR overall can largely be improved in being more rigorous (e.g. #13010 (comment) is not tested, and #13010 (comment)).

I would feel way more comfortable if we added more unit tests. For example, somethign like this for chargeDeposit (#13010 (comment)): CancelRatio=0 CancelRatio=0.5 CancelRatio=1 dest="" x x x dest="addr" x x x dest="community_pool" x x x

Would love to see an unit test like this where each x is tested.

@amaurym added tests for ChargeDeposit https://github.com/cosmos/cosmos-sdk/pull/13010/commits/388e97f2028e7ced706b3c62a21784b49700e1d9#diff-bcd7bacc86d5fd9d6ad48f3d1a5f4ff8975e8249a07a1d9d686e3376bf124dcdR219

[amaury1093]

@robert-zaremba @anilCSE @alexanderbez are you able to give another look at this PR, or lift your block?

[tac0turtle]

enabled auto merge, don't see any followups left

[gsk967]

Thanks all whoever support this feature, specially @anilCSE @robert-zaremba 🙏🏻🙏🏻🙏🏻