Disputable Voting: Support quiet ending #1205
Conversation
| @@ -690,7 +739,7 @@ contract DisputableVoting is DisputableAragonApp, IForwarder { | |||
| * @return True if the given voter can participate a certain vote | |||
| */ | |||
| function _canVote(Vote storage vote_, address _voter) internal view returns (bool) { | |||
| return _isVoteOpenForVoting(vote_) && _hasVotingPower(vote_, _voter); | |||
| return _isVoteOpenForVoting(vote_) && _hasVotingPower(vote_, _voter) && _voteCaster(vote_, _voter) != _voter; | |||
There was a problem hiding this comment.
This means that voters can’t change their cast votes, right? Some discussion rings a bell about this, but I don’t remember it. What was the rationale for this change?
There was a problem hiding this comment.
Yes, simply because since multiple extensions are allowed, if we allow voters to change their vote, then they could extend votes indefinitely
| function _hasNotVotedYet(Vote storage vote_, address _voter) internal view returns (bool) { | ||
| return _voteCaster(vote_, _voter) != _voter; | ||
| function _hasCastedVote(Vote storage vote_, address _voter) internal view returns (bool) { | ||
| return _voterState(vote_, _voter) != VoterState.Absent; |
There was a problem hiding this comment.
This changes also means that representatives can’t change votes either, which is consistent with holders not being able.
There was a problem hiding this comment.
Exactly, since multiple extensions are allowed, if we allow representatives to change their vote, then they could extend votes indefinitely due to quiet ending
| } else if (state == VoterState.Nay) { | ||
| vote_.nay = vote_.nay.sub(voterStake); | ||
| // Note that votes can be changed only during the overrule window | ||
| VoterState previousVoterState = _voterState(vote_, _voter); |
There was a problem hiding this comment.
It’s actually unrelated to this PR, but I wonder if it would be worth it to check if the new vote is different than the old one.
There was a problem hiding this comment.
I tried it but I honestly don't think it looks cleaner. If you see a cleaner way please share!
| * @param _quietEndingExtension New quiet ending extension in seconds | ||
| */ | ||
| function _changeQuietEndingPeriod(uint64 _quietEndingPeriod, uint64 _quietEndingExtension) internal { | ||
| require(_quietEndingPeriod <= voteTime, ERROR_INVALID_QUIET_ENDING_PERIOD); |
There was a problem hiding this comment.
Shouldn’t we enforce that it’s not greater than the overrule window? The diagram in the PR description seems to suggest that.
There was a problem hiding this comment.
Not necessarily, look at the diagrams in section (2). It is possible to have a quiet ending period that encapsulates the overrule window.
There was a problem hiding this comment.
I see, thanks! And actually, as all those parameters can be zero, this way we can have quiet ending without overrule window.
But now I have another question: why does the quiet ending period need to be <= the quiet ending extension?
Finally, I would personally understand better those diagrams if the “Flipped” arrow was disassociated from the start of the quiet ending extension.
| if (voterStake < yeas.add(nays) && _withinQuietEndingPeriod(vote_)) { | ||
| bool isAccepted = _isAccepted(yeas, yeas.add(nays), votingPower, supportRequired, minimumAcceptanceQuorum); | ||
| if (wasAccepted != isAccepted) { | ||
| vote_.quietEndingExtendedSeconds = vote_.quietEndingExtendedSeconds.add(vote_.quietEndingExtension); |
There was a problem hiding this comment.
Why are we adding here? If multiple flips happen (although it’s unlikely), the vote could get very long. Isn’t it enough with one extension?
There was a problem hiding this comment.
Yes, but that is the main purpose of quiet ending
| * [ vote active ][ vote paused ][ . vote active ] | ||
| * ^ ^ ^ | ||
| * | | | | ||
| * vote starts duration start date vote ends |
* DisputableVoting: disallow changing votes * DisputableVoting: split create and vote tests * DisputableVoting: fix can execute * DisputableVoting: implement quiet ending * DisputableVoting: Minor enhancements based on review from @bingen * DisputableVoting: Add tests for challenged states
Fixes aragon/client#836
This PR proposed the following implementation: