add tests

aragon · Aug 14, 2024 · decd4ff · decd4ff
1 parent b233838
commit decd4ff
Show file tree

Hide file tree

Showing 15 changed files with 487 additions and 246 deletions.
diff --git a/contracts/src/mocks/plugin/PluginCloneableMock.sol b/contracts/src/mocks/plugin/PluginCloneableMock.sol
@@ -16,6 +16,23 @@ contract PluginCloneableMockBuild1 is PluginCloneable {
         __PluginCloneable_init(_dao);
         state1 = 1;
     }
+
+    function execute(
+        uint256 _callId,
+        IDAO.Action[] memory _actions,
+        uint256 _allowFailureMap
+    ) external returns (bytes[] memory execResults, uint256 failureMap) {
+        (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap);
+    }
+
+    function execute(
+        address _target,
+        uint256 _callId,
+        IDAO.Action[] memory _actions,
+        uint256 _allowFailureMap
+    ) external returns (bytes[] memory execResults, uint256 failureMap) {
+        (execResults, failureMap) = _execute(_target, bytes32(_callId), _actions, _allowFailureMap);
+    }
 }
 
 /// @notice A mock cloneable plugin to be deployed via the minimal proxy pattern.

diff --git a/contracts/src/mocks/plugin/PluginMock.sol b/contracts/src/mocks/plugin/PluginMock.sol
@@ -14,4 +14,21 @@ contract PluginMockBuild1 is Plugin {
     constructor(IDAO _dao) Plugin(_dao) {
         state1 = 1;
     }
+
+    function execute(
+        uint256 _callId,
+        IDAO.Action[] memory _actions,
+        uint256 _allowFailureMap
+    ) external returns (bytes[] memory execResults, uint256 failureMap) {
+        (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap);
+    }
+
+    function execute(
+        address _target,
+        uint256 _callId,
+        IDAO.Action[] memory _actions,
+        uint256 _allowFailureMap
+    ) external returns (bytes[] memory execResults, uint256 failureMap) {
+        (execResults, failureMap) = _execute(_target, bytes32(_callId), _actions, _allowFailureMap);
+    }
 }
diff --git a/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol b/contracts/src/mocks/plugin/PluginUUPSUpgradeableMock.sol
@@ -16,6 +16,23 @@ contract PluginUUPSUpgradeableMockBuild1 is PluginUUPSUpgradeable {
         __PluginUUPSUpgradeable_init(_dao);
         state1 = 1;
     }
+
+    function execute(
+        uint256 _callId,
+        IDAO.Action[] memory _actions,
+        uint256 _allowFailureMap
+    ) external returns (bytes[] memory execResults, uint256 failureMap) {
+        (execResults, failureMap) = _execute(bytes32(_callId), _actions, _allowFailureMap);
+    }
+
+    function execute(
+        address _target,
+        uint256 _callId,
+        IDAO.Action[] memory _actions,
+        uint256 _allowFailureMap
+    ) external returns (bytes[] memory execResults, uint256 failureMap) {
+        (execResults, failureMap) = _execute(_target, bytes32(_callId), _actions, _allowFailureMap);
+    }
 }
 
 /// @notice A mock upgradeable plugin to be deployed via the UUPS proxy pattern.

