feat: set veto guardian in execution strategy setup (#163)

* feat: veto guardian in timelock setup * feat: veto guardian in comp timelock setup * chore: updated tests * chore: fixed tests * feat: setup events in timelock exec strats * chore: inccrease codecov threshold * chore: increased coverage * chore: removed patch cov requirement --------- Co-authored-by: Orlando <[email protected]>
snapshot-labs · May 16, 2023 · 538ae1c · 538ae1c
1 parent 428ca36
commit 538ae1c
Show file tree

Hide file tree

Showing 10 changed files with 116 additions and 29 deletions.
diff --git a/.forge-snapshots/ProposeSigComp.snap b/.forge-snapshots/ProposeSigComp.snap
@@ -1 +1 @@
-170929
+171501
diff --git a/.forge-snapshots/VoteSigComp.snap b/.forge-snapshots/VoteSigComp.snap
@@ -1 +1 @@
-52147
+52138
diff --git a/.forge-snapshots/VoteSigCompMetadata.snap b/.forge-snapshots/VoteSigCompMetadata.snap
@@ -1 +1 @@
-53833
+53824
diff --git a/.forge-snapshots/VoteTxComp.snap b/.forge-snapshots/VoteTxComp.snap
@@ -1 +1 @@
-45334
+45325
diff --git a/.forge-snapshots/VoteTxCompMetadata.snap b/.forge-snapshots/VoteTxCompMetadata.snap
@@ -1 +1 @@
-46925
+46916
diff --git a/codecov.yml b/codecov.yml
@@ -7,4 +7,7 @@ coverage:
     project:
       default:
         target: auto
-        threshold: 0.5%
+        threshold: 1%
+    patch:
+      default:
+        target: 0%
diff --git a/src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol b/src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol
@@ -22,6 +22,13 @@ contract CompTimelockCompatibleExecutionStrategy is SimpleQuorumExecutionStrateg
     /// @notice Thrown if the transaction is invalid.
     error InvalidTransaction();
 
+    event CompTimelockCompatibleExecutionStrategySetUp(
+        address owner,
+        address vetoGuardian,
+        address[] spaces,
+        uint256 quorum,
+        address timelock
+    );
     event TransactionQueued(MetaTransaction transaction, uint256 executionTime);
     event TransactionExecuted(MetaTransaction transaction);
     event TransactionVetoed(MetaTransaction transaction);
@@ -41,23 +48,23 @@ contract CompTimelockCompatibleExecutionStrategy is SimpleQuorumExecutionStrateg
 
     /// @notice Constructor
     /// @param _owner Address of the owner of this contract.
+    /// @param _vetoGuardian Address of the veto guardian.
     /// @param _spaces Array of whitelisted space contracts.
     /// @param _quorum The quorum required to execute a proposal.
-    constructor(address _owner, address[] memory _spaces, uint256 _quorum, address _timelock) {
-        setUp(abi.encode(_owner, _spaces, _quorum, _timelock));
+    constructor(address _owner, address _vetoGuardian, address[] memory _spaces, uint256 _quorum, address _timelock) {
+        setUp(abi.encode(_owner, _vetoGuardian, _spaces, _quorum, _timelock));
     }
 
     function setUp(bytes memory initializeParams) public initializer {
-        (address _owner, address[] memory _spaces, uint256 _quorum, address _timelock) = abi.decode(
-            initializeParams,
-            (address, address[], uint256, address)
-        );
+        (address _owner, address _vetoGuardian, address[] memory _spaces, uint256 _quorum, address _timelock) = abi
+            .decode(initializeParams, (address, address, address[], uint256, address));
         __Ownable_init();
         transferOwnership(_owner);
+        vetoGuardian = _vetoGuardian;
         __SpaceManager_init(_spaces);
         __SimpleQuorumExecutionStrategy_init(_quorum);
-
         timelock = ICompTimelock(_timelock);
+        emit CompTimelockCompatibleExecutionStrategySetUp(_owner, _vetoGuardian, _spaces, _quorum, _timelock);
     }
 
     /// @notice Accepts admin role of the timelock contract. Must be called before using the timelock.

diff --git a/src/execution-strategies/TimelockExecutionStrategy.sol b/src/execution-strategies/TimelockExecutionStrategy.sol
@@ -33,6 +33,20 @@ contract TimelockExecutionStrategy is SimpleQuorumExecutionStrategy, IERC1155Rec
     /// @param transaction The transaction that was executed.
     event TransactionExecuted(MetaTransaction transaction);
 
+    /// @notice Emitted when a new Timelock is set up.
+    /// @param owner The owner of the Timelock.
+    /// @param vetoGuardian The veto guardian of the Timelock.
+    /// @param spaces The spaces that are whitelisted for this Timelock.
+    /// @param quorum The quorum required to execute a proposal.
+    /// @param timelockDelay The delay in seconds between a proposal being queued and the execution of the proposal.
+    event TimelockExecutionStrategySetUp(
+        address owner,
+        address vetoGuardian,
+        address[] spaces,
+        uint256 quorum,
+        uint256 timelockDelay
+    );
+
     /// @notice Emitted when a veto guardian is set.
     /// @param vetoGuardian The old veto guardian.
     /// @param newVetoGuardian The new veto guardian.
@@ -63,25 +77,32 @@ contract TimelockExecutionStrategy is SimpleQuorumExecutionStrategy, IERC1155Rec
 
     /// @notice Constructor
     /// @param _owner Address of the owner of this contract.
+    /// @param _vetoGuardian Address of the veto guardian.
     /// @param _spaces Array of whitelisted space contracts.
     /// @param _timelockDelay The timelock delay in seconds.
     /// @param _quorum The quorum required to execute a proposal.
-    constructor(address _owner, address[] memory _spaces, uint256 _timelockDelay, uint256 _quorum) {
-        setUp(abi.encode(_owner, _spaces, _timelockDelay, _quorum));
+    constructor(
+        address _owner,
+        address _vetoGuardian,
+        address[] memory _spaces,
+        uint256 _timelockDelay,
+        uint256 _quorum
+    ) {
+        setUp(abi.encode(_owner, _vetoGuardian, _spaces, _timelockDelay, _quorum));
     }
 
     /// @notice Initialization function, should be called immediately after deploying a new proxy to this contract.
     /// @param initParams ABI encoded parameters, in the same order as the constructor.
     function setUp(bytes memory initParams) public initializer {
-        (address _owner, address[] memory _spaces, uint256 _timelockDelay, uint256 _quorum) = abi.decode(
-            initParams,
-            (address, address[], uint256, uint256)
-        );
+        (address _owner, address _vetoGuardian, address[] memory _spaces, uint256 _timelockDelay, uint256 _quorum) = abi
+            .decode(initParams, (address, address, address[], uint256, uint256));
         __Ownable_init();
         transferOwnership(_owner);
+        vetoGuardian = _vetoGuardian;
         __SpaceManager_init(_spaces);
         __SimpleQuorumExecutionStrategy_init(_quorum);
         timelockDelay = _timelockDelay;
+        emit TimelockExecutionStrategySetUp(_owner, _vetoGuardian, _spaces, _quorum, _timelockDelay);
     }
 
     /// @notice Executes a proposal by queueing its transactions in the timelock. Can only be called by approved spaces.

diff --git a/test/CompTimelockExecutionStrategy.t.sol b/test/CompTimelockExecutionStrategy.t.sol
@@ -20,14 +20,22 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest {
     error DuplicateExecutionPayloadHash();
     error OnlyVetoGuardian();
     error InvalidTransaction();
+    event CompTimelockCompatibleExecutionStrategySetUp(
+        address owner,
+        address vetoGuardian,
+        address[] spaces,
+        uint256 quorum,
+        address timelock
+    );
     event TransactionQueued(MetaTransaction transaction, uint256 executionTime);
     event ProposalVetoed(bytes32 executionPayloadHash);
     event VetoGuardianSet(address vetoGuardian, address newVetoGuardian);
 
     CompTimelockCompatibleExecutionStrategy public timelockExecutionStrategy;
     CompTimelock public timelock = new CompTimelock(address(this), 1000);
 
-    address private recipient = address(0xc0ffee);
+    address public vetoGuardian = address(0);
+    address public recipient = address(0xc0ffee);
 
     function finishSetUp() public {
         vm.deal(address(owner), 1000);
@@ -456,6 +464,24 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest {
     function testViewFunctions() public {
         assertEq(timelockExecutionStrategy.getStrategyType(), "CompTimelockCompatibleSimpleQuorum");
     }
+
+    function testSetUp() public {
+        address[] memory spaces = new address[](1);
+        spaces[0] = address(space);
+        timelockExecutionStrategy = new CompTimelockCompatibleExecutionStrategy(
+            owner,
+            vetoGuardian,
+            spaces,
+            quorum,
+            address(timelock)
+        );
+
+        assertEq(timelockExecutionStrategy.owner(), owner);
+        assertEq(timelockExecutionStrategy.vetoGuardian(), vetoGuardian);
+        assertEq(timelockExecutionStrategy.quorum(), quorum);
+        assertEq(address(timelockExecutionStrategy.timelock()), address(timelock));
+        assertEq(timelockExecutionStrategy.isSpaceEnabled(address(space)), true);
+    }
 }
 
 contract CompTimelockExecutionStrategyTestDirect is CompTimelockExecutionStrategyTest {
@@ -467,6 +493,7 @@ contract CompTimelockExecutionStrategyTestDirect is CompTimelockExecutionStrateg
 
         timelockExecutionStrategy = new CompTimelockCompatibleExecutionStrategy(
             owner,
+            vetoGuardian,
             spaces,
             quorum,
             address(timelock)
@@ -482,11 +509,13 @@ contract CompTimelockExecutionStrategyTestProxy is CompTimelockExecutionStrategy
 
         address[] memory spaces = new address[](1);
         spaces[0] = address(space);
+        address[] memory emptyArray = new address[](1);
         CompTimelockCompatibleExecutionStrategy masterExecutionStrategy = new CompTimelockCompatibleExecutionStrategy(
-            owner,
-            spaces,
-            quorum,
-            address(timelock)
+            address(1),
+            address(0),
+            emptyArray,
+            0,
+            address(0)
         );
 
         timelockExecutionStrategy = CompTimelockCompatibleExecutionStrategy(
@@ -495,7 +524,7 @@ contract CompTimelockExecutionStrategyTestProxy is CompTimelockExecutionStrategy
                     address(masterExecutionStrategy),
                     abi.encodeWithSelector(
                         CompTimelockCompatibleExecutionStrategy.setUp.selector,
-                        abi.encode(owner, spaces, quorum, address(timelock))
+                        abi.encode(owner, vetoGuardian, spaces, quorum, address(timelock))
                     )
                 )
             )

diff --git a/test/TimelockExecutionStrategy.t.sol b/test/TimelockExecutionStrategy.t.sol
@@ -18,13 +18,21 @@ abstract contract TimelockExecutionStrategyTest is SpaceTest {
     error ProposalNotQueued();
     error DuplicateExecutionPayloadHash();
     error OnlyVetoGuardian();
+    event TimelockExecutionStrategySetUp(
+        address owner,
+        address vetoGuardian,
+        address[] spaces,
+        uint256 quorum,
+        uint256 timelockDelay
+    );
     event TransactionQueued(MetaTransaction transaction, uint256 executionTime);
     event ProposalVetoed(bytes32 executionPayloadHash);
     event VetoGuardianSet(address vetoGuardian, address newVetoGuardian);
 
     TimelockExecutionStrategy public timelockExecutionStrategy;
 
-    address private recipient = address(0xc0ffee);
+    address public vetoGuardian = address(0);
+    address public recipient = address(0xc0ffee);
 
     function testQueueingFromUnauthorizedSpace() external {
         timelockExecutionStrategy.disableSpace(address(space));
@@ -470,6 +478,18 @@ abstract contract TimelockExecutionStrategyTest is SpaceTest {
         assertTrue(timelockExecutionStrategy.supportsInterface(type(IERC1155Receiver).interfaceId));
         assertEq(timelockExecutionStrategy.getStrategyType(), "SimpleQuorumTimelock");
     }
+
+    function testSetUp() public {
+        address[] memory spaces = new address[](1);
+        spaces[0] = address(space);
+        timelockExecutionStrategy = new TimelockExecutionStrategy(owner, vetoGuardian, spaces, 1000, quorum);
+
+        assertEq(timelockExecutionStrategy.owner(), owner);
+        assertEq(timelockExecutionStrategy.vetoGuardian(), vetoGuardian);
+        assertEq(timelockExecutionStrategy.quorum(), quorum);
+        assertEq(timelockExecutionStrategy.timelockDelay(), 1000);
+        assertEq(timelockExecutionStrategy.isSpaceEnabled(address(space)), true);
+    }
 }
 
 contract TimelockExecutionStrategyTestDirect is TimelockExecutionStrategyTest {
@@ -479,7 +499,7 @@ contract TimelockExecutionStrategyTestDirect is TimelockExecutionStrategyTest {
         address[] memory spaces = new address[](1);
         spaces[0] = address(space);
 
-        timelockExecutionStrategy = new TimelockExecutionStrategy(owner, spaces, 1000, quorum);
+        timelockExecutionStrategy = new TimelockExecutionStrategy(owner, vetoGuardian, spaces, 1000, quorum);
         vm.deal(address(owner), 1000);
         payable(timelockExecutionStrategy).transfer(1000);
     }
@@ -491,15 +511,22 @@ contract TimelockExecutionStrategyTestProxy is TimelockExecutionStrategyTest {
 
         address[] memory spaces = new address[](1);
         spaces[0] = address(space);
-        TimelockExecutionStrategy masterExecutionStrategy = new TimelockExecutionStrategy(owner, spaces, 1000, quorum);
+        address[] memory emptyArray = new address[](1);
+        TimelockExecutionStrategy masterExecutionStrategy = new TimelockExecutionStrategy(
+            address(1),
+            address(0),
+            emptyArray,
+            0,
+            0
+        );
 
         timelockExecutionStrategy = TimelockExecutionStrategy(
             payable(
                 new ERC1967Proxy(
                     address(masterExecutionStrategy),
                     abi.encodeWithSelector(
                         TimelockExecutionStrategy.setUp.selector,
-                        abi.encode(owner, spaces, 1000, quorum)
+                        abi.encode(owner, vetoGuardian, spaces, 1000, quorum)
                     )
                 )
             )