fix: return all gov votes by tx query

[livthomas]

Description

If you try to fetch votes for any given governance proposal via REST API, you will get no more than 30 records. This is the default value originally set by Tendermint. But there are no parameters that would allow users to get the second and all subsequent pages.

The changes introduced in this pull request aggregate votes from all pages in a response using maximum page size in each query. Ideally, there should be pagination params introduced to both the REST API and CLI methods. However, that would require adding pagination info to the response data which would probably be a breaking change.

Steps to reproduce

1. Fetch gov votes via Heimdall API: https://heimdall-api.polygon.technology/gov/proposals/12/votes
2. Fetch gov vote transactions via Peppermint RPC: https://polygon-mainnet-rpc.allthatnode.com:26657/tx_search?query=%22message.action%3D'vote'%20AND%20proposal_vote.proposal_id%20%3D%2012%22&per_page=100
3. Compare the number of results
4. Try to call Peppermint RPC without per_page parameter

Changes

• [x] Bugfix (non-breaking change that solves an issue)
• [ ] Hotfix (change that solves an urgent issue, and requires immediate attention)
• [ ] New feature (non-breaking change that adds functionality)
• [ ] Breaking change (change that is not backwards-compatible and/or changes current functionality)
• [ ] Changes only for a subset of nodes

Nodes audience

In case this PR includes changes that must be applied only to a subset of nodes, please specify how you handled it (e.g. by adding a flag with a default value...)

Checklist

• [ ] I have added at least 2 reviewer or the whole pos-v1 team
• [ ] I have added sufficient documentation in code
• [ ] I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply

Cross repository changes

• [ ] This PR requires changes to bor
  • In case link the PR here:
• [ ] This PR requires changes to matic-cli
  • In case link the PR here:

Testing

• [ ] I have added unit tests
• [ ] I have added tests to CI
• [ ] I have tested this code manually on local environment
• [ ] I have tested this code manually on remote devnet using express-cli
• [ ] I have tested this code manually on mumbai
• [ ] I have created new e2e tests into express-cli

Additional comments

There might be similar issues with other REST API methods (e.g. deposits) but I haven't tested them.

[marcello33]

Hi @livthomas Thanks for your contribution Can you please raise against develop branch? I'll run the CI on it.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.59%. Comparing base (90895e8) to head (90b84c6). Report is 24 commits behind head on develop.

:exclamation: Current head 90b84c6 differs from pull request most recent head c438f69

Please upload reports for the commit c438f69 to get more accurate results.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1138   +/-   ##
========================================
  Coverage    76.59%   76.59%           
========================================
  Files           53       53           
  Lines         5922     5922           
========================================
  Hits          4536     4536           
  Misses        1128     1128           
  Partials       258      258           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[livthomas]

@marcello33 I've changed it.

[svenski123]

This is the same fix to governance that I raised in the PR below.

The team decided to close that PR rather than fix the obvious bug - hopefully this time it will get through.

https://github.com/maticnetwork/heimdall/pull/1031

[marcello33]

Hey, @livthomas can you please add a unit/e2e test to make sure pagination works as expected? Thanks

[marcello33]

This is the same fix to governance that I raised in the PR below.

The team decided to close that PR rather than fix the obvious bug - hopefully this time it will get through.

#1031

Hi @svenski123 It's not exactly the same PR as yours was changing the defaultLimit, which violated the comment // should be consistent with maxPerPage in tendermint/tendermint/rpc/core/pipe.go:23

We asked you here to make it consistent with such limit, and implement tests here.

Since you raised the PR (August 2023, 10th) we asked you multiple times to make such changes until we eventually closed the PR on December 2023, 14th, after some months without any reply.

We welcome any external contributor, we just wanted to make sure the requested changes were provided before merging the PR. Sorry if this was misinterpreted.

[livthomas]

@marcello33 I can write some unit tests for this but it looks like the tests of the gov module are disabled by default: https://github.com/maticnetwork/heimdall/blob/master/Makefile#L22-L25

And when I try to run them using go test -v ./gov/, I get the following failures:

go test  -v ./gov
# github.com/maticnetwork/heimdall/gov [github.com/maticnetwork/heimdall/gov.test]
gov/querier_test.go:17:94: undefined: DepositParams
gov/querier_test.go:17:109: undefined: VotingParams
gov/querier_test.go:17:123: undefined: TallyParams
gov/querier_test.go:60:114: undefined: Proposal
gov/querier_test.go:76:136: undefined: ProposalStatus
gov/querier_test.go:76:168: undefined: Proposal
gov/querier_test.go:92:139: undefined: Deposit
gov/querier_test.go:108:116: undefined: Deposit
gov/querier_test.go:124:132: undefined: Vote
gov/querier_test.go:140:113: undefined: Vote
gov/querier_test.go:140:113: too many errors
FAIL    github.com/maticnetwork/heimdall/gov [build failed]
FAIL

[marcello33]

@marcello33 I can write some unit tests for this but it looks like the tests of the gov module are disabled by default: https://github.com/maticnetwork/heimdall/blob/master/Makefile#L22-L25

And when I try to run them using go test -v ./gov/, I get the following failures:

go test  -v ./gov
# github.com/maticnetwork/heimdall/gov [github.com/maticnetwork/heimdall/gov.test]
gov/querier_test.go:17:94: undefined: DepositParams
gov/querier_test.go:17:109: undefined: VotingParams
gov/querier_test.go:17:123: undefined: TallyParams
gov/querier_test.go:60:114: undefined: Proposal
gov/querier_test.go:76:136: undefined: ProposalStatus
gov/querier_test.go:76:168: undefined: Proposal
gov/querier_test.go:92:139: undefined: Deposit
gov/querier_test.go:108:116: undefined: Deposit
gov/querier_test.go:124:132: undefined: Vote
gov/querier_test.go:140:113: undefined: Vote
gov/querier_test.go:140:113: too many errors
FAIL    github.com/maticnetwork/heimdall/gov [build failed]
FAIL

Ok. Will test this and get back then. Thank you

[marcello33]

Hey @livthomas After deploying this branch to a mainnet node, I still get the same response (30 results). Did you test it on a running node? Can we get any instructions/proof on how to test this properly? Thanks

[svenski123]

Hi @svenski123 It's not exactly the same PR as yours was changing the defaultLimit, which violated the comment // should be consistent with maxPerPage in tendermint/tendermint/rpc/core/pipe.go:23 The governance tool is broken due to pagination - both PRs address this by looping over the result set and are effectively the same, aside from immaterial differences.

There is no need for defaultLimit to be the same in the client and server code - indeed anyone can query the server side using whatever http client they passing whatever page size value they wish and the server side should handle it all gracefully (and it does AFAIK).

The purpose of the PR was to fix governance tools there were clearly broken following the on chain vote I participated in.

We asked you here to make it consistent with such limit, and implement tests here.

You asked for changes over three months after I raised the PR.

Since you raised the PR (August 2023, 10th) we asked you multiple times to make such changes until we eventually closed the PR on December 2023, 14th, after some months without any reply.

Again, over three months after I raised the PR. Broken governance tools are a particular concern as a validator. My expectation was that your team would eventually fix the bug in a way it saw fit - as it did for PR #1004. That it did not is extremely disappointing.

We welcome any external contributor, we just wanted to make sure the requested changes were provided before merging the PR. Sorry if this was misinterpreted.

Professionally run projects acknowledge contributions promptly (within a business day). Given a delay of months, any competent contributor will correctly infer that their time is not valued and go elsewhere. A technical project that continues to do this is "NGMI".

[svenski123]

Hey @livthomas After deploying this branch to a mainnet node, I still get the same response (30 results). Did you test it on a running node? Can we get any instructions/proof on how to test this properly? Thanks

I can see @livthomas posted reproduction steps in the description. I also included a simple reproduction command line in the description of #1031.

[livthomas]

Hey @livthomas After deploying this branch to a mainnet node, I still get the same response (30 results). Did you test it on a running node? Can we get any instructions/proof on how to test this properly? Thanks

No, I haven't tested this. I was hoping it was covered by tests run in CI. I was trying to fix gov/querier_test.go but there were just too many errors. It looks like this file hasn't been touched for over 4 years so I guess it is probably quite outdated.

What is the easiest way for me to quickly run the Heimdall node in order to test this fix?

[marcello33]

Hey @livthomas After deploying this branch to a mainnet node, I still get the same response (30 results). Did you test it on a running node? Can we get any instructions/proof on how to test this properly? Thanks

No, I haven't tested this. I was hoping it was covered by tests run in CI. I was trying to fix gov/querier_test.go but there were just too many errors. It looks like this file hasn't been touched for over 4 years so I guess it is probably quite outdated.

What is the easiest way for me to quickly run the Heimdall node in order to test this fix?

Hey @livthomas I simply deployed your changes on a mainnet node, but unfortunately it does not work as expected. So, if you're not running any node on mainnet, mumbai or amoy, then the easiest way for you to test (and fix) this code would be to use matic-cli. You can use that tool to spin up a devnet (on your local system or remotely using the supported cloud platforms AWS or GCP). It supports both bare setup and dockerized environments. Then you can use that devnet to simulate this use case. Thank you

[marcello33]

Hey @livthomas any updates on this? Thank you.

[github-actions[bot]]

This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions[bot]]

This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days.