Feat: Delay module is zodiac compliant

README.md

@@ -1,4 +1,4 @@
-# DAO Module
+# Delay Module
 [![Build Status](https://github.com/gnosis/SafeDelay/workflows/SafeDelay/badge.svg?branch=main)](https://github.com/gnosis/SafeDelay/actions)
 [![Coverage Status](https://coveralls.io/repos/github/gnosis/SafeDelay/badge.svg?branch=main)](https://coveralls.io/github/gnosis/SafeDelay)
 

contracts/DelayModule.sol

@@ -1,28 +1,9 @@
 // SPDX-License-Identifier: LGPL-3.0-only
 pragma solidity >=0.8.0;
 
-contract Enum {
-    enum Operation {
-        Call,
-        DelegateCall
-    }
-}
-
-interface Executor {
-    /// @dev Allows a Module to execute a transaction.
-    /// @param to Destination address of module transaction.
-    /// @param value Ether value of module transaction.
-    /// @param data Data payload of module transaction.
-    /// @param operation Operation type of module transaction.
-    function execTransactionFromModule(
-        address to,
-        uint256 value,
-        bytes calldata data,
-        Enum.Operation operation
-    ) external returns (bool success);
-}
+import "@gnosis/zodiac/contracts/core/Modifier.sol";
 
-contract DelayModule {
+contract DelayModule is Modifier {
     event DelayModuleSetup(address indexed initiator, address indexed safe);
     event TransactionAdded(
         uint256 indexed queueNonce,
@@ -33,12 +14,6 @@ contract DelayModule {
         Enum.Operation operation
     );
 
-    event EnabledModule(address module);
-    event DisabledModule(address module);
-
-    address internal constant SENTINEL_MODULES = address(0x1);
-
-    Executor public executor;
     uint256 public txCooldown;
     uint256 public txExpiration;
     uint256 public txNonce;
@@ -47,11 +22,9 @@ contract DelayModule {
     mapping(uint256 => bytes32) public txHash;
     // Mapping of queue nonce to creation timestamp.
     mapping(uint256 => uint256) public txCreatedAt;
-    // Mapping of modules
-    mapping(address => address) internal modules;
 
     constructor(
-        Executor _executor,
+        address _executor,
         uint256 cooldown,
         uint256 expiration
     ) {
@@ -63,89 +36,45 @@ contract DelayModule {
     /// @param expiration Duration that a proposed transaction is valid for after the cooldown, in seconds (or 0 if valid forever)
     /// @notice There need to be at least 60 seconds between end of cooldown and expiration
     function setUp(
-        Executor _executor,
+        address _executor,
         uint256 cooldown,
         uint256 expiration
     ) public {
-        require(
-            address(executor) == address(0),
-            "Module is already initialized"
-        );
+        require(executor == address(0), "Module is already initialized");
         require(
             expiration == 0 || expiration >= 60,
             "Expiratition must be 0 or at least 60 seconds"
         );
-        if (address(_executor) != address(0)) setupModules();
 
         executor = _executor;
         txExpiration = expiration;
         txCooldown = cooldown;
 
-        emit DelayModuleSetup(msg.sender, address(_executor));
-    }
-
-    modifier executorOnly() {
-        require(msg.sender == address(executor), "Not authorized");
-        _;
-    }
-
-    modifier moduleOnly() {
-        require(modules[msg.sender] != address(0), "Module not authorized");
-        _;
+        if (_executor != address(0)) setupModules();
+        emit DelayModuleSetup(msg.sender, _executor);
     }
 
     function setupModules() internal {
         require(
             modules[SENTINEL_MODULES] == address(0),
             "setUpModules has already been called"
         );
+        transferOwnership(executor);
         modules[SENTINEL_MODULES] = SENTINEL_MODULES;
     }
 
-    /// @dev Disables a module that can add transactions to the queue
-    /// @param prevModule Module that pointed to the module to be removed in the linked list
-    /// @param module Module to be removed
-    /// @notice This can only be called by the executor
-    function disableModule(address prevModule, address module)
-        public
-        executorOnly()
-    {
-        require(
-            module != address(0) && module != SENTINEL_MODULES,
-            "Invalid module"
-        );
-        require(modules[prevModule] == module, "Module already disabled");
-        modules[prevModule] = modules[module];
-        modules[module] = address(0);
-        emit DisabledModule(module);
-    }
-
-    /// @dev Enables a module that can add transactions to the queue
-    /// @param module Address of the module to be enabled
-    /// @notice This can only be called by the executor
-    function enableModule(address module) public executorOnly() {
-        require(
-            module != address(0) && module != SENTINEL_MODULES,
-            "Invalid module"
-        );
-        require(modules[module] == address(0), "Module already enabled");
-        modules[module] = modules[SENTINEL_MODULES];
-        modules[SENTINEL_MODULES] = module;
-        emit EnabledModule(module);
-    }
-
     /// @dev Sets the cooldown before a transaction can be executed.
     /// @param cooldown Cooldown in seconds that should be required before the transaction can be executed
     /// @notice This can only be called by the executor
-    function setTxCooldown(uint256 cooldown) public executorOnly() {
+    function setTxCooldown(uint256 cooldown) public onlyOwner {
         txCooldown = cooldown;
     }
 
     /// @dev Sets the duration for which a transaction is valid.
     /// @param expiration Duration that a transaction is valid in seconds (or 0 if valid forever) after the cooldown
     /// @notice There need to be at least 60 seconds between end of cooldown and expiration
     /// @notice This can only be called by the executor
-    function setTxExpiration(uint256 expiration) public executorOnly() {
+    function setTxExpiration(uint256 expiration) public onlyOwner {
         require(
             expiration == 0 || expiration >= 60,
             "Expiratition must be 0 or at least 60 seconds"
@@ -156,7 +85,7 @@ contract DelayModule {
     /// @dev Sets transaction nonce. Used to invalidate or skip transactions in queue.
     /// @param _nonce New transaction nonce
     /// @notice This can only be called by the executor
-    function setTxNonce(uint256 _nonce) public executorOnly() {
+    function setTxNonce(uint256 _nonce) public onlyOwner {
         require(
             _nonce > txNonce,
             "New nonce must be higher than current txNonce"
@@ -176,7 +105,7 @@ contract DelayModule {
         uint256 value,
         bytes calldata data,
         Enum.Operation operation
-    ) public moduleOnly() {
+    ) public override moduleOnly returns (bool success) {
         txHash[queueNonce] = getTransactionHash(to, value, data, operation);
         txCreatedAt[queueNonce] = block.timestamp;
         emit TransactionAdded(
@@ -188,6 +117,7 @@ contract DelayModule {
             operation
         );
         queueNonce++;
+        success = true;
     }
 
     /// @dev Executes the next transaction only if the cooldown has passed and the transaction has not expired
@@ -215,10 +145,7 @@ contract DelayModule {
             txHash[txNonce] == getTransactionHash(to, value, data, operation),
             "Transaction hashes do not match"
         );
-        require(
-            executor.execTransactionFromModule(to, value, data, operation),
-            "Module transaction failed"
-        );
+        require(exec(to, value, data, operation), "Module transaction failed");
         txNonce++;
     }
 
@@ -249,43 +176,4 @@ contract DelayModule {
     function getTxCreatedAt(uint256 _nonce) public view returns (uint256) {
         return (txCreatedAt[_nonce]);
     }
-
-    /// @dev Returns if an module is enabled
-    /// @return True if the module is enabled
-    function isModuleEnabled(address _module) public view returns (bool) {
-        return SENTINEL_MODULES != _module && modules[_module] != address(0);
-    }
-
-    /// @dev Returns array of modules.
-    /// @param start Start of the page.
-    /// @param pageSize Maximum number of modules that should be returned.
-    /// @return array Array of modules.
-    /// @return next Start of the next page.
-    function getModulesPaginated(address start, uint256 pageSize)
-        external
-        view
-        returns (address[] memory array, address next)
-    {
-        // Init array with max page size
-        array = new address[](pageSize);
-
-        // Populate return array
-        uint256 moduleCount = 0;
-        address currentModule = modules[start];
-        while (
-            currentModule != address(0x0) &&
-            currentModule != SENTINEL_MODULES &&
-            moduleCount < pageSize
-        ) {
-            array[moduleCount] = currentModule;
-            currentModule = modules[currentModule];
-            moduleCount++;
-        }
-        next = currentModule;
-        // Set correct size of returned array
-        // solhint-disable-next-line no-inline-assembly
-        assembly {
-            mstore(array, moduleCount)
-        }
-    }
 }

package.json

@@ -22,6 +22,7 @@
   "author": "auryn.macmillan@gnosis.io",
   "license": "MIT",
   "devDependencies": {
+    "@gnosis/zodiac": "github:gnosis/zodiac#master",
     "@nomiclabs/hardhat-ethers": "^2.0.0",
     "@nomiclabs/hardhat-etherscan": "^2.1.0",
     "@nomiclabs/hardhat-waffle": "^2.0.0",

test/DelayModule.spec.ts

@@ -1,11 +1,10 @@
 import { expect } from "chai";
-import hre, { deployments, ethers, waffle } from "hardhat";
+import hre, { deployments, waffle } from "hardhat";
 import "@nomiclabs/hardhat-ethers";
 
-const ZERO_STATE =
-  "0x0000000000000000000000000000000000000000000000000000000000000000";
-const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000";
-const FIRST_ADDRESS = "0x0000000000000000000000000000000000000001";
+const ZeroState = "0x0000000000000000000000000000000000000000000000000000000000000000";
+const ZeroAddress = "0x0000000000000000000000000000000000000000";
+const FirstAddress = "0x0000000000000000000000000000000000000001";
 
 describe("DelayModule", async () => {
   const baseSetup = deployments.createFixture(async () => {
@@ -20,7 +19,7 @@ describe("DelayModule", async () => {
   const setupTestWithTestExecutor = deployments.createFixture(async () => {
     const base = await baseSetup();
     const Module = await hre.ethers.getContractFactory("DelayModule");
-    const module = await Module.deploy(ZERO_ADDRESS, 0, "0x1337");
+    const module = await Module.deploy(ZeroAddress, 0, "0x1337");
     await module.setUp(base.executor.address, 0, "0x1337");
     return { ...base, Module, module };
   });
@@ -30,7 +29,7 @@ describe("DelayModule", async () => {
   describe("setUp()", async () => {
     it("throws if not enough time between txCooldown and txExpiration", async () => {
       const Module = await hre.ethers.getContractFactory("DelayModule");
-      await expect(Module.deploy(ZERO_ADDRESS, 1, 59)).to.be.revertedWith(
+      await expect(Module.deploy(ZeroAddress, 1, 59)).to.be.revertedWith(
         "Expiratition must be 0 or at least 60 seconds"
       );
     });
@@ -53,15 +52,15 @@ describe("DelayModule", async () => {
     it("throws if not authorized", async () => {
       const { module } = await setupTestWithTestExecutor();
       await expect(
-        module.disableModule(FIRST_ADDRESS, user1.address)
-      ).to.be.revertedWith("Not authorized");
+        module.disableModule(FirstAddress, user1.address)
+      ).to.be.revertedWith("Ownable: caller is not the owner");
     });
 
     it("throws if module is null or sentinel", async () => {
       const { executor, module } = await setupTestWithTestExecutor();
       const disable = await module.populateTransaction.disableModule(
-        FIRST_ADDRESS,
-        FIRST_ADDRESS
+        FirstAddress,
+        FirstAddress
       );
       await expect(
         executor.exec(module.address, 0, disable.data)
@@ -71,7 +70,7 @@ describe("DelayModule", async () => {
     it("throws if module is not added ", async () => {
       const { executor, module } = await setupTestWithTestExecutor();
       const disable = await module.populateTransaction.disableModule(
-        ZERO_ADDRESS,
+        ZeroAddress,
         user1.address
       );
       await expect(
@@ -85,7 +84,7 @@ describe("DelayModule", async () => {
         user1.address
       );
       const disable = await module.populateTransaction.disableModule(
-        FIRST_ADDRESS,
+        FirstAddress,
         user1.address
       );
 
@@ -104,7 +103,7 @@ describe("DelayModule", async () => {
     it("throws if not authorized", async () => {
       const { module } = await setupTestWithTestExecutor();
       await expect(module.enableModule(user1.address)).to.be.revertedWith(
-        "Not authorized"
+        "Ownable: caller is not the owner"
       );
     });
 
@@ -123,7 +122,7 @@ describe("DelayModule", async () => {
     it("throws because module is invalid ", async () => {
       const { executor, module } = await setupTestWithTestExecutor();
       const enable = await module.populateTransaction.enableModule(
-        FIRST_ADDRESS
+        FirstAddress
       );
 
       await expect(
@@ -142,16 +141,16 @@ describe("DelayModule", async () => {
         true
       );
       await expect(
-        await module.getModulesPaginated(FIRST_ADDRESS, 10)
-      ).to.be.deep.equal([[user1.address], FIRST_ADDRESS]);
+        await module.getModulesPaginated(FirstAddress, 10)
+      ).to.be.deep.equal([[user1.address], FirstAddress]);
     });
   });
 
   describe("setTxCooldown()", async () => {
     it("throws if not authorized", async () => {
       const { module } = await setupTestWithTestExecutor();
       await expect(module.setTxCooldown(42)).to.be.revertedWith(
-        "Not authorized"
+        "Ownable: caller is not the owner"
       );
     });
 
@@ -171,7 +170,7 @@ describe("DelayModule", async () => {
     it("throws if not authorized", async () => {
       const { module } = await setupTestWithTestExecutor();
       await expect(module.setTxExpiration(42)).to.be.revertedWith(
-        "Not authorized"
+        "Ownable: caller is not the owner"
       );
     });
 
@@ -199,7 +198,9 @@ describe("DelayModule", async () => {
   describe("setTxNonce()", async () => {
     it("throws if not authorized", async () => {
       const { module } = await setupTestWithTestExecutor();
-      await expect(module.setTxNonce(42)).to.be.revertedWith("Not authorized");
+      await expect(module.setTxNonce(42)).to.be.revertedWith(
+        "Ownable: caller is not the owner"
+      );
     });
 
     it("thows if nonce is less than current nonce.", async () => {
@@ -267,7 +268,7 @@ describe("DelayModule", async () => {
 
       let txHash = await module.getTransactionHash(user1.address, 0, "0x", 0);
 
-      await expect(await module.getTxHash(0)).to.be.equals(ZERO_STATE);
+      await expect(await module.getTxHash(0)).to.be.equals(ZeroState);
       await module.execTransactionFromModule(user1.address, 0, "0x", 0);
       await expect(await module.getTxHash(0)).to.be.equals(txHash);
     });