From 51db9a3ff4a6c412238102c8ffde327c9dc66ac7 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 15 Oct 2024 13:28:05 -0400 Subject: [PATCH 01/16] use up exec npm package --- package.json | 1 + remappings.txt | 1 + src/L1GovernanceFactory.sol | 2 +- src/L2GovernanceFactory.sol | 2 +- src/UpgradeExecRouteBuilder.sol | 4 +- src/UpgradeExecutor.sol | 60 -------- .../GovernanceChainSCMgmtActivationAction.sol | 4 +- .../L1SCMgmtActivationAction.sol | 6 +- ...nGovernanceChainSCMgmtActivationAction.sol | 4 +- .../SecurityCouncilMgmtUpgradeLib.sol | 4 +- src/interfaces/IUpgradeExecutor.sol | 10 -- .../SecurityCouncilManager.sol | 2 +- .../SecurityCouncilMemberElectionGovernor.sol | 1 + .../SecurityCouncilMemberRemovalGovernor.sol | 1 + ...SecurityCouncilNomineeElectionGovernor.sol | 1 + test/L2GovernanceFactory.t.sol | 2 +- test/UpgradeExecutor.t.sol | 145 ------------------ .../gov-actions/AIPNovaFeeRoutingAction.t.sol | 2 +- .../NomineeGovernorV2UpgradeAction.t.sol | 1 + test/gov-actions/ProxyAdminUpgrader.t.sol | 2 +- .../SwitchManagerRolesAction.t.sol | 1 + test/security-council-mgmt/E2E.t.sol | 3 +- .../SecurityCouncilMemberSyncAction.t.sol | 2 +- .../SecurityCouncilUpgradeAction.t.sol | 2 +- test/util/ActionTestBase.sol | 2 +- 25 files changed, 28 insertions(+), 237 deletions(-) delete mode 100644 src/UpgradeExecutor.sol delete mode 100644 src/interfaces/IUpgradeExecutor.sol delete mode 100644 test/UpgradeExecutor.t.sol diff --git a/package.json b/package.json index 54acd0e9e..5a5a4aea8 100644 --- a/package.json +++ b/package.json @@ -79,6 +79,7 @@ "@arbitrum/nitro-contracts": "1.1.1", "@arbitrum/token-bridge-contracts": "1.0.0-beta.0", "@gnosis.pm/safe-contracts": "1.3.0", + "@offchainlabs/upgrade-executor": "1.1.0-beta.0", "@openzeppelin/contracts": "4.7.3", "@openzeppelin/contracts-upgradeable": "4.7.3", "@types/yargs": "^17.0.17", diff --git a/remappings.txt b/remappings.txt index aa38660ff..7a98cb9d7 100644 --- a/remappings.txt +++ b/remappings.txt @@ -1,5 +1,6 @@ forge-std/=lib/forge-std/src/ solady/=lib/solady/src/ +@offchainlabs/upgrade-executor/=node_modules/@offchainlabs/upgrade-executor/ @arbitrum/token-bridge-contracts/=node_modules/@arbitrum/token-bridge-contracts/ @arbitrum/nitro-contracts/=node_modules/@arbitrum/nitro-contracts/ @openzeppelin/contracts-upgradeable/=node_modules/@openzeppelin/contracts-upgradeable/ diff --git a/src/L1GovernanceFactory.sol b/src/L1GovernanceFactory.sol index c6292aad5..ebc99b841 100644 --- a/src/L1GovernanceFactory.sol +++ b/src/L1GovernanceFactory.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "./L1ArbitrumTimelock.sol"; -import "./UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; diff --git a/src/L2GovernanceFactory.sol b/src/L2GovernanceFactory.sol index 5a4ad6b9f..5528eb63c 100644 --- a/src/L2GovernanceFactory.sol +++ b/src/L2GovernanceFactory.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.16; import "./L2ArbitrumToken.sol"; import "./L2ArbitrumGovernor.sol"; import "./ArbitrumTimelock.sol"; -import "./UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "./FixedDelegateErc20Wallet.sol"; import "./ArbitrumDAOConstitution.sol"; import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; diff --git a/src/UpgradeExecRouteBuilder.sol b/src/UpgradeExecRouteBuilder.sol index b1f7f6ee1..0bd00e903 100644 --- a/src/UpgradeExecRouteBuilder.sol +++ b/src/UpgradeExecRouteBuilder.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol"; -import "./UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; import "./L1ArbitrumTimelock.sol"; import "./security-council-mgmt/Common.sol"; @@ -136,7 +136,7 @@ contract UpgradeExecRouteBuilder { } bytes memory executorData = abi.encodeWithSelector( - UpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] + IUpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] ); // for L1, inbox is set to address(0): diff --git a/src/UpgradeExecutor.sol b/src/UpgradeExecutor.sol deleted file mode 100644 index adaa6d830..000000000 --- a/src/UpgradeExecutor.sol +++ /dev/null @@ -1,60 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -pragma solidity 0.8.16; - -import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; -import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; -import "@openzeppelin/contracts/utils/Address.sol"; - -/// @title A root contract from which it execute upgrades -/// @notice Does not contain upgrade logic itself, only the means to call upgrade contracts and execute them -/// @dev We use these upgrade contracts as they allow multiple actions to take place in an upgrade -/// and for these actions to interact. However because we are delegatecalling into these upgrade -/// contracts, it's important that these upgrade contract do not touch or modify contract state. -contract UpgradeExecutor is Initializable, AccessControlUpgradeable, ReentrancyGuard { - using Address for address; - - bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE"); - bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE"); - - /// @notice Emitted when an upgrade execution occurs - event UpgradeExecuted(address indexed upgrade, uint256 value, bytes data); - - constructor() { - _disableInitializers(); - } - - /// @notice Initialise the upgrade executor - /// @param admin The admin who can update other roles, and itself - ADMIN_ROLE - /// @param executors Can call the execute function - EXECUTOR_ROLE - function initialize(address admin, address[] memory executors) public initializer { - require(admin != address(0), "UpgradeExecutor: zero admin"); - - __AccessControl_init(); - - _setRoleAdmin(ADMIN_ROLE, ADMIN_ROLE); - _setRoleAdmin(EXECUTOR_ROLE, ADMIN_ROLE); - - _setupRole(ADMIN_ROLE, admin); - for (uint256 i = 0; i < executors.length; ++i) { - _setupRole(EXECUTOR_ROLE, executors[i]); - } - } - - /// @notice Execute an upgrade by delegate calling an upgrade contract - /// @dev Only executor can call this. Since we're using a delegatecall here the Upgrade contract - /// will have access to the state of this contract - including the roles. Only upgrade contracts - /// that do not touch local state should be used. - function execute(address upgrade, bytes memory upgradeCallData) - public - payable - onlyRole(EXECUTOR_ROLE) - nonReentrant - { - // OZ Address library check if the address is a contract and bubble up inner revert reason - address(upgrade).functionDelegateCall( - upgradeCallData, "UpgradeExecutor: inner delegate call failed without reason" - ); - - emit UpgradeExecuted(upgrade, msg.value, upgradeCallData); - } -} diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol index 956ce437d..202a606f7 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol @@ -5,7 +5,7 @@ import "../../../security-council-mgmt/interfaces/IGnosisSafe.sol"; import "../../address-registries/L2AddressRegistryInterfaces.sol"; import "./SecurityCouncilMgmtUpgradeLib.sol"; import "../../../interfaces/IArbitrumDAOConstitution.sol"; -import "../../../interfaces/IUpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "../../../interfaces/ICoreTimelock.sol"; import "@openzeppelin/contracts/utils/Address.sol"; @@ -49,7 +49,7 @@ contract GovernanceChainSCMgmtActivationAction { } function perform() external { - IUpgradeExecutor upgradeExecutor = IUpgradeExecutor(l2AddressRegistry.coreGov().owner()); + UpgradeExecutor upgradeExecutor = UpgradeExecutor(l2AddressRegistry.coreGov().owner()); // swap in new emergency security council SecurityCouncilMgmtUpgradeLib.replaceEmergencySecurityCouncil({ diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol index 602e149cf..58c004740 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "../../../security-council-mgmt/interfaces/IGnosisSafe.sol"; -import "../../../interfaces/IUpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; import "../../../interfaces/ICoreTimelock.sol"; import "./SecurityCouncilMgmtUpgradeLib.sol"; @@ -10,14 +10,14 @@ contract L1SCMgmtActivationAction { IGnosisSafe public immutable newEmergencySecurityCouncil; IGnosisSafe public immutable prevEmergencySecurityCouncil; uint256 public immutable emergencySecurityCouncilThreshold; - IUpgradeExecutor public immutable l1UpgradeExecutor; + UpgradeExecutor public immutable l1UpgradeExecutor; ICoreTimelock public immutable l1Timelock; constructor( IGnosisSafe _newEmergencySecurityCouncil, IGnosisSafe _prevEmergencySecurityCouncil, uint256 _emergencySecurityCouncilThreshold, - IUpgradeExecutor _l1UpgradeExecutor, + UpgradeExecutor _l1UpgradeExecutor, ICoreTimelock _l1Timelock ) { newEmergencySecurityCouncil = _newEmergencySecurityCouncil; diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol index d3c48f057..e5feeb16e 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol @@ -8,13 +8,13 @@ contract NonGovernanceChainSCMgmtActivationAction { IGnosisSafe public immutable newEmergencySecurityCouncil; IGnosisSafe public immutable prevEmergencySecurityCouncil; uint256 public immutable emergencySecurityCouncilThreshold; - IUpgradeExecutor public immutable upgradeExecutor; + UpgradeExecutor public immutable upgradeExecutor; constructor( IGnosisSafe _newEmergencySecurityCouncil, IGnosisSafe _prevEmergencySecurityCouncil, uint256 _emergencySecurityCouncilThreshold, - IUpgradeExecutor _upgradeExecutor + UpgradeExecutor _upgradeExecutor ) { newEmergencySecurityCouncil = _newEmergencySecurityCouncil; prevEmergencySecurityCouncil = _prevEmergencySecurityCouncil; diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol index d12c9212e..958e86ae0 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol @@ -2,14 +2,14 @@ pragma solidity 0.8.16; import "../../../security-council-mgmt/interfaces/IGnosisSafe.sol"; -import "../../../interfaces/IUpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; library SecurityCouncilMgmtUpgradeLib { function replaceEmergencySecurityCouncil( IGnosisSafe _prevSecurityCouncil, IGnosisSafe _newSecurityCouncil, uint256 _threshold, - IUpgradeExecutor _upgradeExecutor + UpgradeExecutor _upgradeExecutor ) internal { requireSafesEquivalent(_prevSecurityCouncil, _newSecurityCouncil, _threshold); bytes32 EXECUTOR_ROLE = _upgradeExecutor.EXECUTOR_ROLE(); diff --git a/src/interfaces/IUpgradeExecutor.sol b/src/interfaces/IUpgradeExecutor.sol deleted file mode 100644 index e9f02416a..000000000 --- a/src/interfaces/IUpgradeExecutor.sol +++ /dev/null @@ -1,10 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -pragma solidity 0.8.16; - -import "@openzeppelin/contracts-upgradeable/access/IAccessControlUpgradeable.sol"; - -interface IUpgradeExecutor is IAccessControlUpgradeable { - function execute(address upgrade, bytes memory upgradeCallData) external; - function ADMIN_ROLE() external returns (bytes32); - function EXECUTOR_ROLE() external returns (bytes32); -} diff --git a/src/security-council-mgmt/SecurityCouncilManager.sol b/src/security-council-mgmt/SecurityCouncilManager.sol index fb03bf742..61dcb7e0d 100644 --- a/src/security-council-mgmt/SecurityCouncilManager.sol +++ b/src/security-council-mgmt/SecurityCouncilManager.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "../ArbitrumTimelock.sol"; -import "../UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; import "../L1ArbitrumTimelock.sol"; import "./SecurityCouncilMgmtUtils.sol"; import "./interfaces/ISecurityCouncilManager.sol"; diff --git a/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol b/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol index 768792d3c..88c9abe05 100644 --- a/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol +++ b/src/security-council-mgmt/governors/SecurityCouncilMemberElectionGovernor.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.16; import "@openzeppelin/contracts-upgradeable/governance/extensions/GovernorVotesUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/governance/extensions/GovernorSettingsUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; import "./modules/SecurityCouncilMemberElectionGovernorCountingUpgradeable.sol"; import "../interfaces/ISecurityCouncilMemberElectionGovernor.sol"; import "../interfaces/ISecurityCouncilNomineeElectionGovernor.sol"; diff --git a/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol b/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol index 3c14d7445..b4a60056a 100644 --- a/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol +++ b/src/security-council-mgmt/governors/SecurityCouncilMemberRemovalGovernor.sol @@ -7,6 +7,7 @@ import "@openzeppelin/contracts-upgradeable/governance/extensions/GovernorCountingSimpleUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/governance/extensions/GovernorSettingsUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; import "./../interfaces/ISecurityCouncilManager.sol"; import "../Common.sol"; import "./modules/ArbitrumGovernorVotesQuorumFractionUpgradeable.sol"; diff --git a/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol b/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol index e1eba1a96..1cc673182 100644 --- a/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol +++ b/src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.16; import "@openzeppelin/contracts-upgradeable/governance/extensions/GovernorSettingsUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import "@openzeppelin/contracts/utils/Address.sol"; import "../interfaces/ISecurityCouncilMemberElectionGovernor.sol"; import "../interfaces/ISecurityCouncilNomineeElectionGovernor.sol"; import "./modules/SecurityCouncilNomineeElectionGovernorCountingUpgradeable.sol"; diff --git a/test/L2GovernanceFactory.t.sol b/test/L2GovernanceFactory.t.sol index 13ad94051..6cbf64687 100644 --- a/test/L2GovernanceFactory.t.sol +++ b/test/L2GovernanceFactory.t.sol @@ -3,7 +3,7 @@ import "../src/L2GovernanceFactory.sol"; import "../src/L2ArbitrumGovernor.sol"; -import "../src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "../src/ArbitrumTimelock.sol"; import "../src/ArbitrumDAOConstitution.sol"; diff --git a/test/UpgradeExecutor.t.sol b/test/UpgradeExecutor.t.sol deleted file mode 100644 index f27e8047c..000000000 --- a/test/UpgradeExecutor.t.sol +++ /dev/null @@ -1,145 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -pragma solidity 0.8.16; - -import "../src/UpgradeExecutor.sol"; -import "./util/TestUtil.sol"; -import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; - -import "forge-std/Test.sol"; - -contract Setter { - uint256 public val = 0; - address public lastSender; - - function setVal(uint256 _val) public { - val = _val; - lastSender = msg.sender; - } -} - -contract SetterUpgrade { - function upgrade(address setter, uint256 val) public { - Setter(setter).setVal(val); - } -} - -contract AccessControlUpgrader { - function grantRole(address target, bytes32 role, address account) public { - AccessControlUpgradeable(target).grantRole(role, account); - } -} - -contract UpgradeExecutorTest is Test { - address executor0 = address(138); - address executor1 = address(139); - address nobody = address(140); - address executor2 = address(141); - - function deployAndInit() internal returns (UpgradeExecutor) { - UpgradeExecutor ue = UpgradeExecutor(TestUtil.deployProxy(address(new UpgradeExecutor()))); - address[] memory executors = new address[](2); - executors[0] = executor0; - executors[1] = executor1; - ue.initialize(address(ue), executors); - return ue; - } - - function testInit() external { - UpgradeExecutor ue = deployAndInit(); - - assertEq(ue.hasRole(ue.EXECUTOR_ROLE(), executor0), true, "Executor 0"); - assertEq(ue.hasRole(ue.EXECUTOR_ROLE(), executor1), true, "Executor 1"); - assertEq(ue.hasRole(ue.ADMIN_ROLE(), address(ue)), true, "Executor 1"); - assertEq(ue.getRoleAdmin(ue.ADMIN_ROLE()), ue.ADMIN_ROLE(), "admin admin"); - assertEq(ue.getRoleAdmin(ue.EXECUTOR_ROLE()), ue.ADMIN_ROLE(), "executor admin"); - } - - function testInitFailsZeroAdmin() external { - UpgradeExecutor ue = UpgradeExecutor(TestUtil.deployProxy(address(new UpgradeExecutor()))); - address[] memory executors = new address[](2); - executors[0] = executor0; - executors[1] = executor1; - - vm.expectRevert("UpgradeExecutor: zero admin"); - ue.initialize(address(0), executors); - } - - function testExecute() external { - UpgradeExecutor ue = deployAndInit(); - Setter setter = new Setter(); - SetterUpgrade se = new SetterUpgrade(); - - uint256 val = 25; - bytes memory data = abi.encodeWithSelector(se.upgrade.selector, address(setter), val); - - assertEq(setter.val(), 0, "Val before"); - assertEq(setter.lastSender(), address(0), "Sender before"); - - vm.prank(executor0); - ue.execute(address(se), data); - - assertEq(setter.val(), val, "Val after"); - assertEq(setter.lastSender(), address(ue), "Sender after"); - } - - function testCantExecuteEOA() external { - UpgradeExecutor ue = deployAndInit(); - bytes memory data; - - vm.prank(executor0); - vm.expectRevert("Address: delegate call to non-contract"); - ue.execute(address(111), data); - } - - function roleError(address account, bytes32 role) internal pure returns (string memory) { - return string( - abi.encodePacked( - "AccessControl: account ", - StringsUpgradeable.toHexString(uint160(account), 20), - " is missing role ", - StringsUpgradeable.toHexString(uint256(role), 32) - ) - ); - } - - function testExecuteFailsForAdmin() external { - UpgradeExecutor ue = deployAndInit(); - Setter setter = new Setter(); - SetterUpgrade se = new SetterUpgrade(); - - uint256 val = 25; - bytes memory data = abi.encodeWithSelector(se.upgrade.selector, address(setter), val); - - vm.expectRevert(bytes(roleError(address(ue), ue.EXECUTOR_ROLE()))); - vm.prank(address(ue)); - ue.execute(address(se), data); - } - - function testExecuteFailsForNobody() external { - UpgradeExecutor ue = deployAndInit(); - Setter setter = new Setter(); - SetterUpgrade se = new SetterUpgrade(); - - uint256 val = 25; - bytes memory data = abi.encodeWithSelector(se.upgrade.selector, address(setter), val); - - vm.expectRevert(bytes(roleError(nobody, ue.EXECUTOR_ROLE()))); - vm.prank(nobody); - ue.execute(address(se), data); - } - - function testAdminCanChangeExecutor() external { - UpgradeExecutor ue = deployAndInit(); - AccessControlUpgrader ae = new AccessControlUpgrader(); - - bytes memory data = abi.encodeWithSelector( - ae.grantRole.selector, address(ue), ue.EXECUTOR_ROLE(), executor2 - ); - - assertEq(ue.hasRole(ue.EXECUTOR_ROLE(), executor2), false, "executor 2 before"); - vm.prank(executor1); - ue.execute(address(ae), data); - - assertEq(ue.hasRole(ue.EXECUTOR_ROLE(), executor2), true, "executor 2 before"); - } -} diff --git a/test/gov-actions/AIPNovaFeeRoutingAction.t.sol b/test/gov-actions/AIPNovaFeeRoutingAction.t.sol index 93b79753d..a1b09d66e 100644 --- a/test/gov-actions/AIPNovaFeeRoutingAction.t.sol +++ b/test/gov-actions/AIPNovaFeeRoutingAction.t.sol @@ -6,7 +6,7 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; import "../../src/gov-action-contracts/AIPs/AIPNovaFeeRoutingAction.sol"; -import "../../src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; contract AIPNovaFeeRoutingActionTest is Test { UpgradeExecutor constant upExec = UpgradeExecutor(0x86a02dD71363c440b21F4c0E5B2Ad01Ffe1A7482); diff --git a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol index d4e5fdf34..cfe511da2 100644 --- a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol +++ b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol @@ -5,6 +5,7 @@ import "forge-std/Test.sol"; import "../../src/gov-action-contracts/AIPs/NomineeGovernorV2UpgradeAction.sol"; import "../../src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; contract NomineeGovernorV2UpgradeActionTest is Test { address oldImplementation = 0x8436A1bc9f9f9EB0cF1B51942C5657b60A40CCDD; diff --git a/test/gov-actions/ProxyAdminUpgrader.t.sol b/test/gov-actions/ProxyAdminUpgrader.t.sol index 39b3f4244..605ef39b0 100644 --- a/test/gov-actions/ProxyAdminUpgrader.t.sol +++ b/test/gov-actions/ProxyAdminUpgrader.t.sol @@ -4,7 +4,7 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; import "../util/TestUtil.sol"; -import "../../src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "../../src/gov-action-contracts/gov-upgrade-contracts/upgrade-proxy/ProxyUpgradeAndCallAction.sol"; import "../../src/gov-action-contracts/gov-upgrade-contracts/upgrade-proxy/ProxyUpgradeAction.sol"; diff --git a/test/gov-actions/SwitchManagerRolesAction.t.sol b/test/gov-actions/SwitchManagerRolesAction.t.sol index d36021385..b6264ff8a 100644 --- a/test/gov-actions/SwitchManagerRolesAction.t.sol +++ b/test/gov-actions/SwitchManagerRolesAction.t.sol @@ -4,6 +4,7 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; import "../../src/gov-action-contracts/nonemergency/SwitchManagerRolesAction.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; contract SwitchManagerRolesActionTest is Test { UpgradeExecutor arbOneUe = UpgradeExecutor(0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827); diff --git a/test/security-council-mgmt/E2E.t.sol b/test/security-council-mgmt/E2E.t.sol index d53809430..b0dfb760d 100644 --- a/test/security-council-mgmt/E2E.t.sol +++ b/test/security-council-mgmt/E2E.t.sol @@ -3,7 +3,6 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; -import "../../src/UpgradeExecutor.sol"; import "../../src/ArbitrumTimelock.sol"; import "../../src/L2ArbitrumGovernor.sol"; import "../../src/FixedDelegateErc20Wallet.sol"; @@ -404,7 +403,7 @@ contract E2E is Test, DeployGnosisWithModule { IGnosisSafe(address(vars.moduleL1Safe)), IGnosisSafe(l1EmergencyCouncil), secCouncilThreshold, - IUpgradeExecutor(address(vars.l1Executor)), + UpgradeExecutor(address(vars.l1Executor)), ICoreTimelock(address(vars.l1Timelock)) ); diff --git a/test/security-council-mgmt/SecurityCouncilMemberSyncAction.t.sol b/test/security-council-mgmt/SecurityCouncilMemberSyncAction.t.sol index 1f9414743..710ceffb4 100644 --- a/test/security-council-mgmt/SecurityCouncilMemberSyncAction.t.sol +++ b/test/security-council-mgmt/SecurityCouncilMemberSyncAction.t.sol @@ -5,7 +5,7 @@ pragma solidity 0.8.16; import "@gnosis.pm/safe-contracts/contracts/GnosisSafeL2.sol"; import "../util/TestUtil.sol"; import "../util/DeployGnosisWithModule.sol"; -import "../../src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "../../src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol"; import "../../src/security-council-mgmt/interfaces/IGnosisSafe.sol"; diff --git a/test/security-council-mgmt/SecurityCouncilUpgradeAction.t.sol b/test/security-council-mgmt/SecurityCouncilUpgradeAction.t.sol index aa67cd967..cdd397709 100644 --- a/test/security-council-mgmt/SecurityCouncilUpgradeAction.t.sol +++ b/test/security-council-mgmt/SecurityCouncilUpgradeAction.t.sol @@ -5,7 +5,7 @@ pragma solidity 0.8.16; import "@gnosis.pm/safe-contracts/contracts/GnosisSafeL2.sol"; import "../util/TestUtil.sol"; import "../util/DeployGnosisWithModule.sol"; -import "../../src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "../../src/security-council-mgmt/SecurityCouncilMemberSyncAction.sol"; import "../../src/security-council-mgmt/interfaces/IGnosisSafe.sol"; diff --git a/test/util/ActionTestBase.sol b/test/util/ActionTestBase.sol index d82c7aa8e..5be3c1837 100644 --- a/test/util/ActionTestBase.sol +++ b/test/util/ActionTestBase.sol @@ -15,7 +15,7 @@ import "../../src/ArbitrumTimelock.sol"; import "../../src/L1ArbitrumTimelock.sol"; import "../../src/FixedDelegateErc20Wallet.sol"; import "../util/TestUtil.sol"; -import "../../src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "../../src/ArbitrumDAOConstitution.sol"; import "../../src/gov-action-contracts/address-registries/L1AddressRegistry.sol" as _ar; import "../../src/gov-action-contracts/address-registries/L2AddressRegistry.sol" as _ar1; From d74a6d9373506202b42d4e3bdbff58d1fe0d6bfa Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:40:01 -0400 Subject: [PATCH 02/16] add UpgradeExecutorUpgradeAction --- .../UpgradeExecutorUpgradeAction.sol | 53 +++++++++++++++++++ .../UpgradeExecutorUpgradeAction.t.sol | 51 ++++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol create mode 100644 test/gov-actions/UpgradeExecutorUpgradeAction.t.sol diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol new file mode 100644 index 000000000..798df3cd4 --- /dev/null +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity 0.8.16; + +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; + +import { + ProxyAdmin, + TransparentUpgradeableProxy +} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; + +contract UpgradeExecutorUpgradeAction { + address public immutable newUpgradeExecutorImplementation; + ProxyAdmin public immutable proxyAdmin; + + constructor(ProxyAdmin _proxyAdmin) { + proxyAdmin = _proxyAdmin; + newUpgradeExecutorImplementation = address(new UpgradeExecutor()); + } + + function perform() external { + TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(payable(address(this))); + + ProxyAdmin(proxyAdmin).upgrade(proxy, newUpgradeExecutorImplementation); + + require( + ProxyAdmin(proxyAdmin).getProxyImplementation(proxy) == newUpgradeExecutorImplementation, + "UpgradeExecutorUpgradeAction: upgrade failed" + ); + } +} + +// Proxy Admins: +// Arb1: 0xdb216562328215E010F819B5aBe947bad4ca961e +// Nova: 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9 +// L1: 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678 + +contract ArbOneUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { + constructor() + UpgradeExecutorUpgradeAction(ProxyAdmin(0xdb216562328215E010F819B5aBe947bad4ca961e)) + {} +} + +contract NovaUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { + constructor() + UpgradeExecutorUpgradeAction(ProxyAdmin(0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9)) + {} +} + +contract L1UpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { + constructor() + UpgradeExecutorUpgradeAction(ProxyAdmin(0x5613AF0474EB9c528A34701A5b1662E3C8FA0678)) + {} +} diff --git a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol new file mode 100644 index 000000000..c89a8d7ab --- /dev/null +++ b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: Apache-2.0 +pragma solidity 0.8.16; + +import "forge-std/Test.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; + +import "src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol"; + +contract UpgradeExecutorUpgradeActionTest is Test { + function testArbOne() external { + vm.createSelectFork(vm.envString("ARB_URL")); + _testUpgrade( + new ArbOneUpgradeExecutorUpgradeAction(), + UpgradeExecutor(0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827), + 0xf7951D92B0C345144506576eC13Ecf5103aC905a + ); + } + + function testNova() external { + vm.createSelectFork(vm.envString("NOVA_URL")); + _testUpgrade( + new NovaUpgradeExecutorUpgradeAction(), + UpgradeExecutor(0x86a02dD71363c440b21F4c0E5B2Ad01Ffe1A7482), + 0xf7951D92B0C345144506576eC13Ecf5103aC905a + ); + } + + function testL1() external { + vm.createSelectFork(vm.envString("ETH_URL")); + _testUpgrade( + new L1UpgradeExecutorUpgradeAction(), + UpgradeExecutor(0x3ffFbAdAF827559da092217e474760E2b2c3CeDd), + 0xE6841D92B0C345144506576eC13ECf5103aC7f49 + ); + } + + function _testUpgrade( + UpgradeExecutorUpgradeAction action, + UpgradeExecutor ue, + address executor + ) internal { + vm.prank(executor); + ue.execute(address(action), abi.encodeWithSignature("perform()")); + + assertTrue( + ProxyAdmin(action.proxyAdmin()).getProxyImplementation( + TransparentUpgradeableProxy(payable(address(ue))) + ) == action.newUpgradeExecutorImplementation() + ); + } +} From cc2a6a021ef0b2e937b9775886b5e686c6b4b5b7 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:43:33 -0400 Subject: [PATCH 03/16] print storage --- test/storage/UpgradeExecutor | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/storage/UpgradeExecutor b/test/storage/UpgradeExecutor index 85636f270..28cb3be09 100644 --- a/test/storage/UpgradeExecutor +++ b/test/storage/UpgradeExecutor @@ -1,9 +1,9 @@ -| Name | Type | Slot | Offset | Bytes | Contract | -|---------------|--------------------------------------------------------------|------|--------|-------|-----------------------------------------| -| _initialized | uint8 | 0 | 0 | 1 | src/UpgradeExecutor.sol:UpgradeExecutor | -| _initializing | bool | 0 | 1 | 1 | src/UpgradeExecutor.sol:UpgradeExecutor | -| __gap | uint256[50] | 1 | 0 | 1600 | src/UpgradeExecutor.sol:UpgradeExecutor | -| __gap | uint256[50] | 51 | 0 | 1600 | src/UpgradeExecutor.sol:UpgradeExecutor | -| _roles | mapping(bytes32 => struct AccessControlUpgradeable.RoleData) | 101 | 0 | 32 | src/UpgradeExecutor.sol:UpgradeExecutor | -| __gap | uint256[49] | 102 | 0 | 1568 | src/UpgradeExecutor.sol:UpgradeExecutor | -| _status | uint256 | 151 | 0 | 32 | src/UpgradeExecutor.sol:UpgradeExecutor | +| Name | Type | Slot | Offset | Bytes | Contract | +|---------------|--------------------------------------------------------------|------|--------|-------|-------------------------------------------------------------------------------------| +| _initialized | uint8 | 0 | 0 | 1 | node_modules/@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor | +| _initializing | bool | 0 | 1 | 1 | node_modules/@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor | +| __gap | uint256[50] | 1 | 0 | 1600 | node_modules/@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor | +| __gap | uint256[50] | 51 | 0 | 1600 | node_modules/@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor | +| _roles | mapping(bytes32 => struct AccessControlUpgradeable.RoleData) | 101 | 0 | 32 | node_modules/@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor | +| __gap | uint256[49] | 102 | 0 | 1568 | node_modules/@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor | +| _status | uint256 | 151 | 0 | 32 | node_modules/@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol:UpgradeExecutor | From 90fb0bc329c32656af3843d5f9ee7746da79b64d Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:54:23 -0400 Subject: [PATCH 04/16] get admin at perform time --- .../UpgradeExecutorUpgradeAction.sol | 34 ++++--------------- .../UpgradeExecutorUpgradeAction.t.sol | 21 ++++++------ 2 files changed, 17 insertions(+), 38 deletions(-) diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol index 798df3cd4..abb75b50b 100644 --- a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -8,16 +8,17 @@ import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; -contract UpgradeExecutorUpgradeAction { +import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Upgrade.sol"; + +contract UpgradeExecutorUpgradeAction is ERC1967Upgrade { address public immutable newUpgradeExecutorImplementation; - ProxyAdmin public immutable proxyAdmin; - constructor(ProxyAdmin _proxyAdmin) { - proxyAdmin = _proxyAdmin; + constructor() { newUpgradeExecutorImplementation = address(new UpgradeExecutor()); } function perform() external { + ProxyAdmin proxyAdmin = ProxyAdmin(_getAdmin()); TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(payable(address(this))); ProxyAdmin(proxyAdmin).upgrade(proxy, newUpgradeExecutorImplementation); @@ -27,27 +28,4 @@ contract UpgradeExecutorUpgradeAction { "UpgradeExecutorUpgradeAction: upgrade failed" ); } -} - -// Proxy Admins: -// Arb1: 0xdb216562328215E010F819B5aBe947bad4ca961e -// Nova: 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9 -// L1: 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678 - -contract ArbOneUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() - UpgradeExecutorUpgradeAction(ProxyAdmin(0xdb216562328215E010F819B5aBe947bad4ca961e)) - {} -} - -contract NovaUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() - UpgradeExecutorUpgradeAction(ProxyAdmin(0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9)) - {} -} - -contract L1UpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() - UpgradeExecutorUpgradeAction(ProxyAdmin(0x5613AF0474EB9c528A34701A5b1662E3C8FA0678)) - {} -} +} \ No newline at end of file diff --git a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol index c89a8d7ab..cc7dd1259 100644 --- a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol +++ b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol @@ -10,8 +10,8 @@ contract UpgradeExecutorUpgradeActionTest is Test { function testArbOne() external { vm.createSelectFork(vm.envString("ARB_URL")); _testUpgrade( - new ArbOneUpgradeExecutorUpgradeAction(), - UpgradeExecutor(0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827), + 0xdb216562328215E010F819B5aBe947bad4ca961e, + 0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827, 0xf7951D92B0C345144506576eC13Ecf5103aC905a ); } @@ -19,8 +19,8 @@ contract UpgradeExecutorUpgradeActionTest is Test { function testNova() external { vm.createSelectFork(vm.envString("NOVA_URL")); _testUpgrade( - new NovaUpgradeExecutorUpgradeAction(), - UpgradeExecutor(0x86a02dD71363c440b21F4c0E5B2Ad01Ffe1A7482), + 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9, + 0x86a02dD71363c440b21F4c0E5B2Ad01Ffe1A7482, 0xf7951D92B0C345144506576eC13Ecf5103aC905a ); } @@ -28,22 +28,23 @@ contract UpgradeExecutorUpgradeActionTest is Test { function testL1() external { vm.createSelectFork(vm.envString("ETH_URL")); _testUpgrade( - new L1UpgradeExecutorUpgradeAction(), - UpgradeExecutor(0x3ffFbAdAF827559da092217e474760E2b2c3CeDd), + 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678, + 0x3ffFbAdAF827559da092217e474760E2b2c3CeDd, 0xE6841D92B0C345144506576eC13ECf5103aC7f49 ); } function _testUpgrade( - UpgradeExecutorUpgradeAction action, - UpgradeExecutor ue, + address admin, + address ue, address executor ) internal { + UpgradeExecutorUpgradeAction action = new UpgradeExecutorUpgradeAction(); vm.prank(executor); - ue.execute(address(action), abi.encodeWithSignature("perform()")); + UpgradeExecutor(ue).execute(address(action), abi.encodeWithSignature("perform()")); assertTrue( - ProxyAdmin(action.proxyAdmin()).getProxyImplementation( + ProxyAdmin(admin).getProxyImplementation( TransparentUpgradeableProxy(payable(address(ue))) ) == action.newUpgradeExecutorImplementation() ); From 557b6c4af1eca17bfa1cb9239e7908451059f7e3 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Wed, 16 Oct 2024 14:56:29 -0400 Subject: [PATCH 05/16] simplify --- .../UpgradeExecutorUpgradeAction.sol | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol index abb75b50b..b24160799 100644 --- a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -1,21 +1,17 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity 0.8.16; -import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; +import {UpgradeExecutor} from "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import { ProxyAdmin, TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; -import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Upgrade.sol"; +import {ERC1967Upgrade} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Upgrade.sol"; contract UpgradeExecutorUpgradeAction is ERC1967Upgrade { - address public immutable newUpgradeExecutorImplementation; - - constructor() { - newUpgradeExecutorImplementation = address(new UpgradeExecutor()); - } + address public immutable newUpgradeExecutorImplementation = address(new UpgradeExecutor()); function perform() external { ProxyAdmin proxyAdmin = ProxyAdmin(_getAdmin()); @@ -28,4 +24,4 @@ contract UpgradeExecutorUpgradeAction is ERC1967Upgrade { "UpgradeExecutorUpgradeAction: upgrade failed" ); } -} \ No newline at end of file +} From 773bed663f80c2ea2817948a0213e179d7017524 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Wed, 16 Oct 2024 15:14:58 -0400 Subject: [PATCH 06/16] even simpler --- .../UpgradeExecutorUpgradeAction.sol | 14 ++------------ .../gov-actions/UpgradeExecutorUpgradeAction.t.sol | 5 +++++ 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol index b24160799..a97a73ce6 100644 --- a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -2,25 +2,15 @@ pragma solidity 0.8.16; import {UpgradeExecutor} from "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; - -import { - ProxyAdmin, - TransparentUpgradeableProxy -} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; - import {ERC1967Upgrade} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Upgrade.sol"; contract UpgradeExecutorUpgradeAction is ERC1967Upgrade { address public immutable newUpgradeExecutorImplementation = address(new UpgradeExecutor()); function perform() external { - ProxyAdmin proxyAdmin = ProxyAdmin(_getAdmin()); - TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(payable(address(this))); - - ProxyAdmin(proxyAdmin).upgrade(proxy, newUpgradeExecutorImplementation); - + _upgradeTo(newUpgradeExecutorImplementation); require( - ProxyAdmin(proxyAdmin).getProxyImplementation(proxy) == newUpgradeExecutorImplementation, + _getImplementation() == newUpgradeExecutorImplementation, "UpgradeExecutorUpgradeAction: upgrade failed" ); } diff --git a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol index cc7dd1259..f77e84887 100644 --- a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol +++ b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol @@ -4,6 +4,11 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; +import { + ProxyAdmin, + TransparentUpgradeableProxy +} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; + import "src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol"; contract UpgradeExecutorUpgradeActionTest is Test { From 3a902bf49e9c74c7a194073452174dbec369e604 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Fri, 18 Oct 2024 12:46:40 -0400 Subject: [PATCH 07/16] use latest package --- package.json | 2 +- yarn.lock | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 5a5a4aea8..940763f1c 100644 --- a/package.json +++ b/package.json @@ -79,7 +79,7 @@ "@arbitrum/nitro-contracts": "1.1.1", "@arbitrum/token-bridge-contracts": "1.0.0-beta.0", "@gnosis.pm/safe-contracts": "1.3.0", - "@offchainlabs/upgrade-executor": "1.1.0-beta.0", + "@offchainlabs/upgrade-executor": "1.1.1", "@openzeppelin/contracts": "4.7.3", "@openzeppelin/contracts-upgradeable": "4.7.3", "@types/yargs": "^17.0.17", diff --git a/yarn.lock b/yarn.lock index d4ee3322d..0a5463e6e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -939,6 +939,14 @@ "@openzeppelin/contracts" "4.7.3" "@openzeppelin/contracts-upgradeable" "4.7.3" +"@offchainlabs/upgrade-executor@1.1.1": + version "1.1.1" + resolved "https://registry.yarnpkg.com/@offchainlabs/upgrade-executor/-/upgrade-executor-1.1.1.tgz#ae3dfe4a183d88c0c3fd3b39ab6dd6508b371b53" + integrity sha512-/hnUblMbzXS4asPdTSWjFUn/N5+E71dVfDW1314j/r/847OIy1VHZDFhw+fOlUGL5/qrZu2UV34oZAwwrRV4tg== + dependencies: + "@openzeppelin/contracts" "4.7.3" + "@openzeppelin/contracts-upgradeable" "4.7.3" + "@openzeppelin/contracts-upgradeable@3.4.2": version "3.4.2" resolved "https://registry.npmjs.org/@openzeppelin/contracts-upgradeable/-/contracts-upgradeable-3.4.2.tgz" From 97e31034ea07136607fd64b4ad18441064733200 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Fri, 18 Oct 2024 12:47:50 -0400 Subject: [PATCH 08/16] use explicit block nums --- test/gov-actions/UpgradeExecutorUpgradeAction.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol index f77e84887..18bc1ee7f 100644 --- a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol +++ b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol @@ -13,7 +13,7 @@ import "src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUp contract UpgradeExecutorUpgradeActionTest is Test { function testArbOne() external { - vm.createSelectFork(vm.envString("ARB_URL")); + vm.createSelectFork(vm.envString("ARB_URL"), 265159958); _testUpgrade( 0xdb216562328215E010F819B5aBe947bad4ca961e, 0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827, @@ -22,7 +22,7 @@ contract UpgradeExecutorUpgradeActionTest is Test { } function testNova() external { - vm.createSelectFork(vm.envString("NOVA_URL")); + vm.createSelectFork(vm.envString("NOVA_URL"), 78263024); _testUpgrade( 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9, 0x86a02dD71363c440b21F4c0E5B2Ad01Ffe1A7482, @@ -31,7 +31,7 @@ contract UpgradeExecutorUpgradeActionTest is Test { } function testL1() external { - vm.createSelectFork(vm.envString("ETH_URL")); + vm.createSelectFork(vm.envString("ETH_URL"), 20993735); _testUpgrade( 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678, 0x3ffFbAdAF827559da092217e474760E2b2c3CeDd, From cbc5c7df114d76435ac4dfec4dd10f7c20efcc60 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Mon, 21 Oct 2024 09:37:17 -0400 Subject: [PATCH 09/16] use interface where it was used before --- src/UpgradeExecRouteBuilder.sol | 4 ++-- .../GovernanceChainSCMgmtActivationAction.sol | 9 +++++---- .../L1SCMgmtActivationAction.sol | 4 ++-- .../NonGovernanceChainSCMgmtActivationAction.sol | 8 ++++---- .../SecurityCouncilMgmtUpgradeLib.sol | 13 +++++++------ test/security-council-mgmt/E2E.t.sol | 2 +- 6 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/UpgradeExecRouteBuilder.sol b/src/UpgradeExecRouteBuilder.sol index 0bd00e903..679111f86 100644 --- a/src/UpgradeExecRouteBuilder.sol +++ b/src/UpgradeExecRouteBuilder.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol"; -import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "./L1ArbitrumTimelock.sol"; import "./security-council-mgmt/Common.sol"; @@ -136,7 +136,7 @@ contract UpgradeExecRouteBuilder { } bytes memory executorData = abi.encodeWithSelector( - IUpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] + UpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] ); // for L1, inbox is set to address(0): diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol index 202a606f7..4c0647809 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/GovernanceChainSCMgmtActivationAction.sol @@ -5,9 +5,10 @@ import "../../../security-council-mgmt/interfaces/IGnosisSafe.sol"; import "../../address-registries/L2AddressRegistryInterfaces.sol"; import "./SecurityCouncilMgmtUpgradeLib.sol"; import "../../../interfaces/IArbitrumDAOConstitution.sol"; -import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; import "../../../interfaces/ICoreTimelock.sol"; import "@openzeppelin/contracts/utils/Address.sol"; +import "@openzeppelin/contracts/access/IAccessControl.sol"; contract GovernanceChainSCMgmtActivationAction { IGnosisSafe public immutable newEmergencySecurityCouncil; @@ -49,7 +50,7 @@ contract GovernanceChainSCMgmtActivationAction { } function perform() external { - UpgradeExecutor upgradeExecutor = UpgradeExecutor(l2AddressRegistry.coreGov().owner()); + IUpgradeExecutor upgradeExecutor = IUpgradeExecutor(l2AddressRegistry.coreGov().owner()); // swap in new emergency security council SecurityCouncilMgmtUpgradeLib.replaceEmergencySecurityCouncil({ @@ -116,11 +117,11 @@ contract GovernanceChainSCMgmtActivationAction { // confirm updates bytes32 EXECUTOR_ROLE = upgradeExecutor.EXECUTOR_ROLE(); require( - upgradeExecutor.hasRole(EXECUTOR_ROLE, address(newEmergencySecurityCouncil)), + IAccessControl(address(upgradeExecutor)).hasRole(EXECUTOR_ROLE, address(newEmergencySecurityCouncil)), "NonGovernanceChainSCMgmtActivationAction: new emergency security council not set" ); require( - !upgradeExecutor.hasRole(EXECUTOR_ROLE, address(prevEmergencySecurityCouncil)), + !IAccessControl(address(upgradeExecutor)).hasRole(EXECUTOR_ROLE, address(prevEmergencySecurityCouncil)), "NonGovernanceChainSCMgmtActivationAction: prev emergency security council still set" ); diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol index 58c004740..e1cebc91f 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/L1SCMgmtActivationAction.sol @@ -10,14 +10,14 @@ contract L1SCMgmtActivationAction { IGnosisSafe public immutable newEmergencySecurityCouncil; IGnosisSafe public immutable prevEmergencySecurityCouncil; uint256 public immutable emergencySecurityCouncilThreshold; - UpgradeExecutor public immutable l1UpgradeExecutor; + IUpgradeExecutor public immutable l1UpgradeExecutor; ICoreTimelock public immutable l1Timelock; constructor( IGnosisSafe _newEmergencySecurityCouncil, IGnosisSafe _prevEmergencySecurityCouncil, uint256 _emergencySecurityCouncilThreshold, - UpgradeExecutor _l1UpgradeExecutor, + IUpgradeExecutor _l1UpgradeExecutor, ICoreTimelock _l1Timelock ) { newEmergencySecurityCouncil = _newEmergencySecurityCouncil; diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol index e5feeb16e..9004ebbbb 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/NonGovernanceChainSCMgmtActivationAction.sol @@ -8,13 +8,13 @@ contract NonGovernanceChainSCMgmtActivationAction { IGnosisSafe public immutable newEmergencySecurityCouncil; IGnosisSafe public immutable prevEmergencySecurityCouncil; uint256 public immutable emergencySecurityCouncilThreshold; - UpgradeExecutor public immutable upgradeExecutor; + IUpgradeExecutor public immutable upgradeExecutor; constructor( IGnosisSafe _newEmergencySecurityCouncil, IGnosisSafe _prevEmergencySecurityCouncil, uint256 _emergencySecurityCouncilThreshold, - UpgradeExecutor _upgradeExecutor + IUpgradeExecutor _upgradeExecutor ) { newEmergencySecurityCouncil = _newEmergencySecurityCouncil; prevEmergencySecurityCouncil = _prevEmergencySecurityCouncil; @@ -34,11 +34,11 @@ contract NonGovernanceChainSCMgmtActivationAction { // confirm updates bytes32 EXECUTOR_ROLE = upgradeExecutor.EXECUTOR_ROLE(); require( - upgradeExecutor.hasRole(EXECUTOR_ROLE, address(newEmergencySecurityCouncil)), + IAccessControl(address(upgradeExecutor)).hasRole(EXECUTOR_ROLE, address(newEmergencySecurityCouncil)), "NonGovernanceChainSCMgmtActivationAction: new emergency security council not set" ); require( - !upgradeExecutor.hasRole(EXECUTOR_ROLE, address(prevEmergencySecurityCouncil)), + !IAccessControl(address(upgradeExecutor)).hasRole(EXECUTOR_ROLE, address(prevEmergencySecurityCouncil)), "NonGovernanceChainSCMgmtActivationAction: prev emergency security council still set" ); } diff --git a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol index 958e86ae0..95ab0db95 100644 --- a/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol +++ b/src/gov-action-contracts/AIPs/SecurityCouncilMgmt/SecurityCouncilMgmtUpgradeLib.sol @@ -2,28 +2,29 @@ pragma solidity 0.8.16; import "../../../security-council-mgmt/interfaces/IGnosisSafe.sol"; -import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; +import "@openzeppelin/contracts/access/IAccessControl.sol"; library SecurityCouncilMgmtUpgradeLib { function replaceEmergencySecurityCouncil( IGnosisSafe _prevSecurityCouncil, IGnosisSafe _newSecurityCouncil, uint256 _threshold, - UpgradeExecutor _upgradeExecutor + IUpgradeExecutor _upgradeExecutor ) internal { requireSafesEquivalent(_prevSecurityCouncil, _newSecurityCouncil, _threshold); bytes32 EXECUTOR_ROLE = _upgradeExecutor.EXECUTOR_ROLE(); require( - _upgradeExecutor.hasRole(EXECUTOR_ROLE, address(_prevSecurityCouncil)), + IAccessControl(address(_upgradeExecutor)).hasRole(EXECUTOR_ROLE, address(_prevSecurityCouncil)), "SecurityCouncilMgmtUpgradeLib: prev council not executor" ); require( - !_upgradeExecutor.hasRole(EXECUTOR_ROLE, address(_newSecurityCouncil)), + !IAccessControl(address(_upgradeExecutor)).hasRole(EXECUTOR_ROLE, address(_newSecurityCouncil)), "SecurityCouncilMgmtUpgradeLib: new council already executor" ); - _upgradeExecutor.revokeRole(EXECUTOR_ROLE, address(_prevSecurityCouncil)); - _upgradeExecutor.grantRole(EXECUTOR_ROLE, address(_newSecurityCouncil)); + IAccessControl(address(_upgradeExecutor)).revokeRole(EXECUTOR_ROLE, address(_prevSecurityCouncil)); + IAccessControl(address(_upgradeExecutor)).grantRole(EXECUTOR_ROLE, address(_newSecurityCouncil)); } function requireSafesEquivalent( diff --git a/test/security-council-mgmt/E2E.t.sol b/test/security-council-mgmt/E2E.t.sol index b0dfb760d..4933cecc9 100644 --- a/test/security-council-mgmt/E2E.t.sol +++ b/test/security-council-mgmt/E2E.t.sol @@ -403,7 +403,7 @@ contract E2E is Test, DeployGnosisWithModule { IGnosisSafe(address(vars.moduleL1Safe)), IGnosisSafe(l1EmergencyCouncil), secCouncilThreshold, - UpgradeExecutor(address(vars.l1Executor)), + IUpgradeExecutor(address(vars.l1Executor)), ICoreTimelock(address(vars.l1Timelock)) ); From 98638327898efc2ea2620dbcb12d724bc5f652d7 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Mon, 21 Oct 2024 09:43:50 -0400 Subject: [PATCH 10/16] go thru admin --- .../UpgradeExecutorUpgradeAction.sol | 45 ++++++++++++++++--- .../UpgradeExecutorUpgradeAction.t.sol | 16 +++---- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol index a97a73ce6..94e55b5d7 100644 --- a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -2,16 +2,51 @@ pragma solidity 0.8.16; import {UpgradeExecutor} from "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; -import {ERC1967Upgrade} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Upgrade.sol"; +import { + ProxyAdmin, + TransparentUpgradeableProxy +} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; -contract UpgradeExecutorUpgradeAction is ERC1967Upgrade { - address public immutable newUpgradeExecutorImplementation = address(new UpgradeExecutor()); +contract UpgradeExecutorUpgradeAction { + address public immutable newUpgradeExecutorImplementation; + ProxyAdmin public immutable proxyAdmin; + + constructor(address _proxyAdmin) { + proxyAdmin = ProxyAdmin(_proxyAdmin); + newUpgradeExecutorImplementation = address(new UpgradeExecutor()); + } function perform() external { - _upgradeTo(newUpgradeExecutorImplementation); + TransparentUpgradeableProxy proxy = TransparentUpgradeableProxy(payable(address(this))); + + proxyAdmin.upgrade(proxy, newUpgradeExecutorImplementation); + require( - _getImplementation() == newUpgradeExecutorImplementation, + proxyAdmin.getProxyImplementation(proxy) == newUpgradeExecutorImplementation, "UpgradeExecutorUpgradeAction: upgrade failed" ); } } + +// Proxy Admins: +// Arb1: 0xdb216562328215E010F819B5aBe947bad4ca961e +// Nova: 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9 +// L1: 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678 + +contract ArbOneUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { + constructor() + UpgradeExecutorUpgradeAction(0xdb216562328215E010F819B5aBe947bad4ca961e) + {} +} + +contract NovaUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { + constructor() + UpgradeExecutorUpgradeAction(0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9) + {} +} + +contract L1UpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { + constructor() + UpgradeExecutorUpgradeAction(0x5613AF0474EB9c528A34701A5b1662E3C8FA0678) + {} +} diff --git a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol index 18bc1ee7f..1e7c8f9c1 100644 --- a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol +++ b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol @@ -4,18 +4,13 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; -import { - ProxyAdmin, - TransparentUpgradeableProxy -} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; - import "src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol"; contract UpgradeExecutorUpgradeActionTest is Test { function testArbOne() external { vm.createSelectFork(vm.envString("ARB_URL"), 265159958); _testUpgrade( - 0xdb216562328215E010F819B5aBe947bad4ca961e, + new ArbOneUpgradeExecutorUpgradeAction(), 0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827, 0xf7951D92B0C345144506576eC13Ecf5103aC905a ); @@ -24,7 +19,7 @@ contract UpgradeExecutorUpgradeActionTest is Test { function testNova() external { vm.createSelectFork(vm.envString("NOVA_URL"), 78263024); _testUpgrade( - 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9, + new NovaUpgradeExecutorUpgradeAction(), 0x86a02dD71363c440b21F4c0E5B2Ad01Ffe1A7482, 0xf7951D92B0C345144506576eC13Ecf5103aC905a ); @@ -33,23 +28,22 @@ contract UpgradeExecutorUpgradeActionTest is Test { function testL1() external { vm.createSelectFork(vm.envString("ETH_URL"), 20993735); _testUpgrade( - 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678, + new L1UpgradeExecutorUpgradeAction(), 0x3ffFbAdAF827559da092217e474760E2b2c3CeDd, 0xE6841D92B0C345144506576eC13ECf5103aC7f49 ); } function _testUpgrade( - address admin, + UpgradeExecutorUpgradeAction action, address ue, address executor ) internal { - UpgradeExecutorUpgradeAction action = new UpgradeExecutorUpgradeAction(); vm.prank(executor); UpgradeExecutor(ue).execute(address(action), abi.encodeWithSignature("perform()")); assertTrue( - ProxyAdmin(admin).getProxyImplementation( + ProxyAdmin(action.proxyAdmin()).getProxyImplementation( TransparentUpgradeableProxy(payable(address(ue))) ) == action.newUpgradeExecutorImplementation() ); From bdec6be625f46d4ac2b65dfb900ad68e34bae1a7 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Mon, 21 Oct 2024 09:44:12 -0400 Subject: [PATCH 11/16] fmt --- .../UpgradeExecutorUpgradeAction.sol | 12 +++--------- .../gov-actions/UpgradeExecutorUpgradeAction.t.sol | 14 ++++++-------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol index 94e55b5d7..a28fa266a 100644 --- a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -34,19 +34,13 @@ contract UpgradeExecutorUpgradeAction { // L1: 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678 contract ArbOneUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() - UpgradeExecutorUpgradeAction(0xdb216562328215E010F819B5aBe947bad4ca961e) - {} + constructor() UpgradeExecutorUpgradeAction(0xdb216562328215E010F819B5aBe947bad4ca961e) {} } contract NovaUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() - UpgradeExecutorUpgradeAction(0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9) - {} + constructor() UpgradeExecutorUpgradeAction(0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9) {} } contract L1UpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() - UpgradeExecutorUpgradeAction(0x5613AF0474EB9c528A34701A5b1662E3C8FA0678) - {} + constructor() UpgradeExecutorUpgradeAction(0x5613AF0474EB9c528A34701A5b1662E3C8FA0678) {} } diff --git a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol index 1e7c8f9c1..fba2ff147 100644 --- a/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol +++ b/test/gov-actions/UpgradeExecutorUpgradeAction.t.sol @@ -8,7 +8,7 @@ import "src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUp contract UpgradeExecutorUpgradeActionTest is Test { function testArbOne() external { - vm.createSelectFork(vm.envString("ARB_URL"), 265159958); + vm.createSelectFork(vm.envString("ARB_URL"), 265_159_958); _testUpgrade( new ArbOneUpgradeExecutorUpgradeAction(), 0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827, @@ -17,7 +17,7 @@ contract UpgradeExecutorUpgradeActionTest is Test { } function testNova() external { - vm.createSelectFork(vm.envString("NOVA_URL"), 78263024); + vm.createSelectFork(vm.envString("NOVA_URL"), 78_263_024); _testUpgrade( new NovaUpgradeExecutorUpgradeAction(), 0x86a02dD71363c440b21F4c0E5B2Ad01Ffe1A7482, @@ -26,7 +26,7 @@ contract UpgradeExecutorUpgradeActionTest is Test { } function testL1() external { - vm.createSelectFork(vm.envString("ETH_URL"), 20993735); + vm.createSelectFork(vm.envString("ETH_URL"), 20_993_735); _testUpgrade( new L1UpgradeExecutorUpgradeAction(), 0x3ffFbAdAF827559da092217e474760E2b2c3CeDd, @@ -34,11 +34,9 @@ contract UpgradeExecutorUpgradeActionTest is Test { ); } - function _testUpgrade( - UpgradeExecutorUpgradeAction action, - address ue, - address executor - ) internal { + function _testUpgrade(UpgradeExecutorUpgradeAction action, address ue, address executor) + internal + { vm.prank(executor); UpgradeExecutor(ue).execute(address(action), abi.encodeWithSignature("perform()")); From f42d459d1d4ed695dfbfed3844f0c07a265531c6 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Mon, 21 Oct 2024 10:00:38 -0400 Subject: [PATCH 12/16] clean up imports --- src/security-council-mgmt/SecurityCouncilManager.sol | 2 +- test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol | 1 - test/gov-actions/SwitchManagerRolesAction.t.sol | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/security-council-mgmt/SecurityCouncilManager.sol b/src/security-council-mgmt/SecurityCouncilManager.sol index 61dcb7e0d..c657401a5 100644 --- a/src/security-council-mgmt/SecurityCouncilManager.sol +++ b/src/security-council-mgmt/SecurityCouncilManager.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "../ArbitrumTimelock.sol"; -import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "../L1ArbitrumTimelock.sol"; import "./SecurityCouncilMgmtUtils.sol"; import "./interfaces/ISecurityCouncilManager.sol"; diff --git a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol index cfe511da2..d4e5fdf34 100644 --- a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol +++ b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol @@ -5,7 +5,6 @@ import "forge-std/Test.sol"; import "../../src/gov-action-contracts/AIPs/NomineeGovernorV2UpgradeAction.sol"; import "../../src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol"; -import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; contract NomineeGovernorV2UpgradeActionTest is Test { address oldImplementation = 0x8436A1bc9f9f9EB0cF1B51942C5657b60A40CCDD; diff --git a/test/gov-actions/SwitchManagerRolesAction.t.sol b/test/gov-actions/SwitchManagerRolesAction.t.sol index b6264ff8a..d36021385 100644 --- a/test/gov-actions/SwitchManagerRolesAction.t.sol +++ b/test/gov-actions/SwitchManagerRolesAction.t.sol @@ -4,7 +4,6 @@ pragma solidity 0.8.16; import "forge-std/Test.sol"; import "../../src/gov-action-contracts/nonemergency/SwitchManagerRolesAction.sol"; -import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; contract SwitchManagerRolesActionTest is Test { UpgradeExecutor arbOneUe = UpgradeExecutor(0xCF57572261c7c2BCF21ffD220ea7d1a27D40A827); From e8163949a54ca2280b15d11fd2fa12c16f9eeda6 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 22 Oct 2024 09:38:38 -0400 Subject: [PATCH 13/16] deploy exec separately --- .../UpgradeExecutorUpgradeAction.sol | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol index a28fa266a..2513a4404 100644 --- a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -1,19 +1,19 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity 0.8.16; -import {UpgradeExecutor} from "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; +import {UpgradeExecutor} from "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; // todo: deploy UpgradeExecutor separately import { ProxyAdmin, TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol"; contract UpgradeExecutorUpgradeAction { - address public immutable newUpgradeExecutorImplementation; ProxyAdmin public immutable proxyAdmin; + address public immutable newUpgradeExecutorImplementation; - constructor(address _proxyAdmin) { + constructor(address _proxyAdmin, address _newUpgradeExecutorImplementation) { proxyAdmin = ProxyAdmin(_proxyAdmin); - newUpgradeExecutorImplementation = address(new UpgradeExecutor()); + newUpgradeExecutorImplementation = _newUpgradeExecutorImplementation; } function perform() external { @@ -34,13 +34,28 @@ contract UpgradeExecutorUpgradeAction { // L1: 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678 contract ArbOneUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() UpgradeExecutorUpgradeAction(0xdb216562328215E010F819B5aBe947bad4ca961e) {} + constructor() + UpgradeExecutorUpgradeAction( + 0xdb216562328215E010F819B5aBe947bad4ca961e, + address(new UpgradeExecutor()) // todo: deploy UpgradeExecutor separately + ) + {} } contract NovaUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() UpgradeExecutorUpgradeAction(0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9) {} + constructor() + UpgradeExecutorUpgradeAction( + 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9, + address(new UpgradeExecutor()) // todo: deploy UpgradeExecutor separately + ) + {} } contract L1UpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { - constructor() UpgradeExecutorUpgradeAction(0x5613AF0474EB9c528A34701A5b1662E3C8FA0678) {} + constructor() + UpgradeExecutorUpgradeAction( + 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678, + address(new UpgradeExecutor()) // todo: deploy UpgradeExecutor separately + ) + {} } From c29aeedb15d2009e38ff9d1c9b9915d0fafe03ae Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:49:13 -0500 Subject: [PATCH 14/16] add impls --- .../UpgradeExecutorUpgradeAction.sol | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol index 2513a4404..aa5f52361 100644 --- a/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol +++ b/src/gov-action-contracts/AIPs/upgrade-executor-upgrade/UpgradeExecutorUpgradeAction.sol @@ -33,11 +33,16 @@ contract UpgradeExecutorUpgradeAction { // Nova: 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9 // L1: 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678 +// Upgrade Executor Impls: +// Arb1: 0x12B1389Fbf261E781bdc3094d28636Abfb03C5b3 +// Nova: 0xebb11Bbd7d72165FaC86bb5AB1B07A602540b286 +// L1: 0xDE505e42D50abd07c8D39Dcf692920d56cBA35Da + contract ArbOneUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { constructor() UpgradeExecutorUpgradeAction( 0xdb216562328215E010F819B5aBe947bad4ca961e, - address(new UpgradeExecutor()) // todo: deploy UpgradeExecutor separately + 0x12B1389Fbf261E781bdc3094d28636Abfb03C5b3 ) {} } @@ -46,7 +51,7 @@ contract NovaUpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { constructor() UpgradeExecutorUpgradeAction( 0xf58eA15B20983116c21b05c876cc8e6CDAe5C2b9, - address(new UpgradeExecutor()) // todo: deploy UpgradeExecutor separately + 0xebb11Bbd7d72165FaC86bb5AB1B07A602540b286 ) {} } @@ -55,7 +60,7 @@ contract L1UpgradeExecutorUpgradeAction is UpgradeExecutorUpgradeAction { constructor() UpgradeExecutorUpgradeAction( 0x5613AF0474EB9c528A34701A5b1662E3C8FA0678, - address(new UpgradeExecutor()) // todo: deploy UpgradeExecutor separately + 0xDE505e42D50abd07c8D39Dcf692920d56cBA35Da ) {} } From 81236c364c3df916fca17910b4deb7463d66e257 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:57:16 -0500 Subject: [PATCH 15/16] use interface --- src/UpgradeExecRouteBuilder.sol | 4 ++-- test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/UpgradeExecRouteBuilder.sol b/src/UpgradeExecRouteBuilder.sol index 679111f86..0bd00e903 100644 --- a/src/UpgradeExecRouteBuilder.sol +++ b/src/UpgradeExecRouteBuilder.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol"; -import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; import "./L1ArbitrumTimelock.sol"; import "./security-council-mgmt/Common.sol"; @@ -136,7 +136,7 @@ contract UpgradeExecRouteBuilder { } bytes memory executorData = abi.encodeWithSelector( - UpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] + IUpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] ); // for L1, inbox is set to address(0): diff --git a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol index d4e5fdf34..cfe511da2 100644 --- a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol +++ b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol @@ -5,6 +5,7 @@ import "forge-std/Test.sol"; import "../../src/gov-action-contracts/AIPs/NomineeGovernorV2UpgradeAction.sol"; import "../../src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; contract NomineeGovernorV2UpgradeActionTest is Test { address oldImplementation = 0x8436A1bc9f9f9EB0cF1B51942C5657b60A40CCDD; From e4d4cc1db2b9aac73bfe56069d9af5cd5fe22e33 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:57:43 -0500 Subject: [PATCH 16/16] Revert "use interface" This reverts commit 81236c364c3df916fca17910b4deb7463d66e257. --- src/UpgradeExecRouteBuilder.sol | 4 ++-- test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/UpgradeExecRouteBuilder.sol b/src/UpgradeExecRouteBuilder.sol index 0bd00e903..679111f86 100644 --- a/src/UpgradeExecRouteBuilder.sol +++ b/src/UpgradeExecRouteBuilder.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.16; import "@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol"; -import "@offchainlabs/upgrade-executor/src/IUpgradeExecutor.sol"; +import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; import "./L1ArbitrumTimelock.sol"; import "./security-council-mgmt/Common.sol"; @@ -136,7 +136,7 @@ contract UpgradeExecRouteBuilder { } bytes memory executorData = abi.encodeWithSelector( - IUpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] + UpgradeExecutor.execute.selector, actionAddresses[i], actionDatas[i] ); // for L1, inbox is set to address(0): diff --git a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol index cfe511da2..d4e5fdf34 100644 --- a/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol +++ b/test/gov-actions/NomineeGovernorV2UpgradeAction.t.sol @@ -5,7 +5,6 @@ import "forge-std/Test.sol"; import "../../src/gov-action-contracts/AIPs/NomineeGovernorV2UpgradeAction.sol"; import "../../src/security-council-mgmt/governors/SecurityCouncilNomineeElectionGovernor.sol"; -import "@offchainlabs/upgrade-executor/src/UpgradeExecutor.sol"; contract NomineeGovernorV2UpgradeActionTest is Test { address oldImplementation = 0x8436A1bc9f9f9EB0cF1B51942C5657b60A40CCDD;