Skip to content

N
nebula
Commit f23bae0b authored by Maurelian's avatar Maurelian Committed by GitHub
Browse files
Options

bedrock: Proxy Admin improvements (#2787) 

* bedrock: Use startPrank to clean up tests

* bedrock: Simplify static calling code in proxy admin

* bedrock: rename OpenZeppelin proxy to ERC1967
parent ae036d9a
Show whitespace changes
Inline Side-by-side
Showing with 96 additions and 83 deletions
.changeset/cool-islands-dream.md 0 → 100644
View file @ f23bae0b
---
'@eth-optimism/contracts-bedrock': patch
---
bedrock: ProxyAdmin rename OpenZeppelin proxy to ERC1967
.changeset/lemon-glasses-trade.md 0 → 100644
View file @ f23bae0b
---
'@eth-optimism/contracts-bedrock': patch
---
bedrock: Simplify ProxyAdmin static calls
packages/contracts-bedrock/.gas-snapshot
View file @ f23bae0b
......@@ -126,28 +126,28 @@ Proxy_Test:test_payableUpgradeToAndCall() (gas: 53887)
Proxy_Test:test_revertUpgradeToAndCall() (gas: 104501)
Proxy_Test:test_upgradeToAndCall() (gas: 125238)
Proxy_Test:test_zeroAddressCaller() (gas: 14758)
ProxyAdmin_Test:test_chugsplashChangeProxyAdmin() (gas: 35994)
ProxyAdmin_Test:test_chugsplashGetProxyAdmin() (gas: 16107)
ProxyAdmin_Test:test_chugsplashGetProxyImplementation() (gas: 51947)
ProxyAdmin_Test:test_chugsplashUpgrade() (gas: 49443)
ProxyAdmin_Test:test_chugsplashUpgradeAndCall() (gas: 82729)
ProxyAdmin_Test:test_delegateResolvedChangeProxyAdmin() (gas: 33846)
ProxyAdmin_Test:test_delegateResolvedGetProxyAdmin() (gas: 18102)
ProxyAdmin_Test:test_delegateResolvedGetProxyImplementation() (gas: 63364)
ProxyAdmin_Test:test_delegateResolvedUpgrade() (gas: 59156)
ProxyAdmin_Test:test_delegateResolvedUpgradeAndCall() (gas: 98627)
ProxyAdmin_Test:test_isUpgrading() (gas: 19531)
ProxyAdmin_Test:test_onlyOwner() (gas: 22672)
ProxyAdmin_Test:test_chugsplashChangeProxyAdmin() (gas: 35647)
ProxyAdmin_Test:test_chugsplashGetProxyAdmin() (gas: 15711)
ProxyAdmin_Test:test_chugsplashGetProxyImplementation() (gas: 51122)
ProxyAdmin_Test:test_chugsplashUpgrade() (gas: 48994)
ProxyAdmin_Test:test_chugsplashUpgradeAndCall() (gas: 82311)
ProxyAdmin_Test:test_delegateResolvedChangeProxyAdmin() (gas: 33976)
ProxyAdmin_Test:test_delegateResolvedGetProxyAdmin() (gas: 17685)
ProxyAdmin_Test:test_delegateResolvedGetProxyImplementation() (gas: 62016)
ProxyAdmin_Test:test_delegateResolvedUpgrade() (gas: 58422)
ProxyAdmin_Test:test_delegateResolvedUpgradeAndCall() (gas: 97948)
ProxyAdmin_Test:test_erc1967ChangeProxyAdmin() (gas: 33862)
ProxyAdmin_Test:test_erc1967GetProxyAdmin() (gas: 15672)
ProxyAdmin_Test:test_erc1967GetProxyImplementation() (gas: 52124)
ProxyAdmin_Test:test_erc1967Upgrade() (gas: 50036)
ProxyAdmin_Test:test_erc1967UpgradeAndCall() (gas: 79019)
ProxyAdmin_Test:test_isUpgrading() (gas: 19530)
ProxyAdmin_Test:test_onlyOwner() (gas: 22715)
ProxyAdmin_Test:test_onlyOwnerSetAddressManager() (gas: 10622)
ProxyAdmin_Test:test_onlyOwnerSetImplementationName() (gas: 11113)
ProxyAdmin_Test:test_onlyOwnerSetProxyType() (gas: 10774)
ProxyAdmin_Test:test_openZeppelinChangeProxyAdmin() (gas: 34248)
ProxyAdmin_Test:test_openZeppelinGetProxyAdmin() (gas: 16090)
ProxyAdmin_Test:test_openZeppelinGetProxyImplementation() (gas: 52950)
ProxyAdmin_Test:test_openZeppelinUpgrade() (gas: 50507)
ProxyAdmin_Test:test_openZeppelinUpgradeAndCall() (gas: 79435)
ProxyAdmin_Test:test_owner() (gas: 9796)
ProxyAdmin_Test:test_proxyType() (gas: 20644)
ProxyAdmin_Test:test_onlyOwnerSetImplementationName() (gas: 11135)
ProxyAdmin_Test:test_onlyOwnerSetProxyType() (gas: 10751)
ProxyAdmin_Test:test_owner() (gas: 9776)
ProxyAdmin_Test:test_proxyType() (gas: 20622)
ProxyAdmin_Test:test_setImplementationName() (gas: 38957)
ResourceMetering_Test:test_initialResourceParams() (gas: 8986)
ResourceMetering_Test:test_updateNoGasDelta() (gas: 2008269)
......
packages/contracts-bedrock/contracts/test/ProxyAdmin.t.sol
View file @ f23bae0b
......@@ -40,21 +40,21 @@ contract ProxyAdmin_Test is Test {
// that is used for the implementation.
resolved = new Lib_ResolvedDelegateProxy(address(addressManager), "a");
// Impersonate alice for setting up the admin.
vm.startPrank(alice);
// Set the address of the address manager in the admin so that it
// can resolve the implementation address of legacy
// Lib_ResolvedDelegateProxy based proxies.
vm.prank(alice);
admin.setAddressManager(address(addressManager));
admin.setAddressManager(addressManager);
// Set the reverse lookup of the Lib_ResolvedDelegateProxy
// proxy
vm.prank(alice);
admin.setImplementationName(address(resolved), "a");
// Set the proxy types
vm.prank(alice);
admin.setProxyType(address(proxy), ProxyAdmin.ProxyType.ERC1967);
admin.setProxyType(address(chugsplash), ProxyAdmin.ProxyType.Chugsplash);
vm.prank(alice);
admin.setProxyType(address(resolved), ProxyAdmin.ProxyType.ResolvedDelegate);
vm.stopPrank();
implementation = new SimpleStorage();
}
......@@ -70,7 +70,7 @@ contract ProxyAdmin_Test is Test {
function test_onlyOwnerSetAddressManager() external {
vm.expectRevert("UNAUTHORIZED");
admin.setAddressManager(address(0));
admin.setAddressManager(Lib_AddressManager((address(0))));
}
function test_onlyOwnerSetImplementationName() external {
......@@ -90,7 +90,7 @@ contract ProxyAdmin_Test is Test {
function test_proxyType() external {
assertEq(
uint256(admin.proxyType(address(proxy))),
uint256(ProxyAdmin.ProxyType.OpenZeppelin)
uint256(ProxyAdmin.ProxyType.ERC1967)
);
assertEq(
uint256(admin.proxyType(address(chugsplash))),
......@@ -102,7 +102,7 @@ contract ProxyAdmin_Test is Test {
);
}
function test_openZeppelinGetProxyImplementation() external {
function test_erc1967GetProxyImplementation() external {
getProxyImplementation(proxy);
}
......@@ -129,7 +129,7 @@ contract ProxyAdmin_Test is Test {
}
}
function test_openZeppelinGetProxyAdmin() external {
function test_erc1967GetProxyAdmin() external {
getProxyAdmin(proxy);
}
......@@ -146,7 +146,7 @@ contract ProxyAdmin_Test is Test {
assertEq(owner, address(admin));
}
function test_openZeppelinChangeProxyAdmin() external {
function test_erc1967ChangeProxyAdmin() external {
changeProxyAdmin(proxy);
}
......@@ -164,19 +164,26 @@ contract ProxyAdmin_Test is Test {
vm.prank(alice);
admin.changeProxyAdmin(_proxy, address(128));
// The proxy is not longer the admin and can
// The proxy is no longer the admin and can
// no longer call the proxy interface except for
// the ResolvedDelegate type which anybody can call
// the admin interface
if (proxyType != ProxyAdmin.ProxyType.ResolvedDelegate) {
vm.expectRevert();
// the ResolvedDelegate type on which anybody can
// call the admin interface.
if (proxyType == ProxyAdmin.ProxyType.ERC1967) {
vm.expectRevert("Proxy: implementation not initialized");
admin.getProxyAdmin(_proxy);
} else if (proxyType == ProxyAdmin.ProxyType.Chugsplash) {
vm.expectRevert("L1ChugSplashProxy: implementation is not set yet");
admin.getProxyAdmin(_proxy);
} else if (proxyType == ProxyAdmin.ProxyType.ResolvedDelegate) {
// Just an empty block to show that all cases are covered
} else {
vm.expectRevert("ProxyAdmin: unknown proxy type");
}
// Call the proxy contract directly to get the admin.
// Different proxy types have different interfaces.
vm.prank(address(128));
if (proxyType == ProxyAdmin.ProxyType.OpenZeppelin) {
if (proxyType == ProxyAdmin.ProxyType.ERC1967) {
assertEq(_proxy.admin(), address(128));
} else if (proxyType == ProxyAdmin.ProxyType.Chugsplash) {
assertEq(
......@@ -193,7 +200,7 @@ contract ProxyAdmin_Test is Test {
}
}
function test_openZeppelinUpgrade() external {
function test_erc1967Upgrade() external {
upgrade(proxy);
}
......@@ -213,7 +220,7 @@ contract ProxyAdmin_Test is Test {
assertEq(impl, address(implementation));
}
function test_openZeppelinUpgradeAndCall() external {
function test_erc1967UpgradeAndCall() external {
upgradeAndCall(proxy);
}
......
packages/contracts-bedrock/contracts/universal/ProxyAdmin.sol
View file @ f23bae0b
......@@ -6,25 +6,39 @@ import { Owned } from "@rari-capital/solmate/src/auth/Owned.sol";
import { Lib_AddressManager } from "../legacy/Lib_AddressManager.sol";
import { L1ChugSplashProxy } from "../legacy/L1ChugSplashProxy.sol";
// Define static interfaces of these proxies so that we can easily
// use staticcall on the getters we need.
interface IStatic_ERC1967Proxy {
function implementation() external view returns (address);
function admin() external view returns (address);
}
interface IStatic_L1ChugSplashProxy {
function getImplementation() external view returns (address);
function getOwner() external view returns (address);
}
/**
* @title ProxyAdmin
* @dev This is an auxiliary contract meant to be assigned as the admin of a Proxy, based on
* the OpenZeppelin implementation. It has backwards compatibility logic to work with the
* various types of proxies that have been deployed by Optimism.
* @dev This is an auxiliary contract meant to be assigned as the admin of an ERC1967 Proxy,
* based on the OpenZeppelin implementation. It has backwards compatibility logic to work with
* the various types of proxies that have been deployed by Optimism.
*/
contract ProxyAdmin is Owned {
/**
* @notice The proxy types that the ProxyAdmin can manage.
*
* @custom:value OpenZeppelin Represents the OpenZeppelin style transparent proxy
* interface, this is the standard.
* @custom:value ERC1967 Represents an ERC1967 compliant transparent proxy
* interface, this is the default.
* @custom:value Chugsplash Represents the Chugsplash proxy interface,
* this is legacy.
* @custom:value ResolvedDelegate Represents the ResolvedDelegate proxy
* interface, this is legacy.
*/
enum ProxyType {
OpenZeppelin,
ERC1967,
Chugsplash,
ResolvedDelegate
}
......@@ -88,8 +102,8 @@ contract ProxyAdmin is Owned {
*
* @param _address The address of the address manager.
*/
function setAddressManager(address _address) external onlyOwner {
addressManager = Lib_AddressManager(_address);
function setAddressManager(Lib_AddressManager _address) external onlyOwner {
addressManager = _address;
}
/**
......@@ -135,28 +149,15 @@ contract ProxyAdmin is Owned {
function getProxyImplementation(Proxy proxy) external view returns (address) {
ProxyType proxyType = proxyType[address(proxy)];
// We need to manually run the static call since the getter cannot be flagged as view
address target;
bytes memory data;
if (proxyType == ProxyType.OpenZeppelin) {
target = address(proxy);
data = abi.encodeWithSelector(Proxy.implementation.selector);
if (proxyType == ProxyType.ERC1967) {
return IStatic_ERC1967Proxy(address(proxy)).implementation();
} else if (proxyType == ProxyType.Chugsplash) {
target = address(proxy);
data = abi.encodeWithSelector(L1ChugSplashProxy.getImplementation.selector);
return IStatic_L1ChugSplashProxy(address(proxy)).getImplementation();
} else if (proxyType == ProxyType.ResolvedDelegate) {
target = address(addressManager);
data = abi.encodeWithSelector(
Lib_AddressManager.getAddress.selector,
implementationName[address(proxy)]
);
return addressManager.getAddress(implementationName[address(proxy)]);
} else {
revert("ProxyAdmin: unknown proxy type");
}
(bool success, bytes memory returndata) = target.staticcall(data);
require(success);
return abi.decode(returndata, (address));
}
/**
......@@ -169,25 +170,15 @@ contract ProxyAdmin is Owned {
function getProxyAdmin(Proxy proxy) external view returns (address) {
ProxyType proxyType = proxyType[address(proxy)];
// We need to manually run the static call since the getter cannot be flagged as view
address target;
bytes memory data;
if (proxyType == ProxyType.OpenZeppelin) {
target = address(proxy);
data = abi.encodeWithSelector(Proxy.admin.selector);
if (proxyType == ProxyType.ERC1967) {
return IStatic_ERC1967Proxy(address(proxy)).admin();
} else if (proxyType == ProxyType.Chugsplash) {
target = address(proxy);
data = abi.encodeWithSelector(L1ChugSplashProxy.getOwner.selector);
return IStatic_L1ChugSplashProxy(address(proxy)).getOwner();
} else if (proxyType == ProxyType.ResolvedDelegate) {
target = address(addressManager);
data = abi.encodeWithSignature("owner()");
return addressManager.owner();
} else {
revert("ProxyAdmin: unknown proxy type");
}
(bool success, bytes memory returndata) = target.staticcall(data);
require(success);
return abi.decode(returndata, (address));
}
/**
......@@ -200,12 +191,14 @@ contract ProxyAdmin is Owned {
function changeProxyAdmin(Proxy proxy, address newAdmin) external onlyOwner {
ProxyType proxyType = proxyType[address(proxy)];
if (proxyType == ProxyType.OpenZeppelin) {
if (proxyType == ProxyType.ERC1967) {
proxy.changeAdmin(newAdmin);
} else if (proxyType == ProxyType.Chugsplash) {
L1ChugSplashProxy(payable(proxy)).setOwner(newAdmin);
} else if (proxyType == ProxyType.ResolvedDelegate) {
Lib_AddressManager(addressManager).transferOwnership(newAdmin);
addressManager.transferOwnership(newAdmin);
} else {
revert("ProxyAdmin: unknown proxy type");
}
}
......@@ -218,7 +211,7 @@ contract ProxyAdmin is Owned {
function upgrade(Proxy proxy, address implementation) public onlyOwner {
ProxyType proxyType = proxyType[address(proxy)];
if (proxyType == ProxyType.OpenZeppelin) {
if (proxyType == ProxyType.ERC1967) {
proxy.upgradeTo(implementation);
} else if (proxyType == ProxyType.Chugsplash) {
L1ChugSplashProxy(payable(proxy)).setStorage(
......@@ -227,7 +220,9 @@ contract ProxyAdmin is Owned {
);
} else if (proxyType == ProxyType.ResolvedDelegate) {
string memory name = implementationName[address(proxy)];
Lib_AddressManager(addressManager).setAddress(name, implementation);
addressManager.setAddress(name, implementation);
} else {
revert("ProxyAdmin: unknown proxy type");
}
}
......@@ -246,9 +241,10 @@ contract ProxyAdmin is Owned {
) external payable onlyOwner {
ProxyType proxyType = proxyType[address(proxy)];
if (proxyType == ProxyType.OpenZeppelin) {
if (proxyType == ProxyType.ERC1967) {
proxy.upgradeToAndCall{ value: msg.value }(implementation, data);
} else {
// reverts if proxy type is unknown
upgrade(proxy, implementation);
(bool success, ) = address(proxy).call{ value: msg.value }(data);
require(success);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment