Merge pull request #231 from lidofinance/refactor/events

Refactor events
lidofinance · Dec 6, 2024 · 8c28258 · 8c28258
2 parents 5aaab5b + b985214
commit 8c28258
Show file tree

Hide file tree

Showing 13 changed files with 233 additions and 87 deletions.
diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol
@@ -200,7 +200,9 @@ contract DualGovernance is IDualGovernance {
             revert ProposalSubmissionBlocked();
         }
         Proposers.Proposer memory proposer = _proposers.getProposer(msg.sender);
-        proposalId = TIMELOCK.submit(proposer.account, proposer.executor, calls, metadata);
+        proposalId = TIMELOCK.submit(proposer.executor, calls);
+
+        emit ProposalSubmitted(proposer.account, proposalId, metadata);
     }
 
     /// @notice Schedules a previously submitted proposal for execution in the Dual Governance system.

diff --git a/contracts/EmergencyProtectedTimelock.sol b/contracts/EmergencyProtectedTimelock.sol
@@ -102,19 +102,12 @@ contract EmergencyProtectedTimelock is IEmergencyProtectedTimelock {
     // ---
 
     /// @notice Submits a new proposal to execute a series of calls through an executor.
-    /// @param proposer The address of the proposer submitting the proposal.
     /// @param executor The address of the executor contract that will execute the calls.
     /// @param calls An array of `ExternalCall` structs representing the calls to be executed.
-    /// @param metadata A string containing additional information about the proposal.
     /// @return newProposalId The id of the newly created proposal.
-    function submit(
-        address proposer,
-        address executor,
-        ExternalCall[] calldata calls,
-        string calldata metadata
-    ) external returns (uint256 newProposalId) {
+    function submit(address executor, ExternalCall[] calldata calls) external returns (uint256 newProposalId) {
         _timelockState.checkCallerIsGovernance();
-        newProposalId = _proposals.submit(proposer, executor, calls, metadata);
+        newProposalId = _proposals.submit(executor, calls);
     }
 
     /// @notice Schedules a proposal for execution after a specified delay.

diff --git a/contracts/Executor.sol b/contracts/Executor.sol
@@ -14,7 +14,8 @@ contract Executor is IExternalExecutor, Ownable {
     // Events
     // ---
 
-    event Execute(address indexed sender, address indexed target, uint256 ethValue, bytes data);
+    event ETHReceived(address sender, uint256 value);
+    event Executed(address indexed target, uint256 ethValue, bytes data, bytes returndata);
 
     // ---
     // Constructor
@@ -32,10 +33,12 @@ contract Executor is IExternalExecutor, Ownable {
     /// @param payload The calldata for the function call.
     function execute(address target, uint256 value, bytes calldata payload) external payable {
         _checkOwner();
-        Address.functionCallWithValue(target, payload, value);
-        emit Execute(msg.sender, target, value, payload);
+        bytes memory returndata = Address.functionCallWithValue(target, payload, value);
+        emit Executed(target, value, payload, returndata);
     }
 
     /// @notice Allows the contract to receive ether.
-    receive() external payable {}
+    receive() external payable {
+        emit ETHReceived(msg.sender, msg.value);
+    }
 }
diff --git a/contracts/TimelockedGovernance.sol b/contracts/TimelockedGovernance.sol
@@ -51,7 +51,8 @@ contract TimelockedGovernance is IGovernance {
         string calldata metadata
     ) external returns (uint256 proposalId) {
         _checkCallerIsGovernance();
-        return TIMELOCK.submit(msg.sender, TIMELOCK.getAdminExecutor(), calls, metadata);
+        proposalId = TIMELOCK.submit(TIMELOCK.getAdminExecutor(), calls);
+        emit ProposalSubmitted(msg.sender, proposalId, metadata);
     }
 
     /// @notice Schedules a submitted proposal.

diff --git a/contracts/interfaces/IGovernance.sol b/contracts/interfaces/IGovernance.sol
@@ -6,6 +6,8 @@ import {ITimelock} from "./ITimelock.sol";
 import {ExternalCall} from "../libraries/ExternalCalls.sol";
 
 interface IGovernance {
+    event ProposalSubmitted(address indexed proposerAccount, uint256 indexed proposalId, string metadata);
+
     function TIMELOCK() external view returns (ITimelock);
     function submitProposal(
         ExternalCall[] calldata calls,

diff --git a/contracts/interfaces/ITimelock.sol b/contracts/interfaces/ITimelock.sol
@@ -16,12 +16,7 @@ interface ITimelock {
         ProposalStatus status;
     }
 
-    function submit(
-        address proposer,
-        address executor,
-        ExternalCall[] calldata calls,
-        string calldata metadata
-    ) external returns (uint256 newProposalId);
+    function submit(address executor, ExternalCall[] calldata calls) external returns (uint256 newProposalId);
     function schedule(uint256 proposalId) external;
     function execute(uint256 proposalId) external;
     function cancelAllNonExecutedProposals() external;

diff --git a/contracts/libraries/ExecutableProposals.sol b/contracts/libraries/ExecutableProposals.sol
@@ -87,9 +87,7 @@ library ExecutableProposals {
     // Events
     // ---
 
-    event ProposalSubmitted(
-        uint256 indexed id, address indexed proposer, address indexed executor, ExternalCall[] calls, string metadata
-    );
+    event ProposalSubmitted(uint256 indexed id, address indexed executor, ExternalCall[] calls);
     event ProposalScheduled(uint256 indexed id);
     event ProposalExecuted(uint256 indexed id);
     event ProposalsCancelledTill(uint256 proposalId);
@@ -100,17 +98,13 @@ library ExecutableProposals {
 
     /// @notice Submits a new proposal with the specified executor and external calls.
     /// @param self The context of the Executable Proposal library.
-    /// @param proposer The address of the proposer submitting the proposal.
     /// @param executor The address authorized to execute the proposal.
     /// @param calls The list of external calls to include in the proposal.
-    /// @param metadata Metadata describing the proposal.
     /// @return newProposalId The id of the newly submitted proposal.
     function submit(
         Context storage self,
-        address proposer,
         address executor,
-        ExternalCall[] memory calls,
-        string memory metadata
+        ExternalCall[] memory calls
     ) internal returns (uint256 newProposalId) {
         if (calls.length == 0) {
             revert EmptyCalls();
@@ -129,7 +123,7 @@ library ExecutableProposals {
             newProposal.calls.push(calls[i]);
         }
 
-        emit ProposalSubmitted(newProposalId, proposer, executor, calls, metadata);
+        emit ProposalSubmitted(newProposalId, executor, calls);
     }
 
     /// @notice Marks a previously submitted proposal as scheduled for execution if the required delay period

diff --git a/test/mocks/TimelockMock.sol b/test/mocks/TimelockMock.sol
@@ -26,12 +26,7 @@ contract TimelockMock is ITimelock {
 
     address internal governance;
 
-    function submit(
-        address,
-        address,
-        ExternalCall[] calldata,
-        string calldata
-    ) external returns (uint256 newProposalId) {
+    function submit(address, ExternalCall[] calldata) external returns (uint256 newProposalId) {
         newProposalId = submittedProposals.length + OFFSET;
         submittedProposals.push(newProposalId);
         canScheduleProposal[newProposalId] = false;

diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol
@@ -14,7 +14,10 @@ import {Tiebreaker} from "contracts/libraries/Tiebreaker.sol";
 import {Resealer} from "contracts/libraries/Resealer.sol";
 import {Status as ProposalStatus} from "contracts/libraries/ExecutableProposals.sol";
 import {Proposers} from "contracts/libraries/Proposers.sol";
+
+import {IGovernance} from "contracts/interfaces/IGovernance.sol";
 import {IResealManager} from "contracts/interfaces/IResealManager.sol";
+
 import {
     DualGovernanceConfig,
     IDualGovernanceConfigProvider,
@@ -225,13 +228,15 @@ contract DualGovernanceUnitTests is UnitTest {
     function test_submitProposal_HappyPath() external {
         ExternalCall[] memory calls = _generateExternalCalls();
         Proposers.Proposer memory proposer = _dualGovernance.getProposer(address(this));
-        vm.expectCall(
-            address(_timelock),
-            0,
-            abi.encodeWithSelector(TimelockMock.submit.selector, address(this), proposer.executor, calls, "")
-        );
+        vm.expectCall(address(_timelock), 0, abi.encodeCall(TimelockMock.submit, (proposer.executor, calls)));
+
+        uint256 expectedProposalId = 1;
+        string memory metadata = "New proposal description";
 
-        uint256 proposalId = _dualGovernance.submitProposal(calls, "");
+        vm.expectEmit();
+        emit IGovernance.ProposalSubmitted(proposer.account, expectedProposalId, metadata);
+
+        uint256 proposalId = _dualGovernance.submitProposal(calls, metadata);
         uint256[] memory submittedProposals = _timelock.getSubmittedProposals();
 
         assertEq(submittedProposals.length, 1);
@@ -1107,11 +1112,11 @@ contract DualGovernanceUnitTests is UnitTest {
         vm.expectEmit();
         emit DualGovernanceStateMachine.ConfigProviderSet(IDualGovernanceConfigProvider(address(newConfigProvider)));
         vm.expectEmit();
-        emit Executor.Execute(
-            address(this),
+        emit Executor.Executed(
             address(_dualGovernance),
             0,
-            abi.encodeWithSelector(DualGovernance.setConfigProvider.selector, address(newConfigProvider))
+            abi.encodeWithSelector(DualGovernance.setConfigProvider.selector, address(newConfigProvider)),
+            new bytes(0)
         );
 
         vm.expectCall(
@@ -2420,8 +2425,7 @@ contract DualGovernanceUnitTests is UnitTest {
     // ---
 
     function _submitMockProposal() internal {
-        // mock timelock doesn't uses proposal data
-        _timelock.submit(msg.sender, address(0), new ExternalCall[](0), "");
+        _timelock.submit(address(0), new ExternalCall[](0));
     }
 
     function _scheduleProposal(uint256 proposalId, Timestamp submittedAt) internal {

diff --git a/test/unit/EmergencyProtectedTimelock.t.sol b/test/unit/EmergencyProtectedTimelock.t.sol
@@ -186,21 +186,19 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
 
         vm.prank(stranger);
         vm.expectRevert(abi.encodeWithSelector(TimelockState.CallerIsNotGovernance.selector, [stranger]));
-        _timelock.submit(stranger, _adminExecutor, new ExternalCall[](0), "");
+        _timelock.submit(_adminExecutor, new ExternalCall[](0));
         assertEq(_timelock.getProposalsCount(), 0);
     }
 
     function test_submit_HappyPath() external {
         string memory testMetadata = "testMetadata";
 
-        vm.expectEmit(true, true, true, true);
+        vm.expectEmit();
         emit ExecutableProposals.ProposalSubmitted(
-            1, _dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), testMetadata
+            1, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock))
         );
         vm.prank(_dualGovernance);
-        _timelock.submit(
-            _dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), testMetadata
-        );
+        _timelock.submit(_adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)));
 
         assertEq(_timelock.getProposalsCount(), 1);
 
@@ -593,7 +591,7 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
 
     function test_emergencyExecute_RevertOn_ModeNotActive() external {
         vm.startPrank(_dualGovernance);
-        _timelock.submit(_dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), "");
+        _timelock.submit(_adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)));
 
         assertEq(_timelock.getProposalsCount(), 1);
 
@@ -1045,8 +1043,8 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
         vm.startPrank(_dualGovernance);
         ExternalCall[] memory executorCalls = _getMockTargetRegularStaffCalls(address(_targetMock));
         ExternalCall[] memory anotherExecutorCalls = _getMockTargetRegularStaffCalls(address(_anotherTargetMock));
-        _timelock.submit(_dualGovernance, _adminExecutor, executorCalls, "");
-        _timelock.submit(_dualGovernance, _adminExecutor, anotherExecutorCalls, "");
+        _timelock.submit(_adminExecutor, executorCalls);
+        _timelock.submit(_adminExecutor, anotherExecutorCalls);
 
         (ITimelock.ProposalDetails memory submittedProposal, ExternalCall[] memory calls) = _timelock.getProposal(1);
 
@@ -1203,7 +1201,7 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
     function test_getProposalCalls() external {
         ExternalCall[] memory executorCalls = _getMockTargetRegularStaffCalls(address(_targetMock));
         vm.prank(_dualGovernance);
-        _timelock.submit(_dualGovernance, _adminExecutor, executorCalls, "");
+        _timelock.submit(_adminExecutor, executorCalls);
 
         ExternalCall[] memory calls = _timelock.getProposalCalls(1);
 
@@ -1254,7 +1252,7 @@ contract EmergencyProtectedTimelockUnitTests is UnitTest {
 
     function _submitProposal() internal {
         vm.prank(_dualGovernance);
-        _timelock.submit(_dualGovernance, _adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)), "");
+        _timelock.submit(_adminExecutor, _getMockTargetRegularStaffCalls(address(_targetMock)));
     }
 
     function _scheduleProposal(uint256 proposalId) internal {