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

Keep track of historical quorum values #3561

Merged
merged 11 commits into from Jul 27, 2022
Merged

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jul 18, 2022

Checkpointing quorum is necessary to avoid quorum changes making old, failed because of quorum, proposals suddenly successful.

PR Checklist

  • Tests
  • Changelog entry

frangio
frangio previously approved these changes Jul 19, 2022
@frangio frangio dismissed their stale review July 19, 2022 15:33

Re-reviewing

@frangio frangio added the on hold Put on hold for some reason that must be specified in a comment. label Jul 19, 2022
@frangio
Copy link
Contributor

frangio commented Jul 19, 2022

Waiting until next week to discuss a more efficient fix.

@frangio
Copy link
Contributor

frangio commented Jul 25, 2022

The issue with this PR is that it increases the cost of voting when the PreventLateQuorum module is used. It seems like this can be fixed by first checking if the latest checkpoint is the one we're looking for (in quorumNumerator(uint blockNumber)) and in that case avoid the binary search. Generally, it is indeed the latest checkpoint that will be needed for voting. The checkpoint history with binary search is still appropriate for the state function.

@frangio frangio removed the on hold Put on hold for some reason that must be specified in a comment. label Jul 26, 2022
@Amxx
Copy link
Collaborator Author

Amxx commented Jul 27, 2022

I've implemented an optimistic lookup, that skips the binary search if the checkpoint we are looking for is the last one. However that only concerns quorumNumerator(uint256). This function is mostly called through

function quorum(uint256 blockNumber) public view virtual override returns (uint256) {
    return (token.getPastTotalSupply(blockNumber) * quorumNumerator(blockNumber)) / quorumDenominator();
}

The token.getPastTotalSupply(blockNumber) call will not be optimized the same way

@Amxx
Copy link
Collaborator Author

Amxx commented Jul 27, 2022

Not, the parts not covered are mechanism used in cases of migration.

@frangio frangio changed the title Keep historical quorum values Keep track of historical quorum values Jul 27, 2022
@frangio frangio enabled auto-merge (squash) July 27, 2022 16:22
@frangio frangio disabled auto-merge July 27, 2022 16:23
@frangio frangio merged commit 8ea1fc8 into OpenZeppelin:master Jul 27, 2022
frangio pushed a commit that referenced this pull request Jul 27, 2022
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
(cherry picked from commit 8ea1fc8)
@Amxx Amxx deleted the fix/quorum branch July 27, 2022 21:11
ronhuafeng added a commit to ronhuafeng/openzeppelin-contracts that referenced this pull request Sep 9, 2022
Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants