Skip to content

Commit

Permalink
contracts-bedrock: flake fix attempt (ethereum-optimism#9082)
Browse files Browse the repository at this point in the history
* contracts-bedrock: flake fix attempt

Progress towards fixing flakes in CI. Using `etch` to set contracts in
state instead of deploying will help to fix the nonce mismatches between
the L1 contract addresses and the L2 genesis that contains L1 contract
addresses.

Developers will need to run `forge clean` to remove artifacts for the
tests to pass after this commit is merged. It enforces a single version
of `FFIInterface` to be compiled instead of it being compiled with
multiple compiler versions. The `vm.getCode` family of cheats fail when
there are multiple compiled versions of the same contract because the
artfact has the compiler version appended to the name of the artifact.

This will also help to improve compiler time since the compiled
contracts do not need to include the compiled bytecode of the deploy and
ffiinterface contracts, the code is loaded dynamically instead of being
deployed with `CREATE`.

* contracts-bedrock: more cleanup

Deletes a dead file that is no longer needed and sets the deploy config
in state using `etch`. `DeployL2` is not used, we should be using the
`superchain-ops` repo for deploying new L2 contracts or deploy them
through L1. Since `DeployL2` used code using solc `0.8.19` (EAS) it
caused multiple versions of the `DeployConfig` to be compiled which
causes headaches when using `vm.getCode`.

* contracts-bedrock: update gas snapshot

* contracts-bedrock: more cleanup

* contracts-bedrock: gas-snapshot

* lint: fix
  • Loading branch information
tynes authored Jan 18, 2024
1 parent f648016 commit 78a5c4f
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 108 deletions.
18 changes: 9 additions & 9 deletions packages/contracts-bedrock/.gas-snapshot
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_0() (gas: 352278)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_1() (gas: 2950440)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 540648)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 4052841)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 442007)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_0() (gas: 352255)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_1() (gas: 2950462)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 540625)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 4052818)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 442029)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 3487756)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 55367)
GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 86629)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 68450)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 68899)
GasBenchMark_OptimismPortal:test_proveWithdrawalTransaction_benchmark() (gas: 153493)
GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 86651)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 68472)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 68909)
GasBenchMark_OptimismPortal:test_proveWithdrawalTransaction_benchmark() (gas: 153504)
12 changes: 8 additions & 4 deletions packages/contracts-bedrock/scripts/Deploy.s.sol
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,11 @@ import { EIP1967Helper } from "test/mocks/EIP1967Helper.sol";
/// Then add a call to that function inside of `run`. Be sure to call the `save` function after each
/// deployment so that hardhat-deploy style artifacts can be generated using a call to `sync()`.
/// The `CONTRACT_ADDRESSES_PATH` environment variable can be set to a path that contains a JSON file full of
/// contract name to address pairs. That enables this script to be much more flexible in the way
/// it is used.
/// contract name to address pairs. That enables this script to be much more flexible in the way it is used.
/// This contract must not have constructor logic because it is set into state using `etch`.
contract Deploy is Deployer {
DeployConfig public cfg;
DeployConfig public constant cfg =
DeployConfig(address(uint160(uint256(keccak256(abi.encode("optimism.deployconfig"))))));

using stdJson for string;

Expand Down Expand Up @@ -221,7 +222,10 @@ contract Deploy is Deployer {
super.setUp();

string memory path = string.concat(vm.projectRoot(), "/deploy-config/", deploymentContext, ".json");
cfg = new DeployConfig(path);
vm.etch(address(cfg), vm.getDeployedCode("DeployConfig.s.sol:DeployConfig"));
vm.label(address(cfg), "DeployConfig");
vm.allowCheatcodes(address(cfg));
cfg.read(path);

console.log("Deploying from %s", deployScript);
console.log("Deployment context: %s", deploymentContext);
Expand Down
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/scripts/DeployConfig.s.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
pragma solidity 0.8.15;

import { Script } from "forge-std/Script.sol";
import { console2 as console } from "forge-std/console2.sol";
Expand Down Expand Up @@ -59,7 +59,7 @@ contract DeployConfig is Script {
uint256 public requiredProtocolVersion;
uint256 public recommendedProtocolVersion;

constructor(string memory _path) {
function read(string memory _path) public {
console.log("DeployConfig: reading file %s", _path);
try vm.readFile(_path) returns (string memory data) {
_json = data;
Expand Down
69 changes: 0 additions & 69 deletions packages/contracts-bedrock/scripts/DeployL2.s.sol

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ import { MerkleTrie } from "src/libraries/trie/MerkleTrie.sol";
import { FFIInterface } from "test/setup/FFIInterface.sol";

contract MerkleTrie_get_Test is Test {
FFIInterface ffi;
FFIInterface constant ffi = FFIInterface(address(uint160(uint256(keccak256(abi.encode("optimism.ffi"))))));

function setUp() public {
ffi = new FFIInterface();
vm.etch(address(ffi), vm.getDeployedCode("FFIInterface.sol:FFIInterface"));
vm.label(address(ffi), "FFIInterface");
}

function test_get_validProof1_succeeds() external {
Expand Down
10 changes: 5 additions & 5 deletions packages/contracts-bedrock/test/setup/CommonTest.sol
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import { FFIInterface } from "test/setup/FFIInterface.sol";

/// @title CommonTest
/// @dev An extenstion to `Test` that sets up the optimism smart contracts.
contract CommonTest is Setup, Test, Events {
contract CommonTest is Test, Setup, Events {
address alice;
address bob;

bytes32 constant nonZeroHash = keccak256(abi.encode("NON_ZERO"));

FFIInterface ffi;
FFIInterface constant ffi = FFIInterface(address(uint160(uint256(keccak256(abi.encode("optimism.ffi"))))));

function setUp() public virtual override {
alice = makeAddr("alice");
Expand All @@ -23,8 +23,8 @@ contract CommonTest is Setup, Test, Events {
vm.deal(bob, 10000 ether);

Setup.setUp();
vm.prank(deployer);
ffi = new FFIInterface();
vm.etch(address(ffi), vm.getDeployedCode("FFIInterface.sol:FFIInterface"));
vm.label(address(ffi), "FFIInterface");

// Make sure the base fee is non zero
vm.fee(1 gwei);
Expand All @@ -36,7 +36,7 @@ contract CommonTest is Setup, Test, Events {
// Deploy L1
Setup.L1();
// Deploy L2
Setup.L2({ cfg: deploy.cfg() });
Setup.L2();
}

/// @dev Helper function that wraps `TransactionDeposited` event.
Expand Down
5 changes: 4 additions & 1 deletion packages/contracts-bedrock/test/setup/FFIInterface.sol
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
pragma solidity 0.8.15;

import { Types } from "src/libraries/Types.sol";
import { Vm } from "forge-std/Vm.sol";
import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";

/// @title FFIInterface
/// @notice This contract is set into state using `etch` and therefore must not have constructor logic.
/// It also MUST be compiled with `0.8.15` because `vm.getDeployedCode` will break if there
/// are multiple artifacts for different compiler versions.
contract FFIInterface {
Vm internal constant vm = Vm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);

Expand Down
24 changes: 8 additions & 16 deletions packages/contracts-bedrock/test/setup/Setup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@ import { SuperchainConfig } from "src/L1/SuperchainConfig.sol";
/// up behind proxies. In the future we will migrate to importing the genesis JSON
/// file that is created to set up the L2 contracts instead of setting them up manually.
contract Setup {
/// @notice The address of the foundry Vm contract.
Vm private constant vm = Vm(0x7109709ECfa91a80626fF3989D68f67F5b1DD12D);

Deploy internal deploy;
address deployer = address(0xd3607);
/// @notice The address of the Deploy contract. Set into state with `etch` to avoid
/// mutating any nonces. MUST not have constructor logic.
Deploy internal constant deploy = Deploy(address(uint160(uint256(keccak256(abi.encode("optimism.deploy"))))));

OptimismPortal optimismPortal;
L2OutputOracle l2OutputOracle;
Expand Down Expand Up @@ -77,21 +79,11 @@ contract Setup {
/// will also need to include the bytecode for the Deploy contract.
/// This is a hack as we are pushing solidity to the edge.
function setUp() public virtual {
deploy = Deploy(_create(vm.getCode("Deploy.s.sol:Deploy")));
vm.etch(address(deploy), vm.getDeployedCode("Deploy.s.sol:Deploy"));
vm.allowCheatcodes(address(deploy));
deploy.setUp();
}

/// @dev Simple wrapper around the `create` opcode that uses a particular
/// deployer account.
function _create(bytes memory _code) internal returns (address addr_) {
vm.deal(deployer, 1 ether);
vm.prank(deployer);
assembly {
addr_ := create(0, add(_code, 0x20), mload(_code))
}
require(addr_ != address(0), "Setup: cannot create");
}

/// @dev Sets up the L1 contracts.
function L1() public {
// Set the deterministic deployer in state to ensure that it is there
Expand Down Expand Up @@ -137,7 +129,7 @@ contract Setup {
}

/// @dev Sets up the L2 contracts. Depends on `L1()` being called first.
function L2(DeployConfig cfg) public {
function L2() public {
string memory allocsPath = string.concat(vm.projectRoot(), "/.testdata/genesis.json");
if (vm.isFile(allocsPath) == false) {
string[] memory args = new string[](3);
Expand All @@ -155,7 +147,7 @@ contract Setup {
vm.loadAllocs(allocsPath);

// Set the governance token's owner to be the final system owner
address finalSystemOwner = cfg.finalSystemOwner();
address finalSystemOwner = deploy.cfg().finalSystemOwner();
vm.prank(governanceToken.owner());
governanceToken.transferOwnership(finalSystemOwner);

Expand Down

0 comments on commit 78a5c4f

Please sign in to comment.