From e8a2688db4eacbb6850526e5615218bf64e16369 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Fri, 6 Dec 2024 04:38:44 +0400 Subject: [PATCH 1/4] Refactor Executor event --- contracts/Executor.sol | 6 +++--- test/unit/DualGovernance.t.sol | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/contracts/Executor.sol b/contracts/Executor.sol index 53e64014..f2b2ec25 100644 --- a/contracts/Executor.sol +++ b/contracts/Executor.sol @@ -14,7 +14,7 @@ contract Executor is IExternalExecutor, Ownable { // Events // --- - event Execute(address indexed sender, address indexed target, uint256 ethValue, bytes data); + event Executed(address indexed target, uint256 ethValue, bytes data, bytes result); // --- // Constructor @@ -32,8 +32,8 @@ 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 result = Address.functionCallWithValue(target, payload, value); + emit Executed(target, value, payload, result); } /// @notice Allows the contract to receive ether. diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index add00fc9..ed35a999 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -1073,11 +1073,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( From 80fa52e61eb5efb7c88542d65c208d0ae75d1216 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Fri, 6 Dec 2024 06:26:25 +0400 Subject: [PATCH 2/4] Add unit tests for Executor contract --- contracts/Executor.sol | 11 ++- test/unit/Executor.t.sol | 153 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 4 deletions(-) create mode 100644 test/unit/Executor.t.sol diff --git a/contracts/Executor.sol b/contracts/Executor.sol index f2b2ec25..dd649fc5 100644 --- a/contracts/Executor.sol +++ b/contracts/Executor.sol @@ -14,7 +14,8 @@ contract Executor is IExternalExecutor, Ownable { // Events // --- - event Executed(address indexed target, uint256 ethValue, bytes data, bytes result); + 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(); - bytes memory result = Address.functionCallWithValue(target, payload, value); - emit Executed(target, value, payload, result); + 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/test/unit/Executor.t.sol b/test/unit/Executor.t.sol new file mode 100644 index 00000000..086f7515 --- /dev/null +++ b/test/unit/Executor.t.sol @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.26; + +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; + +import {Executor} from "contracts/Executor.sol"; +import {UnitTest} from "test/utils/unit-test.sol"; + +contract ExecutorTarget { + event NonPayableMethodCalled(address caller); + event PayableMethodCalled(address caller, uint256 value); + + function nonPayableMethod() external { + emit NonPayableMethodCalled(msg.sender); + } + + function payableMethod() external payable { + emit PayableMethodCalled(msg.sender, msg.value); + } +} + +contract ExecutorUnitTests is UnitTest { + address internal _owner = makeAddr("OWNER"); + + Executor internal _executor; + ExecutorTarget internal _target; + + function setUp() external { + _target = new ExecutorTarget(); + _executor = new Executor(_owner); + } + + function test_constructor_HappyPath() external { + assertEq(_executor.owner(), _owner); + } + + function test_constructor_RevertOn_ZeroOwnerAddress() external { + vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableInvalidOwner.selector, address(0))); + new Executor(address(0)); + } + + // --- + // execute() + // --- + + function test_execute_HappyPath_NonPayableMethod_ZeroValue() external { + vm.expectEmit(); + emit ExecutorTarget.NonPayableMethodCalled(address(_executor)); + + vm.expectEmit(); + emit Executor.Executed(address(_target), 0, abi.encodeCall(_target.nonPayableMethod, ()), new bytes(0)); + + vm.prank(_owner); + _executor.execute({target: address(_target), value: 0, payload: abi.encodeCall(_target.nonPayableMethod, ())}); + } + + function test_execute_HappyPath_PayableMethod_ZeroValue() external { + vm.expectEmit(); + emit ExecutorTarget.PayableMethodCalled(address(_executor), 0); + + vm.expectEmit(); + emit Executor.Executed(address(_target), 0, abi.encodeCall(_target.payableMethod, ()), new bytes(0)); + + vm.prank(_owner); + _executor.execute({target: address(_target), value: 0, payload: abi.encodeCall(_target.payableMethod, ())}); + } + + function test_execute_HappyPath_PayableMethod_NonZeroValue() external { + uint256 valueAmount = 1 ether; + + vm.deal(address(_executor), valueAmount); + + assertEq(address(_target).balance, 0); + assertEq(address(_executor).balance, 1 ether); + + vm.expectEmit(); + emit ExecutorTarget.PayableMethodCalled(address(_executor), valueAmount); + + vm.expectEmit(); + emit Executor.Executed(address(_target), valueAmount, abi.encodeCall(_target.payableMethod, ()), new bytes(0)); + + vm.prank(_owner); + _executor.execute({ + target: address(_target), + value: valueAmount, + payload: abi.encodeCall(_target.payableMethod, ()) + }); + + assertEq(address(_target).balance, valueAmount); + assertEq(address(_executor).balance, 0); + } + + function test_execute_RevertOn_NonPayableMethod_NonZeroValue() external { + uint256 callValue = 1 ether; + + vm.deal(address(_executor), callValue); + assertEq(address(_executor).balance, callValue); + + vm.prank(_owner); + vm.expectRevert(Address.FailedInnerCall.selector); + _executor.execute({ + target: address(_target), + value: callValue, + payload: abi.encodeCall(_target.nonPayableMethod, ()) + }); + } + // --- + // receive() + // --- + + function test_receive_HappyPath() external { + uint256 sendValue = 1 ether; + + vm.deal(address(this), sendValue); + + assertEq(address(this).balance, sendValue); + assertEq(address(_executor).balance, 0); + + Address.sendValue(payable(address(_executor)), sendValue); + + assertEq(address(this).balance, 0); + assertEq(address(_executor).balance, sendValue); + } + + function test_receive_HappyPath_UsingSend() external { + uint256 sendValue = 1 ether; + + vm.deal(address(this), sendValue); + + assertEq(address(this).balance, sendValue); + assertEq(address(_executor).balance, 0); + + bool success = payable(address(_executor)).send(sendValue); + + assertTrue(success); + assertEq(address(this).balance, 0); + assertEq(address(_executor).balance, sendValue); + } + + // --- + // Custom call + // --- + + function test_RevertOnInvalidMethodCall() external { + vm.prank(_owner); + (bool success, bytes memory returndata) = + address(_executor).call{value: 1 ether}(abi.encodeWithSelector(bytes4(0xdeadbeaf), 42)); + + assertFalse(success); + assertEq(returndata, new bytes(0)); + } +} From 784dfd51f629512f9f13a5508a0f17566bad623b Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Fri, 6 Dec 2024 06:28:33 +0400 Subject: [PATCH 3/4] Add separate ProposalReported event --- contracts/DualGovernance.sol | 4 +- contracts/EmergencyProtectedTimelock.sol | 11 +--- contracts/TimelockedGovernance.sol | 3 +- contracts/interfaces/IGovernance.sol | 2 + contracts/interfaces/ITimelock.sol | 7 +-- contracts/libraries/ExecutableProposals.sol | 12 +--- test/mocks/TimelockMock.sol | 7 +-- test/unit/DualGovernance.t.sol | 20 ++++--- test/unit/EmergencyProtectedTimelock.t.sol | 20 +++---- test/unit/TimelockedGovernance.t.sol | 9 ++- test/unit/libraries/ExecutableProposals.t.sol | 55 +++++++++---------- 11 files changed, 70 insertions(+), 80 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index 19d4426d..26123b27 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -188,7 +188,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 ProposalReported(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 index 55e4b69b..e3ebdab2 100644 --- 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/TimelockedGovernance.sol b/contracts/TimelockedGovernance.sol index 25f08fd3..5cc4fb44 100644 --- 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 ProposalReported(msg.sender, proposalId, metadata); } /// @notice Schedules a submitted proposal. diff --git a/contracts/interfaces/IGovernance.sol b/contracts/interfaces/IGovernance.sol index 51652b40..fd3421a1 100644 --- 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 ProposalReported(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 index f94fe3e6..234a000d 100644 --- 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 index 070af315..9405bae0 100644 --- a/contracts/libraries/ExecutableProposals.sol +++ b/contracts/libraries/ExecutableProposals.sol @@ -89,9 +89,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); @@ -102,17 +100,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(); @@ -131,7 +125,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 index 7ae66eab..a818ab26 100644 --- 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 index ed35a999..67c7c9ea 100644 --- 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, @@ -191,13 +194,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"; + + vm.expectEmit(); + emit IGovernance.ProposalReported(proposer.account, expectedProposalId, metadata); - uint256 proposalId = _dualGovernance.submitProposal(calls, ""); + uint256 proposalId = _dualGovernance.submitProposal(calls, metadata); uint256[] memory submittedProposals = _timelock.getSubmittedProposals(); assertEq(submittedProposals.length, 1); @@ -2384,8 +2389,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 index b0881965..2f235c1d 100644 --- 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 { diff --git a/test/unit/TimelockedGovernance.t.sol b/test/unit/TimelockedGovernance.t.sol index 8437bb55..80b83b45 100644 --- a/test/unit/TimelockedGovernance.t.sol +++ b/test/unit/TimelockedGovernance.t.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.26; import {ITimelock} from "contracts/interfaces/ITimelock.sol"; +import {IGovernance} from "contracts/interfaces/IGovernance.sol"; import {TimelockedGovernance} from "contracts/TimelockedGovernance.sol"; import {UnitTest} from "test/utils/unit-test.sol"; @@ -43,8 +44,14 @@ contract TimelockedGovernanceUnitTests is UnitTest { function test_submit_proposal() external { assertEq(_timelock.getSubmittedProposals().length, 0); + uint256 expectedProposalId = 1; + string memory metadata = "proposal description"; + + vm.expectEmit(); + emit IGovernance.ProposalReported(_governance, expectedProposalId, metadata); + vm.prank(_governance); - _timelockedGovernance.submitProposal(_getMockTargetRegularStaffCalls(address(0x1)), ""); + _timelockedGovernance.submitProposal(_getMockTargetRegularStaffCalls(address(0x1)), metadata); assertEq(_timelock.getSubmittedProposals().length, 1); } diff --git a/test/unit/libraries/ExecutableProposals.t.sol b/test/unit/libraries/ExecutableProposals.t.sol index d95c328a..9b6f9f1f 100644 --- a/test/unit/libraries/ExecutableProposals.t.sol +++ b/test/unit/libraries/ExecutableProposals.t.sol @@ -32,7 +32,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function test_submit_reverts_if_empty_proposals() external { vm.expectRevert(ExecutableProposals.EmptyCalls.selector); - _proposals.submit(proposer, address(0), new ExternalCall[](0), "Empty calls"); + _proposals.submit(address(0), new ExternalCall[](0)); } function test_submit_proposal() external { @@ -41,14 +41,13 @@ contract ExecutableProposalsUnitTests is UnitTest { ExternalCall[] memory calls = _getMockTargetRegularStaffCalls(address(_targetMock)); uint256 expectedProposalId = proposalsCount + PROPOSAL_ID_OFFSET; - string memory description = "Regular staff calls"; vm.expectEmit(); - emit ExecutableProposals.ProposalSubmitted(expectedProposalId, proposer, address(_executor), calls, description); + emit ExecutableProposals.ProposalSubmitted(expectedProposalId, address(_executor), calls); vm.recordLogs(); - _proposals.submit(proposer, address(_executor), calls, description); + _proposals.submit(address(_executor), calls); Vm.Log[] memory entries = vm.getRecordedLogs(); assertEq(entries.length, 1); @@ -74,7 +73,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function testFuzz_schedule_proposal(Duration delay) external { vm.assume(delay > Durations.ZERO && delay.toSeconds() <= MAX_DURATION_VALUE); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 expectedProposalId = 1; ExecutableProposals.Proposal memory proposal = _proposals.proposals[expectedProposalId]; @@ -106,7 +105,7 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_cannot_schedule_proposal_twice() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = 1; _proposals.schedule(proposalId, Durations.ZERO); @@ -117,7 +116,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function testFuzz_cannot_schedule_proposal_before_delay_passed(Duration delay) external { vm.assume(delay > Durations.ZERO && delay.toSeconds() <= MAX_DURATION_VALUE); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); _wait(delay.minusSeconds(1 seconds)); @@ -128,7 +127,7 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_cannot_schedule_cancelled_proposal() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); _proposals.cancelAll(); uint256 proposalId = _proposals.getProposalsCount(); @@ -140,7 +139,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function testFuzz_execute_proposal(Duration delay) external { vm.assume(delay > Durations.ZERO && delay.toSeconds() <= MAX_DURATION_VALUE); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); _proposals.schedule(proposalId, Durations.ZERO); @@ -176,7 +175,7 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_cannot_execute_unscheduled_proposal() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); vm.expectRevert(abi.encodeWithSelector(ExecutableProposals.ProposalNotScheduled.selector, proposalId)); @@ -184,7 +183,7 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_cannot_execute_twice() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); _proposals.schedule(proposalId, Durations.ZERO); _proposals.execute(proposalId, Durations.ZERO); @@ -194,7 +193,7 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_cannot_execute_cancelled_proposal() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); _proposals.schedule(proposalId, Durations.ZERO); _proposals.cancelAll(); @@ -205,7 +204,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function testFuzz_cannot_execute_before_delay_passed(Duration delay) external { vm.assume(delay > Durations.ZERO && delay.toSeconds() <= MAX_DURATION_VALUE); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); _proposals.schedule(proposalId, Durations.ZERO); @@ -216,8 +215,8 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_cancel_all_proposals() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalsCount = _proposals.getProposalsCount(); @@ -233,7 +232,7 @@ contract ExecutableProposalsUnitTests is UnitTest { // TODO: change this test completely to use getters function test_get_proposal_info_and_external_calls() external { ExternalCall[] memory expectedCalls = _getMockTargetRegularStaffCalls(address(_targetMock)); - _proposals.submit(proposer, address(_executor), expectedCalls, ""); + _proposals.submit(address(_executor), expectedCalls); uint256 proposalId = _proposals.getProposalsCount(); ITimelock.ProposalDetails memory proposalDetails = _proposals.getProposalDetails(proposalId); @@ -295,7 +294,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function test_get_cancelled_proposal() external { ExternalCall[] memory expectedCalls = _getMockTargetRegularStaffCalls(address(_targetMock)); - _proposals.submit(proposer, address(_executor), expectedCalls, ""); + _proposals.submit(address(_executor), expectedCalls); uint256 proposalId = _proposals.getProposalsCount(); ITimelock.ProposalDetails memory proposalDetails = _proposals.getProposalDetails(proposalId); @@ -346,16 +345,16 @@ contract ExecutableProposalsUnitTests is UnitTest { function test_count_proposals() external { assertEq(_proposals.getProposalsCount(), 0); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); assertEq(_proposals.getProposalsCount(), 1); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); assertEq(_proposals.getProposalsCount(), 2); _proposals.schedule(1, Durations.ZERO); assertEq(_proposals.getProposalsCount(), 2); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); assertEq(_proposals.getProposalsCount(), 3); _proposals.schedule(2, Durations.ZERO); @@ -364,7 +363,7 @@ contract ExecutableProposalsUnitTests is UnitTest { _proposals.execute(1, Durations.ZERO); assertEq(_proposals.getProposalsCount(), 3); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); assertEq(_proposals.getProposalsCount(), 4); _proposals.cancelAll(); @@ -373,7 +372,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function test_can_execute_proposal() external { Duration delay = Durations.from(100 seconds); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); assert(!_proposals.canExecute(proposalId, Durations.ZERO)); @@ -392,7 +391,7 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_can_not_execute_cancelled_proposal() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); _proposals.schedule(proposalId, Durations.ZERO); @@ -403,18 +402,18 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_cancelAll_DoesNotModifyStateOfExecutedProposals() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); assertEq(_proposals.getProposalsCount(), 1); uint256 executedProposalId = 1; _proposals.schedule(executedProposalId, Durations.ZERO); _proposals.execute(executedProposalId, Durations.ZERO); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); assertEq(_proposals.getProposalsCount(), 2); uint256 scheduledProposalId = 2; _proposals.schedule(scheduledProposalId, Durations.ZERO); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); assertEq(_proposals.getProposalsCount(), 3); uint256 submittedProposalId = 3; @@ -436,7 +435,7 @@ contract ExecutableProposalsUnitTests is UnitTest { function test_can_schedule_proposal() external { Duration delay = Durations.from(100 seconds); - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); assert(!_proposals.canSchedule(proposalId, delay)); @@ -451,7 +450,7 @@ contract ExecutableProposalsUnitTests is UnitTest { } function test_can_not_schedule_cancelled_proposal() external { - _proposals.submit(proposer, address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock)), ""); + _proposals.submit(address(_executor), _getMockTargetRegularStaffCalls(address(_targetMock))); uint256 proposalId = _proposals.getProposalsCount(); assert(_proposals.canSchedule(proposalId, Durations.ZERO)); From b9852146385e6224b6c90f0c2db7ae0ce302b50c Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Fri, 6 Dec 2024 20:19:34 +0400 Subject: [PATCH 4/4] IGovernance.ProposalReported() -> IGovernance.ProposalSubmitted() --- contracts/DualGovernance.sol | 2 +- contracts/TimelockedGovernance.sol | 2 +- contracts/interfaces/IGovernance.sol | 2 +- test/unit/DualGovernance.t.sol | 2 +- test/unit/TimelockedGovernance.t.sol | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/contracts/DualGovernance.sol b/contracts/DualGovernance.sol index 26123b27..8ff969e9 100644 --- a/contracts/DualGovernance.sol +++ b/contracts/DualGovernance.sol @@ -190,7 +190,7 @@ contract DualGovernance is IDualGovernance { Proposers.Proposer memory proposer = _proposers.getProposer(msg.sender); proposalId = TIMELOCK.submit(proposer.executor, calls); - emit ProposalReported(proposer.account, proposalId, metadata); + emit ProposalSubmitted(proposer.account, proposalId, metadata); } /// @notice Schedules a previously submitted proposal for execution in the Dual Governance system. diff --git a/contracts/TimelockedGovernance.sol b/contracts/TimelockedGovernance.sol index 5cc4fb44..348d927e 100644 --- a/contracts/TimelockedGovernance.sol +++ b/contracts/TimelockedGovernance.sol @@ -52,7 +52,7 @@ contract TimelockedGovernance is IGovernance { ) external returns (uint256 proposalId) { _checkCallerIsGovernance(); proposalId = TIMELOCK.submit(TIMELOCK.getAdminExecutor(), calls); - emit ProposalReported(msg.sender, proposalId, metadata); + emit ProposalSubmitted(msg.sender, proposalId, metadata); } /// @notice Schedules a submitted proposal. diff --git a/contracts/interfaces/IGovernance.sol b/contracts/interfaces/IGovernance.sol index fd3421a1..ebf31d6d 100644 --- a/contracts/interfaces/IGovernance.sol +++ b/contracts/interfaces/IGovernance.sol @@ -6,7 +6,7 @@ import {ITimelock} from "./ITimelock.sol"; import {ExternalCall} from "../libraries/ExternalCalls.sol"; interface IGovernance { - event ProposalReported(address indexed proposerAccount, uint256 indexed proposalId, string metadata); + event ProposalSubmitted(address indexed proposerAccount, uint256 indexed proposalId, string metadata); function TIMELOCK() external view returns (ITimelock); function submitProposal( diff --git a/test/unit/DualGovernance.t.sol b/test/unit/DualGovernance.t.sol index 67c7c9ea..ece1ea86 100644 --- a/test/unit/DualGovernance.t.sol +++ b/test/unit/DualGovernance.t.sol @@ -200,7 +200,7 @@ contract DualGovernanceUnitTests is UnitTest { string memory metadata = "New proposal description"; vm.expectEmit(); - emit IGovernance.ProposalReported(proposer.account, expectedProposalId, metadata); + emit IGovernance.ProposalSubmitted(proposer.account, expectedProposalId, metadata); uint256 proposalId = _dualGovernance.submitProposal(calls, metadata); uint256[] memory submittedProposals = _timelock.getSubmittedProposals(); diff --git a/test/unit/TimelockedGovernance.t.sol b/test/unit/TimelockedGovernance.t.sol index 80b83b45..10a6f35e 100644 --- a/test/unit/TimelockedGovernance.t.sol +++ b/test/unit/TimelockedGovernance.t.sol @@ -48,7 +48,7 @@ contract TimelockedGovernanceUnitTests is UnitTest { string memory metadata = "proposal description"; vm.expectEmit(); - emit IGovernance.ProposalReported(_governance, expectedProposalId, metadata); + emit IGovernance.ProposalSubmitted(_governance, expectedProposalId, metadata); vm.prank(_governance); _timelockedGovernance.submitProposal(_getMockTargetRegularStaffCalls(address(0x1)), metadata);