From 6479a0d0523434b22361dbac015cf335775edf43 Mon Sep 17 00:00:00 2001 From: Ido Date: Fri, 23 Aug 2024 19:00:33 +0200 Subject: [PATCH] documentation and optimizations --- src/JokeraceEligibility.sol | 45 +++++++++++++++++++--------------- test/JokeraceEligibility.t.sol | 45 ++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 20 deletions(-) diff --git a/src/JokeraceEligibility.sol b/src/JokeraceEligibility.sol index b131127..cf428e3 100644 --- a/src/JokeraceEligibility.sol +++ b/src/JokeraceEligibility.sol @@ -31,9 +31,9 @@ contract JokeraceEligibility is HatsEligibilityModule { EVENTS //////////////////////////////////////////////////////////////*/ - /// @notice Emitted when a reelection is set + /// @notice Emitted when the next term is set event NextTermSet(address NewContest, uint256 newTopK, uint256 newTermEnd, uint256 newTransitionPeriod); - /// @notice Emitted when election's results are pulled + /// @notice Emitted when the next term is started event TermStarted(address contest, uint256 topK, uint256 termEnd, uint256 transitionPeriod); /*////////////////////////////////////////////////////////////// @@ -73,14 +73,14 @@ contract JokeraceEligibility is HatsEligibilityModule { //////////////////////////////////////////////////////////////*/ struct TermDetails { - /// @notice Current Jokerace contest (election) + /// @notice Jokerace contest (election) address contest; /// @notice First K winners of the contest will be eligible uint96 topK; - /// @notice First second after the term (a unix timestamp) + /// @notice Term's ending time (a unix timestamp) uint256 termEnd; /// @notice Period of time after the term end when previous elected members are still considered eligible until a - /// new term is set. + /// new term begins. uint256 transitionPeriod; } @@ -104,9 +104,9 @@ contract JokeraceEligibility is HatsEligibilityModule { * this ensures it can only be called once per instance, and that the implementation contract is never initialized. * @param _initData Packed initialization data with the following parameters: * _contest - Jokerace contest. The contest must have down-voting disabled and sorting enabled. - * _termEnd - Final second of the current term (a unix timestamp) - * _transitionPeriod - Period of time after the term end when calling reelection is allowed and previous elected are - * still eligibile as long as reelection was not called + * _termEnd - term's ending time (a unix timestamp) + * _transitionPeriod - Period of time after the term end when previous elected members are still considered eligible + * until a new term begins. * _topK - First K winners of the contest will be eligible */ function _setUp(bytes calldata _initData) internal override { @@ -141,8 +141,9 @@ contract JokeraceEligibility is HatsEligibilityModule { returns (bool eligible, bool standing) { standing = true; - TermDetails memory currentTerm = terms[currentTermIndex]; - if (block.timestamp < currentTerm.termEnd + currentTerm.transitionPeriod) { + uint256 currentTermEnd = terms[currentTermIndex].termEnd; + uint256 currentTransitionPeriod = terms[currentTermIndex].transitionPeriod; + if (block.timestamp < currentTermEnd + currentTransitionPeriod) { eligible = eligibleWearersPerTerm[_wearer][currentTermIndex]; } } @@ -152,7 +153,7 @@ contract JokeraceEligibility is HatsEligibilityModule { //////////////////////////////////////////////////////////////*/ /** - * @notice Pulls the contest results from the jokerace contest contract. + * @notice Pulls the contest results from the next term's Jokerace contest contract and activates the next term. * @dev The eligible wearers for a given completed contest are the top K winners of the contest. In case there is a * tie, meaning that candidates in places K and K+1 have the same score, then the results of this contest are * rejected. @@ -162,11 +163,11 @@ contract JokeraceEligibility is HatsEligibilityModule { GovernorCountingSimple contest = GovernorCountingSimple(payable(nextTerm.contest)); uint96 k = nextTerm.topK; - if (!_canStartNextTerm(terms[currentTermIndex].termEnd)) { + if (!_currentTermEnded(terms[currentTermIndex].termEnd)) { revert JokeraceEligibility_TermNotCompleted(); } - if (contest.state() != Governor.ContestState.Completed) { + if (!_nextContestCompleted(contest)) { revert JokeraceEligibility_ContestNotCompleted(); } @@ -213,8 +214,8 @@ contract JokeraceEligibility is HatsEligibilityModule { } /** - * @notice Sets a reelection, i.e. updates the module with a new term. - * @dev Only the module's admin/s have the permission to set a reelection. If an admin is not set at the module + * @notice Sets the next term. + * @dev Only the module's admin/s have the permission to set the next term. If an admin is not set at the module * creation, then any admin of hatId is considered an admin by the module. */ function setNextTerm(address newContest, uint256 newTermEnd, uint256 newTransitionPeriod, uint96 newTopK) public { @@ -242,21 +243,25 @@ contract JokeraceEligibility is HatsEligibilityModule { VIEW FUNCTIONS //////////////////////////////////////////////////////////////*/ - /// @notice Check if setting a new election is allowed. + /// @notice Check if starting the next term is allowed, meaning that the current term has ended and the next contest + /// is completed function canStartNextTerm() public view returns (bool allowed) { - // If the current term has ended - return _canStartNextTerm(terms[currentTermIndex].termEnd); + return _currentTermEnded(terms[currentTermIndex].termEnd) + && _nextContestCompleted(GovernorCountingSimple(payable(terms[currentTermIndex + 1].contest))); } /*////////////////////////////////////////////////////////////// INTERNAL FUNCTIONS //////////////////////////////////////////////////////////////*/ - /// @notice Check if setting a new election is allowed. - function _canStartNextTerm(uint256 currentTermEnd) internal view returns (bool allowed) { + function _currentTermEnded(uint256 currentTermEnd) internal view returns (bool allowed) { allowed = block.timestamp > currentTermEnd; } + function _nextContestCompleted(GovernorCountingSimple nextContest) internal view returns (bool completed) { + completed = nextContest.state() == Governor.ContestState.Completed; + } + function _getCandidate(GovernorCountingSimple contest, uint256 proposalId) internal view returns (address candidate) { candidate = contest.getProposal(proposalId).author; } diff --git a/test/JokeraceEligibility.t.sol b/test/JokeraceEligibility.t.sol index 516a87c..9ea2e89 100644 --- a/test/JokeraceEligibility.t.sol +++ b/test/JokeraceEligibility.t.sol @@ -231,6 +231,11 @@ contract TestDeployment is TestSetup { HATS.getHatEligibilityModule(winnersHat), address(instanceDefaultAdmin), "eligibility module of winners hat" ); } + + function test_canStartNextTerm() public { + bool canStart = instanceDefaultAdmin.canStartNextTerm(); + assertEq(canStart, false); + } } // Three candidates propose @@ -393,6 +398,8 @@ contract ContestCompletedProposing2Scenario is Proposing2Scenario { super.setUp(); // set time to contest completion vm.warp(contestStart + voteDelay + votePeriod + 1); + bool canStart = instanceDefaultAdmin.canStartNextTerm(); + assertEq(canStart, true); uint256 currentTermIndex = instanceDefaultAdmin.currentTermIndex(); TermDetails memory nextTerm; (nextTerm.contest, nextTerm.topK, nextTerm.termEnd, nextTerm.transitionPeriod) = @@ -418,6 +425,11 @@ contract TestContestCompletedProposing2Scenario is ContestCompletedProposing2Sce assertEq(HATS.isEligible(candidate2, winnersHat), false, "candidate 2 eligibility"); assertEq(HATS.isEligible(candidate3, winnersHat), false, "candidate 3 eligibility"); } + + function test_canStartNextTerm() public { + bool canStart = instanceDefaultAdmin.canStartNextTerm(); + assertEq(canStart, false); + } } contract TestContestCompletedVoting2Proposing1Scenario is ContestCompletedVoting2Proposing1Scenario { @@ -479,6 +491,20 @@ contract TestContestCompletedVoting1Proposing1Scenario is ContestCompletedVoting } } +contract TestTermNotCompleted is ContestCompletedVoting1Proposing1Scenario { + function test_canStartNextTerm() public { + bool canStart = instanceDefaultAdmin.canStartNextTerm(); + assertEq(canStart, false); + } + + function test_startNextTerm_reverts() public { + vm.prank(dao); + instanceDefaultAdmin.setNextTerm(address(contest), termEnd1 + 86_400, transitionPeriod, 3); + vm.expectRevert(JokeraceEligibility_TermNotCompleted.selector); + instanceDefaultAdmin.startNextTerm(); + } +} + // Current term ended, ready for reelection contract TermEndedVoting1Proposing1Scenario is ContestCompletedVoting1Proposing1Scenario { function setUp() public virtual override { @@ -488,6 +514,18 @@ contract TermEndedVoting1Proposing1Scenario is ContestCompletedVoting1Proposing1 } } +contract TestStartEmptyTerm is TermEndedVoting1Proposing1Scenario { + function test_canStartNextTerm() public { + vm.expectRevert(); + instanceDefaultAdmin.canStartNextTerm(); + } + + function test_startNextTerm_reverts() public { + vm.expectRevert(); + instanceDefaultAdmin.startNextTerm(); + } +} + contract TestTermEndedVoting1Proposing1Scenario is TermEndedVoting1Proposing1Scenario { function test_setNextTermnNotAdmin_reverts() public { vm.startPrank(candidate1); @@ -552,6 +590,11 @@ contract NextContestCanceledVoting1Proposing1Scenario is TermEndedVoting1Proposi } contract TestNextContestCanceledVoting1Proposing1Scenario is NextContestCanceledVoting1Proposing1Scenario { + function test_canStartNextTerm() public { + bool canStart = instanceDefaultAdmin.canStartNextTerm(); + assertEq(canStart, false); + } + function test_startNextTerm_reverts() public { vm.expectRevert(JokeraceEligibility_ContestNotCompleted.selector); instanceDefaultAdmin.startNextTerm(); @@ -564,6 +607,8 @@ contract TestNextContestCanceledVoting1Proposing1Scenario is NextContestCanceled vm.expectEmit(); emit NextTermSet(address(contest), newTopK, newTermEnd, transitionPeriod2); instanceDefaultAdmin.setNextTerm(address(contest), newTermEnd, transitionPeriod2, newTopK); + bool canStart = instanceDefaultAdmin.canStartNextTerm(); + assertEq(canStart, true); } }