fix(contract): remove token delegation threshold and fix lock (#1228)

Eliminated the token threshold mechanism from the `River` contract, including the removal of the `MIN_TOKEN_THRESHOLD` variable and the `setTokenThreshold` function. Updated related tests to reflect these changes, ensuring consistent functionality throughout the contract. Replaced `IntrospectionBase` and `LockBase` with `IntrospectionFacet` and `LockFacet` for better modularization. Removed redundant lock methods and corrected minor typos in comments for improved code clarity and maintainability. Update logic in `_setDelegators` to handle edge cases for delegate assignment more efficiently. Simplify `River._delegate` by reusing the current delegate storage. Fix delegation lock and add tests.
river-build · Oct 16, 2024 · 4b2cde9 · 4b2cde9
1 parent 5b1e8e0
commit 4b2cde9
Show file tree

Hide file tree

Showing 6 changed files with 114 additions and 127 deletions.
diff --git a/contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol b/contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol
@@ -36,12 +36,18 @@ abstract contract VotesEnumerable is IVotesEnumerable {
   ) internal virtual {
     VotesEnumerableStorage.Layout storage ds = VotesEnumerableStorage.layout();
 
-    ds.delegators.remove(account);
-    ds.delegatorsByDelegatee[currentDelegatee].remove(account);
-
-    // if the delegatee is not address(0) then add the account and is not already a delegator then add it
-    if (newDelegatee != address(0)) {
+    // if the current delegatee is address(0) then add the account
+    if (currentDelegatee == address(0)) {
       ds.delegators.add(account);
+    } else {
+      ds.delegatorsByDelegatee[currentDelegatee].remove(account);
+    }
+
+    if (newDelegatee == address(0)) {
+      ds.delegators.remove(account);
+      ds.delegationTimeForDelegator[account] = 0;
+    } else {
+      // if the new delegatee is not address(0) then add the account
       ds.delegatorsByDelegatee[newDelegatee].add(account);
       ds.delegationTimeForDelegator[account] = block.timestamp;
     }

diff --git a/contracts/src/tokens/lock/ILock.sol b/contracts/src/tokens/lock/ILock.sol
@@ -15,8 +15,7 @@ interface ILockBase {
   event LockUpdated(
     address indexed caller,
     bool indexed enabled,
-    uint256 cooldown,
-    uint256 timestamp
+    uint256 cooldown
   );
 }
 

diff --git a/contracts/src/tokens/lock/LockBase.sol b/contracts/src/tokens/lock/LockBase.sol
@@ -5,6 +5,7 @@ pragma solidity ^0.8.23;
 import {ILockBase} from "./ILock.sol";
 
 // libraries
+import {CustomRevert} from "contracts/src/utils/libraries/CustomRevert.sol";
 import {LockStorage} from "./LockStorage.sol";
 
 abstract contract LockBase is ILockBase {
@@ -24,31 +25,25 @@ abstract contract LockBase is ILockBase {
   function _enableLock(address caller) internal {
     LockStorage.Layout storage ds = LockStorage.layout();
 
-    if (ds.enabledByAddress[caller]) {
-      revert LockAlreadyEnabled();
-    }
+    if (ds.enabledByAddress[caller])
+      CustomRevert.revertWith(LockAlreadyEnabled.selector);
 
     ds.enabledByAddress[caller] = true;
 
-    emit LockUpdated(caller, true, 0, block.timestamp);
+    emit LockUpdated(caller, true, 0);
   }
 
   function _disableLock(address caller) internal {
     LockStorage.Layout storage ds = LockStorage.layout();
 
-    if (ds.enabledByAddress[caller] == false) {
-      revert LockAlreadyDisabled();
-    }
+    if (!ds.enabledByAddress[caller])
+      CustomRevert.revertWith(LockAlreadyDisabled.selector);
 
+    uint256 cooldown = block.timestamp + ds.defaultCooldown;
     ds.enabledByAddress[caller] = false;
-    ds.cooldownByAddress[caller] = block.timestamp + ds.defaultCooldown;
-
-    emit LockUpdated(
-      caller,
-      false,
-      block.timestamp + ds.defaultCooldown,
-      block.timestamp
-    );
+    ds.cooldownByAddress[caller] = cooldown;
+
+    emit LockUpdated(caller, false, cooldown);
   }
 
   function _lockCooldown(address caller) internal view returns (uint256) {
@@ -58,9 +53,9 @@ abstract contract LockBase is ILockBase {
   function _lockEnabled(address caller) internal view returns (bool) {
     LockStorage.Layout storage ds = LockStorage.layout();
 
-    return
-      ds.enabledByAddress[caller] == true ||
-      block.timestamp < ds.cooldownByAddress[caller];
+    if (ds.enabledByAddress[caller]) return true;
+
+    return block.timestamp < ds.cooldownByAddress[caller];
   }
 
   function _canLock() internal view virtual returns (bool);

diff --git a/contracts/src/tokens/river/base/River.sol b/contracts/src/tokens/river/base/River.sol
@@ -22,27 +22,25 @@ import {ERC20Votes} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Vo
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 
 import {VotesEnumerable} from "contracts/src/diamond/facets/governance/votes/enumerable/VotesEnumerable.sol";
-import {IntrospectionBase} from "contracts/src/diamond/facets/introspection/IntrospectionBase.sol";
-import {LockBase} from "contracts/src/tokens/lock/LockBase.sol";
+import {IntrospectionFacet} from "contracts/src/diamond/facets/introspection/IntrospectionFacet.sol";
+import {LockFacet} from "contracts/src/tokens/lock/LockFacet.sol";
 
 contract River is
   IOptimismMintableERC20,
   ILegacyMintableERC20,
   ISemver,
-  ILock,
+  Ownable,
   ERC20Permit,
   ERC20Votes,
-  Ownable,
   VotesEnumerable,
-  IntrospectionBase,
-  LockBase
+  LockFacet,
+  IntrospectionFacet
 {
   // =============================================================
   //                           Errors
   // =============================================================
   error River__TransferLockEnabled();
   error River__DelegateeSameAsCurrent();
-  error River__InvalidTokenAmount();
 
   // =============================================================
   //                           Events
@@ -62,11 +60,6 @@ contract River is
   /// @notice Address of the StandardBridge on this network.
   address public immutable BRIDGE;
 
-  // =============================================================
-  //                           Variables
-  // =============================================================
-  uint256 public MIN_TOKEN_THRESHOLD = 10 ether;
-
   // =============================================================
   //                           Modifiers
   // =============================================================
@@ -150,7 +143,8 @@ contract River is
   // =============================================================
   //                           Votes
   // =============================================================
-  /// @notice Clock used for flagging checkpoints, overriden to implement timestamp based
+
+  /// @notice Clock used for flagging checkpoints, overridden to implement timestamp based
   /// checkpoints (and voting).
   function clock() public view override returns (uint48) {
     return uint48(block.timestamp);
@@ -163,7 +157,7 @@ contract River is
 
   function nonces(
     address owner
-  ) public view virtual override(ERC20Permit, Nonces) returns (uint256) {
+  ) public view override(ERC20Permit, Nonces) returns (uint256) {
     return super.nonces(owner);
   }
 
@@ -172,91 +166,51 @@ contract River is
   // =============================================================
 
   /// @inheritdoc ILock
-  function isLockEnabled(address account) external view virtual returns (bool) {
-    return _lockEnabled(account);
-  }
-
-  function lockCooldown(
-    address account
-  ) external view virtual returns (uint256) {
-    return _lockCooldown(account);
-  }
-
-  /// @inheritdoc ILock
-  function enableLock(address account) external virtual onlyOwner {}
+  function enableLock(address account) external override onlyOwner {}
 
   /// @inheritdoc ILock
-  function disableLock(address account) external virtual onlyOwner {}
+  function disableLock(address account) external override onlyOwner {}
 
   /// @inheritdoc ILock
-  function setLockCooldown(uint256 cooldown) external virtual onlyOwner {
+  function setLockCooldown(uint256 cooldown) external override onlyOwner {
     _setDefaultCooldown(cooldown);
   }
 
-  // =============================================================
-  //                           IERC165
-  // =============================================================
-
-  /// @inheritdoc IERC165
-  function supportsInterface(bytes4 interfaceId) public view returns (bool) {
-    return _supportsInterface(interfaceId);
-  }
-
-  // =============================================================
-  //                           Token
-  // =============================================================
-  function setTokenThreshold(uint256 threshold) external onlyOwner {
-    if (threshold > totalSupply()) revert River__InvalidTokenAmount();
-    MIN_TOKEN_THRESHOLD = threshold;
-    emit TokenThresholdSet(threshold);
-  }
-
   // =============================================================
   //                           Hooks
   // =============================================================
+
   function _update(
     address from,
     address to,
     uint256 value
-  ) internal virtual override(ERC20, ERC20Votes) {
+  ) internal override(ERC20, ERC20Votes) {
     if (from != address(0) && _lockEnabled(from)) {
-      // allow transfering at minting time
+      // allow transferring at minting time
       revert River__TransferLockEnabled();
     }
 
     super._update(from, to, value);
   }
 
-  function _getVotingUnits(
-    address account
-  ) internal view override returns (uint256) {
-    return balanceOf(account);
-  }
-
   /// @dev Hook that gets called before any external enable and disable lock function
   function _canLock() internal view override returns (bool) {
     return msg.sender == owner();
   }
 
-  function _delegate(
-    address account,
-    address delegatee
-  ) internal virtual override {
-    // revert if the delegatee is the same as the current delegatee
-    if (delegates(account) == delegatee) revert River__DelegateeSameAsCurrent();
+  function _delegate(address account, address delegatee) internal override {
+    address currentDelegatee = delegates(account);
 
-    // revert if the balance is below the threshold
-    if (balanceOf(account) < MIN_TOKEN_THRESHOLD)
-      revert River__InvalidTokenAmount();
+    // revert if the delegatee is the same as the current delegatee
+    if (currentDelegatee == delegatee) revert River__DelegateeSameAsCurrent();
 
     // if the delegatee is the zero address, initialize disable lock
     if (delegatee == address(0)) {
       _disableLock(account);
     } else {
-      if (!_lockEnabled(account)) _enableLock(account);
+      _enableLock(account);
     }
 
-    address currentDelegatee = delegates(account);
     super._delegate(account, delegatee);
 
     _setDelegators(account, delegatee, currentDelegatee);

diff --git a/contracts/src/tokens/river/mainnet/River.sol b/contracts/src/tokens/river/mainnet/River.sol
@@ -130,7 +130,7 @@ contract River is
     uint256 value
   ) internal virtual override(ERC20, ERC20Votes) {
     if (from != address(0) && _lockEnabled(from)) {
-      // allow transfering at minting time
+      // allow transferring at minting time
       revert River__TransferLockEnabled();
     }
     super._update(from, to, value);
@@ -170,7 +170,7 @@ contract River is
   /// @dev Do not allow disabling lock without delegating
   function disableLock(address account) external override onlyAllowed {}
 
-  /// @notice Clock used for flagging checkpoints, overriden to implement timestamp based
+  /// @notice Clock used for flagging checkpoints, overridden to implement timestamp based
   /// checkpoints (and voting).
   function clock() public view override returns (uint48) {
     return uint48(block.timestamp);