Commit af5ad1d2 authored by Mark Tyneway's avatar Mark Tyneway

contracts-bedrock: fix test legibility

Adds a new helper library called `EIP1967Helper` that can
get the admin and impl storage slots from a `Proxy` implementing
ERC1967. This is more helpful to use than hardcoded magic values
such as `multisig` because it is not clear who the multisig is
since its value is assigned in a different file. We want to decouple
the value from a magic value and set it to exactly what we want it
to be which is the admin. This will work in all cases no matter what
the admin is since it dynamically pulls the value from storage for
the tests.
parent 489055cb
...@@ -249,7 +249,7 @@ L2OutputOracleUpgradeable_Test:test_initValuesOnImpl_succeeds() (gas: 23902) ...@@ -249,7 +249,7 @@ L2OutputOracleUpgradeable_Test:test_initValuesOnImpl_succeeds() (gas: 23902)
L2OutputOracleUpgradeable_Test:test_initValuesOnProxy_succeeds() (gas: 46800) L2OutputOracleUpgradeable_Test:test_initValuesOnProxy_succeeds() (gas: 46800)
L2OutputOracleUpgradeable_Test:test_initializeImpl_alreadyInitialized_reverts() (gas: 15216) L2OutputOracleUpgradeable_Test:test_initializeImpl_alreadyInitialized_reverts() (gas: 15216)
L2OutputOracleUpgradeable_Test:test_initializeProxy_alreadyInitialized_reverts() (gas: 20216) L2OutputOracleUpgradeable_Test:test_initializeProxy_alreadyInitialized_reverts() (gas: 20216)
L2OutputOracleUpgradeable_Test:test_upgrading_succeeds() (gas: 191455) L2OutputOracleUpgradeable_Test:test_upgrading_succeeds() (gas: 187875)
L2OutputOracle_constructor_Test:test_constructor_l2BlockTimeZero_reverts() (gas: 39022) L2OutputOracle_constructor_Test:test_constructor_l2BlockTimeZero_reverts() (gas: 39022)
L2OutputOracle_constructor_Test:test_constructor_submissionInterval_reverts() (gas: 39032) L2OutputOracle_constructor_Test:test_constructor_submissionInterval_reverts() (gas: 39032)
L2OutputOracle_constructor_Test:test_constructor_succeeds() (gas: 51777) L2OutputOracle_constructor_Test:test_constructor_succeeds() (gas: 51777)
......
...@@ -3,6 +3,7 @@ pragma solidity 0.8.15; ...@@ -3,6 +3,7 @@ pragma solidity 0.8.15;
// Testing utilities // Testing utilities
import { Test, StdUtils } from "forge-std/Test.sol"; import { Test, StdUtils } from "forge-std/Test.sol";
import { Vm } from "forge-std/Vm.sol";
import { L2OutputOracle } from "src/L1/L2OutputOracle.sol"; import { L2OutputOracle } from "src/L1/L2OutputOracle.sol";
import { L2ToL1MessagePasser } from "src/L2/L2ToL1MessagePasser.sol"; import { L2ToL1MessagePasser } from "src/L2/L2ToL1MessagePasser.sol";
import { L1StandardBridge } from "src/L1/L1StandardBridge.sol"; import { L1StandardBridge } from "src/L1/L1StandardBridge.sol";
...@@ -731,6 +732,18 @@ contract FFIInterface is Test { ...@@ -731,6 +732,18 @@ contract FFIInterface is Test {
} }
} }
library EIP1967Helper {
Vm internal constant vm = Vm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);
function getAdmin(address _proxy) internal view returns (address) {
return address(uint160(uint256(vm.load(address(_proxy), Constants.PROXY_OWNER_ADDRESS))));
}
function getImplementation(address _proxy) internal view returns (address) {
return address(uint160(uint256(vm.load(address(_proxy), Constants.PROXY_IMPLEMENTATION_ADDRESS))));
}
}
// Used for testing a future upgrade beyond the current implementations. // Used for testing a future upgrade beyond the current implementations.
// We include some variables so that we can sanity check accessing storage values after an upgrade. // We include some variables so that we can sanity check accessing storage values after an upgrade.
contract NextImpl is Initializable { contract NextImpl is Initializable {
......
...@@ -3,7 +3,7 @@ pragma solidity 0.8.15; ...@@ -3,7 +3,7 @@ pragma solidity 0.8.15;
// Testing utilities // Testing utilities
import { stdError } from "forge-std/Test.sol"; import { stdError } from "forge-std/Test.sol";
import { L2OutputOracle_Initializer, NextImpl } from "test/CommonTest.t.sol"; import { L2OutputOracle_Initializer, NextImpl, EIP1967Helper } from "test/CommonTest.t.sol";
// Libraries // Libraries
import { Types } from "src/libraries/Types.sol"; import { Types } from "src/libraries/Types.sol";
...@@ -463,7 +463,7 @@ contract L2OutputOracleUpgradeable_Test is L2OutputOracle_Initializer { ...@@ -463,7 +463,7 @@ contract L2OutputOracleUpgradeable_Test is L2OutputOracle_Initializer {
assertEq(bytes32(0), slot21Before); assertEq(bytes32(0), slot21Before);
NextImpl nextImpl = new NextImpl(); NextImpl nextImpl = new NextImpl();
vm.startPrank(multisig); vm.startPrank(EIP1967Helper.getAdmin(address(proxy)));
proxy.upgradeToAndCall( proxy.upgradeToAndCall(
address(nextImpl), abi.encodeWithSelector(NextImpl.initialize.selector, Constants.INITIALIZER + 1) address(nextImpl), abi.encodeWithSelector(NextImpl.initialize.selector, Constants.INITIALIZER + 1)
); );
......
...@@ -12,6 +12,7 @@ import { ResourceMetering } from "src/L1/ResourceMetering.sol"; ...@@ -12,6 +12,7 @@ import { ResourceMetering } from "src/L1/ResourceMetering.sol";
import { Constants } from "src/libraries/Constants.sol"; import { Constants } from "src/libraries/Constants.sol";
import { Portal_Initializer } from "test/CommonTest.t.sol"; import { Portal_Initializer } from "test/CommonTest.t.sol";
import { EIP1967Helper } from "test/CommonTest.t.sol";
import { Types } from "src/libraries/Types.sol"; import { Types } from "src/libraries/Types.sol";
contract OptimismPortal_Depositor is StdUtils, ResourceMetering { contract OptimismPortal_Depositor is StdUtils, ResourceMetering {
...@@ -158,8 +159,8 @@ contract OptimismPortal_CannotTimeTravel is OptimismPortal_Invariant_Harness { ...@@ -158,8 +159,8 @@ contract OptimismPortal_CannotTimeTravel is OptimismPortal_Invariant_Harness {
// Set the target contract to the portal proxy // Set the target contract to the portal proxy
targetContract(address(op)); targetContract(address(op));
// Exclude the proxy multisig from the senders so that the proxy cannot be upgraded // Exclude the proxy admin from the senders so that the proxy cannot be upgraded
excludeSender(address(multisig)); excludeSender(EIP1967Helper.getAdmin(address(op)));
} }
/// @custom:invariant `finalizeWithdrawalTransaction` should revert if the finalization /// @custom:invariant `finalizeWithdrawalTransaction` should revert if the finalization
...@@ -188,8 +189,8 @@ contract OptimismPortal_CannotFinalizeTwice is OptimismPortal_Invariant_Harness ...@@ -188,8 +189,8 @@ contract OptimismPortal_CannotFinalizeTwice is OptimismPortal_Invariant_Harness
// Set the target contract to the portal proxy // Set the target contract to the portal proxy
targetContract(address(op)); targetContract(address(op));
// Exclude the proxy multisig from the senders so that the proxy cannot be upgraded // Exclude the proxy admin from the senders so that the proxy cannot be upgraded
excludeSender(address(multisig)); excludeSender(EIP1967Helper.getAdmin(address(op)));
} }
/// @custom:invariant `finalizeWithdrawalTransaction` should revert if the withdrawal /// @custom:invariant `finalizeWithdrawalTransaction` should revert if the withdrawal
...@@ -215,8 +216,8 @@ contract OptimismPortal_CanAlwaysFinalizeAfterWindow is OptimismPortal_Invariant ...@@ -215,8 +216,8 @@ contract OptimismPortal_CanAlwaysFinalizeAfterWindow is OptimismPortal_Invariant
// Set the target contract to the portal proxy // Set the target contract to the portal proxy
targetContract(address(op)); targetContract(address(op));
// Exclude the proxy multisig from the senders so that the proxy cannot be upgraded // Exclude the proxy admin from the senders so that the proxy cannot be upgraded
excludeSender(address(multisig)); excludeSender(EIP1967Helper.getAdmin(address(op)));
} }
/// @custom:invariant A withdrawal should **always** be able to be finalized /// @custom:invariant A withdrawal should **always** be able to be finalized
......
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