livthomas commented 7 months ago

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

Fetch gov votes via Heimdall API: https://heimdall-api.polygon.technology/gov/proposals/12/votes
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
Compare the number of results
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 commented 7 months ago

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

codecov[bot] commented 7 months ago

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

```diff @@ 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 commented 7 months ago

@marcello33 I've changed it.

svenski123 commented 6 months ago

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 commented 6 months ago

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

marcello33 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

Hey @livthomas any updates on this? Thank you.

github-actions[bot] commented 5 months ago

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] commented 4 months ago

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.

maticnetwork / heimdall

fix: return all gov votes by tx query #1138

Description

Steps to reproduce

Changes

Nodes audience

Checklist

Cross repository changes

Testing

Additional comments

Codecov Report

1031