diff --git a/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol b/contracts/src/mocks/plugin/extensions/proposal/ProposalMock.sol
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+import {Proposal} from "../../../../plugin/extensions/proposal/Proposal.sol";
+import {IDAO} from "../../../../dao/IDAO.sol";
+
+/// @notice A mock contract.
+/// @dev DO NOT USE IN PRODUCTION!
+contract ProposalMock is Proposal {
+    // We don't need to test these below functions as they will be tested in the actual plugins.
+    // This mock contract is only used to test `supportsInterface` function.
+
+    function createProposal(
+        bytes memory data,
+        IDAO.Action[] memory actions,
+        uint64 startDate,
+        uint64 endDate
+    ) external returns (uint256 proposalId) {}
+
+    function canExecute(uint256 proposalId) external returns (bool) {}
+}
diff --git a/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol b/contracts/src/mocks/plugin/extensions/proposal/ProposalUpgradeableMock.sol
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+import {ProposalUpgradeable} from "../../../../plugin/extensions/proposal/ProposalUpgradeable.sol";
+import {IDAO} from "../../../../dao/IDAO.sol";
+
+/// @notice A mock contract.
+/// @dev DO NOT USE IN PRODUCTION!
+contract ProposalUpgradeableMock is ProposalUpgradeable {
+    // We don't need to test these below functions as they will be tested in the actual plugins.
+    // This mock contract is only used to test `supportsInterface` function.
+
+    function createProposal(
+        bytes memory data,
+        IDAO.Action[] memory actions,
+        uint64 startDate,
+        uint64 endDate
+    ) external returns (uint256 proposalId) {}
+
+    function canExecute(uint256 proposalId) external returns (bool) {}
+}
diff --git a/contracts/src/plugin/Plugin.sol b/contracts/src/plugin/Plugin.sol
@@ -15,11 +15,14 @@ import {IPlugin} from "./IPlugin.sol";
 /// @notice An abstract, non-upgradeable contract to inherit from when creating a plugin being deployed via the `new` keyword.
 /// @custom:security-contact [email protected]
 abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion {
-    address public target;
+    address private target;
 
     /// @dev Emitted each time the Target is set.
     event TargetSet(address indexed previousTarget, address indexed newTarget);
 
+    /// @notice The ID of the permission required to call the `setTarget` function.
+    bytes32 public constant SET_TARGET_PERMISSION_ID = keccak256("SET_TARGET_PERMISSION");
+
     /// @notice Constructs the plugin by storing the associated DAO.
     /// @param _dao The DAO contract.
     constructor(IDAO _dao) DaoAuthorizable(_dao) {}
@@ -30,11 +33,14 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion {
     }
 
     /// @dev Sets the target to a new target (`newTarget`).
-    /// @notice Can only be called by the current owner. TODO
-    function setTarget(address _target) public {
-        address previousTarget = target;
-        target = _target;
-        emit TargetSet(previousTarget, _target);
+    /// @param _target The target contract.
+    function setTarget(address _target) public auth(SET_TARGET_PERMISSION_ID) {
+        _setTarget(_target);
+    }
+
+    /// @notice Returns the currently set target contract.
+    function getTarget() public view returns (address) {
+        return target;
     }
 
     /// @notice Checks if this or the parent contract supports an interface by its ID.
@@ -44,9 +50,18 @@ abstract contract Plugin is IPlugin, ERC165, DaoAuthorizable, ProtocolVersion {
         return
             _interfaceId == type(IPlugin).interfaceId ||
             _interfaceId == type(IProtocolVersion).interfaceId ||
+            _interfaceId == this.setTarget.selector ^ this.getTarget.selector ||
             super.supportsInterface(_interfaceId);
     }
 
+    /// @notice Sets the target to a new target (`newTarget`).
+    /// @param _target The target contract.
+    function _setTarget(address _target) internal virtual {
+        address previousTarget = target;
+        target = _target;
+        emit TargetSet(previousTarget, _target);
+    }
+
     /// @notice Forwards the actions to the currently set `target` for the execution.
     /// @param _callId Identifier for this execution.
     /// @param _actions actions that will be eventually called.

diff --git a/contracts/src/plugin/PluginCloneable.sol b/contracts/src/plugin/PluginCloneable.sol
@@ -20,11 +20,14 @@ abstract contract PluginCloneable is
     DaoAuthorizableUpgradeable,
     ProtocolVersion
 {
-    address public target;
+    address private target;
 
     /// @dev Emitted each time the Target is set.
     event TargetSet(address indexed previousTarget, address indexed newTarget);
 
+    /// @notice The ID of the permission required to call the `setTarget` function.
+    bytes32 public constant SET_TARGET_PERMISSION_ID = keccak256("SET_TARGET_PERMISSION");
+
     /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized.
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
@@ -39,28 +42,40 @@ abstract contract PluginCloneable is
     }
 
     /// @dev Sets the target to a new target (`newTarget`).
-    /// @notice Can only be called by the current owner. TODO
-    function setTarget(address _target) public {
-        address previousTarget = target;
-        target = _target;
-        emit TargetSet(previousTarget, _target);
+    /// @param _target The target contract.
+    function setTarget(address _target) public auth(SET_TARGET_PERMISSION_ID) {
+        _setTarget(_target);
     }
 
     /// @inheritdoc IPlugin
     function pluginType() public pure override returns (PluginType) {
         return PluginType.Cloneable;
     }
 
+    /// @notice Returns the currently set target contract.
+    function getTarget() public view returns (address) {
+        return target;
+    }
+
     /// @notice Checks if this or the parent contract supports an interface by its ID.
     /// @param _interfaceId The ID of the interface.
     /// @return Returns `true` if the interface is supported.
     function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
         return
             _interfaceId == type(IPlugin).interfaceId ||
             _interfaceId == type(IProtocolVersion).interfaceId ||
+            _interfaceId == this.setTarget.selector ^ this.getTarget.selector ||
             super.supportsInterface(_interfaceId);
     }
 
+    /// @notice Sets the target to a new target (`newTarget`).
+    /// @param _target The target contract.
+    function _setTarget(address _target) internal virtual {
+        address previousTarget = target;
+        target = _target;
+        emit TargetSet(previousTarget, _target);
+    }
+
     /// @notice Forwards the actions to the currently set `target` for the execution.
     /// @param _callId Identifier for this execution.
     /// @param _actions actions that will be eventually called.

diff --git a/contracts/src/plugin/PluginUUPSUpgradeable.sol b/contracts/src/plugin/PluginUUPSUpgradeable.sol
@@ -24,11 +24,17 @@ abstract contract PluginUUPSUpgradeable is
     ProtocolVersion
 {
     // NOTE: When adding new state variables to the contract, the size of `_gap` has to be adapted below as well.
-    address public target;
+    address private target;
 
     /// @dev Emitted each time the Target is set.
     event TargetSet(address indexed previousTarget, address indexed newTarget);
 
+    /// @notice The ID of the permission required to call the `setTarget` function.
+    bytes32 public constant SET_TARGET_PERMISSION_ID = keccak256("SET_TARGET_PERMISSION");
+
+    /// @notice The ID of the permission required to call the `_authorizeUpgrade` function.
+    bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION");
+
     /// @notice Disables the initializers on the implementation contract to prevent it from being left uninitialized.
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
@@ -40,8 +46,10 @@ abstract contract PluginUUPSUpgradeable is
         return PluginType.UUPS;
     }
 
-    /// @notice The ID of the permission required to call the `_authorizeUpgrade` function.
-    bytes32 public constant UPGRADE_PLUGIN_PERMISSION_ID = keccak256("UPGRADE_PLUGIN_PERMISSION");
+    /// @notice Returns the currently set target contract.
+    function getTarget() public view returns (address) {
+        return target;
+    }
 
     /// @notice Initializes the plugin by storing the associated DAO.
     /// @param _dao The DAO contract.
@@ -51,11 +59,9 @@ abstract contract PluginUUPSUpgradeable is
     }
 
     /// @dev Sets the target to a new target (`newTarget`).
-    /// @notice Can only be called by the current owner. TODO
-    function setTarget(address _target) public {
-        address previousTarget = target;
-        target = _target;
-        emit TargetSet(previousTarget, _target);
+    /// @param _target The target contract.
+    function setTarget(address _target) public auth(SET_TARGET_PERMISSION_ID) {
+        _setTarget(_target);
     }
 
     /// @notice Checks if an interface is supported by this or its parent contract.
@@ -66,6 +72,7 @@ abstract contract PluginUUPSUpgradeable is
             _interfaceId == type(IPlugin).interfaceId ||
             _interfaceId == type(IProtocolVersion).interfaceId ||
             _interfaceId == type(IERC1822ProxiableUpgradeable).interfaceId ||
+            _interfaceId == this.setTarget.selector ^ this.getTarget.selector ||
             super.supportsInterface(_interfaceId);
     }
 
@@ -75,6 +82,14 @@ abstract contract PluginUUPSUpgradeable is
         return _getImplementation();
     }
 
