Proposal tally not increasing after voting

[fedekunze]

Summary of Bug

The proposal Tally Result is not increasing after submitting a vote.

Ref: luniehq/lunie#1591

Steps to Reproduce

Added a test case on lcd_test.go and failed.

We are not increasing the proposal tally after voting:

cosmos-sdk/x/gov/keeper.go

Lines 288 to 309 in 6e813ab

	func (keeper Keeper) AddVote(ctx sdk.Context, proposalID uint64, voterAddr sdk.AccAddress, option VoteOption) sdk.Error {
	proposal := keeper.GetProposal(ctx, proposalID)
	if proposal == nil {
	return ErrUnknownProposal(keeper.codespace, proposalID)
	}
	if proposal.GetStatus() != StatusVotingPeriod {
	return ErrInactiveProposal(keeper.codespace, proposalID)
	}
	if !validVoteOption(option) {
	return ErrInvalidVote(keeper.codespace, option)
	}
	vote := Vote{
	ProposalID: proposalID,
	Voter: voterAddr,
	Option: option,
	}
	keeper.setVote(ctx, proposalID, voterAddr, vote)
	return nil
	}

Actionable Item(s):

• Rename the field tally_result => final_tally_result

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[sunnya97]

That's not what TallyResult is for! It's the result of the vote and takes into account the staked weights of the voter. The TallyResult in state is only filled in once the voting is finished. If the proposal is still in the VotingPeriod, using the Tally querier endpoint, it doesn't read from state, but rather runs a tally at the current time, based on current stake weights (the stake weight may change by the time the real tally is run.

[fedekunze]

I'll reopen this bc I still think it needs further discussion. Maybe we can implement a workaround without rallying on the tally endpoint.

cc: @jackzampolin @johnmcdowall @alexanderbez @faboweb

[jackzampolin]

think it needs further discussion can you elaborate?

[fedekunze]

So right now if the proposal is on voting period the only way to get the current tally is using the /tally endpoint. We can get rid of that endpoint by returning a proposal with an updated tally_result from the tally querier if the proposal is on voting period:

if proposal.ProposalStatus == VotingPeriod {
   proposal.TallyResult = tallyQuerier()
}

[alexanderbez]

I agree in that I think it'll be a better UX if it's directly updated in the proposal. When I was playing around with proposals, I'd vote and query the proposal directly after and saw no updates to the tally...I thought that to be strange and it's not directly obvious to me that there is a tally endpoint/command.

[johnmcdowall]

Yes this was another point of API ergonomics that surprised me as well. Again, naming seems to be the issue here. I think the field in the proposal response should be called FinalTallyResult to clearly indicate that it is indeed the final static tally count, and convey that one should not expect it to update in real time.

Again though, we've chosen to store the final tally result here AND have an API which also will give you the final tally after the voting period, but we won't store the proposer address...

EDIT: I don't think we should get rid of the /tally endpoint as it is useful to display in realtime on UIs as votes are made.

[johnmcdowall]

Thinking on this a bit more I agree with @fedekunze. Since the TallyResult can only possibly change when a vote occurs, and that should take into account the stake weighting at the time, unless @sunnya97 knows any reason to not do that. I'm not sure what the difference between 'the real tally' and whatever the /tally endpoint is doing is.

[sunnya97]

I think the field in the proposal response should be called FinalTallyResult to clearly indicate that it is indeed the final static tally count, and convey that one should not expect it to update in real time.

I like this idea.

returning a proposal with an updated tally_result from the tally querier if the proposal is on voting period

   proposal.TallyResult = tallyQuerier()
}

This also works, but I prefer @johnmcdowall's proposal above because running a pseudo tally is a "somewhat expensive" operation, so I don't want to run it every time a proposal is queried. I think tally should be its own endpoint. Perhaps some full nodes might even want to not expose that endpoint.

I don't think we should get rid of the /tally endpoint as it is useful to display in realtime on UIs as votes are made.

Yep, this was the intention of this endpoint.

Since the TallyResult can only possibly change when a vote occurs, and that should take into account the stake weighting at the time

The governance tallying only takes into account the stake weighting at the end of the VotingPeriod, when the final tally takes place. Otherwise, I could delegate to one validator, they vote, then I redelegate my stake to another validator and they vote. Now my stake got double counted. The /tally endpoint (when called during a live VotingPeriod), runs a "pseudo-tally", it gives the result as if the VotingPeriod ended right now. However, the pseudo-tally returned by this endpoint might change by the end (as in go down), because let's say a bunch of stake that voted Yes suddenly unbonds.

[fedekunze]

EDIT: I don't think we should get rid of the /tally endpoint as it is useful to display in realtime on UIs as votes are made.

I personally think it's completely unnecessary since you can only use it when the proposal is on Voting Period.

The other option could be getting rid of the tally_result property on the proposal. I don't like the idea of having 2 ways to query the tally, it's just adding additional complexity.

[sunnya97]

@fedekunze
Calculating the current result of the tally during a voting period is super useful. A lot of UIs will want to show that. And I've been using it a lot to track the status of the active proposals on GoS.

What do you think about the suggestion:

I think the field in the proposal response should be called FinalTallyResult to clearly indicate that it is indeed the final static tally count, and convey that one should not expect it to update in real time.

[fedekunze]

Calculating the current result of the tally during a voting period is super useful.

I completely agree with that, it's just that having if-else checks for which way to query the partial result or the final result is not good.

I like the idea of the FinalTallyResult iff the /tally endpoint always returns the results (partial or completed) of the proposal tally

[sunnya97]

iff the /tally endpoint always returns the results (partial or completed) of the proposal tally

This is what it already currently does.

[fedekunze]

Cool ! then I fully agree on your proposal

[alexanderbez]

So is the actionable item here to simply rename the field tally_result => final_tally_result?

[jackzampolin]

Is that really it?

[alexanderbez]

@sunnya97 @fedekunze ?

[sunnya97]

I think so.