replace static signerCount with validSignerCount()

Hats-Protocol · Mar 27, 2023 · a29582c · a29582c
1 parent e3c681c
commit a29582c
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 102 deletions.
diff --git a/src/HatsSignerGate.sol b/src/HatsSignerGate.sol
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: CC0
 pragma solidity >=0.8.13;
 
-// import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
+import { Test, console2 } from "forge-std/Test.sol"; // remove after testing
 import { HatsSignerGateBase, IGnosisSafe, Enum } from "./HatsSignerGateBase.sol";
 import "./HSGLib.sol";
 
@@ -35,7 +35,8 @@ contract HatsSignerGate is HatsSignerGateBase {
     /// @dev Reverts if `maxSigners` has been reached, the caller is either invalid or has already claimed. Swaps caller with existing invalid owner if relevant.
     function claimSigner() public virtual {
         uint256 maxSigs = maxSigners; // save SLOADs
-        uint256 currentSignerCount = signerCount;
+        address[] memory owners = safe.getOwners();
+        uint256 currentSignerCount = _countValidSigners(owners);
 
         if (currentSignerCount >= maxSigs) {
             revert MaxSignersReached();
@@ -49,16 +50,14 @@ contract HatsSignerGate is HatsSignerGateBase {
             revert NotSignerHatWearer(msg.sender);
         }
 
-        /* 
-        We check the safe owner count in case there are existing owners who are no longer valid signers. 
+        /*
+        We check the safe owner count in case there are existing owners who are no longer valid signers.
         If we're already at maxSigners, we'll replace one of the invalid owners by swapping the signer.
         Otherwise, we'll simply add the new signer.
         */
-        address[] memory owners = safe.getOwners();
         uint256 ownerCount = owners.length;
-
         if (ownerCount >= maxSigs) {
-            bool swapped = _swapSigner(owners, ownerCount, maxSigs, currentSignerCount, msg.sender);
+            bool swapped = _swapSigner(owners, ownerCount, msg.sender);
             if (!swapped) {
                 // if there are no invalid owners, we can't add a new signer, so we revert
                 revert NoInvalidSignersToReplace();
@@ -73,6 +72,8 @@ contract HatsSignerGate is HatsSignerGateBase {
     /// @param _account The address to check
     /// @return valid Whether `_account` is a valid signer
     function isValidSigner(address _account) public view override returns (bool valid) {
+        // console2.log("ivs 1");
         valid = HATS.isWearerOfHat(_account, signersHatId);
+        // console2.log("ivs 2");
     }
 }
diff --git a/src/HatsSignerGateBase.sol b/src/HatsSignerGateBase.sol
@@ -23,8 +23,8 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
     /// @notice The maximum number of signers allowed for the `safe`
     uint256 public maxSigners;
 
-    /// @notice The current number of signers on the `safe`
-    uint256 public signerCount;
+    // /// @notice The current number of signers on the `safe`
+    // uint256 public signerCount;
 
     /// @notice The version of HatsSignerGate used in this contract
     string public version;
@@ -99,7 +99,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         if (_targetThreshold != targetThreshold) {
             _setTargetThreshold(_targetThreshold);
 
-            if (signerCount > 1) _setSafeThreshold(_targetThreshold);
+            if (validSignerCount() > 1) _setSafeThreshold(_targetThreshold);
 
             emit HSGLib.TargetThresholdSet(_targetThreshold);
         }
@@ -121,11 +121,11 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
     /// @param _threshold The threshold to set on the `safe`
     function _setSafeThreshold(uint256 _threshold) internal {
         uint256 newThreshold = _threshold;
-        uint256 signerCount_ = signerCount; // save an SLOAD
+        uint256 signerCount = validSignerCount();
 
         // ensure that txs can't execute if fewer signers than target threshold
-        if (signerCount_ <= _threshold) {
-            newThreshold = signerCount_;
+        if (signerCount <= _threshold) {
+            newThreshold = signerCount;
         }
         if (newThreshold != safe.getThreshold()) {
             bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", newThreshold);
@@ -181,30 +181,26 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         }
     }
 
-    /// @notice Tallies the number of existing `safe` owners that wear a signer hat, sets `signerCount` to that value, and updates the `safe` threshold if necessary
+    /// @notice Tallies the number of existing `safe` owners that wear a signer hat and updates the `safe` threshold if necessary
     /// @dev Does NOT remove invalid `safe` owners
     function reconcileSignerCount() public {
-        address[] memory owners = safe.getOwners();
-        uint256 validSignerCount = _countValidSigners(owners);
+        uint256 signerCount = validSignerCount();
 
-        if (validSignerCount > maxSigners) {
+        if (signerCount > maxSigners) {
             revert MaxSignersReached();
         }
 
-        // update the signer count accordingly
-        signerCount = validSignerCount;
-
         uint256 currentThreshold = safe.getThreshold();
         uint256 newThreshold;
         uint256 target = targetThreshold; // save SLOADs
 
-        if (validSignerCount <= target && validSignerCount != currentThreshold) {
-            newThreshold = validSignerCount;
-        } else if (validSignerCount > target && currentThreshold < target) {
+        if (signerCount <= target && signerCount != currentThreshold) {
+            newThreshold = signerCount;
+        } else if (signerCount > target && currentThreshold < target) {
             newThreshold = target;
         }
         if (newThreshold > 0) {
-            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", validSignerCount);
+            bytes memory data = abi.encodeWithSignature("changeThreshold(uint256)", signerCount);
 
             bool success = safe.execTransactionFromModule(
                 address(safe), // to
@@ -219,17 +215,21 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         }
     }
 
+    function validSignerCount() public view returns (uint256 signerCount) {
+        signerCount = _countValidSigners(safe.getOwners());
+    }
+
     /// @notice Internal function to count the number of valid signers in an array of addresses
     /// @param owners The addresses to check for validity
-    /// @return validSignerCount The number of valid signers in `owners`
-    function _countValidSigners(address[] memory owners) internal view returns (uint256 validSignerCount) {
+    /// @return signerCount The number of valid signers in `owners`
+    function _countValidSigners(address[] memory owners) internal view returns (uint256 signerCount) {
         uint256 length = owners.length;
         // count the existing safe owners that wear the signer hat
         for (uint256 i; i < length;) {
             if (isValidSigner(owners[i])) {
                 // shouldn't overflow given reasonable owners array length
                 unchecked {
-                    ++validSignerCount;
+                    ++signerCount;
                 }
             }
             // shouldn't overflow given reasonable owners array length
@@ -284,9 +284,6 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
             addOwnerData = abi.encodeWithSignature("addOwnerWithThreshold(address,uint256)", _signer, newThreshold);
         }
 
-        // increment signer count
-        signerCount = newSignerCount;
-
         // execute the call
         bool success = safe.execTransactionFromModule(
             address(safe), // to
@@ -304,17 +301,12 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
     /// @dev Unsafe. Does not check if `_signer` is a valid signer.
     /// @param _owners Array of owners on the `safe`
     /// @param _ownerCount The number of owners on the `safe` (length of `_owners` array)
-    /// @param _maxSigners The maximum number of signers allowed
-    /// @param _currentSignerCount The current number of signers
     /// @param _signer The address to add as a new `safe` owner
     /// @return success Whether an invalid signer was found and successfully replaced with `_signer`
-    function _swapSigner(
-        address[] memory _owners,
-        uint256 _ownerCount,
-        uint256 _maxSigners,
-        uint256 _currentSignerCount,
-        address _signer
-    ) internal returns (bool success) {
+    function _swapSigner(address[] memory _owners, uint256 _ownerCount, address _signer)
+        internal
+        returns (bool success)
+    {
         address ownerToCheck;
         bytes memory data;
 
@@ -342,8 +334,6 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
                     revert FailedExecRemoveSigner();
                 }
 
-                // increment the signer count if signerCount was correct, ie `reconcileSignerCount` was called prior
-                if (_currentSignerCount < _maxSigners) ++signerCount;
                 break;
             }
             unchecked {
@@ -368,10 +358,10 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
     function _removeSigner(address _signer) internal {
         bytes memory removeOwnerData;
         address[] memory owners = safe.getOwners();
-        uint256 currentSignerCount = signerCount; // save an SLOAD
-        uint256 newSignerCount;
+        uint256 validSigners = _countValidSigners(owners);
+        // uint256 newSignerCount;
 
-        if (currentSignerCount < 2 && owners.length == 1) {
+        if (validSigners < 2 && owners.length == 1) {
             // signerCount could be 0 after reconcileSignerCount
             // make address(this) the only owner
             removeOwnerData = abi.encodeWithSignature(
@@ -385,27 +375,18 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
         } else {
             uint256 currentThreshold = safe.getThreshold();
             uint256 newThreshold = currentThreshold;
-            uint256 validSignerCount = _countValidSigners(owners);
-
-            if (validSignerCount == currentSignerCount) {
-                newSignerCount = currentSignerCount;
-            } else {
-                newSignerCount = currentSignerCount - 1;
-            }
+            // uint256 validSignerCount = _countValidSigners(owners);
 
             // ensure that txs can't execute if fewer signers than target threshold
-            if (newSignerCount <= targetThreshold) {
-                newThreshold = newSignerCount;
+            if (validSigners <= targetThreshold) {
+                newThreshold = validSigners;
             }
 
             removeOwnerData = abi.encodeWithSignature(
                 "removeOwner(address,address,uint256)", _findPrevOwner(owners, _signer), _signer, newThreshold
             );
         }
 
-        // update signerCount
-        signerCount = newSignerCount;
-
         bool success = safe.execTransactionFromModule(
             address(safe), // to
             0, // value
@@ -552,7 +533,7 @@ abstract contract HatsSignerGateBase is BaseGuard, SignatureDecoder, HatsOwnedIn
     /// @notice Internal function to calculate the threshold that `safe` should have, given the correct `signerCount`, `minThreshold`, and `targetThreshold`
     /// @return _threshold The correct threshold
     function _getCorrectThreshold() internal view returns (uint256 _threshold) {
-        uint256 count = _countValidSigners(safe.getOwners());
+        uint256 count = validSignerCount();
         uint256 min = minThreshold;
         uint256 max = targetThreshold;
         if (count < min) _threshold = min;

diff --git a/src/MultiHatsSignerGate.sol b/src/MultiHatsSignerGate.sol
@@ -40,7 +40,8 @@ contract MultiHatsSignerGate is HatsSignerGateBase {
     /// @param _hatId The hat id to claim signer rights for
     function claimSigner(uint256 _hatId) public {
         uint256 maxSigs = maxSigners; // save SLOADs
-        uint256 currentSignerCount = signerCount;
+        address[] memory owners = safe.getOwners();
+        uint256 currentSignerCount = _countValidSigners(owners);
 
         if (currentSignerCount >= maxSigs) {
             revert MaxSignersReached();
@@ -63,11 +64,10 @@ contract MultiHatsSignerGate is HatsSignerGateBase {
         If we're already at maxSigners, we'll replace one of the invalid owners by swapping the signer.
         Otherwise, we'll simply add the new signer.
         */
-        address[] memory owners = safe.getOwners();
         uint256 ownerCount = owners.length;
 
         if (ownerCount >= maxSigs) {
-            bool swapped = _swapSigner(owners, ownerCount, maxSigs, currentSignerCount, msg.sender);
+            bool swapped = _swapSigner(owners, ownerCount, msg.sender);
             if (!swapped) {
                 // if there are no invalid owners, we can't add a new signer, so we revert
                 revert NoInvalidSignersToReplace();

diff --git a/test/HSGTestSetup.t.sol b/test/HSGTestSetup.t.sol
@@ -44,6 +44,7 @@ contract HSGTestSetup is HSGFactoryTestSetup, SignatureDecoder {
         );
 
         (hatsSignerGate, safe) = deployHSGAndSafe(ownerHat, signerHat, minThreshold, targetThreshold, maxSigners);
+        mockIsWearerCall(address(hatsSignerGate), signerHat, false);
     }
 
     //// HELPER FUNCTIONS ////
-Original file line number
+Diff line change
@@ Expand Up @@
             );
             (hatsSignerGate, safe) = deployHSGAndSafe(ownerHat, signerHat, minThreshold, targetThreshold, maxSigners);
+            mockIsWearerCall(address(hatsSignerGate), signerHat, false);
         }
         //// HELPER FUNCTIONS ////
@@ Expand Down @@