Commit 45b4b1f0 authored by Matt Solomon's avatar Matt Solomon Committed by GitHub

OPSM: miscellaneous fixes and improvements (#11935)

* chore: fix comment

* feat: add assertions

* fix: use method instead of new

* refactor: make salt into an input

* refactor: unify checkOutput function signatures

* chore: update snapshots

* test: fix testContractAuth

* test: fix checkOutput signatures

* fix: update go DeployImplementationsInput struct with salt

* pr feedback

* chore: semver lock
parent c72a75e8
......@@ -10,6 +10,7 @@ import (
)
type DeployImplementationsInput struct {
Salt common.Hash
WithdrawalDelaySeconds *big.Int
MinProposalSizeBytes *big.Int
ChallengePeriodSeconds *big.Int
......@@ -42,7 +43,7 @@ type DeployImplementationsOutput struct {
DisputeGameFactoryImpl common.Address
}
func (output *DeployImplementationsOutput) CheckOutput() error {
func (output *DeployImplementationsOutput) CheckOutput(input common.Address) error {
return nil
}
......
......@@ -48,7 +48,7 @@ type DeployOPChainOutput struct {
DelayedWETHPermissionlessGameProxy common.Address
}
func (output *DeployOPChainOutput) CheckOutput() error {
func (output *DeployOPChainOutput) CheckOutput(input common.Address) error {
return nil
}
......
......@@ -34,7 +34,7 @@ type DeploySuperchainOutput struct {
ProtocolVersionsProxy common.Address
}
func (output *DeploySuperchainOutput) CheckOutput() error {
func (output *DeploySuperchainOutput) CheckOutput(input common.Address) error {
return nil
}
......
......@@ -33,13 +33,13 @@ import { OptimismPortalInterop } from "src/L1/OptimismPortalInterop.sol";
import { SystemConfigInterop } from "src/L1/SystemConfigInterop.sol";
import { Blueprint } from "src/libraries/Blueprint.sol";
import { Config } from "scripts/libraries/Config.sol";
import { DeployUtils } from "scripts/libraries/DeployUtils.sol";
import { Solarray } from "scripts/libraries/Solarray.sol";
// See DeploySuperchain.s.sol for detailed comments on the script architecture used here.
contract DeployImplementationsInput is CommonBase {
bytes32 internal _salt;
uint256 internal _withdrawalDelaySeconds;
uint256 internal _minProposalSizeBytes;
uint256 internal _challengePeriodSeconds;
......@@ -85,11 +85,21 @@ contract DeployImplementationsInput is CommonBase {
else revert("DeployImplementationsInput: unknown selector");
}
function set(bytes4 sel, bytes32 _value) public {
if (sel == this.salt.selector) _salt = _value;
else revert("DeployImplementationsInput: unknown selector");
}
function loadInputFile(string memory _infile) public pure {
_infile;
require(false, "DeployImplementationsInput: not implemented");
}
function salt() public view returns (bytes32) {
// TODO check if implementations are deployed based on code+salt and skip deploy if so.
return _salt;
}
function withdrawalDelaySeconds() public view returns (uint256) {
require(_withdrawalDelaySeconds != 0, "DeployImplementationsInput: not set");
return _withdrawalDelaySeconds;
......@@ -180,7 +190,7 @@ contract DeployImplementationsOutput {
require(false, "DeployImplementationsOutput: not implemented");
}
function checkOutput() public {
function checkOutput(DeployImplementationsInput) public {
address[] memory addrs = Solarray.addresses(
address(this.opsm()),
address(this.optimismPortalImpl()),
......@@ -282,7 +292,7 @@ contract DeployImplementations is Script {
// Deploy the OP Stack Manager with the new implementations set.
deployOPStackManager(_dii, _dio);
_dio.checkOutput();
_dio.checkOutput(_dii);
}
// -------- Deployment Steps --------
......@@ -339,7 +349,7 @@ contract DeployImplementations is Script {
// First we deploy the blueprints for the singletons deployed by OPSM.
// forgefmt: disable-start
bytes32 salt = keccak256(bytes(Config.implSalt()));
bytes32 salt = _dii.salt();
OPStackManager.Blueprints memory blueprints;
vm.startBroadcast(msg.sender);
......@@ -456,6 +466,7 @@ contract DeployImplementations is Script {
// "implementations", and when shared contracts are not proxied, they are "singletons". So
// here we deploy:
//
// - DisputeGameFactory (implementation)
// - OptimismPortal2 (implementation)
// - DelayedWETH (implementation)
// - PreimageOracle (singleton)
......
......@@ -8,6 +8,9 @@ import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol";
import { DeployUtils } from "scripts/libraries/DeployUtils.sol";
import { Solarray } from "scripts/libraries/Solarray.sol";
import { IResourceMetering } from "src/L1/interfaces/IResourceMetering.sol";
import { Constants } from "src/libraries/Constants.sol";
import { ProxyAdmin } from "src/universal/ProxyAdmin.sol";
import { AddressManager } from "src/legacy/AddressManager.sol";
......@@ -168,7 +171,7 @@ contract DeployOPChainOutput {
require(false, "DeployOPChainOutput: not implemented");
}
function checkOutput() public view {
function checkOutput(DeployOPChainInput _doi) public view {
// With 16 addresses, we'd get a stack too deep error if we tried to do this inline as a
// single call to `Solarray.addresses`. So we split it into two calls.
address[] memory addrs1 = Solarray.addresses(
......@@ -192,6 +195,8 @@ contract DeployOPChainOutput {
address(_delayedWETHPermissionlessGameProxy)
);
DeployUtils.assertValidContractAddresses(Solarray.extend(addrs1, addrs2));
assertValidDeploy(_doi);
}
function opChainProxyAdmin() public view returns (ProxyAdmin) {
......@@ -273,6 +278,46 @@ contract DeployOPChainOutput {
DeployUtils.assertValidContractAddress(address(_delayedWETHPermissionlessGameProxy));
return _delayedWETHPermissionlessGameProxy;
}
// -------- Assertions on chain architecture --------
function assertValidDeploy(DeployOPChainInput _doi) internal view {
// TODO Add other assertions from ChainAssertions.sol here
assertValidSystemConfig(_doi);
}
function assertValidSystemConfig(DeployOPChainInput _doi) internal view {
// forgefmt: disable-start
require(systemConfigProxy().owner() == _doi.systemConfigOwner(), "SC10");
require(systemConfigProxy().basefeeScalar() == _doi.basefeeScalar(), "SC20");
require(systemConfigProxy().blobbasefeeScalar() == _doi.blobBaseFeeScalar(), "SC30");
require(systemConfigProxy().batcherHash() == bytes32(uint256(uint160(_doi.batcher()))), "SC40");
require(systemConfigProxy().gasLimit() == uint64(30000000), "SC50");// TODO allow other gas limits?
require(systemConfigProxy().unsafeBlockSigner() == _doi.unsafeBlockSigner(), "SC60");
require(systemConfigProxy().scalar() >> 248 == 1, "SC70");
IResourceMetering.ResourceConfig memory rconfig = Constants.DEFAULT_RESOURCE_CONFIG();
IResourceMetering.ResourceConfig memory outputConfig = systemConfigProxy().resourceConfig();
require(outputConfig.maxResourceLimit == rconfig.maxResourceLimit, "SC80");
require(outputConfig.elasticityMultiplier == rconfig.elasticityMultiplier, "SC90");
require(outputConfig.baseFeeMaxChangeDenominator == rconfig.baseFeeMaxChangeDenominator, "SC100");
require(outputConfig.systemTxMaxGas == rconfig.systemTxMaxGas, "SC110");
require(outputConfig.minimumBaseFee == rconfig.minimumBaseFee, "SC120");
require(outputConfig.maximumBaseFee == rconfig.maximumBaseFee, "SC130");
require(systemConfigProxy().startBlock() == block.number, "SC140");
require(systemConfigProxy().batchInbox() == _doi.opsm().chainIdToBatchInboxAddress(_doi.l2ChainId()), "SC150");
require(systemConfigProxy().l1CrossDomainMessenger() == address(l1CrossDomainMessengerProxy()), "SC160");
require(systemConfigProxy().l1ERC721Bridge() == address(l1ERC721BridgeProxy()), "SC170");
require(systemConfigProxy().l1StandardBridge() == address(l1StandardBridgeProxy()), "SC180");
require(systemConfigProxy().disputeGameFactory() == address(disputeGameFactoryProxy()), "SC190");
require(systemConfigProxy().optimismPortal() == address(optimismPortalProxy()), "SC200");
require(systemConfigProxy().optimismMintableERC20Factory() == address(optimismMintableERC20FactoryProxy()), "SC210");
(address gasPayingToken,) = systemConfigProxy().gasPayingToken();
require(gasPayingToken == Constants.ETHER, "SC220");
// forgefmt: disable-end
}
}
contract DeployOPChain is Script {
......@@ -345,7 +390,7 @@ contract DeployOPChain is Script {
_doo.delayedWETHPermissionlessGameProxy.selector, address(deployOutput.delayedWETHPermissionlessGameProxy)
);
_doo.checkOutput();
_doo.checkOutput(_doi);
}
// -------- Utilities --------
......
......@@ -205,7 +205,7 @@ contract DeploySuperchainOutput is CommonBase {
// This function can be called to ensure all outputs are correct. Similar to `writeOutputFile`,
// it fetches the output values using external calls to the getter methods for safety.
function checkOutput() public {
function checkOutput(DeploySuperchainInput) public {
address[] memory addrs = Solarray.addresses(
address(this.superchainProxyAdmin()),
address(this.superchainConfigImpl()),
......@@ -298,7 +298,7 @@ contract DeploySuperchain is Script {
transferProxyAdminOwnership(_dsi, _dso);
// Output assertions, to make sure outputs were assigned correctly.
_dso.checkOutput();
_dso.checkOutput(_dsi);
}
// -------- Deployment Steps --------
......
......@@ -32,8 +32,8 @@
"sourceCodeHash": "0xde4df0f9633dc0cdb1c9f634003ea5b0f7c5c1aebc407bc1b2f44c0ecf938649"
},
"src/L1/OPStackManager.sol": {
"initCodeHash": "0x8081ca5dd48497b74758d1425ad6f025d6fd3cb144b4c5d4335b9a04e78b8474",
"sourceCodeHash": "0xb5fb50a9ddf8c0aee6d0e545f8ef4528f27698f3522cab744cd44ffaef6364d2"
"initCodeHash": "0x31378d0ea88a358f517927f5bc530a5b958f43c5462e4a822388ec70bd45e8d4",
"sourceCodeHash": "0xf8ac1ce2fbaa9aa21bd6c60e0bf863b06ea3bdd1c4ed801ed70ee64c1b534f67"
},
"src/L1/OptimismPortal.sol": {
"initCodeHash": "0xb7a7a28d5b3b88334e7cb4bc1c5fbbf9f691d934e907a2fed6a30e461eb1c0f6",
......
......@@ -55,6 +55,25 @@
"stateMutability": "view",
"type": "function"
},
{
"inputs": [
{
"internalType": "uint256",
"name": "_l2ChainId",
"type": "uint256"
}
],
"name": "chainIdToBatchInboxAddress",
"outputs": [
{
"internalType": "address",
"name": "",
"type": "address"
}
],
"stateMutability": "pure",
"type": "function"
},
{
"inputs": [
{
......@@ -439,17 +458,6 @@
"name": "AddressHasNoCode",
"type": "error"
},
{
"inputs": [
{
"internalType": "string",
"name": "addressName",
"type": "string"
}
],
"name": "AddressMismatch",
"type": "error"
},
{
"inputs": [
{
......
......@@ -55,6 +55,25 @@
"stateMutability": "view",
"type": "function"
},
{
"inputs": [
{
"internalType": "uint256",
"name": "_l2ChainId",
"type": "uint256"
}
],
"name": "chainIdToBatchInboxAddress",
"outputs": [
{
"internalType": "address",
"name": "",
"type": "address"
}
],
"stateMutability": "pure",
"type": "function"
},
{
"inputs": [
{
......@@ -439,17 +458,6 @@
"name": "AddressHasNoCode",
"type": "error"
},
{
"inputs": [
{
"internalType": "string",
"name": "addressName",
"type": "string"
}
],
"name": "AddressMismatch",
"type": "error"
},
{
"inputs": [
{
......
......@@ -2,10 +2,13 @@
pragma solidity 0.8.15;
import { Blueprint } from "src/libraries/Blueprint.sol";
import { Constants } from "src/libraries/Constants.sol";
import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
import { ISemver } from "src/universal/interfaces/ISemver.sol";
import { IResourceMetering } from "src/L1/interfaces/IResourceMetering.sol";
import { Proxy } from "src/universal/Proxy.sol";
import { ProxyAdmin } from "src/universal/ProxyAdmin.sol";
import { SuperchainConfig } from "src/L1/SuperchainConfig.sol";
......@@ -145,9 +148,6 @@ contract OPStackManager is ISemver, Initializable {
// -------- Errors --------
/// @notice Throw when two addresses do not match but are expected to.
error AddressMismatch(string addressName);
/// @notice Thrown when an address is the zero address.
error AddressNotFound(address who);
......@@ -203,6 +203,20 @@ contract OPStackManager is ISemver, Initializable {
bytes32 salt = bytes32(_input.l2ChainId);
DeployOutput memory output;
// -------- TODO: Placeholders --------
// For contracts we don't yet deploy, we set the outputs to dummy proxies so they have code to pass assertions.
// We do these first, that way the disputeGameFactoryProxy is set when passed to the SystemConfig input.
output.disputeGameFactoryProxy = DisputeGameFactory(deployProxy(l2ChainId, output.opChainProxyAdmin, "1"));
output.disputeGameFactoryImpl = DisputeGameFactory(deployProxy(l2ChainId, output.opChainProxyAdmin, "2"));
output.anchorStateRegistryProxy = AnchorStateRegistry(deployProxy(l2ChainId, output.opChainProxyAdmin, "3"));
output.anchorStateRegistryImpl = AnchorStateRegistry(deployProxy(l2ChainId, output.opChainProxyAdmin, "4"));
output.faultDisputeGame = FaultDisputeGame(deployProxy(l2ChainId, output.opChainProxyAdmin, "5"));
output.permissionedDisputeGame = PermissionedDisputeGame(deployProxy(l2ChainId, output.opChainProxyAdmin, "6"));
output.delayedWETHPermissionedGameProxy =
DelayedWETH(payable(deployProxy(l2ChainId, output.opChainProxyAdmin, "7")));
output.delayedWETHPermissionlessGameProxy =
DelayedWETH(payable(deployProxy(l2ChainId, output.opChainProxyAdmin, "8")));
// -------- Deploy Chain Singletons --------
// The ProxyAdmin is the owner of all proxies for the chain. We temporarily set the owner to
......@@ -264,11 +278,6 @@ contract OPStackManager is ISemver, Initializable {
upgradeAndCall(output.opChainProxyAdmin, address(output.optimismMintableERC20FactoryProxy), impl.logic, data);
impl = getLatestImplementation("L1CrossDomainMessenger");
// TODO add this check back in
// require(
// impl.logic == referenceAddressManager.getAddress("OVM_L1CrossDomainMessenger"),
// "OpStackManager: L1CrossDomainMessenger implementation mismatch"
// );
data = encodeL1CrossDomainMessengerInitializer(impl.initializer, output);
upgradeAndCall(output.opChainProxyAdmin, address(output.l1CrossDomainMessengerProxy), impl.logic, data);
......@@ -276,45 +285,10 @@ contract OPStackManager is ISemver, Initializable {
data = encodeL1StandardBridgeInitializer(impl.initializer, output);
upgradeAndCall(output.opChainProxyAdmin, address(output.l1StandardBridgeProxy), impl.logic, data);
// -------- TODO: Placeholders --------
// For contracts we don't yet deploy, we set the outputs to dummy proxies so they have code to pass assertions.
output.disputeGameFactoryProxy = DisputeGameFactory(deployProxy(l2ChainId, output.opChainProxyAdmin, "1"));
output.disputeGameFactoryImpl = DisputeGameFactory(deployProxy(l2ChainId, output.opChainProxyAdmin, "2"));
output.anchorStateRegistryProxy = AnchorStateRegistry(deployProxy(l2ChainId, output.opChainProxyAdmin, "3"));
output.anchorStateRegistryImpl = AnchorStateRegistry(deployProxy(l2ChainId, output.opChainProxyAdmin, "4"));
output.faultDisputeGame = FaultDisputeGame(deployProxy(l2ChainId, output.opChainProxyAdmin, "5"));
output.permissionedDisputeGame = PermissionedDisputeGame(deployProxy(l2ChainId, output.opChainProxyAdmin, "6"));
output.delayedWETHPermissionedGameProxy =
DelayedWETH(payable(deployProxy(l2ChainId, output.opChainProxyAdmin, "7")));
output.delayedWETHPermissionlessGameProxy =
DelayedWETH(payable(deployProxy(l2ChainId, output.opChainProxyAdmin, "8")));
// -------- Finalize Deployment --------
// Transfer ownership of the ProxyAdmin from this contract to the specified owner.
output.opChainProxyAdmin.transferOwnership(_input.roles.opChainProxyAdminOwner);
// Correctness checks.
// TODO these currently fail in tests because the tests use dummy implementation addresses that have no code.
// if (output.systemConfigProxy.owner() != _input.roles.systemConfigOwner) {
// revert AddressMismatch("systemConfigOwner");
// }
// if (output.systemConfigProxy.l1CrossDomainMessenger() != address(output.l1CrossDomainMessengerProxy)) {
// revert AddressMismatch("l1CrossDomainMessengerProxy");
// }
// if (output.systemConfigProxy.l1ERC721Bridge() != address(output.l1ERC721BridgeProxy)) {
// revert AddressMismatch("l1ERC721BridgeProxy");
// }
// if (output.systemConfigProxy.l1StandardBridge() != address(output.l1StandardBridgeProxy)) {
// revert AddressMismatch("l1StandardBridgeProxy");
// }
// if (output.systemConfigProxy.optimismPortal() != address(output.optimismPortalProxy)) {
// revert AddressMismatch("optimismPortalProxy");
// }
// if (
// output.systemConfigProxy.optimismMintableERC20Factory() !=
// address(output.optimismMintableERC20FactoryProxy)
// ) revert AddressMismatch("optimismMintableERC20FactoryProxy");
return output;
}
......@@ -337,7 +311,7 @@ contract OPStackManager is ISemver, Initializable {
/// configuration's convention. This convention is `versionByte || keccak256(bytes32(chainId))[:19]`,
/// where || denotes concatenation`, versionByte is 0x00, and chainId is a uint256.
/// https://specs.optimism.io/protocol/configurability.html#consensus-parameters
function chainIdToBatchInboxAddress(uint256 _l2ChainId) internal pure returns (address) {
function chainIdToBatchInboxAddress(uint256 _l2ChainId) public pure returns (address) {
bytes1 versionByte = 0x00;
bytes32 hashedChainId = keccak256(bytes.concat(bytes32(_l2ChainId)));
bytes19 first19Bytes = bytes19(hashedChainId);
......@@ -403,7 +377,7 @@ contract OPStackManager is ISemver, Initializable {
DeployOutput memory _output
)
internal
pure
view
virtual
returns (bytes memory)
{
......@@ -474,31 +448,33 @@ contract OPStackManager is ISemver, Initializable {
DeployOutput memory _output
)
internal
pure
view
virtual
returns (ResourceMetering.ResourceConfig memory, SystemConfig.Addresses memory)
returns (ResourceMetering.ResourceConfig memory resourceConfig_, SystemConfig.Addresses memory opChainAddrs_)
{
// TODO do any of these need to be configurable? are these values correct?
ResourceMetering.ResourceConfig memory referenceResourceConfig = ResourceMetering.ResourceConfig({
maxResourceLimit: 2e7,
elasticityMultiplier: 10,
baseFeeMaxChangeDenominator: 8,
minimumBaseFee: 1e9,
systemTxMaxGas: 1e6,
maximumBaseFee: 340282366920938463463374607431768211455
});
// We use assembly to easily convert from IResourceMetering.ResourceConfig to ResourceMetering.ResourceConfig.
// This is required because we have not yet fully migrated the codebase to be interface-based.
IResourceMetering.ResourceConfig memory resourceConfig = Constants.DEFAULT_RESOURCE_CONFIG();
assembly ("memory-safe") {
resourceConfig_ := resourceConfig
}
SystemConfig.Addresses memory opChainAddrs = SystemConfig.Addresses({
opChainAddrs_ = SystemConfig.Addresses({
l1CrossDomainMessenger: address(_output.l1CrossDomainMessengerProxy),
l1ERC721Bridge: address(_output.l1ERC721BridgeProxy),
l1StandardBridge: address(_output.l1StandardBridgeProxy),
disputeGameFactory: address(_output.disputeGameFactoryProxy),
optimismPortal: address(_output.optimismPortalProxy),
optimismMintableERC20Factory: address(_output.optimismMintableERC20FactoryProxy),
gasPayingToken: address(0)
gasPayingToken: Constants.ETHER
});
return (referenceResourceConfig, opChainAddrs);
assertValidContractAddress(opChainAddrs_.l1CrossDomainMessenger);
assertValidContractAddress(opChainAddrs_.l1ERC721Bridge);
assertValidContractAddress(opChainAddrs_.l1StandardBridge);
assertValidContractAddress(opChainAddrs_.disputeGameFactory);
assertValidContractAddress(opChainAddrs_.optimismPortal);
assertValidContractAddress(opChainAddrs_.optimismMintableERC20Factory);
}
/// @notice Makes an external call to the target to initialize the proxy with the specified data.
......
......@@ -25,7 +25,7 @@ contract OPStackManagerInterop is OPStackManager {
DeployOutput memory _output
)
internal
pure
view
virtual
override
returns (bytes memory)
......
......@@ -328,7 +328,7 @@ contract DeployImplementations_Test is Test {
// Ensure that `checkOutput` passes. This is called by the `run` function during execution,
// so this just acts as a sanity check. It reverts on failure.
dio.checkOutput();
dio.checkOutput(dii);
}
function testFuzz_run_largeChallengePeriodSeconds_reverts(uint256 _challengePeriodSeconds) public {
......
......@@ -360,7 +360,7 @@ contract DeployOPChain_TestBase is Test {
dsi.set(dsi.requiredProtocolVersion.selector, requiredProtocolVersion);
dsi.set(dsi.recommendedProtocolVersion.selector, recommendedProtocolVersion);
DeployImplementations deployImplementations = new DeployImplementations();
DeployImplementations deployImplementations = createDeployImplementationsContract();
(DeployImplementationsInput dii, DeployImplementationsOutput dio) = deployImplementations.etchIOContracts();
deployOPChain = new DeployOPChain();
......
......@@ -250,7 +250,7 @@ contract DeploySuperchain_Test is Test {
// Ensure that `checkOutput` passes. This is called by the `run` function during execution,
// so this just acts as a sanity check. It reverts on failure.
dso.checkOutput();
dso.checkOutput(dsi);
}
function test_run_io_succeeds() public {
......
......@@ -846,6 +846,7 @@ contract Specification_Test is CommonTest {
_addSpec({ _name: "OPStackManager", _sel: OPStackManager.initialize.selector });
_addSpec({ _name: "OPStackManager", _sel: OPStackManager.deploy.selector });
_addSpec({ _name: "OPStackManager", _sel: OPStackManager.blueprints.selector });
_addSpec({ _name: "OPStackManager", _sel: OPStackManager.chainIdToBatchInboxAddress.selector });
// OPStackManagerInterop
_addSpec({ _name: "OPStackManagerInterop", _sel: _getSel("version()") });
......@@ -857,6 +858,7 @@ contract Specification_Test is CommonTest {
_addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.initialize.selector });
_addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.deploy.selector });
_addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.blueprints.selector });
_addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.chainIdToBatchInboxAddress.selector });
// DeputyGuardianModule
_addSpec({
......
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