Make TransparentUpgradeableProxy deploy its ProxyAdmin and optimize proxy interfaces
#4382
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,17 +6,16 @@ pragma solidity ^0.8.19; | |
| import {ERC1967Utils} from "../ERC1967/ERC1967Utils.sol"; | ||
| import {ERC1967Proxy} from "../ERC1967/ERC1967Proxy.sol"; | ||
| import {IERC1967} from "../../interfaces/IERC1967.sol"; | ||
| import {ProxyAdmin} from "./ProxyAdmin.sol"; | ||
|
|
||
| /** | ||
| * @dev Interface for {TransparentUpgradeableProxy}. In order to implement transparency, {TransparentUpgradeableProxy} | ||
| * does not implement this interface directly, and its upgradeability mechanism is implemented by an internal dispatch | ||
| * mechanism. The compiler is unaware that these functions are implemented by {TransparentUpgradeableProxy} and will not | ||
| * include them in the ABI so this interface must be used to interact with it. | ||
| */ | ||
| interface ITransparentUpgradeableProxy is IERC1967 { | ||
| function upgradeToAndCall(address, bytes calldata) external payable; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -28,8 +27,8 @@ interface ITransparentUpgradeableProxy is IERC1967 { | |
| * things that go hand in hand: | ||
| * | ||
| * 1. If any account other than the admin calls the proxy, the call will be forwarded to the implementation, even if | ||
| * that call matches the {ITransparentUpgradeableProxy-upgradeTo} function exposed by the proxy itself. | ||
| * 2. If the admin calls the proxy, it can call the `upgradeTo` function but any other call won't be forwarded to the | ||
| * implementation. If the admin tries to call a function on the implementation it will fail with an error indicating the | ||
| * proxy admin cannot fallback to the target implementation. | ||
| * | ||
|
|
@@ -42,9 +41,9 @@ interface ITransparentUpgradeableProxy is IERC1967 { | |
| * {Ownable} contract to allow for changing the proxy's admin owner. | ||
| * | ||
| * NOTE: The real interface of this proxy is that defined in `ITransparentUpgradeableProxy`. This contract does not | ||
| * inherit from that interface, and instead the `upgradeTo` is implicitly implemented using a custom dispatch mechanism | ||
| * in `_fallback`. Consequently, the compiler will not produce an ABI for this contract. This is necessary to fully | ||
| * implement transparency without decoding reverts caused by selector clashes between the proxy and the | ||
| * implementation. | ||
| * | ||
| * IMPORTANT: This contract avoids unnecessary storage reads by setting the admin only during construction as an immutable variable, | ||
|
|
@@ -55,7 +54,7 @@ interface ITransparentUpgradeableProxy is IERC1967 { | |
| * WARNING: It is not recommended to extend this contract to add additional external functions. If you do so, the compiler | ||
| * will not check that there are no selector conflicts, due to the note above. A selector clash between any new function | ||
| * and the functions declared in {ITransparentUpgradeableProxy} will be resolved in favor of the new one. This could | ||
| * render the `upgradeTo` function inaccessible, preventing upgradeability and compromising transparency. | ||
| */ | ||
| contract TransparentUpgradeableProxy is ERC1967Proxy { | ||
| // An immutable address for the admin avoid unnecessary SLOADs before each call | ||
ernestognw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
@@ -78,10 +77,10 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { | |
| * @dev Initializes an upgradeable proxy managed by `_admin`, backed by the implementation at `_logic`, and | ||
| * optionally initialized with `_data` as explained in {ERC1967Proxy-constructor}. | ||
ernestognw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| constructor(address _logic, address initialOwner, bytes memory _data) payable ERC1967Proxy(_logic, _data) { | ||
|
There was a problem hiding this comment. Hey i just stumbled over this change when reviewing v5 and was wondering if it wouldn't make sense to have an overload with This way one could easily reuse a single ProxyAdmin for multiple contracts. There was a problem hiding this comment.
This used ti be the case in v4. We decided to change that. In v4, the admin was stored in storage, which means that any call going trough the proxy would have to load that value, costing 2100 gas. function _fallback() internal virtual override {
if (msg.sender == _admin) {
...We wanted to reduce that gas cost, and so we decided to make the admin immutable. That means that it is not possible for a proxy to change which admin it uses. You could change the value stored in the ERC-1967 slot, but that would have no effect. That means that if multiple proxy use the same ProxyAdmin, then you can separate the ownership. You may be able to change who owns the ProxyAdmin, but every proxy that point to it will share the same owner, forever. That is why we decided to make the There was a problem hiding this comment. Agree with @Amxx. We agreed that it's better to reduce the SLOAD cost in the proxy than allowing to reuse ProxyAdmins. The tradeoff is fine because this not only increases deployment costs but reduces every proxy interaction cost. Consider regular proxy interactions are more frequent (potentially millions of times) than admin interactions. There was a problem hiding this comment. I just stumbled upon this while deploying a proxy using v5. Being used to the v4 behavior, I wrongly passed in a proxy admin making the deployment unusable. As far as I could tell, this change is not documented in any changelog. I think that it would be good to add this to the v5 changelog. There was a problem hiding this comment. Thanks for sharing @danhper! The change is documented on both OZ upgrades plugins and OZ Contracts (see Proxy section). Though I know it's counterintuitive, we strongly advice users to be careful about major releases. We'd be happy to hear suggestions on how to improve the way we communicate these changes. |
||
| _admin = address(new ProxyAdmin(initialOwner)); | ||
| // Set the storage value and emit an event for ERC-1967 compatibility | ||
| ERC1967Utils.changeAdmin(_admin); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -91,9 +90,7 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { | |
| if (msg.sender == _admin) { | ||
| bytes memory ret; | ||
| bytes4 selector = msg.sig; | ||
| if (selector == ITransparentUpgradeableProxy.upgradeToAndCall.selector) { | ||
| ret = _dispatchUpgradeToAndCall(); | ||
| } else { | ||
| revert ProxyDeniedAdminAccess(); | ||
|
|
@@ -109,23 +106,11 @@ contract TransparentUpgradeableProxy is ERC1967Proxy { | |
| /** | ||
| * @dev Upgrade the implementation of the proxy. | ||
| */ | ||
| function _dispatchUpgradeToAndCall() private returns (bytes memory) { | ||
| bool forceCall = msg.value > 0; | ||
frangio marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| (address newImplementation, bytes memory data) = abi.decode(msg.data[4:], (address, bytes)); | ||
| ERC1967Utils.upgradeToAndCall(newImplementation, data, forceCall); | ||
|
|
||
| return ""; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.