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

DisputableVoting: Optimize quiet ending #1234

Merged
merged 6 commits into from Jul 30, 2020

Conversation

facuspagnuolo
Copy link
Contributor

This PR introduces an improvement to the quiet ending feature. The main purpose is to compare the result at the beginning and at the end of the quiet ending period, and extend it only if it differs.

@facuspagnuolo facuspagnuolo self-assigned this Jul 28, 2020
@facuspagnuolo facuspagnuolo changed the title DisputableVoting: Improve quiet ending DisputableVoting: Optimize quiet ending Jul 28, 2020
Copy link
Contributor

@bingen bingen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have 2 general comments:

  • Going from Absent to whatever else (Y or N) is not considered a flip, right? Just to clarify.
  • Seeing the complexity, I’m even more convinced that I would do only one extension ;-P
    Some other comments inline.

apps/voting-disputable/contracts/DisputableVoting.sol Outdated Show resolved Hide resolved
apps/voting-disputable/contracts/DisputableVoting.sol Outdated Show resolved Hide resolved
apps/voting-disputable/contracts/DisputableVoting.sol Outdated Show resolved Hide resolved
} else {
// First, we make sure the extension is persisted, if are voting within the extension and it was not considered yet, we store it.
// Note that we are trusting `_canVote`, if we reached this point, it means the vote's flip was already confirmed.
if (getTimestamp64() >= _finalVoteEndDate(vote_)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely quite counter-intuitive! haha (but looks fine to me)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, honestly I couldn't come up with a clearer but similarly optimized as this one to compute multiple extensions

apps/voting-disputable/test/contracts/voting_disputable.js Outdated Show resolved Hide resolved
})
})
})
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests!!

@facuspagnuolo
Copy link
Contributor Author

@bingen thx a lot for the review! let me know your thoughts :D

@coveralls
Copy link

coveralls commented Jul 30, 2020

Coverage Status

Coverage remained the same at 97.338% when pulling b875758 on improve_quiet_ending into 035024e on disputable_voting_app.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 96.165% when pulling b1f0b9f on improve_quiet_ending into 035024e on disputable_voting_app.

Copy link
Contributor

@bingen bingen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Great improvement overall!!

@@ -256,10 +256,10 @@ contract DisputableVoting is IForwarder, DisputableAragonApp {
* @param _supports Whether voter supports the vote
*/
function vote(uint256 _voteId, bool _supports) external {
Vote storage vote_ = _getVote(_voteId);
require(_canVote(vote_, msg.sender), ERROR_CANNOT_VOTE);
Vote storage _vote = _getVote(_voteId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant only for function params, but this looks fine to me too.

@facuspagnuolo facuspagnuolo merged commit 73d2d76 into disputable_voting_app Jul 30, 2020
@facuspagnuolo facuspagnuolo deleted the improve_quiet_ending branch July 30, 2020 15:01
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
* DisputableVoting: Improve quiet ending

* DisputableVoting: Make `wasFlipped` implementation clearer

* DisputableVoting: Use `_` prefix instead of suffix for vote storage variables

* DisputableVoting: Fix linter

* DisputableVoting: Use `_` prefix only for args
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants