Make GovernorCountingFractional accept partial votes

[davidlaprade]

Implements #13

[mds1]

looks great!

[apbendi]

Looking good @davidlaprade, thanks!

[apbendi]

Hmm, I think there's a subtle issue here that I'm a little worried about: currently if hasVoted returns true there is an implicit assumption that they won't vote again and their votes won't change. We're changing that, and I could see it breaking tooling or extension contracts that make that assumption. Not sure what if any action we can take is, though. Hmmm.

[mds1]

Interesting point. The docs for hasVoted are (understandly) ambiguous on this point:

Returns whether account has cast a vote on proposalId.

So we aren't breaking that definition. But to your point people almost certainly have interpreted this as a binary "all or none", since there was no flex voting until now. I think the options are:

1. Do nothing, since this is technically fine. Confirm this is ok and won't break much by searching for some on/off-chain hasVoted usage via sourcegraph
2. Add a new hasFinishedVoting that returns true if you've voted with all your weight, to ensure there's a method with the original implied semantics
3. Restore the implied semantics of this method and add a hasPartiallyVoted method

I'm personally ok with any of these, perhaps we pull this out into a follow up issue and ask OZ what they'd prefer, to increases chances of getting this upstreamed?

[davidlaprade]

Yeah, it's a good point. I'm also not sure what to do about this. Having this return false until all weight is cast seems confusing as well -- maybe even more so.

I think my current vote is to leave as-is since (as @mds1 pointed out) that's consistent with the docs, then ask OZ what they recommend

[davidlaprade]

@rsolari (since I know you discussed this feature with @apbendi) I'm curious if you have any take on this question.

TLDR: how should hasVoted work if it's possible for people to cast partial votes?

Some options:

1. it could return true so long as some weight has been cast, even if not all weight has been cast
2. it could return false unless all weight has been cast
3. some combination of (1) and then also add another function like hasFullyVoted to capture the behavior that hasVoted used to have
4. something else… we’re all ears

What would be best from your (Tally's) point of view

[rsolari]

Tally doesn't use hasVoted() right now, so I don't have a strong opinion from Tally's perspective.

I can imagine an existing frontend using hasVoted() to see whether a user is still able to vote. I could also imagine an existing subgraph/indexer to using it to count up votes cast.

For a Flexible Voter, both an indexer and a frontend would need to know how many votes are cast so far. We can't answer that with a boolean, so I don't think option 3 really helps. I'd suggest option 1 because it keeps the semantics from IGovernor: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/91e8d0ba3c3beb8a1db31310d8599664e48639ef/contracts/governance/IGovernor.sol#L187

I'd also suggest adding a helper e.g. votesCast(uint256 proposalId, address account) for getting the votes so far. Tally would just index the events here to avoid RPC calls, but it's nice to give the option to smart contracts and frontends.

[mds1]

@davidlaprade mind pulling this out into a separate issue, then we can mark resolved?

[davidlaprade]

Awesome, thanks so much for the input @rsolari!

I think I agree that adding a new helper would be the best approach here. Tracking this in #37

[apbendi]

So, if I understand how this implementation works, the call to castVote submits the marginal votes (i.e. the new votes) to the contract. This works, but I'm trying to think through the implications for the delegate contract, which now needs to track how much weight has or hasn't been submitted, or else look it up from the Governor.

The alternative approach would be to assume the delegate contract always votes with 100% of votes each time, and then have our Governor check that the For/Against/Abstain values for the current vote are less than the For/Against/Abstain values for the previous vote.

Not sure one is better than the other just trying to make sure we've considered the tradeoffs. What are your thoughts?

[mds1]

Good callout, I don't think we really discussed and I think this approach was just implied.

Seems the current approach is cheaper because we don't currently track an individual user's split, whereas voting with the new full total amount would require us to track that for each user, so I think you'd end up doing more reading/writing than currently

[davidlaprade]

This works, but I'm trying to think through the implications for the delegate contract, which now needs to track how much weight has or hasn't been submitted, or else look it up from the Governor

Does it? I don't think that's necessarily true. For example, when we remove CAST_VOTE_WINDOW in our flex voting AToken in #18 I don't think we'll need to do it. As long as the client/delegate contract knows how many votes any given user is owed for a given proposal, and it does this internal accounting properly, that should be sufficient.

Just to make this concrete, here's an example:

• Alice deposits 25 UNI into a flex voting aToken
• Bob deposits 60 UNI
• Charlie deposits 15 UNI
• so 100 UNI has been deposited in total now
• Dave borrows 80 UNI
• the aToken's UNI balance is now 20 UNI
• a UNI proposal is issued
• Alice expresses her FOR vote to the aToken
• Bob expresses an AGAINST vote to the aToken
• someone calls aToken.castVote, rolling these two votes up together and submitting them fractionally to the governor
  • to do this, the aToken asks the governance token what its balance (i.e. voting weight) was at the proposal snapshot, it is 20 UNI
  • the aToken knows that Alice is responsible for 25 of the 100 total UNI deposited, so it casts 5 FOR votes for her (25% of its available 20 UNI voting weight)
  • the aToken knows that Bob is responsible for 60 of the 100 total UNI deposited, so it casts 12 AGAINST votes for him (60% of its available 20 UNI voting weight)
• sometime later, Charlie expresses a FOR vote to the aToken, then calls aToken.castVote
  • the aToken doesn't need to lookup its remaining votes, since it knows that Charlie is responsible for 15 of the 100 total UNI deposited, so it casts 3 FOR votes for him (15% of its available 20 UNI voting weight)

[mds1]

This works, but I'm trying to think through the implications for the delegate contract, which now needs to track how much weight has or hasn't been submitted, or else look it up from the Governor

Ah I overlooked this and didn't address it. I agree with @davidlaprade here though, I don't think this has downsides for the delegate contract (aToken, in this example). Instead of (from the above example) this flow:

• Alice expresses her FOR vote to the aToken
• Bob expresses an AGAINST vote to the aToken
• someone calls aToken.castVote, rolling these two votes up together and submitting them fractionally to the governor

You can also have the delegate contract designed as follows:

• Alice casts her FOR vote to the aToken, by calling aToken.castVote() which forwards her vote to the governor
• Bob casts an AGAINST vote to the aToken, by calling aToken.castVote() which forwards his vote to the governor

In either flow, the aToken contract just needs to have one storage write per vote (whether my cast vote or david's express vote) indicating alice voted. Currently, it does a storage write anyway to save off the expressed vote counts, so you just change what gets stored

[davidlaprade]

Yup agreed, that would also work

[apbendi]

@davidlaprade in your example above, when Charlie calls atToken.castVote, how does the aToken know to submit only his vote, and not to resbumit Alice and Bob's?

[davidlaprade]

how does the aToken know to submit only his vote, and not to resbumit Alice and Bob's

It would have to clear Alice and Bob's votes from storage after submitting them

[davidlaprade]

Okay, I implemented both approaches so that we can compare:

• incremental partial voting (i.e. what this PR assumed)
• replacement parial voting (i.e. the alternative Ben pointed out)

Grain of salt: the Governor diffs aren't a fair gauge of complexity since they are both based on this PR and this PR already implemented incremental partial voting in the Governor, so that diff for #33 is going to be smaller. The better place to compare complexity is in the client, i.e. the changes to ATokenFlexVoting. And there I think you'll see that the two are basically identical in complexity.

With respect to gas costs, the two functions we care about areexpressVote and castVote. Here's how they compare between the two implementations:

function	implementation	min	avg	median	max
expressVote	replacement	2467	36498	47776	48320
expressVote	incremental	2489	37316	47798	48342
castVote	replacement	12076	66037	70596	108396
castVote	incremental	12053	49295	53887	68207

That last row is pretty dramatic.

One last data point, I asked @jferas @wildmolasses and @garyghayrat the following question:

If I told you that a governance system allowed you to cast partial votes, how would you assume that system tallied the votes you cast? More concretely: if you had 20 weight to cast, and you voted 3 times -- first by casting 3 votes, next by casting 7 votes, and third by casting 10 votes -- how much of your voting weight would you assume had been used up in the process?

Everyone assumed that in a scenario like the one described, you would have cast all of your weight with these votes, i.e. the default assumption was that the governor would treat partial votes incrementally.

By no means is this conclusive, but I think it does say something about ease of integration that 5/6 assumed the incremental behavior was how this would work.

[davidlaprade]

As a note: @apbendi, @mds1, and I just got on a call and discussed this and agreed that the incremental approach is probably best. It's cheaper and doesn't seem like it has a downside for the client.

An idempotent approach (like the replacement approach Ben suggested) would work really well in an environment where you have to worry about lock contention. But that isn't a concern here, since we effectively hold a global lock within the context of each transaction.

[mds1]

Probably can resolve this thread now?