From d3750a5fc6e051b7117fcfbb8e1850090fb2e541 Mon Sep 17 00:00:00 2001 From: wildmolasses Date: Wed, 28 Aug 2024 16:19:40 -0400 Subject: [PATCH] additional cleanup --- .env.example | 4 + remappings.txt | 1 - script/DeployTimelockRolesUpgrader.s.sol | 4 +- script/SharedGovernorConstants.sol | 4 +- script/SubmitUpgradeProposalScript.s.sol | 4 +- src/L2ArbitrumGovernorV2.sol | 2 +- test/L2ArbitrumGovernorV2.t.sol | 12 +- test/SubmitUpgradeProposal.t.sol | 173 +++++++++++++++++++++++ test/util/SetupNewGovernors.sol | 8 +- 9 files changed, 194 insertions(+), 18 deletions(-) create mode 100644 test/SubmitUpgradeProposal.t.sol diff --git a/.env.example b/.env.example index 84bd8648..a6e26db2 100644 --- a/.env.example +++ b/.env.example @@ -1 +1,5 @@ +# RPC for running governor v2 upgrade tests ARBITRUM_ONE_RPC_URL= + +# Proposer address for governor v2 upgrade +PROPOSER_ADDRESS= diff --git a/remappings.txt b/remappings.txt index 9f36a8e6..2a572f11 100644 --- a/remappings.txt +++ b/remappings.txt @@ -8,5 +8,4 @@ solady/=lib/solady/src/ @gnosis.pm/safe-contracts/=node_modules/@gnosis.pm/safe-contracts/ ds-test/=lib/forge-std/lib/ds-test/src/ openzeppelin-v5/=lib/openzeppelin-contracts-v5/contracts -openzeppelin-v4/=lib/openzeppelin-contracts-v4/contracts openzeppelin-upgradeable-v5/=lib/openzeppelin-contracts-upgradeable-v5/contracts diff --git a/script/DeployTimelockRolesUpgrader.s.sol b/script/DeployTimelockRolesUpgrader.s.sol index 3fce13fe..ad0dedef 100644 --- a/script/DeployTimelockRolesUpgrader.s.sol +++ b/script/DeployTimelockRolesUpgrader.s.sol @@ -14,10 +14,10 @@ contract DeployTimelockRolesUpgrader is BaseDeployer, SharedGovernorConstants { timelockRolesUpgrader = new TimelockRolesUpgrader( L2_CORE_GOVERNOR_TIMELOCK, L2_CORE_GOVERNOR, - L2_CORE_GOVERNOR_ONCHAIN, + L2_CORE_GOVERNOR_NEW_DEPLOY, L2_TREASURY_GOVERNOR_TIMELOCK, L2_TREASURY_GOVERNOR, - L2_TREASURY_GOVERNOR_ONCHAIN + L2_TREASURY_GOVERNOR_NEW_DEPLOY ); vm.stopBroadcast(); } diff --git a/script/SharedGovernorConstants.sol b/script/SharedGovernorConstants.sol index d4b4f4f2..a1beb0dc 100644 --- a/script/SharedGovernorConstants.sol +++ b/script/SharedGovernorConstants.sol @@ -26,8 +26,8 @@ contract SharedGovernorConstants { uint256 public constant L1_TIMELOCK_MIN_DELAY = 259_200; // TODO: Make sure this is up to date. address public constant L1_ARB_ONE_DELAYED_INBOX = 0x4Dbd4fc535Ac27206064B68FfCf827b0A60BAB3f; - address public constant L2_CORE_GOVERNOR_ONCHAIN = 0x7796F378B3c56ceD57350B938561D8c52256456b; - address public constant L2_TREASURY_GOVERNOR_ONCHAIN = + address public constant L2_CORE_GOVERNOR_NEW_DEPLOY = 0x7796F378B3c56ceD57350B938561D8c52256456b; + address public constant L2_TREASURY_GOVERNOR_NEW_DEPLOY = 0x4fd1216c8b5E72b22785169Ae5C1e8f3b30C19E4; bool public constant UPGRADE_PROPOSAL_PASSED_ONCHAIN = false; // TODO: Update after the upgrade proposal is passed. diff --git a/script/SubmitUpgradeProposalScript.s.sol b/script/SubmitUpgradeProposalScript.s.sol index d64988f2..e6977b3b 100644 --- a/script/SubmitUpgradeProposalScript.s.sol +++ b/script/SubmitUpgradeProposalScript.s.sol @@ -55,8 +55,8 @@ contract SubmitUpgradeProposalScript is Script, SharedGovernorConstants, CreateL \ ### **Technical Details** \ - The new Governor contracts have been deployed on Arbitrum One at the following addresses: \ - TODO: [Insert new Core Governor address] \ - TODO: [Insert new Treasury Governor address] \ + Core Governor: 0x7796F378B3c56ceD57350B938561D8c52256456b \ + Treasury Governor: 0x4fd1216c8b5E72b22785169Ae5C1e8f3b30C19E4 \ \ - These new contracts include the following enhancements: \ 1. Proposal Cancellation: Allows the delegate who submitted a proposal to cancel it during the delay phase, before voting begins. \ diff --git a/src/L2ArbitrumGovernorV2.sol b/src/L2ArbitrumGovernorV2.sol index 263fef49..bf863dc7 100644 --- a/src/L2ArbitrumGovernorV2.sol +++ b/src/L2ArbitrumGovernorV2.sol @@ -6,7 +6,7 @@ import {GovernorUpgradeable} from "openzeppelin-upgradeable-v5/governance/Govern import {GovernorSettingsUpgradeable} from "openzeppelin-upgradeable-v5/governance/extensions/GovernorSettingsUpgradeable.sol"; import {GovernorCountingFractionalUpgradeable} from - "./lib/GovernorCountingFractionalUpgradeable.sol"; + "src/lib/GovernorCountingFractionalUpgradeable.sol"; import {GovernorTimelockControlUpgradeable} from "openzeppelin-upgradeable-v5/governance/extensions/GovernorTimelockControlUpgradeable.sol"; import {GovernorVotesQuorumFractionUpgradeable} from diff --git a/test/L2ArbitrumGovernorV2.t.sol b/test/L2ArbitrumGovernorV2.t.sol index ec816665..c6bf49a6 100644 --- a/test/L2ArbitrumGovernorV2.t.sol +++ b/test/L2ArbitrumGovernorV2.t.sol @@ -112,8 +112,8 @@ abstract contract L2ArbitrumGovernorV2Test is SetupNewGovernors { function _skipToPostUpgrade() internal { if ( - UPGRADE_PROPOSAL_PASSED_ONCHAIN && L2_CORE_GOVERNOR_ONCHAIN != address(0) - && L2_TREASURY_GOVERNOR_ONCHAIN != address(0) + UPGRADE_PROPOSAL_PASSED_ONCHAIN && L2_CORE_GOVERNOR_NEW_DEPLOY != address(0) + && L2_TREASURY_GOVERNOR_NEW_DEPLOY != address(0) ) { return; } @@ -171,9 +171,9 @@ abstract contract CoreGovernorBase is L2ArbitrumGovernorV2Test { function setUp() public virtual override { super.setUp(); // If no deployed governor address is set, we use the locally deployed governor - governor = L2_CORE_GOVERNOR_ONCHAIN == address(0) + governor = L2_CORE_GOVERNOR_NEW_DEPLOY == address(0) ? newCoreGovernor - : L2ArbitrumGovernorV2(payable(L2_CORE_GOVERNOR_ONCHAIN)); + : L2ArbitrumGovernorV2(payable(L2_CORE_GOVERNOR_NEW_DEPLOY)); _oldGovernor = currentCoreGovernor; timelock = currentCoreTimelock; proxyDeployer = proxyCoreGovernorDeployer; @@ -208,9 +208,9 @@ abstract contract TreasuryGovernorBase is L2ArbitrumGovernorV2Test { function setUp() public virtual override { super.setUp(); // If no deployed governor address is set, we use the locally deployed governor - governor = L2_TREASURY_GOVERNOR_ONCHAIN == address(0) + governor = L2_TREASURY_GOVERNOR_NEW_DEPLOY == address(0) ? newTreasuryGovernor - : L2ArbitrumGovernorV2(payable(L2_TREASURY_GOVERNOR_ONCHAIN)); + : L2ArbitrumGovernorV2(payable(L2_TREASURY_GOVERNOR_NEW_DEPLOY)); _oldGovernor = currentTreasuryGovernor; timelock = currentTreasuryTimelock; proxyDeployer = proxyTreasuryGovernorDeployer; diff --git a/test/SubmitUpgradeProposal.t.sol b/test/SubmitUpgradeProposal.t.sol new file mode 100644 index 00000000..5ce6145c --- /dev/null +++ b/test/SubmitUpgradeProposal.t.sol @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: AGPL-3.0-only +pragma solidity 0.8.26; + +import {Test, console2} from "forge-std/Test.sol"; +import {SubmitUpgradeProposalScript} from "script/SubmitUpgradeProposalScript.s.sol"; +import {IGovernor} from "openzeppelin-v5/governance/IGovernor.sol"; +import {TimelockRolesUpgrader} from + "src/gov-action-contracts/gov-upgrade-contracts/update-timelock-roles/TimelockRolesUpgrader.sol"; +import {SetupNewGovernors} from "test/util/SetupNewGovernors.sol"; + +contract SubmitUpgradeProposalTest is SetupNewGovernors { + function test_SuccessfullyExecuteUpgradeProposal() public { + TimelockRolesUpgrader timelockRolesUpgrader = new TimelockRolesUpgrader( + L2_CORE_GOVERNOR_TIMELOCK, + L2_CORE_GOVERNOR, + L2_CORE_GOVERNOR_NEW_DEPLOY, + L2_TREASURY_GOVERNOR_TIMELOCK, + L2_TREASURY_GOVERNOR, + L2_TREASURY_GOVERNOR_NEW_DEPLOY + ); + + // Propose + ( + address[] memory _targets, + uint256[] memory _values, + bytes[] memory _calldatas, + string memory _description, + uint256 _proposalId + ) = submitUpgradeProposalScript.run(address(timelockRolesUpgrader), L1_TIMELOCK_MIN_DELAY); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), + uint256(IGovernor.ProposalState.Pending) + ); + vm.roll(vm.getBlockNumber() + currentCoreGovernor.votingDelay() + 1); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), uint256(IGovernor.ProposalState.Active) + ); + + // Vote + for (uint256 i; i < _majorDelegates.length; i++) { + vm.prank(_majorDelegates[i]); + currentCoreGovernor.castVote(_proposalId, uint8(VoteType.For)); + } + + // Success + vm.roll(vm.getBlockNumber() + currentCoreGovernor.votingPeriod() + 1); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), + uint256(IGovernor.ProposalState.Succeeded) + ); + + // Queue + currentCoreGovernor.queue(_targets, _values, _calldatas, keccak256(bytes(_description))); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), uint256(IGovernor.ProposalState.Queued) + ); + vm.warp(vm.getBlockTimestamp() + currentCoreTimelock.getMinDelay() + 1); + + // Execute + currentCoreGovernor.execute(_targets, _values, _calldatas, keccak256(bytes(_description))); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), + uint256(IGovernor.ProposalState.Executed) + ); + + assertEq( + currentCoreTimelock.hasRole(keccak256("PROPOSER_ROLE"), address(newCoreGovernor)), true + ); + assertEq( + currentCoreTimelock.hasRole(keccak256("CANCELLER_ROLE"), address(newCoreGovernor)), true + ); + assertEq(currentCoreTimelock.hasRole(keccak256("PROPOSER_ROLE"), L2_CORE_GOVERNOR), false); + assertEq(currentCoreTimelock.hasRole(keccak256("CANCELLER_ROLE"), L2_CORE_GOVERNOR), false); + + assertEq( + currentTreasuryTimelock.hasRole( + keccak256("PROPOSER_ROLE"), address(newTreasuryGovernor) + ), + true + ); + assertEq( + currentTreasuryTimelock.hasRole( + keccak256("CANCELLER_ROLE"), address(newTreasuryGovernor) + ), + true + ); + assertEq( + currentTreasuryTimelock.hasRole(keccak256("PROPOSER_ROLE"), L2_TREASURY_GOVERNOR), false + ); + assertEq( + currentTreasuryTimelock.hasRole(keccak256("CANCELLER_ROLE"), L2_TREASURY_GOVERNOR), + false + ); + } + + function test_DefeatedExecuteUpgradeProposalDoesNotChangeRoles() public { + TimelockRolesUpgrader timelockRolesUpgrader = new TimelockRolesUpgrader( + L2_CORE_GOVERNOR_TIMELOCK, + L2_CORE_GOVERNOR, + address(newCoreGovernor), + L2_TREASURY_GOVERNOR_TIMELOCK, + L2_TREASURY_GOVERNOR, + address(newTreasuryGovernor) + ); + + // Propose + ( + /*address[] memory _targets*/ + , + /*uint256[] memory _values*/ + , + /*bytes[] memory _calldatas*/ + , + /*string memory _description*/ + , + uint256 _proposalId + ) = submitUpgradeProposalScript.run(address(timelockRolesUpgrader), L1_TIMELOCK_MIN_DELAY); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), + uint256(IGovernor.ProposalState.Pending) + ); + vm.roll(vm.getBlockNumber() + currentCoreGovernor.votingDelay() + 1); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), uint256(IGovernor.ProposalState.Active) + ); + + // Vote + for (uint256 i; i < _majorDelegates.length; i++) { + vm.prank(_majorDelegates[i]); + currentCoreGovernor.castVote(_proposalId, uint8(VoteType.Against)); + } + + // Defeat + vm.roll(vm.getBlockNumber() + currentCoreGovernor.votingPeriod() + 1); + assertEq( + uint256(currentCoreGovernor.state(_proposalId)), + uint256(IGovernor.ProposalState.Defeated) + ); + + assertEq( + currentCoreTimelock.hasRole(keccak256("PROPOSER_ROLE"), address(newCoreGovernor)), false + ); + assertEq( + currentCoreTimelock.hasRole(keccak256("CANCELLER_ROLE"), address(newCoreGovernor)), + false + ); + assertEq(currentCoreTimelock.hasRole(keccak256("PROPOSER_ROLE"), L2_CORE_GOVERNOR), true); + assertEq(currentCoreTimelock.hasRole(keccak256("CANCELLER_ROLE"), L2_CORE_GOVERNOR), true); + + assertEq( + currentTreasuryTimelock.hasRole( + keccak256("PROPOSER_ROLE"), address(newTreasuryGovernor) + ), + false + ); + assertEq( + currentTreasuryTimelock.hasRole( + keccak256("CANCELLER_ROLE"), address(newTreasuryGovernor) + ), + false + ); + assertEq( + currentTreasuryTimelock.hasRole(keccak256("PROPOSER_ROLE"), L2_TREASURY_GOVERNOR), true + ); + assertEq( + currentTreasuryTimelock.hasRole(keccak256("CANCELLER_ROLE"), L2_TREASURY_GOVERNOR), true + ); + } +} + +interface IUpgradeExecutor { + function execute(address to, bytes calldata data) external payable; +} diff --git a/test/util/SetupNewGovernors.sol b/test/util/SetupNewGovernors.sol index f206a13e..ce0160b9 100644 --- a/test/util/SetupNewGovernors.sol +++ b/test/util/SetupNewGovernors.sol @@ -52,12 +52,12 @@ abstract contract SetupNewGovernors is SharedGovernorConstants, Test { proxyTreasuryGovernorDeployer.setUp(); // Deploy Governor proxy contracts - newCoreGovernor = L2_CORE_GOVERNOR_ONCHAIN == address(0) + newCoreGovernor = L2_CORE_GOVERNOR_NEW_DEPLOY == address(0) ? proxyCoreGovernorDeployer.run(_implementation) - : L2ArbitrumGovernorV2(payable(L2_CORE_GOVERNOR_ONCHAIN)); - newTreasuryGovernor = L2_TREASURY_GOVERNOR_ONCHAIN == address(0) + : L2ArbitrumGovernorV2(payable(L2_CORE_GOVERNOR_NEW_DEPLOY)); + newTreasuryGovernor = L2_TREASURY_GOVERNOR_NEW_DEPLOY == address(0) ? proxyTreasuryGovernorDeployer.run(_implementation) - : L2ArbitrumGovernorV2(payable(L2_TREASURY_GOVERNOR_ONCHAIN)); + : L2ArbitrumGovernorV2(payable(L2_TREASURY_GOVERNOR_NEW_DEPLOY)); // Current governors and timelocks currentCoreGovernor = GovernorUpgradeable(payable(L2_CORE_GOVERNOR));