Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initializable delay module #2

Merged
merged 3 commits into from
Jul 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions contracts/DelayModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ interface Executor {
}

contract DelayModule {

event DelayModuleSetup(address indexed initiator, address indexed safe);
event TransactionAdded(
uint indexed queueNonce,
bytes32 indexed txHash,
Expand All @@ -32,7 +32,7 @@ contract DelayModule {
event EnabledModule(address module);
event DisabledModule(address module);

Executor public immutable executor;
Executor public executor;
uint256 public txCooldown;
uint256 public txExpiration;
uint256 public txNonce;
Expand All @@ -44,17 +44,27 @@ contract DelayModule {
// Mapping of approved modules
mapping(address => bool) public modules;

bool public isInitialized = false;

constructor() {
isInitialized = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is isInitialized set to true in the constructor?
Won't this make it always fail the check on line 58? require(!isInitialized, "Module is already initialized");

}

/// @param _executor Address of the executor (e.g. a Safe)
/// @param cooldown Cooldown in seconds that should be required after a transaction is proposed
/// @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
constructor(Executor _executor, uint256 cooldown, uint256 expiration) {
function setUp(Executor _executor, uint256 cooldown, uint256 expiration) external {
require(!isInitialized, "Module is already initialized");
require(cooldown > 0, "Cooldown must to be greater than 0");
require(expiration == 0 || expiration >= 60 , "Expiratition must be 0 or at least 60 seconds");
executor = _executor;
txExpiration = expiration;
txCooldown = cooldown;
}
isInitialized = true;

emit DelayModuleSetup(msg.sender, address(_executor));
}

modifier executorOnly() {
require(msg.sender == address(executor), "Not authorized");
Expand Down
10 changes: 10 additions & 0 deletions contracts/test/DelayModuleMock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.8.0;

import "../DelayModule.sol";

contract DelayModuleMock is DelayModule {
constructor() DelayModule() {
isInitialized = false;
}
}
22 changes: 13 additions & 9 deletions test/DelayModule.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,35 @@ describe("DelayModule", async () => {

const setupTestWithTestExecutor = deployments.createFixture(async () => {
const base = await baseSetup();
const Module = await hre.ethers.getContractFactory("DelayModule");
const module = await Module.deploy(base.executor.address, 42, "0x1337");
const Module = await hre.ethers.getContractFactory("DelayModuleMock");
const module = await Module.deploy();
await module.setUp(base.executor.address, 42, "0x1337");
return { ...base, Module, module };
})

const [user1] = waffle.provider.getWallets();

describe("constructor()", async () => {
describe("setUp()", async () => {
it("throws if cooldown is 0", async () => {
const Module = await hre.ethers.getContractFactory("DelayModule")
const Module = await hre.ethers.getContractFactory("DelayModuleMock")
const module = await Module.deploy()
await expect(
Module.deploy(user1.address, 0, 0)
module.setUp(user1.address, 0, 0)
).to.be.revertedWith("Cooldown must to be greater than 0")
})

it("throws if not enough time between txCooldown and txExpiration", async () => {
const Module = await hre.ethers.getContractFactory("DelayModule")
const Module = await hre.ethers.getContractFactory("DelayModuleMock")
const module = await Module.deploy()
await expect(
Module.deploy(user1.address, 1, 59)
module.setUp(user1.address, 1, 59)
).to.be.revertedWith("Expiratition must be 0 or at least 60 seconds")
})

it("txExpiration can be 0", async () => {
const Module = await hre.ethers.getContractFactory("DelayModule")
await Module.deploy(user1.address, 1, 0)
const Module = await hre.ethers.getContractFactory("DelayModuleMock")
const module = await Module.deploy()
await module.setUp(user1.address, 1, 0)
})
})

Expand Down