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

x/gov: remove querying deposits from events #9419

Closed
4 tasks
aleem1314 opened this issue May 28, 2021 · 2 comments · Fixed by #9519
Closed
4 tasks

x/gov: remove querying deposits from events #9419

aleem1314 opened this issue May 28, 2021 · 2 comments · Fixed by #9519
Assignees
Milestone

Comments

@aleem1314
Copy link
Contributor

aleem1314 commented May 28, 2021

Summary

Remove querying deposits from events in cli.

Problem Definition

#9288 comments

Currently in SDK every proposal's deposits are being removed once after it is completed(completed in the sense either passed or rejected)

  • in case of passed proposals all the deposit amount will be refunded to the depositors and then deposits state will be prune after refunding the deposit amount. (see here)
  • for rejected proposals all the deposits amount are burn and state is pruned. (see here)

Those proposals which did not meet minimum deposits are being removed from proposals state along with it's deposits. In this case deposits are not required to keep in the state since the proposal data itself being removed from state.

  • doubt 1: if we have to keep all the deposits in state, do we need to keep passed proposals deposits as well? (I think we should)
  • doubt 2: currently deleteDeposits function is being used to delete deposits, but along with that it is burning the deposits for removed, rejected proposals (I think we need to separate the logic to two separate functions 1.deleteAndBurnDeposits, 2.burnDeposits)
    • incase of burnDeposits only the deposit amount will get burnt but the deposits for the proposal won't be pruned from state.
    • incase of deleteAndBurnDeposits all the deposits burnt and deposits will be pruned for the proposal.

problems for splitting proposals deleteDeposits:

  • invariants will break. here
  • simulations of genesis import export for x/gov fails. here

if we stop burning tokens for failed proposals there are no changes required in invariants and genesis (but I don't think this is the fix).

problems if we remove query events for deposits:

  • querying initial deposits may not work well(not sure where we need to query the initial deposits) and deposits[0] is not always same if we query the deposits(not sure why)
  • if the same proposer(who created the proposal) deposits again that will be added to the existing deposit amount in the state.

Proposal

  • Update the Proposal deposits record handling logic - to not remove deposits for rejected proposals
  • make a migration to recover deposit records

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093
Copy link
Contributor

Is this actually blocking v0.43, or just a nice-to-have?

@atheeshp
Copy link
Contributor

atheeshp commented Jun 9, 2021

Is this actually blocking v0.43, or just a nice-to-have?

Not a blocker, nice to have

@mergify mergify bot closed this as completed in #9519 Jul 30, 2021
mergify bot pushed a commit that referenced this issue Jul 30, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: #9419 

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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...

- [x] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [x] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [x] reviewed API design and naming
- [x] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants