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

Governance API for votes on a proposal returns null after voting period closes #2879

Closed
4 tasks
jmdfm opened this issue Nov 22, 2018 · 12 comments · Fixed by #3091
Closed
4 tasks

Governance API for votes on a proposal returns null after voting period closes #2879

jmdfm opened this issue Nov 22, 2018 · 12 comments · Fixed by #3091

Comments

@jmdfm
Copy link
Contributor

jmdfm commented Nov 22, 2018

Summary of Bug

The Governance API for votes on a proposal (/gov/proposals/:id/votes) returns null instead of the Votes that were associated with the proposal after the voting period closes.

This is unexpected.

It would be expected that the voting period has no effect on the JSON payload returned by a call to /gov/proposals/:id/votes, and that the call reliably returns the votes cast against a proposal whenever it is called.

Steps to Reproduce

  1. Create a Proposal
  2. Make a Vote against a Proposal
  3. Call the Governance API to retrieve the Votes against a Proposal, see that you get a valid JSON payload as a response.
  4. Wait for the Voting period to close.
  5. Make the same call you made in step 3, except this time you will receive null as a response.

For Admin Use

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

I'm shocked that this does not work. Is this expected behavior @sunnya97?

@sunnya97
Copy link
Member

After the votes have been counted and tallied, they are pruned from state.

You can get the TallyResult by using the Tally endpoint, but to get the actual individual votes themselves, you're going to need to query a past block height, during the voting period.

@rfunduk
Copy link

rfunduk commented Nov 24, 2018

Can LCD not handle this behind the scenes? We currently are scanning and storing limited info for every block, but as I understand it that is not what the Cosmos team considers ideal or necessary for projects like Hubble (which this issue is ultimately related to). We have plans to prune our store of old blocks at some point, and this would put a distinct extra caveat on that -- making sure not to prune a block we may need to have later in order to find votes for a proposal.

Conceptually, it seems to me that the LCD should handle this transparently and just always return the most recent vote set when querying /gov/proposals/:id/votes. The 'wording', if you will, of that endpoint suggests that's what will happen since it does not take a block height or anything as argument as far as I know. I would expect to just get a high level response of 'the votes' at this endpoint and not have to take into account implementation details (like pruning) and a bunch of other intricacies such as the voting period and what blocks would have been part of that period, etc.

@jmdfm
Copy link
Contributor Author

jmdfm commented Nov 26, 2018

@sunnya97 What's the rationale for pruning? Seems like a minimal amount of state space to save at the increase of a much worse scanning requirement?

@fedekunze
Copy link
Collaborator

but to get the actual individual votes themselves, you're going to need to query a past block height, during the voting period.

@sunnya97 clients should be able to query a proposal votes at all times, same with deposits. CC: @faboweb

@faboweb
Copy link
Contributor

faboweb commented Nov 27, 2018

I agree with the previous posters. Let's design the API so it behaves like users and devs expect it to behave.

@ValarDragon
Copy link
Contributor

@sunnya97 What's the rationale for pruning? Seems like a minimal amount of state space to save at the increase of a much worse scanning requirement?

Not at all. We anticipate far more than just the validators to be submitting votes. I don't see how not pruning votes from state would even be an option. State is only meant for things that need to be persisted for all perpetuity, and should be quite costly. We are planning on one day incorporating storage rent so that it costs money per block to have things in state.

The indexer is meant to handle things like this. We can make the API for the LCD perhaps use the tx indexer in this case.

@alexanderbez
Copy link
Contributor

We could use tags for something like this, right?

@sunnya97
Copy link
Member

sunnya97 commented Nov 29, 2018

Regarding not pruning, yes, exactly what @ValarDragon said. It's not feasible to keep stuff in state, just because clients want to be able to read it. The LCD at the current moment, is based off of queriers who query state at a specific block height, which is why if you need to pass in a block height at which the votes were still in state if the proposal's voting period has ended (you can get the time at which it ended, and then use the last block height before that time. I think it would be fair to add to each proposal, the height at which it ended, so you don't have to calculate it just off of the time).

Now, the question is, do you want the LCD to do all that fanciness itself, or leave this logic up to the client (wallet or explorer or whatever). Here's how I see it:

Pros of doing this logic in the LCD:

  • Simpler for clients

Cons of doing it in the LCD:

  • What do you do if the node the LCD is talking to is "super-pruning"? As in it doesn't have any history whatsoever, other than the latest block. It won't be able to do this logic, and we need to add these errors and edge case handling for all these different scenarios into the LCD logic.
  • I actually want to move towards a model where the developers don't write LCDs and CLIs themselves, but rather they are auto-generated based off of the queriers.

@fedekunze
Copy link
Collaborator

fedekunze commented Nov 30, 2018

@sunnya97 @alexanderbez @ValarDragon what do you think about doing a tx search with tags when the proposal is not active (i.e Passed or Rejected) ? In that case the user will still be able to query the endpoint and have the same logic + the votes/deposits will be pruned from the state.

The only thing we'd need to add will be keeping only the last vote of the voter and tossing the rest from the search results. In the case of the deposits we'd have to sum the amounts deposited.

@alexanderbez
Copy link
Contributor

I think generally that is what we've been suggesting is it not? Regarding tossing the vote -- why? Isn't it just a proposalID => votes tag?

@jmdfm
Copy link
Contributor Author

jmdfm commented Dec 6, 2018

The LCD at the current moment, is based off of queriers who query state at a specific block height, which is why if you need to pass in a block height at which the votes were still in state if the proposal's voting period has ended (you can get the time at which it ended, and then use the last block height before that time.

I don't see any way to use LCD to get the block height for a given time. How would you attempt this right now, given the voting period close time? Or is it expected you would just manually scan the blocks until you find the block preceding the vote closing time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants