Skip to content

Commit

Permalink
feat: Make contract deployment names consistent with names used in OP…
Browse files Browse the repository at this point in the history 
…CM (#13563)

* feat: Fix contract labels and make more explicit

Fixes #13391

* fix: Initializable test

* update gas-snapshot

* feat: fix constructor tests

* fix: Initializable test

* feat: remove unused import

* fix: DeployOwnership script

* feat: cleanup

* feat: Label contracts in save() function

* feat: Add _removeSuffix

* Address feedback

* fix OptimismPortal2Impl name
  • Loading branch information
@maurelian
maurelian authored Jan 8, 2025
1 parent 9742e40 commit bc02e90
Show file tree
Hide file tree
Showing 14 changed files with 119 additions and 153 deletions.
25 changes: 14 additions & 11 deletions packages/contracts-bedrock/scripts/Artifacts.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { Vm } from "forge-std/Vm.sol";
import { Predeploys } from "src/libraries/Predeploys.sol";
import { Config } from "scripts/libraries/Config.sol";
import { StorageSlot } from "scripts/libraries/ForgeArtifacts.sol";
import { LibString } from "@solady/utils/LibString.sol";
import { ForgeArtifacts } from "scripts/libraries/ForgeArtifacts.sol";
import { Process } from "scripts/libraries/Process.sol";

Expand Down Expand Up @@ -191,6 +190,8 @@ abstract contract Artifacts {
_namedDeployments[_name] = deployment;
_newDeployments.push(deployment);
_appendDeployment(_name, _deployed);

vm.label(_deployed, _name);
}

/// @notice Adds a deployment to the temp deployments file
Expand All @@ -211,16 +212,18 @@ abstract contract Artifacts {
}

/// @notice Returns the value of the internal `_initialized` storage slot for a given contract.
function loadInitializedSlot(string memory _contractName) public returns (uint8 initialized_) {
address contractAddress = mustGetAddress(_contractName);

// Check if the contract name ends with `Proxy` and, if so override the contract name which is used to
// retrieve the storage layout.
if (LibString.endsWith(_contractName, "Proxy")) {
_contractName = LibString.slice(_contractName, 0, bytes(_contractName).length - 5);
}

StorageSlot memory slot = ForgeArtifacts.getInitializedSlot(_contractName);
/// @param _sourceName The name of the contract in the source code
/// @param _deploymentName The name used to save() the deployed contract
function loadInitializedSlot(
string memory _sourceName,
string memory _deploymentName
)
public
returns (uint8 initialized_)
{
address contractAddress = mustGetAddress(_deploymentName);

StorageSlot memory slot = ForgeArtifacts.getInitializedSlot(_sourceName);
bytes32 slotVal = vm.load(contractAddress, bytes32(slot.slot));
initialized_ = uint8((uint256(slotVal) >> (slot.offset * 8)) & 0xFF);
}
Expand Down
76 changes: 38 additions & 38 deletions packages/contracts-bedrock/scripts/deploy/Deploy.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,21 +130,21 @@ contract Deploy is Deployer {
}

/// @notice Returns the impl addresses, not reverting if any are unset.
function _impls() internal view returns (Types.ContractSet memory proxies_) {
proxies_ = Types.ContractSet({
L1CrossDomainMessenger: getAddress("L1CrossDomainMessenger"),
L1StandardBridge: getAddress("L1StandardBridge"),
L2OutputOracle: getAddress("L2OutputOracle"),
DisputeGameFactory: getAddress("DisputeGameFactory"),
DelayedWETH: getAddress("DelayedWETH"),
PermissionedDelayedWETH: getAddress("PermissionedDelayedWETH"),
AnchorStateRegistry: getAddress("AnchorStateRegistry"),
OptimismMintableERC20Factory: getAddress("OptimismMintableERC20Factory"),
OptimismPortal: getAddress("OptimismPortal2"),
SystemConfig: getAddress("SystemConfig"),
L1ERC721Bridge: getAddress("L1ERC721Bridge"),
ProtocolVersions: getAddress("ProtocolVersions"),
SuperchainConfig: getAddress("SuperchainConfig")
function _impls() internal view returns (Types.ContractSet memory impls_) {
impls_ = Types.ContractSet({
L1CrossDomainMessenger: getAddress("L1CrossDomainMessengerImpl"),
L1StandardBridge: getAddress("L1StandardBridgeImpl"),
L2OutputOracle: getAddress("L2OutputOracleImpl"),
DisputeGameFactory: getAddress("DisputeGameFactoryImpl"),
DelayedWETH: getAddress("DelayedWETHImpl"),
PermissionedDelayedWETH: getAddress("PermissionedDelayedWETHImpl"),
AnchorStateRegistry: getAddress("AnchorStateRegistryImpl"),
OptimismMintableERC20Factory: getAddress("OptimismMintableERC20FactoryImpl"),
OptimismPortal: getAddress("OptimismPortal2Impl"),
SystemConfig: getAddress("SystemConfigImpl"),
L1ERC721Bridge: getAddress("L1ERC721BridgeImpl"),
ProtocolVersions: getAddress("ProtocolVersionsImpl"),
SuperchainConfig: getAddress("SuperchainConfigImpl")
});
}

Expand All @@ -170,11 +170,11 @@ contract Deploy is Deployer {
console.log("Deploying a fresh OP Stack with existing SuperchainConfig and ProtocolVersions");

IProxy scProxy = IProxy(_superchainConfigProxy);
save("SuperchainConfig", scProxy.implementation());
save("SuperchainConfigImpl", scProxy.implementation());
save("SuperchainConfigProxy", _superchainConfigProxy);

IProxy pvProxy = IProxy(_protocolVersionsProxy);
save("ProtocolVersions", pvProxy.implementation());
save("ProtocolVersionsImpl", pvProxy.implementation());
save("ProtocolVersionsProxy", _protocolVersionsProxy);

_run({ _needsSuperchain: false });
Expand Down Expand Up @@ -258,21 +258,21 @@ contract Deploy is Deployer {
ds.run(dsi, dso);
save("SuperchainProxyAdmin", address(dso.superchainProxyAdmin()));
save("SuperchainConfigProxy", address(dso.superchainConfigProxy()));
save("SuperchainConfig", address(dso.superchainConfigImpl()));
save("SuperchainConfigImpl", address(dso.superchainConfigImpl()));
save("ProtocolVersionsProxy", address(dso.protocolVersionsProxy()));
save("ProtocolVersions", address(dso.protocolVersionsImpl()));
save("ProtocolVersionsImpl", address(dso.protocolVersionsImpl()));

// First run assertions for the ProtocolVersions and SuperchainConfig proxy contracts.
Types.ContractSet memory contracts = _proxies();
ChainAssertions.checkProtocolVersions({ _contracts: contracts, _cfg: cfg, _isProxy: true });
ChainAssertions.checkSuperchainConfig({ _contracts: contracts, _cfg: cfg, _isProxy: true, _isPaused: false });

// Then replace the ProtocolVersions proxy with the implementation address and run assertions on it.
contracts.ProtocolVersions = mustGetAddress("ProtocolVersions");
contracts.ProtocolVersions = mustGetAddress("ProtocolVersionsImpl");
ChainAssertions.checkProtocolVersions({ _contracts: contracts, _cfg: cfg, _isProxy: false });

// Finally replace the SuperchainConfig proxy with the implementation address and run assertions on it.
contracts.SuperchainConfig = mustGetAddress("SuperchainConfig");
contracts.SuperchainConfig = mustGetAddress("SuperchainConfigImpl");
ChainAssertions.checkSuperchainConfig({ _contracts: contracts, _cfg: cfg, _isPaused: false, _isProxy: false });
}

Expand Down Expand Up @@ -304,19 +304,18 @@ contract Deploy is Deployer {
}
di.run(dii, dio);

save("L1CrossDomainMessenger", address(dio.l1CrossDomainMessengerImpl()));
save("OptimismMintableERC20Factory", address(dio.optimismMintableERC20FactoryImpl()));
save("SystemConfig", address(dio.systemConfigImpl()));
save("L1StandardBridge", address(dio.l1StandardBridgeImpl()));
save("L1ERC721Bridge", address(dio.l1ERC721BridgeImpl()));
save("L1CrossDomainMessengerImpl", address(dio.l1CrossDomainMessengerImpl()));
save("OptimismMintableERC20FactoryImpl", address(dio.optimismMintableERC20FactoryImpl()));
save("SystemConfigImpl", address(dio.systemConfigImpl()));
save("L1StandardBridgeImpl", address(dio.l1StandardBridgeImpl()));
save("L1ERC721BridgeImpl", address(dio.l1ERC721BridgeImpl()));

// Fault proofs
save("OptimismPortal", address(dio.optimismPortalImpl()));
save("OptimismPortal2", address(dio.optimismPortalImpl()));
save("DisputeGameFactory", address(dio.disputeGameFactoryImpl()));
save("DelayedWETH", address(dio.delayedWETHImpl()));
save("PreimageOracle", address(dio.preimageOracleSingleton()));
save("Mips", address(dio.mipsSingleton()));
save("OptimismPortal2Impl", address(dio.optimismPortalImpl()));
save("DisputeGameFactoryImpl", address(dio.disputeGameFactoryImpl()));
save("DelayedWETHImpl", address(dio.delayedWETHImpl()));
save("PreimageOracleSingleton", address(dio.preimageOracleSingleton()));
save("MipsSingleton", address(dio.mipsSingleton()));
save("OPContractsManager", address(dio.opcm()));

Types.ContractSet memory contracts = _impls();
Expand All @@ -343,7 +342,7 @@ contract Deploy is Deployer {
ChainAssertions.checkOPContractsManager({
_contracts: contracts,
_opcm: OPContractsManager(mustGetAddress("OPContractsManager")),
_mips: IMIPS(mustGetAddress("Mips"))
_mips: IMIPS(mustGetAddress("MipsSingleton"))
});
if (_isInterop) {
ChainAssertions.checkSystemConfigInterop({ _contracts: contracts, _cfg: cfg, _isProxy: false });
Expand Down Expand Up @@ -376,7 +375,7 @@ contract Deploy is Deployer {
save("DisputeGameFactoryProxy", address(deployOutput.disputeGameFactoryProxy));
save("PermissionedDelayedWETHProxy", address(deployOutput.delayedWETHPermissionedGameProxy));
save("AnchorStateRegistryProxy", address(deployOutput.anchorStateRegistryProxy));
save("AnchorStateRegistry", address(deployOutput.anchorStateRegistryImpl));
save("AnchorStateRegistryImpl", address(deployOutput.anchorStateRegistryImpl));
save("PermissionedDisputeGame", address(deployOutput.permissionedDisputeGame));
save("OptimismPortalProxy", address(deployOutput.optimismPortalProxy));
save("OptimismPortal2Proxy", address(deployOutput.optimismPortalProxy));
Expand All @@ -391,7 +390,7 @@ contract Deploy is Deployer {
permissionlessGameImpl == address(0),
"Deploy: The PermissionlessDelayedWETH is already set by the OPCM, it is no longer necessary to deploy it separately."
);
address delayedWETHImpl = mustGetAddress("DelayedWETH");
address delayedWETHImpl = mustGetAddress("DelayedWETHImpl");
address delayedWETHPermissionlessGameProxy = deployERC1967ProxyWithOwner("DelayedWETHProxy", msg.sender);
vm.broadcast(msg.sender);
IProxy(payable(delayedWETHPermissionlessGameProxy)).upgradeToAndCall({
Expand Down Expand Up @@ -481,6 +480,7 @@ contract Deploy is Deployer {
_save: this,
_salt: _implSalt(),
_name: "DataAvailabilityChallenge",
_nick: "DataAvailabilityChallengeImpl",
_args: DeployUtils.encodeConstructor(abi.encodeCall(IDataAvailabilityChallenge.__constructor__, ()))
})
);
Expand All @@ -495,7 +495,7 @@ contract Deploy is Deployer {
function initializeSystemConfig() public broadcast {
console.log("Upgrading and initializing SystemConfig proxy");
address systemConfigProxy = mustGetAddress("SystemConfigProxy");
address systemConfig = mustGetAddress("SystemConfig");
address systemConfig = mustGetAddress("SystemConfigImpl");

bytes32 batcherHash = bytes32(uint256(uint160(cfg.batchSenderAddress())));

Expand Down Expand Up @@ -543,7 +543,7 @@ contract Deploy is Deployer {
function initializeDataAvailabilityChallenge() public broadcast {
console.log("Upgrading and initializing DataAvailabilityChallenge proxy");
address dataAvailabilityChallengeProxy = mustGetAddress("DataAvailabilityChallengeProxy");
address dataAvailabilityChallenge = mustGetAddress("DataAvailabilityChallenge");
address dataAvailabilityChallenge = mustGetAddress("DataAvailabilityChallengeImpl");

address finalSystemOwner = cfg.finalSystemOwner();
uint256 daChallengeWindow = cfg.daChallengeWindow();
Expand Down Expand Up @@ -727,7 +727,7 @@ contract Deploy is Deployer {
splitDepth: cfg.faultGameSplitDepth(),
clockExtension: Duration.wrap(uint64(cfg.faultGameClockExtension())),
maxClockDuration: Duration.wrap(uint64(cfg.faultGameMaxClockDuration())),
vm: IBigStepper(mustGetAddress("Mips")),
vm: IBigStepper(mustGetAddress("MipsSingleton")),
weth: weth,
anchorStateRegistry: IAnchorStateRegistry(mustGetAddress("AnchorStateRegistryProxy")),
l2ChainId: cfg.l2ChainID()
Expand Down
3 changes: 2 additions & 1 deletion packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ contract DeployOwnership is Deploy {
safeConfig: SafeConfig({ threshold: 1, owners: exampleGuardianOwners }),
deputyGuardianModuleConfig: DeputyGuardianModuleConfig({
deputyGuardian: mustGetAddress("FoundationOperationsSafe"),
superchainConfig: ISuperchainConfig(mustGetAddress("SuperchainConfig"))
superchainConfig: ISuperchainConfig(mustGetAddress("SuperchainConfigImpl"))
})
});
}
Expand Down Expand Up @@ -323,6 +323,7 @@ contract DeployOwnership is Deploy {
_save: this,
_salt: _implSalt(),
_name: "SuperchainConfig",
_nick: "SuperchainConfigImpl",
_args: DeployUtils.encodeConstructor(abi.encodeCall(ISuperchainConfig.__constructor__, ()))
})
);
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/L1/DataAvailabilityChallenge.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ contract DataAvailabilityChallengeTest is CommonTest {
// EntryPoint will revert if using amount > type(uint112).max.
vm.assume(sender != Preinstalls.EntryPoint_v060);
vm.assume(sender != address(dataAvailabilityChallenge));
vm.assume(sender != deploy.mustGetAddress("DataAvailabilityChallenge"));
vm.assume(sender != deploy.mustGetAddress("DataAvailabilityChallengeImpl"));
vm.deal(sender, amount);

vm.prank(sender);
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/L1/L1CrossDomainMessenger.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ contract L1CrossDomainMessenger_Test is CommonTest {
/// @notice Marked virtual to be overridden in
/// test/kontrol/deployment/DeploymentSummary.t.sol
function test_constructor_succeeds() external virtual {
IL1CrossDomainMessenger impl = IL1CrossDomainMessenger(deploy.mustGetAddress("L1CrossDomainMessenger"));
IL1CrossDomainMessenger impl = IL1CrossDomainMessenger(deploy.mustGetAddress("L1CrossDomainMessengerImpl"));
assertEq(address(impl.superchainConfig()), address(0));
assertEq(address(impl.PORTAL()), address(0));
assertEq(address(impl.portal()), address(0));
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/L1/L1ERC721Bridge.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ contract L1ERC721Bridge_Test is CommonTest {
/// @notice Marked virtual to be overridden in
/// test/kontrol/deployment/DeploymentSummary.t.sol
function test_constructor_succeeds() public virtual {
IL1ERC721Bridge impl = IL1ERC721Bridge(deploy.mustGetAddress("L1ERC721Bridge"));
IL1ERC721Bridge impl = IL1ERC721Bridge(deploy.mustGetAddress("L1ERC721BridgeImpl"));
assertEq(address(impl.MESSENGER()), address(0));
assertEq(address(impl.messenger()), address(0));
assertEq(address(impl.superchainConfig()), address(0));
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/L1/L1StandardBridge.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ contract L1StandardBridge_Initialize_Test is CommonTest {
/// @notice Marked virtual to be overridden in
/// test/kontrol/deployment/DeploymentSummary.t.sol
function test_constructor_succeeds() external virtual {
IL1StandardBridge impl = IL1StandardBridge(deploy.mustGetAddress("L1StandardBridge"));
IL1StandardBridge impl = IL1StandardBridge(deploy.mustGetAddress("L1StandardBridgeImpl"));
assertEq(address(impl.superchainConfig()), address(0));

// The constructor now uses _disableInitializers, whereas OP Mainnet has these values in storage
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/L1/OptimismPortal2.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract OptimismPortal2_Test is CommonTest {
/// @notice Marked virtual to be overridden in
/// test/kontrol/deployment/DeploymentSummary.t.sol
function test_constructor_succeeds() external virtual {
IOptimismPortal2 opImpl = IOptimismPortal2(payable(deploy.mustGetAddress("OptimismPortal2")));
IOptimismPortal2 opImpl = IOptimismPortal2(payable(deploy.mustGetAddress("OptimismPortal2Impl")));
assertEq(address(opImpl.disputeGameFactory()), address(0));
assertEq(address(opImpl.systemConfig()), address(0));
assertEq(address(opImpl.superchainConfig()), address(0));
Expand Down
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/test/L1/ProtocolVersions.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ contract ProtocolVersions_Initialize_Test is ProtocolVersions_Init {
skipIfForkTest(
"ProtocolVersions_Initialize_Test: cannot test initialization on forked network against hardhat config"
);
IProtocolVersions protocolVersionsImpl = IProtocolVersions(deploy.mustGetAddress("ProtocolVersions"));
IProtocolVersions protocolVersionsImpl = IProtocolVersions(deploy.mustGetAddress("ProtocolVersionsImpl"));
address owner = deploy.cfg().finalSystemOwner();

assertEq(ProtocolVersion.unwrap(protocolVersions.required()), ProtocolVersion.unwrap(required));
Expand All @@ -42,7 +42,7 @@ contract ProtocolVersions_Initialize_Test is ProtocolVersions_Init {

/// @dev Ensures that the events are emitted during initialization.
function test_initialize_events_succeeds() external {
IProtocolVersions protocolVersionsImpl = IProtocolVersions(deploy.mustGetAddress("ProtocolVersions"));
IProtocolVersions protocolVersionsImpl = IProtocolVersions(deploy.mustGetAddress("ProtocolVersionsImpl"));

// Wipe out the initialized slot so the proxy can be initialized again
vm.store(address(protocolVersions), bytes32(0), bytes32(0));
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/L1/SystemConfig.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract SystemConfig_Initialize_Test is SystemConfig_Init {
batcherHash = bytes32(uint256(uint160(deploy.cfg().batchSenderAddress())));
gasLimit = uint64(deploy.cfg().l2GenesisBlockGasLimit());
unsafeBlockSigner = deploy.cfg().p2pSequencerAddress();
systemConfigImpl = deploy.mustGetAddress("SystemConfig");
systemConfigImpl = deploy.mustGetAddress("SystemConfigImpl");
optimismMintableERC20Factory = deploy.mustGetAddress("OptimismMintableERC20FactoryProxy");
}

Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/L2/Predeploys.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ contract PredeploysBaseTest is CommonTest {
}

if (_isInitializable(addr)) {
assertEq(l2Genesis.loadInitializedSlot(cname), uint8(1));
assertEq(l2Genesis.loadInitializedSlot({ _sourceName: cname, _deploymentName: cname }), uint8(1));
}
}
}
Expand Down
Loading

0 comments on commit bc02e90

Please sign in to comment.