+    /// @notice Sets the target to a new target (`newTarget`).
+    /// @param _target The target contract.
+    function _setTarget(address _target) internal virtual {
+        address previousTarget = target;
+        target = _target;
+        emit TargetSet(previousTarget, _target);
+    }
+
     /// @notice Forwards the actions to the currently set `target` for the execution.
     /// @param _callId Identifier for this execution.
     /// @param _actions actions that will be eventually called.

diff --git a/contracts/src/plugin/extensions/proposal/IProposal.sol b/contracts/src/plugin/extensions/proposal/IProposal.sol
@@ -50,7 +50,8 @@ interface IProposal {
     function canExecute(uint256 proposalId) external returns (bool);
 
     /// @notice Returns the proposal count determining the next proposal ID.
-    /// @dev This function is deprecated TODO:
+    /// @dev This function has been deprecated but due to backwards compatibility, it still stays in the interface
+    /// but returns maximum value of uint256 to let consumers know not to depend on it anymore.
     /// @return The proposal count.
     function proposalCount() external view returns (uint256);
 }
diff --git a/contracts/src/plugin/extensions/proposal/Proposal.sol b/contracts/src/plugin/extensions/proposal/Proposal.sol
@@ -2,27 +2,18 @@
 
 pragma solidity ^0.8.8;
 
-import {Counters} from "@openzeppelin/contracts/utils/Counters.sol";
 import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
 
-import {IDAO} from "../../../dao/IDAO.sol";
 import {IProposal} from "./IProposal.sol";
 
 /// @title Proposal
 /// @author Aragon X - 2022-2023
 /// @notice An abstract contract containing the traits and internal functionality to create and execute proposals that can be inherited by non-upgradeable DAO plugins.
 /// @custom:security-contact [email protected]
 abstract contract Proposal is IProposal, ERC165 {
-    using Counters for Counters.Counter;
-
-    /// @notice The incremental ID for proposals and executions.
-    Counters.Counter private proposalCounter;
-
-    /// Shall we remove this ? Does anyone use this ? if we keep having this,
-    // this will not return the correct value anyways anymore.
     /// @inheritdoc IProposal
-    function proposalCount() public view override returns (uint256) {
-        return 0;
+    function proposalCount() public pure override returns (uint256) {
+        return type(uint256).max;
     }
 
     /// @notice Checks if this or the parent contract supports an interface by its ID.

diff --git a/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol b/contracts/src/plugin/extensions/proposal/ProposalUpgradeable.sol
@@ -19,8 +19,8 @@ abstract contract ProposalUpgradeable is IProposal, ERC165Upgradeable {
     CountersUpgradeable.Counter private proposalCounter;
 
     /// @inheritdoc IProposal
-    function proposalCount() public view override returns (uint256) {
-        return proposalCounter.current();
+    function proposalCount() public pure override returns (uint256) {
+        return type(uint256).max;
     }
 
     /// @notice Checks if this or the parent contract supports an interface by its ID.