Add transition period for continuity #18

gershido · 2024-08-12T14:18:07Z

Proposed elections flow:

spengrah · 2024-08-12T17:59:31Z

@gershido I haven't dug into the code in more detail than a cursory review, but overall this approach looks good to me. The diagram is helpful.

Only thing I'd consider is adding an option to set a new transition period as an arg in reelection.

spengrah · 2024-08-14T00:44:56Z

test/JokeraceEligibility.t.sol

@@ -20,15 +20,15 @@ contract DeployImplementationTest is DeployImplementation, Test {
  // bytes32 public SALT;

  uint256 public fork;
-  uint256 public BLOCK_NUMBER = 9_713_194; // the block number where hats module factory was deployed on Goerli;
+  uint256 public BLOCK_NUMBER = 6_488_268; // the block number where hats module factory was deployed on Goerli;


sepolia, i assume? If so, please change the comment

src/JokeraceEligibility.sol

… time

gershido · 2024-08-14T14:30:59Z

Added the following changes:

ElectionResultsPulled event. Currently there's no emitted event when the pullElectionResults is successfully called. Adding the event will help indexing the module.
Added a condition for the reelectionAllowed function, for the case in which we're in a transition period, the next contest was set but then for some reason was cancelled.

spengrah · 2024-08-14T16:04:33Z

src/JokeraceEligibility.sol

@@ -31,6 +31,8 @@ contract JokeraceEligibility is HatsEligibilityModule {

  /// @notice Emitted when a reelection is set
  event NewTerm(address NewContest, uint256 newTopK, uint256 newTermEnd, uint256 newTransitionPeriod);
+  /// @notice Emitted when election's results are pulled
+  event ElectionResultsPulled(address NewContest);


would like to see a test for this event

spengrah · 2024-08-14T16:06:08Z

src/JokeraceEligibility.sol

      || (
        currentContest != address(0)
          && GovernorCountingSimple(payable(currentContest)).state() == Governor.ContestState.Canceled
+      ) // or if we're in a transition period, and the next contest was canceled


Is there a test for this new logic? I might be missing it, but I don't see one

gershido · 2024-08-14T18:52:35Z

Added:

New isTie param in the ElectionResultsPulled event, and emitting the event for the tie scenario also
Event tests

spengrah

looks good! thanks for adding those tests

gershido · 2024-08-25T14:18:20Z

Following last week's discussion about the election flow, changed the implementation to allow setting up the next term even before the current term has ended. The main implication on the implementation side was that it requires to keep the full set of details for the next term (contest, term end, top K and transition period). Here are the main changes:

Terms are saved as indexed TermDetails structs. There's a currentTermIndex variable. The next term will be always saved in currentTermIndex + 1.
Name changes: reelection function changed to setNextTerm, pullElectionResults changed to startNextTerm
Mapping of eligible wearers was changed from eligibleWearesPerContest to eligibleWearersPerTerm

spengrah

approving with several suggestions. otherwise, good to go!

spengrah · 2024-08-26T14:55:31Z

src/JokeraceEligibility.sol

-  /// @notice Emitted when a reelection is set
-  event NewTerm(address NewContest, uint256 newTopK, uint256 newTermEnd);
+  /// @notice Emitted when the next term is set
+  event NextTermSet(address NewContest, uint256 newTopK, uint256 newTermEnd, uint256 newTransitionPeriod);


style: camel case for newContest

spengrah · 2024-08-26T15:10:03Z

src/JokeraceEligibility.sol

+    /// @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 begins.


style: join these two comment lines

It's automatically formatted like that, the line_length in the foundry.toml file is set to 120.

Oh whoops, I misinterpreted github's ui 🫠

spengrah · 2024-08-26T15:11:54Z

src/JokeraceEligibility.sol

+    /// @notice Jokerace contest (election)
+    address contest;
+    /// @notice First K winners of the contest will be eligible
+    uint96 topK;


nice storage slot packing :)

spengrah · 2024-08-26T15:20:55Z

src/JokeraceEligibility.sol

-  function pullElectionResults() public returns (bool success) {
-    GovernorCountingSimple currentContest = GovernorCountingSimple(payable(underlyingContest));
+  function startNextTerm() public {
+    TermDetails memory nextTerm = terms[currentTermIndex + 1];


Since this function only needs topK and contest from the nextTerm object, it will be cheaper here to set nextTerm as a storage pointer rather than SLOADing the entire nextTerm object into memory

ie,

TermDetails stoarge nextTerm = terms[currentTermIndex + 1];

All the details from the nextTerm object are emitted in the event at the end of the function. A different approach would be to just include the next term's index instead of all the details. That's a bit less good for whoever listens to the event because it will require a call to the contract in order to get the details of this new term. What do you think?

ah good point, i missed the event params! Keep it as is, then.

spengrah · 2024-08-26T15:23:26Z

src/JokeraceEligibility.sol

-          address candidate = _getCandidate(currentContest, proposalsOfCurrentRank[proposalIndex]);
-          eligibleWearersPerContest[candidate][address(currentContest)] = true;
+          address candidate = _getCandidate(contest, proposalsOfCurrentRank[proposalIndex]);
+          eligibleWearersPerTerm[candidate][currentTermIndex + 1] = true;


this is the second SLOAD of currentTermIndex, so I'd suggest storing this in memory

spengrah · 2024-08-26T15:25:46Z

src/JokeraceEligibility.sol

  }

  /*//////////////////////////////////////////////////////////////
                        INTERNAL FUNCTIONS
    //////////////////////////////////////////////////////////////*/

+  function _currentTermEnded(uint256 currentTermEnd) internal view returns (bool allowed) {


Any reason these shouldn't be public functions? Might be nice for front ends to be able to easily get these sub-components of canStartNextTerm

Added two separate public functions: currentTermEnded and nextContestCompleted, that wrap the internal ones. The reason for the separate public functions is to make them input-less, while the internal ones receive input params to save fetches from storage (in the cases where the function that uses the internal function already fetched this param from storage for other purposes).

draft

322563d

gershido added 2 commits August 13, 2024 16:25

implementation and tests

3868915

format

eb8adaa

gershido marked this pull request as ready for review August 13, 2024 14:33

gershido requested a review from spengrah August 13, 2024 14:33

spengrah reviewed Aug 14, 2024

View reviewed changes

emit event on election pulling and update logic of allowed reelection…

7de54f8

… time

gershido requested a review from spengrah August 14, 2024 14:38

spengrah reviewed Aug 14, 2024

View reviewed changes

add tie indication in event and test events

6afd3dd

gershido requested a review from spengrah August 15, 2024 11:53

spengrah approved these changes Aug 16, 2024

View reviewed changes

gershido added 3 commits August 23, 2024 16:32

implementation updates

72c09ec

documentation and optimizations

6479a0d

documentation

db7f0b4

gershido requested a review from spengrah August 25, 2024 14:18

spengrah approved these changes Aug 26, 2024

View reviewed changes

gershido added 2 commits August 27, 2024 15:14

optimizations

7547ce9

wrap internal functions

28c9614

gershido merged commit 36634c9 into main Aug 27, 2024
3 checks passed

gershido deleted the add-grace-period branch August 27, 2024 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add transition period for continuity #18

Add transition period for continuity #18

gershido commented Aug 12, 2024

spengrah commented Aug 12, 2024

spengrah Aug 14, 2024

gershido commented Aug 14, 2024

spengrah Aug 14, 2024

spengrah Aug 14, 2024

gershido Aug 14, 2024

gershido commented Aug 14, 2024

spengrah left a comment

gershido commented Aug 25, 2024

spengrah left a comment

spengrah Aug 26, 2024

spengrah Aug 26, 2024

gershido Aug 27, 2024

spengrah Aug 27, 2024

spengrah Aug 26, 2024

spengrah Aug 26, 2024

gershido Aug 27, 2024

spengrah Aug 27, 2024

spengrah Aug 26, 2024

spengrah Aug 26, 2024

gershido Aug 27, 2024 •

edited

Loading

Add transition period for continuity #18

Add transition period for continuity #18

Conversation

gershido commented Aug 12, 2024

spengrah commented Aug 12, 2024

Choose a reason for hiding this comment

gershido commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gershido commented Aug 14, 2024

spengrah left a comment

Choose a reason for hiding this comment

gershido commented Aug 25, 2024

spengrah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gershido Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

gershido Aug 27, 2024 •

edited

Loading