fix: x/gov deposits querier (Initial Deposit) #9288

aleem1314 · 2021-05-10T06:09:20Z

Description

continued from original PR
closes: #8758

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2021-05-10T06:18:38Z

Codecov Report

Merging #9288 (fe6487d) into master (90edeb6) will increase coverage by 0.03%.
The diff coverage is 65.10%.

❗ Current head fe6487d differs from pull request most recent head 4123e05. Consider uploading reports for the commit 4123e05 to get more accurate results

@@            Coverage Diff             @@
##           master    #9288      +/-   ##
==========================================
+ Coverage   60.39%   60.42%   +0.03%     
==========================================
  Files         589      590       +1     
  Lines       37105    37237     +132     
==========================================
+ Hits        22411    22502      +91     
- Misses      12715    12754      +39     
- Partials     1979     1981       +2

Impacted Files	Coverage Δ
x/gov/client/cli/query.go	`0.00% <0.00%> (ø)`
x/gov/client/utils/query.go	`23.67% <0.00%> (-4.49%)`	⬇️
x/gov/client/testutil/deposits.go	`93.75% <93.75%> (ø)`
x/gov/client/testutil/helpers.go	`100.00% <100.00%> (ø)`
store/types/gas.go	`79.16% <0.00%> (-2.32%)`	⬇️

robert-zaremba

I'm not sure why we need to query events - we record deposit when doing SubmitProposal.

Also, we should charge extra fee for browsing events. It will be discarded in query mode, but if we use that query in a transaction mode, then it will be accounted properly.

x/gov/client/utils/query.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

…osmos-sdk into aleem/8758-gov-deposits

atheeshp · 2021-05-11T08:16:34Z

I'm not sure why we need to query events - we record deposit when doing SubmitProposal.

afaik there are few cases where the deposits data will be deleted from store(ref : EndBlocker).

Case 1: If a proposal didn't reach minimum deposit amount falls into inactive state, hence both proposal and deposits will be deleted from store. (to start voting period minimum deposit is required.)

Case 2: In the following possibilities all the deposits will get burnt and it's data will be deleted from store
i. If there is not enough quorum of votes, the proposal fails
ii. If more than 1/3 of voters veto, proposal fails

Hence we need query by events to query the deleted deposits data.

cc: @anilcse, @AmauryM

robert-zaremba · 2021-05-26T16:17:26Z

Plan:

For 0.42 - we use this PR
For 0.43 - we do a new PR:
- Update the Proposal deposits record handling logic - to not remove deposits for rejected proposals
- Port tests from this PR
- make a migration to recover deposit records

robert-zaremba

preapproving

robert-zaremba

We need to add more tests. I suggest to do unit tests - maybe we can break down the functions to allow that so we don't need to use CLI for tests.

Missing tests:

rejected proposal
proposal with not enough deposit
proposal deposit before voting end

robert-zaremba · 2021-05-28T15:47:10Z

x/gov/client/testutil/deposits.go

+	cmd := cli.NewCmdDeposit()
+	_, err = clitestutil.ExecTestCLICmd(clientCtx, cmd, args)
+	s.Require().NoError(err)
+


let's query the deposit before the voting as well.

…osmos-sdk into aleem/8758-gov-deposits

robert-zaremba

good job!

* copied from old PR * fix errors * add test * Update x/gov/client/utils/query.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * fix tests * fix failing test * add test * update test * fix tests * fix deposit query * fix test * update tests * add more tests * address lint error * address lint error * review changes Co-authored-by: Robert Zaremba <robert@zaremba.ch> (cherry picked from commit 66ee994) # Conflicts: # CHANGELOG.md # x/gov/client/cli/query.go # x/gov/client/testutil/cli_test.go # x/gov/client/utils/query.go

* fix: x/gov deposits querier (Initial Deposit) (#9288) * copied from old PR * fix errors * add test * Update x/gov/client/utils/query.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * fix tests * fix failing test * add test * update test * fix tests * fix deposit query * fix test * update tests * add more tests * address lint error * address lint error * review changes Co-authored-by: Robert Zaremba <robert@zaremba.ch> (cherry picked from commit 66ee994) # Conflicts: # CHANGELOG.md # x/gov/client/cli/query.go # x/gov/client/testutil/cli_test.go # x/gov/client/utils/query.go * resolve conflicts Co-authored-by: MD Aleem <72057206+aleem1314@users.noreply.github.com> Co-authored-by: aleem1314 <aleem@vitwit.com>

* CLI: `query ibc-transfer escrow-address` (cosmos#9383) * Fix the liveliness test Docker error (cosmos#9396) (cosmos#9402) (cherry picked from commit 44a4138) Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com> * docs: fix broken interfaces links (backport cosmos#9448) (cosmos#9478) * fix interfaces links (cosmos#9448) Co-authored-by: ryanchrypto <12519942+ryanchrypto@users.noreply.github.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> (cherry picked from commit 37fc37d) # Conflicts: # docs/building-modules/messages-and-queries.md # docs/building-modules/module-interfaces.md # docs/building-modules/module-manager.md # docs/building-modules/query-services.md * resolve merge conflicts * revert rename codec Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> Co-authored-by: ryanchrypto <12519942+ryanchrypto@users.noreply.github.com> * backport: fix ibc genesis export bug (cosmos#9401) * correctly set next identifier sequence in genesis export for clients/connections/channels * add changelog * fix linting (cosmos#9531) * fix: testnet cli command update genesis supply (backport cosmos#9497) (cosmos#9513) * fix: testnet cli command update genesis supply (cosmos#9497)  ## Description closes: cosmos#9372  ### This PR makes the `testnet` command update the bank genesis supply. When using the `testnet` cli command, it creates nodes and balances, but does **not** update the supply. When using this in conjunction with `add-genesis-account` which **does** update the supply, it creates an invalid genesis file. This PR updates the testnet command to properly set the supply. * feat: add header hash to `Context` (backport cosmos#9390) (cosmos#9395) * feat: add header hash to `Context` (cosmos#9390) * baseapp, types: add header hash to * changelog (cherry picked from commit 151d6c5) # Conflicts: # CHANGELOG.md * Fix conflicts Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com> * fix: x/gov deposits querier (Initial Deposit) (backport cosmos#9288) (cosmos#9453) * fix: x/gov deposits querier (Initial Deposit) (cosmos#9288) * copied from old PR * fix errors * add test * Update x/gov/client/utils/query.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * fix tests * fix failing test * add test * update test * fix tests * fix deposit query * fix test * update tests * add more tests * address lint error * address lint error * review changes Co-authored-by: Robert Zaremba <robert@zaremba.ch> (cherry picked from commit 66ee994) # Conflicts: # CHANGELOG.md # x/gov/client/cli/query.go # x/gov/client/testutil/cli_test.go # x/gov/client/utils/query.go * resolve conflicts Co-authored-by: MD Aleem <72057206+aleem1314@users.noreply.github.com> Co-authored-by: aleem1314 <aleem@vitwit.com> * feat: add `RefundGas` function to `GasMeter` (backport cosmos#9403) (cosmos#9444) * feat: add `RefundGas` function to `GasMeter` (cosmos#9403) * feat: add RefundGas function to GasMeter * changelog * add comment about use case * Apply suggestions from code review Co-authored-by: Alessio Treglia <alessio@tendermint.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> (cherry picked from commit 90edeb6) # Conflicts: # CHANGELOG.md * conflicts * fix * Update CHANGELOG.md Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Federico Kunze <federico.kunze94@gmail.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> * fix: Bank module init genesis optimization (backport cosmos#9428) (cosmos#9440) * fix: Bank module init genesis optimization (cosmos#9428) * optimize the bank module genesis initialization * remove k.setBalances & k.clearBalances and update changelog * fix lint Co-authored-by: Aaron Craelius <aaron@regen.network> (cherry picked from commit 2ae7875) # Conflicts: # CHANGELOG.md # x/bank/keeper/genesis.go # x/bank/keeper/send.go * fix conflicts * Update CHANGELOG.md Co-authored-by: yys <sw.yunsuk@gmail.com> Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com> * fix: update simapp to use correct default broadcast mode (backport cosmos#9408) (cosmos#9527) * fix: update simapp to use correct default broadcast mode (cosmos#9408) (cherry picked from commit 80330ec) * Add changelog Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com> * Backport: IBC query header/node-state fixes (cosmos#9385) * fix ibc query header/node-state cmds * changelog Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com> * build(deps): tendermint version (backport cosmos#9541) (cosmos#9542) * build(deps): tendermint version (cosmos#9541) * bump tendermint version * go mod tidy (cherry picked from commit e4673ad) * add changelog entry * Update CHANGELOG.md * Update CHANGELOG.md Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> * feat: add cosmos-sdk Version (backport cosmos#9429) (cosmos#9543) * feat: add cosmos-sdk Version (cosmos#9429)  ## Description Add CosmosSDKVersion to nodeInfo. closes: cosmos#9420 (cherry picked from commit 105ad99) # Conflicts: # CHANGELOG.md # CONTRIBUTING.md # docs/core/proto-docs.md * resolve conflicts * resolve conflicts Co-authored-by: Marko <marbar3778@yahoo.com> * fix: set header hash every block (backport cosmos#9552) (cosmos#9555) * fix: set header hash every block (cosmos#9552) ## Description - Sets the header hash on every block (ref cosmos#9390). Previously was only set during initialization for `deliverState`. - Closes cosmos#9514  * chore: Update release notes and Changelog for 0.42.6 (cosmos#9544) * Update release notes * Clean up changelog Co-authored-by: Ethan Buchman <ethan@coinculture.info> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com> Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> Co-authored-by: ryanchrypto <12519942+ryanchrypto@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com> Co-authored-by: MD Aleem <72057206+aleem1314@users.noreply.github.com> Co-authored-by: aleem1314 <aleem@vitwit.com> Co-authored-by: Federico Kunze <federico.kunze94@gmail.com> Co-authored-by: yys <sw.yunsuk@gmail.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

aleem1314 added 3 commits May 7, 2021 16:26

copied from old PR

6ea333c

fix errors

0de858b

add test

64232e8

github-actions bot added C:CLI C:x/gov labels May 10, 2021

Merge branch 'master' into aleem/8758-gov-deposits

e185e33

aleem1314 marked this pull request as ready for review May 10, 2021 06:28

aleem1314 requested review from aaronc and alexanderbez as code owners May 10, 2021 06:28

aleem1314 requested review from amaury1093, robert-zaremba, anilcse and atheeshp May 10, 2021 06:29

robert-zaremba mentioned this pull request May 10, 2021

[Bugfix] Gov Deposits Querier (Initial Deposit) #8813

Closed

9 tasks

robert-zaremba reviewed May 10, 2021

View reviewed changes

x/gov/client/utils/query.go Outdated Show resolved Hide resolved

x/gov/client/utils/query.go Show resolved Hide resolved

x/gov/client/utils/query.go Show resolved Hide resolved

x/gov/client/utils/query.go Show resolved Hide resolved

aleem1314 and others added 5 commits May 10, 2021 16:30

Update x/gov/client/utils/query.go

0d7b764

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

fix tests

5d8cf40

Merge branch 'aleem/8758-gov-deposits' of https://github.com/cosmos/c…

6e81021

…osmos-sdk into aleem/8758-gov-deposits

Merge branch 'master' into aleem/8758-gov-deposits

8b4d075

fix failing test

0048d2a

aleem1314 marked this pull request as draft May 10, 2021 15:20

add test

2a69de4

aleem1314 added 2 commits May 11, 2021 13:58

update test

12457b9

fix tests

22fe967

clevinson added the backport/0.42.x (Stargate) label May 11, 2021

clevinson added this to the v0.43 milestone May 11, 2021

clevinson added the Status: Backlog label May 11, 2021

fix deposit query

a1daaa4

aleem1314 added 2 commits May 27, 2021 11:45

update tests

ef11cae

Merge branch 'master' into aleem/8758-gov-deposits

ff024ce

aleem1314 changed the title ~~x/gov deposits querier (Initial Deposit)~~ fix: x/gov deposits querier (Initial Deposit) May 27, 2021

Merge branch 'master' into aleem/8758-gov-deposits

1424885

alexanderbez approved these changes May 27, 2021

View reviewed changes

aleem1314 requested a review from robert-zaremba May 27, 2021 12:42

aleem1314 mentioned this pull request May 28, 2021

x/gov: remove querying deposits from events #9419

Closed

4 tasks

Merge branch 'master' into aleem/8758-gov-deposits

c74c3b8

robert-zaremba approved these changes May 28, 2021

View reviewed changes

robert-zaremba requested changes May 28, 2021

View reviewed changes

aleem1314 and others added 6 commits May 31, 2021 16:16

add more tests

fdbc334

address lint error

5ad7a69

address lint error

1725f6e

Merge branch 'master' into aleem/8758-gov-deposits

f4fab49

review changes

7dc95c1

Merge branch 'aleem/8758-gov-deposits' of https://github.com/cosmos/c…

8175663

…osmos-sdk into aleem/8758-gov-deposits

aleem1314 requested a review from robert-zaremba June 1, 2021 12:55

robert-zaremba approved these changes Jun 1, 2021

View reviewed changes

Merge branch 'master' into aleem/8758-gov-deposits

fe6487d

aleem1314 added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 3, 2021

Merge branch 'master' into aleem/8758-gov-deposits

4123e05

mergify bot merged commit 66ee994 into master Jun 3, 2021

mergify bot deleted the aleem/8758-gov-deposits branch June 3, 2021 07:36

mergify bot mentioned this pull request Jun 3, 2021

fix: x/gov deposits querier (Initial Deposit) (backport #9288) #9453

Merged

amaury1093 removed the Status: Backlog label Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: x/gov deposits querier (Initial Deposit) #9288

fix: x/gov deposits querier (Initial Deposit) #9288

aleem1314 commented May 10, 2021 •

edited

Loading

codecov bot commented May 10, 2021 •

edited

Loading

robert-zaremba left a comment

atheeshp commented May 11, 2021 •

edited

Loading

robert-zaremba commented May 26, 2021 •

edited by aleem1314

Loading

robert-zaremba left a comment

robert-zaremba left a comment

robert-zaremba May 28, 2021

robert-zaremba left a comment

fix: x/gov deposits querier (Initial Deposit) #9288

fix: x/gov deposits querier (Initial Deposit) #9288

Conversation

aleem1314 commented May 10, 2021 • edited Loading

Description

codecov bot commented May 10, 2021 • edited Loading

Codecov Report

robert-zaremba left a comment

Choose a reason for hiding this comment

atheeshp commented May 11, 2021 • edited Loading

robert-zaremba commented May 26, 2021 • edited by aleem1314 Loading

Plan:

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba May 28, 2021

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

aleem1314 commented May 10, 2021 •

edited

Loading

codecov bot commented May 10, 2021 •

edited

Loading

atheeshp commented May 11, 2021 •

edited

Loading

robert-zaremba commented May 26, 2021 •

edited by aleem1314

Loading