feat: add multiple update* methods (#149)

* feat: add updateAuthenticators, updateVotingStrategies and updateSettings * feat: add updateAuthenticatorsAndVotingStrategies * fix: add --via-ir to forge coverage * fix: add via_ir in foundry.toml * refactor: split the functions into updateSettings and updateStrategies * fix: check for metadatauriupdated event when testing * fix: check for _min and _max and do not use internal setters * refactor: move back from votingStrategiesMetadataURIs to votingStrategyMetadataURIs * refactor: remove set* entrypoints * refactor: remove EmptyArray check in internal functions * fix: Differentiate NO_UPDATE_METADATA_URI and NO_UPDATE_METADATA_URI_HASH * fix: fix NO_UPDATE_METADATA_URI_HASH * fix: remove useless comments * refactor: changing magic values for NO_UPDATE * refactor: use types instead of variables for no_update * refactor: use input struct for update settings * fix: remove extra comments on UpdateSettingsInput * chore: remove extra comment in updateSettings function * chore: add comments on values of NO_UPDATE_* * chore: fix typo in Space.col comments * chore: updated comments --------- Co-authored-by: Orland0x <[email protected]>
snapshot-labs · May 18, 2023 · cea0f5c · cea0f5c
1 parent 473e50e
commit cea0f5c
Show file tree

Hide file tree

Showing 22 changed files with 958 additions and 272 deletions.
diff --git a/.forge-snapshots/ProposeSigComp.snap b/.forge-snapshots/ProposeSigComp.snap
@@ -1 +1 @@
-171501
+170896
diff --git a/.forge-snapshots/VoteSigComp.snap b/.forge-snapshots/VoteSigComp.snap
@@ -1 +1 @@
-52138
+51979
diff --git a/.forge-snapshots/VoteSigCompMetadata.snap b/.forge-snapshots/VoteSigCompMetadata.snap
@@ -1 +1 @@
-53824
+53665
diff --git a/.forge-snapshots/VoteTxComp.snap b/.forge-snapshots/VoteTxComp.snap
@@ -1 +1 @@
-45325
+45220
diff --git a/.forge-snapshots/VoteTxCompMetadata.snap b/.forge-snapshots/VoteTxCompMetadata.snap
@@ -1 +1 @@
-46916
+46811
diff --git a/foundry.toml b/foundry.toml
@@ -18,4 +18,4 @@ fs_permissions = [{ access = "read-write", path = ".forge-snapshots/"}]
 
 [profile.ci]
 fuzz = { runs = 1_000 }
-verbosity = 4
+verbosity = 4
diff --git a/src/Space.sol b/src/Space.sol
@@ -8,7 +8,15 @@ import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils
 import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
 import { IERC4824 } from "src/interfaces/IERC4824.sol";
 import { ISpace, ISpaceActions, ISpaceState, ISpaceOwnerActions } from "src/interfaces/ISpace.sol";
-import { Choice, FinalizationStatus, IndexedStrategy, Proposal, ProposalStatus, Strategy } from "src/types.sol";
+import {
+    Choice,
+    FinalizationStatus,
+    IndexedStrategy,
+    Proposal,
+    ProposalStatus,
+    Strategy,
+    UpdateSettingsInput
+} from "src/types.sol";
 import { IVotingStrategy } from "src/interfaces/IVotingStrategy.sol";
 import { IExecutionStrategy } from "src/interfaces/IExecutionStrategy.sol";
 import { IProposalValidationStrategy } from "src/interfaces/IProposalValidationStrategy.sol";
@@ -22,6 +30,18 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
     using BitPacker for uint256;
     using SXUtils for IndexedStrategy[];
 
+    /// @dev Placeholder value to indicate the user does not want to update a string.
+    /// @dev Evaluates to: `0xf2cda9b13ed04e585461605c0d6e804933ca828111bd94d4e6a96c75e8b048ba`.
+    bytes32 private constant NO_UPDATE_HASH = keccak256(abi.encodePacked("No update"));
+
+    /// @dev Placeholder value to indicate the user does not want to update an address.
+    /// @dev Evaluates to: `0xf2cda9b13ed04e585461605c0d6e804933ca8281`.
+    address private constant NO_UPDATE_ADDRESS = address(bytes20(keccak256(abi.encodePacked("No update"))));
+
+    /// @dev Placeholder value to indicate the user does not want to update a uint32.
+    /// @dev Evaluates to: `0xf2cda9b1`.
+    uint32 private constant NO_UPDATE_UINT32 = uint32(bytes4(keccak256(abi.encodePacked("No update"))));
+
     /// @inheritdoc IERC4824
     string public daoURI;
     /// @inheritdoc ISpaceState
@@ -70,6 +90,9 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
         _setMinVotingDuration(_minVotingDuration);
         _setProposalValidationStrategy(_proposalValidationStrategy);
         _setVotingDelay(_votingDelay);
+
+        if (_votingStrategies.length == 0) revert EmptyArray();
+        if (_authenticators.length == 0) revert EmptyArray();
         _addVotingStrategies(_votingStrategies);
         _addAuthenticators(_authenticators);
 
@@ -98,68 +121,70 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
     // ------------------------------------
 
     /// @inheritdoc ISpaceOwnerActions
-    function setMaxVotingDuration(uint32 _maxVotingDuration) external override onlyOwner {
-        _setMaxVotingDuration(_maxVotingDuration);
-        emit MaxVotingDurationUpdated(_maxVotingDuration);
-    }
-
-    /// @inheritdoc ISpaceOwnerActions
-    function setMinVotingDuration(uint32 _minVotingDuration) external override onlyOwner {
-        _setMinVotingDuration(_minVotingDuration);
-        emit MinVotingDurationUpdated(_minVotingDuration);
-    }
+    // solhint-disable-next-line code-complexity
+    function updateSettings(UpdateSettingsInput calldata input) external override onlyOwner {
+        if ((input.minVotingDuration != NO_UPDATE_UINT32) && (input.maxVotingDuration != NO_UPDATE_UINT32)) {
+            // Check that min and max VotingDuration are valid
+            // We don't use the internal `_setMinVotingDuration` and `_setMaxVotingDuration` functions because
+            // it would revert when `_minVotingDuration > maxVotingDuration` (when the new `_min` is
+            // bigger than the current `max`).
+            if (input.minVotingDuration > input.maxVotingDuration)
+                revert InvalidDuration(input.minVotingDuration, input.maxVotingDuration);
+
+            minVotingDuration = input.minVotingDuration;
+            emit MinVotingDurationUpdated(input.minVotingDuration);
+
+            maxVotingDuration = input.maxVotingDuration;
+            emit MaxVotingDurationUpdated(input.maxVotingDuration);
+        } else if (input.minVotingDuration != NO_UPDATE_UINT32) {
+            _setMinVotingDuration(input.minVotingDuration);
+            emit MinVotingDurationUpdated(input.minVotingDuration);
+        } else if (input.maxVotingDuration != NO_UPDATE_UINT32) {
+            _setMaxVotingDuration(input.maxVotingDuration);
+            emit MaxVotingDurationUpdated(input.maxVotingDuration);
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function setMetadataURI(string calldata _metadataURI) external override onlyOwner {
-        emit MetadataURIUpdated(_metadataURI);
-    }
+        if (input.votingDelay != NO_UPDATE_UINT32) {
+            _setVotingDelay(input.votingDelay);
+            emit VotingDelayUpdated(input.votingDelay);
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function setDaoURI(string calldata newDaoURI) external override onlyOwner {
-        _setDaoURI(newDaoURI);
-        emit DaoURIUpdated(newDaoURI);
-    }
+        if (keccak256(abi.encodePacked(input.metadataURI)) != NO_UPDATE_HASH) {
+            emit MetadataURIUpdated(input.metadataURI);
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function setProposalValidationStrategy(
-        Strategy calldata _proposalValidationStrategy,
-        string calldata proposalValidationStrategyMetadataURI
-    ) external override onlyOwner {
-        _setProposalValidationStrategy(_proposalValidationStrategy);
-        emit ProposalValidationStrategyUpdated(_proposalValidationStrategy, proposalValidationStrategyMetadataURI);
-    }
+        if (keccak256(abi.encodePacked(input.daoURI)) != NO_UPDATE_HASH) {
+            _setDaoURI(input.daoURI);
+            emit DaoURIUpdated(input.daoURI);
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function setVotingDelay(uint32 _votingDelay) external override onlyOwner {
-        _setVotingDelay(_votingDelay);
-        emit VotingDelayUpdated(_votingDelay);
-    }
+        if (input.proposalValidationStrategy.addr != NO_UPDATE_ADDRESS) {
+            _setProposalValidationStrategy(input.proposalValidationStrategy);
+            emit ProposalValidationStrategyUpdated(
+                input.proposalValidationStrategy,
+                input.proposalValidationStrategyMetadataURI
+            );
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function addVotingStrategies(
-        Strategy[] calldata _votingStrategies,
-        string[] calldata votingStrategyMetadataURIs
-    ) external override onlyOwner {
-        _addVotingStrategies(_votingStrategies);
-        emit VotingStrategiesAdded(_votingStrategies, votingStrategyMetadataURIs);
-    }
+        if (input.authenticatorsToAdd.length > 0) {
+            _addAuthenticators(input.authenticatorsToAdd);
+            emit AuthenticatorsAdded(input.authenticatorsToAdd);
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function removeVotingStrategies(uint8[] calldata _votingStrategyIndices) external override onlyOwner {
-        _removeVotingStrategies(_votingStrategyIndices);
-        emit VotingStrategiesRemoved(_votingStrategyIndices);
-    }
+        if (input.authenticatorsToRemove.length > 0) {
+            _removeAuthenticators(input.authenticatorsToRemove);
+            emit AuthenticatorsRemoved(input.authenticatorsToRemove);
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function addAuthenticators(address[] calldata _authenticators) external override onlyOwner {
-        _addAuthenticators(_authenticators);
-        emit AuthenticatorsAdded(_authenticators);
-    }
+        if (input.votingStrategiesToAdd.length > 0) {
+            _addVotingStrategies(input.votingStrategiesToAdd);
+            emit VotingStrategiesAdded(input.votingStrategiesToAdd, input.votingStrategyMetadataURIsToAdd);
+        }
 
-    /// @inheritdoc ISpaceOwnerActions
-    function removeAuthenticators(address[] calldata _authenticators) external override onlyOwner {
-        _removeAuthenticators(_authenticators);
-        emit AuthenticatorsRemoved(_authenticators);
+        if (input.votingStrategiesToRemove.length > 0) {
+            _removeVotingStrategies(input.votingStrategiesToRemove);
+            emit VotingStrategiesRemoved(input.votingStrategiesToRemove);
+        }
     }
 
     /// @dev Gates access to whitelisted authenticators only.
@@ -354,7 +379,6 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
 
     /// @dev Adds an array of voting strategies.
     function _addVotingStrategies(Strategy[] memory _votingStrategies) internal {
-        if (_votingStrategies.length == 0) revert EmptyArray();
         uint256 cachedActiveVotingStrategies = activeVotingStrategies;
         uint8 cachedNextVotingStrategyIndex = nextVotingStrategyIndex;
         if (cachedNextVotingStrategyIndex >= 256 - _votingStrategies.length) revert ExceedsStrategyLimit();
@@ -370,7 +394,6 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
 
     /// @dev Removes an array of voting strategies, specified by their indices.
     function _removeVotingStrategies(uint8[] memory _votingStrategyIndices) internal {
-        if (_votingStrategyIndices.length == 0) revert EmptyArray();
         for (uint8 i = 0; i < _votingStrategyIndices.length; i++) {
             activeVotingStrategies = activeVotingStrategies.setBit(_votingStrategyIndices[i], false);
         }
@@ -380,15 +403,13 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
 
     /// @dev Adds an array of authenticators.
     function _addAuthenticators(address[] memory _authenticators) internal {
-        if (_authenticators.length == 0) revert EmptyArray();
         for (uint256 i = 0; i < _authenticators.length; i++) {
             authenticators[_authenticators[i]] = true;
         }
     }
 
     /// @dev Removes an array of authenticators.
     function _removeAuthenticators(address[] memory _authenticators) internal {
-        if (_authenticators.length == 0) revert EmptyArray();
         for (uint256 i = 0; i < _authenticators.length; i++) {
             authenticators[_authenticators[i]] = false;
         }

diff --git a/src/interfaces/space/ISpaceEvents.sol b/src/interfaces/space/ISpaceEvents.sol
@@ -116,7 +116,7 @@ interface ISpaceEvents {
 
     /// @notice Emitted when the voting delay is updated.
     /// @param newVotingDelay The new voting delay.
-    event VotingDelayUpdated(uint256 newVotingDelay);
+    event VotingDelayUpdated(uint32 newVotingDelay);
 
     /// @notice Emitted when a proposal is updated.
     /// @param proposalId The proposal id.

diff --git a/src/interfaces/space/ISpaceOwnerActions.sol b/src/interfaces/space/ISpaceOwnerActions.sol
@@ -2,7 +2,7 @@
 
 pragma solidity ^0.8.18;
 
-import { Strategy } from "../../types.sol";
+import { Strategy, UpdateSettingsInput } from "../../types.sol";
 
 /// @title Space Owner Actions
 /// @notice The actions that can be performed by the owner of a Space,
@@ -13,51 +13,22 @@ interface ISpaceOwnerActions {
     /// @param   proposalId  The proposal to cancel.
     function cancel(uint256 proposalId) external;
 
-    /// @notice Sets the voting delay.
-    /// @param delay The new voting delay.
-    function setVotingDelay(uint32 delay) external;
-
-    /// @notice Sets the minimum voting duration.
-    /// @param duration The new minimum voting duration.
-    function setMinVotingDuration(uint32 duration) external;
-
-    /// @notice Sets the maximum voting duration.
-    /// @param duration The new maximum voting duration.
-    function setMaxVotingDuration(uint32 duration) external;
-
-    /// @notice Sets the proposal validation strategy.
-    /// @param proposalValidationStrategy The new proposal validation strategy.
-    /// @param proposalValidationStrategyMetadataURI The new metadata URI for the proposal validation strategy.
-    function setProposalValidationStrategy(
-        Strategy calldata proposalValidationStrategy,
-        string calldata proposalValidationStrategyMetadataURI
-    ) external;
-
-    /// @notice Sets the metadata URI for the Space.
-    /// @param metadataURI The new metadata URI.
-    function setMetadataURI(string calldata metadataURI) external;
-
-    /// @notice Sets the DAO URI for the Space.
-    /// @param daoURI The new DAO URI.
-    function setDaoURI(string calldata daoURI) external;
-
-    /// @notice Adds an array of voting strategies.
-    /// @param votingStrategies The array of voting strategies to add.
-    /// @param votingStrategyMetadataURIs The array of metadata URIs for `votingStrategies`.
-    function addVotingStrategies(
-        Strategy[] calldata votingStrategies,
-        string[] calldata votingStrategyMetadataURIs
-    ) external;
-
-    /// @notice Removes an array of voting strategies.
-    /// @param indicesToRemove The array of indices of the voting strategies to remove.
-    function removeVotingStrategies(uint8[] calldata indicesToRemove) external;
-
-    /// @notice Adds an array of authenticators.
-    /// @param authenticators The array of authenticator addresses to add.
-    function addAuthenticators(address[] calldata authenticators) external;
-
-    /// @notice Removes an array of authenticators.
-    /// @param authenticators The array of authenticator addresses to remove.
-    function removeAuthenticators(address[] calldata authenticators) external;
+    /// @notice Updates the settings.
+    /// @param input The settings to modify
+    /// @dev The structure should consist of:
+    ///     minVotingDuration The new minimum voting duration. Set to `NO_UPDATE_UINT32` to ignore.
+    ///     maxVotingDuration The new maximum voting duration. Set to `NO_UPDATE_UINT32` to ignore.
+    ///     votingDelay The new voting delay. Set to `NO_UPDATE_UINT32` to ignore.
+    ///     metadataURI The new metadataURI. Set to `NO_UPDATE_STRING` to ignore.
+    ///     daoURI The new daoURI. Set to `NO_UPDATE_STRING` to ignore.
+    ///     proposalValidationStrategy The new proposal validation strategy to use. Set
+    ///                 to `NO_UPDATE_STRATEGY` to ignore.
+    ///     proposalValidationStrategyMetadataURI The new metadata URI for the proposal validation strategy.
+    ///     authenticatorsToAdd The authenticators to add. Set to an empty array to ignore.
+    ///     authenticatorsToRemove The authenticators to remove. Set to an empty array to ignore.
+    ///     votingStrategiesToAdd The voting strategies to add. Set to an empty array to ignore.
+    ///     votingStrategyMetadataURIsToAdd The voting strategy metadata uris to add. Set to
+    ///                 an empty array to ignore.
+    ///     votignStrategiesToRemove The indices of voting strategies to remove. Set to empty array to ignore.
+    function updateSettings(UpdateSettingsInput calldata input) external;
 }
diff --git a/src/types.sol b/src/types.sol
@@ -77,3 +77,20 @@ struct MetaTransaction {
     // We require a salt so that the struct can always be unique and we can use its hash as a unique identifier.
     uint256 salt;
 }
+
+/// @dev    Structure used for the function `updateSettings` because of solidity's stack constraints.
+///         For more information, see `ISpaceOwnerActions.sol`.
+struct UpdateSettingsInput {
+    uint32 minVotingDuration;
+    uint32 maxVotingDuration;
+    uint32 votingDelay;
+    string metadataURI;
+    string daoURI;
+    Strategy proposalValidationStrategy;
+    string proposalValidationStrategyMetadataURI;
+    address[] authenticatorsToAdd;
+    address[] authenticatorsToRemove;
+    Strategy[] votingStrategiesToAdd;
+    string[] votingStrategyMetadataURIsToAdd;
+    uint8[] votingStrategiesToRemove;
+}
diff --git a/test/ActiveProposalsLimiterProposalValidationStrategy.t.sol b/test/ActiveProposalsLimiterProposalValidationStrategy.t.sol
@@ -2,7 +2,7 @@
 pragma solidity ^0.8.18;
 
 import { SpaceTest } from "./utils/Space.t.sol";
-import { Strategy } from "../src/types.sol";
+import { Strategy, UpdateSettingsInput } from "../src/types.sol";
 import {
     ActiveProposalsLimiterProposalValidationStrategy
 } from "../src/proposal-validation-strategies/ActiveProposalsLimiterProposalValidationStrategy.sol";
@@ -20,9 +20,25 @@ contract ActiveProposalsLimterTest is SpaceTest {
 
         activeProposalsLimiterProposalValidationStrategy = new ActiveProposalsLimiterProposalValidationStrategy();
 
-        space.setProposalValidationStrategy(
-            Strategy(address(activeProposalsLimiterProposalValidationStrategy), abi.encode(cooldown, maxActive)),
-            ""
+        Strategy memory newProposalStrategy = Strategy(
+            address(activeProposalsLimiterProposalValidationStrategy),
+            abi.encode(cooldown, maxActive)
+        );
+        space.updateSettings(
+            UpdateSettingsInput(
+                NO_UPDATE_UINT32,
+                NO_UPDATE_UINT32,
+                NO_UPDATE_UINT32,
+                NO_UPDATE_STRING,
+                NO_UPDATE_STRING,
+                newProposalStrategy,
+                "",
+                NO_UPDATE_ADDRESSES,
+                NO_UPDATE_ADDRESSES,
+                NO_UPDATE_STRATEGIES,
+                NO_UPDATE_STRINGS,
+                NO_UPDATE_UINT8S
+            )
         );
     }