From de7a38cd78d9696e1e17d7c89cf64c0a44a779ea Mon Sep 17 00:00:00 2001 From: DuBento Date: Tue, 25 Jul 2023 09:37:42 +0100 Subject: [PATCH] feat: GovernorExecutor as default inheritance for governor removed duplicate code, downgraded abstraction improvement of 1% DeployC in GovernorContract (5728315) #11 --- blockchain/contracts/DAO/GovernorContract.sol | 26 +------ .../contracts/DAO/governance/Governor.sol | 72 ++----------------- .../extensions/GovernorExecutor.sol | 11 +-- 3 files changed, 14 insertions(+), 95 deletions(-) diff --git a/blockchain/contracts/DAO/GovernorContract.sol b/blockchain/contracts/DAO/GovernorContract.sol index 32fa02f..681c2f7 100644 --- a/blockchain/contracts/DAO/GovernorContract.sol +++ b/blockchain/contracts/DAO/GovernorContract.sol @@ -5,7 +5,6 @@ import "./governance/extensions/GovernorCountingSimple.sol"; import "./governance/extensions/GovernorVotes.sol"; import "./governance/extensions/GovernorVotesQuorumFraction.sol"; import "./governance/extensions/GovernorSettings.sol"; -import "./governance/extensions/GovernorExecutor.sol"; import "./governance/Governor.sol"; contract GovernorContract is @@ -13,8 +12,7 @@ contract GovernorContract is GovernorSettings, GovernorCountingSimple, GovernorVotes, - GovernorVotesQuorumFraction, - GovernorExecutor + GovernorVotesQuorumFraction { constructor( IVotes token_, @@ -23,11 +21,10 @@ contract GovernorContract is uint256 votingPeriod_, uint256 quorumFraction_ ) - Governor("GovernorContract") + Governor("GovernorContract", executor_) GovernorSettings(votingDelay_, votingPeriod_, 0) GovernorVotes(token_) GovernorVotesQuorumFraction(quorumFraction_) - GovernorExecutor(executor_) {} // The following functions are overrides required by Solidity. @@ -69,23 +66,4 @@ contract GovernorContract is { return super.proposalThreshold(); } - - function _execute( - uint256 proposalId, - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 descriptionHash - ) internal override(Governor, GovernorExecutor) { - super._execute(proposalId, targets, values, calldatas, descriptionHash); - } - - function _executor() - internal - view - override(Governor, GovernorExecutor) - returns (address) - { - return super._executor(); - } } diff --git a/blockchain/contracts/DAO/governance/Governor.sol b/blockchain/contracts/DAO/governance/Governor.sol index 5ab3f79..b565d8d 100644 --- a/blockchain/contracts/DAO/governance/Governor.sol +++ b/blockchain/contracts/DAO/governance/Governor.sol @@ -11,6 +11,7 @@ import "../../OpenZeppelin/utils/introspection/ERC165.sol"; import "../../OpenZeppelin/utils/math/SafeCast.sol"; import "../../OpenZeppelin/utils/Address.sol"; import "../../OpenZeppelin/utils/Context.sol"; +import "./extensions/GovernorExecutor.sol"; import "./IGovernor.sol"; /** @@ -25,6 +26,7 @@ import "./IGovernor.sol"; * Modified version of governance contracts from OpenZeppelin v4.9.1 */ abstract contract Governor is + GovernorExecutor, Context, ERC165, EIP712, @@ -81,7 +83,10 @@ abstract contract Governor is /** * @dev Sets the value for {name} and {version} */ - constructor(string memory name_) EIP712(name_, version()) { + constructor( + string memory name_, + Executor executor_ + ) EIP712(name_, version()) GovernorExecutor(executor_) { _name = name_; } @@ -440,62 +445,6 @@ abstract contract Governor is return _cancel(targets, values, calldatas, descriptionHash); } - /** - * @dev Internal execution mechanism. Can be overridden to implement different execution mechanism - */ - function _execute( - uint256 /* proposalId */, - address[] memory targets, - uint256[] memory values, - bytes[] memory calldatas, - bytes32 /*descriptionHash*/ - ) internal virtual { - for (uint256 i = 0; i < targets.length; ++i) { - (bool success, bytes memory returndata) = targets[i].call{ - value: values[i] - }(calldatas[i]); - Address.verifyCallResult(success, returndata); - } - } - - // TODO remove - - // /** - // * @dev Hook before execution is triggered. - // */ - // function _beforeExecute( - // uint256 /* proposalId */, - // address[] memory targets, - // uint256[] memory /* values */, - // bytes[] memory calldatas, - // bytes32 /*descriptionHash*/ - // ) internal virtual { - // if (_executor() != address(this)) { - // for (uint256 i = 0; i < targets.length; ++i) { - // if (targets[i] == address(this)) { - // _governanceCall.pushBack(keccak256(calldatas[i])); - // } - // } - // } - // } - - // /** - // * @dev Hook after execution is triggered. - // */ - // function _afterExecute( - // uint256 /* proposalId */, - // address[] memory /* targets */, - // uint256[] memory /* values */, - // bytes[] memory /* calldatas */, - // bytes32 /*descriptionHash*/ - // ) internal virtual { - // if (_executor() != address(this)) { - // if (!_governanceCall.empty()) { - // _governanceCall.clear(); - // } - // } - // } - /** * @dev Internal cancel mechanism: locks up the proposal timer, preventing it from being re-submitted. Marks it as * canceled to allow distinguishing it from executed proposals. @@ -719,15 +668,6 @@ abstract contract Governor is Address.verifyCallResult(success, returndata); } - // TODO remove - /** - * @dev Address through which the governor executes action. Will be overloaded by module that execute actions - * through another contract such as a timelock. - */ - function _executor() internal view virtual returns (address) { - return address(this); - } - /** * @dev See {IERC721Receiver-onERC721Received}. * Receiving tokens is disabled if the governance executor is other than the governor itself (eg. when using with a timelock). diff --git a/blockchain/contracts/DAO/governance/extensions/GovernorExecutor.sol b/blockchain/contracts/DAO/governance/extensions/GovernorExecutor.sol index dd6d362..1de8eb0 100644 --- a/blockchain/contracts/DAO/governance/extensions/GovernorExecutor.sol +++ b/blockchain/contracts/DAO/governance/extensions/GovernorExecutor.sol @@ -3,9 +3,7 @@ pragma solidity ^0.8.19; -import "../../../custom/Ownable.sol"; import "../../Executor.sol"; -import "../Governor.sol"; /** * @dev Extension of {Governor} that binds the execution process to an instance of {Executer}. @@ -13,7 +11,7 @@ import "../Governor.sol"; * Using this model means the proposal will be operated by the {Executor} and not by the {Governor}. Thus, * the assets and permissions must be attached to the {Executor} */ -abstract contract GovernorExecutor is Governor, Ownable { +abstract contract GovernorExecutor { Executor private _executorContract; // Type declarations @@ -45,7 +43,7 @@ abstract contract GovernorExecutor is Governor, Ownable { /** * @dev Address through which the governor executes action. In this case, the timelock. */ - function _executor() internal view virtual override returns (address) { + function _executor() internal view virtual returns (address) { return address(_executorContract); } @@ -58,7 +56,7 @@ abstract contract GovernorExecutor is Governor, Ownable { uint256[] memory values, bytes[] memory calldatas, bytes32 descriptionHash - ) internal virtual override { + ) internal virtual { // execute _executorContract.executeBatch{value: msg.value}( targets, @@ -69,6 +67,9 @@ abstract contract GovernorExecutor is Governor, Ownable { ); } + /** + * @dev Update executor contract address. + */ function _updateExecutor(Executor newExecutor) internal { emit ExecutorChange(address(_executorContract), address(newExecutor)); _executorContract = newExecutor;