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

Add internal functions inside modifiers #4472

Merged
5 changes: 5 additions & 0 deletions .changeset/swift-numbers-cry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`Governor`, `Initializable`, and `UUPSUpgradeable`: Use internal functions in modifiers to optimize bytecode size.
25 changes: 17 additions & 8 deletions contracts/governance/Governor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
* governance protocol (since v4.6).
*/
modifier onlyGovernance() {
if (_executor() != _msgSender()) {
revert GovernorOnlyExecutor(_msgSender());
}
if (_executor() != address(this)) {
bytes32 msgDataHash = keccak256(_msgData());
// loop until popping the expected operation - throw if deque is empty (operation not authorized)
while (_governanceCall.popFront() != msgDataHash) {}
}
_checkGovernance();
_;
}

Expand Down Expand Up @@ -227,6 +220,22 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72
return _proposals[proposalId].proposer;
}

/**
* @dev Reverts if the `msg.sender` is not the executor. In case the executor is not this contract
* itself, the function reverts if `msg.data` is not whitelisted as a result of an {execute}
* operation. See {onlyGovernance}.
*/
function _checkGovernance() internal virtual {
if (_executor() != _msgSender()) {
revert GovernorOnlyExecutor(_msgSender());
}
if (_executor() != address(this)) {
bytes32 msgDataHash = keccak256(_msgData());
// loop until popping the expected operation - throw if deque is empty (operation not authorized)
while (_governanceCall.popFront() != msgDataHash) {}
}
}

/**
* @dev Amount of votes already cast passes the threshold limit.
*/
Expand Down
9 changes: 8 additions & 1 deletion contracts/proxy/utils/Initializable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,17 @@ abstract contract Initializable {
* {initializer} and {reinitializer} modifiers, directly or indirectly.
*/
modifier onlyInitializing() {
_checkInitializing();
_;
}

/**
* @dev Reverts if the contract is not in an initializing state. See {onlyInitializing}.
*/
function _checkInitializing() internal view virtual {
if (!_initializing) {
revert NotInitializing();
}
_;
}

/**
Expand Down
37 changes: 27 additions & 10 deletions contracts/proxy/utils/UUPSUpgradeable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
* fail.
*/
modifier onlyProxy() {
if (
address(this) == __self || // Must be called through delegatecall
ERC1967Utils.getImplementation() != __self // Must be called through an active proxy
) {
revert UUPSUnauthorizedCallContext();
}
_checkProxy();
_;
}

Expand All @@ -62,10 +57,7 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
* callable on the implementing contract but not through proxies.
*/
modifier notDelegated() {
if (address(this) != __self) {
// Must not be called through delegatecall
revert UUPSUnauthorizedCallContext();
}
_checkNotDelegated();
_;
}

Expand Down Expand Up @@ -96,6 +88,31 @@ abstract contract UUPSUpgradeable is IERC1822Proxiable {
_upgradeToAndCallUUPS(newImplementation, data);
}

/**
* @dev Reverts if the execution is not performed via delegatecall or the execution
* context is not of a proxy with an ERC1967-compliant implementation pointing to self.
* See {_onlyProxy}.
*/
function _checkProxy() internal view virtual {
if (
address(this) == __self || // Must be called through delegatecall
ERC1967Utils.getImplementation() != __self // Must be called through an active proxy
) {
revert UUPSUnauthorizedCallContext();
}
}

/**
* @dev Reverts if the execution is performed via delegatecall.
* See {notDelegated}.
*/
function _checkNotDelegated() internal view virtual {
if (address(this) != __self) {
// Must not be called through delegatecall
revert UUPSUnauthorizedCallContext();
}
}

/**
* @dev Function that should revert when `msg.sender` is not authorized to upgrade the contract. Called by
* {upgradeToAndCall}.
Expand Down