Test Clean Up

packages/contracts-bedrock/README.md

@@ -194,3 +194,40 @@ After the initial Bedrock upgrade, contracts MUST use the following versioning s
 #### Exceptions
 
 We have made an exception to the `Semver` rule for the `WETH` contract to avoid making changes to a well-known, simple, and recognizable contract.
+
+### Tests
+
+Tests are written using Foundry.
+
+#### Organizing Principles
+
+- Solidity `contract`s are used to organize the test suite similar to how mocha uses describe.
+- Every non-trivial state changing function should have a separate contract for happy and sad path
+   tests. This helps to make it very obvious where there are not yet sad path tests.
+- Simpler functions like getters and setters are grouped together into test contracts.
+
+All contracts and functions should be consistently named following a convention.
+
+#### Contract Naming Conventions
+
+Test contracts should be named one of the following according to their use:
+
+- `TargetContract_Init` for contracts that perform basic setup to be reused in other test contracts.
+- `TargetContract_Function_Test` for contracts containing happy path tests for a given function.
+- `TargetContract_Function_TestFail` for contracts containing sad path tests for a given function.
+
+
+#### Test naming convention
+
+Test function names are split by underscores, into 3 or 4 parts. An example function name is `test_onlyOwner_callerIsNotOwner_reverts()`.
+
+The parts are: `[method]_[FunctionName]_[reason]_[success]`, where:
+
+- `[method]` is either `test`, `testFuzz`, or `testDiff`
+- `[FunctionName]` is the name of the function or higher level behavior being tested.
+- `[reason]` is an optional description for the behavior being tested.
+- `[status]` must be one of:
+  - `succeeds`: used for most happy path cases
+  - `reverts`: used for most sad path cases
+  - `works`: used for tests which include a mix of happy and sad assertions (these should be broken up if possible)
+  - `fails`: used for tests which 'fail' in some way other than reverting

packages/contracts-bedrock/contracts/test/AddressAliasHelper.t.sol

@@ -5,7 +5,7 @@ import { Test } from "forge-std/Test.sol";
 import { AddressAliasHelper } from "../vendor/AddressAliasHelper.sol";
 
 contract AddressAliasHelper_Test is Test {
-    function test_fuzz_roundtrip(address _address) external {
+    function testFuzz_applyAndUndo_succeeds(address _address) external {
         address aliased = AddressAliasHelper.applyL1ToL2Alias(_address);
         address unaliased = AddressAliasHelper.undoL1ToL2Alias(aliased);
         assertEq(_address, unaliased);

packages/contracts-bedrock/contracts/test/BenchmarkTest.t.sol

@@ -60,14 +60,14 @@ contract GasBenchMark_OptimismPortal is Portal_Initializer {
 }
 
 contract GasBenchMark_L1CrossDomainMessenger is Messenger_Initializer {
-    function test_L1MessengerSendMessage_benchmark_0() external {
+    function test_sendMessage_benchmark_0() external {
         // The amount of data typically sent during a bridge deposit.
         bytes
             memory data = hex"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff";
         L1Messenger.sendMessage(bob, data, uint32(100));
     }
 
-    function test_L1MessengerSendMessage_benchmark_1() external {
+    function test_sendMessage_benchmark_1() external {
         setPrevBaseFee(vm, address(op), INITIAL_BASE_FEE);
         // The amount of data typically sent during a bridge deposit.
         bytes

packages/contracts-bedrock/contracts/test/CrossDomainMessenger.t.sol

@@ -5,15 +5,15 @@ import { Messenger_Initializer, Reverter, CallerCaller } from "./CommonTest.t.so
 
 // CrossDomainMessenger_Test is for testing functionality which is common to both the L1 and L2
 // CrossDomainMessenger contracts. For simplicity, we use the L1 Messenger as the test contract.
-contract CrossDomainMessenger_Test is Messenger_Initializer {
+contract CrossDomainMessenger_BaseGas_Test is Messenger_Initializer {
     // Ensure that baseGas passes for the max value of _minGasLimit,
     // this is about 4 Billion.
-    function test_baseGas() external view {
+    function test_baseGas_succeeds() external view {
         L1Messenger.baseGas(hex"ff", type(uint32).max);
     }
 
     // Fuzz for other values which might cause a revert in baseGas.
-    function testFuzz_baseGas(uint32 _minGasLimit) external view {
+    function testFuzz_baseGas_succeeds(uint32 _minGasLimit) external view {
         L1Messenger.baseGas(hex"ff", _minGasLimit);
     }
 }

packages/contracts-bedrock/contracts/test/CrossDomainOwnable.t.sol

@@ -15,21 +15,17 @@ contract XDomainSetter is CrossDomainOwnable {
     }
 }
 
-contract CrossDomainOwnable_Test is CommonTest {
+contract CrossDomainOwnable_TestInit is CommonTest {
     XDomainSetter setter;
 
-    function setUp() external {
+    function setUp() public virtual {
         setter = new XDomainSetter();
     }
+}
 
-    // Check that the revert message is correct
-    function test_revertOnlyOwner() external {
-        vm.expectRevert("CrossDomainOwnable: caller is not the owner");
-        setter.set(1);
-    }
-
+contract CrossDomainOwnable_OnlyOwner_Test is CrossDomainOwnable_TestInit {
     // Check that making a call can set the value properly
-    function test_onlyOwner() external {
+    function test_onlyOwner_succeeds() external {
         assertEq(setter.value(), 0);
 
         vm.prank(AddressAliasHelper.applyL1ToL2Alias(setter.owner()));
@@ -38,7 +34,15 @@ contract CrossDomainOwnable_Test is CommonTest {
     }
 }
 
-contract CrossDomainOwnableThroughPortal_Test is Portal_Initializer {
+contract CrossDomainOwnable_OnlyOwner_TestFail is CrossDomainOwnable_TestInit {
+    // Check that the revert message is correct
+    function test_onlyOwner_callerIsNotOwner_reverts() external {
+        vm.expectRevert("CrossDomainOwnable: caller is not the owner");
+        setter.set(1);
+    }
+}
+
+contract CrossDomainOwnable_ThroughPortal_Test is Portal_Initializer {
     XDomainSetter setter;
 
     function setUp() public override {
@@ -48,7 +52,7 @@ contract CrossDomainOwnableThroughPortal_Test is Portal_Initializer {
         setter = new XDomainSetter();
     }
 
-    function test_depositTransaction_crossDomainOwner() external {
+    function test_checkOwner_isOwner_succeeds() external {
         vm.recordLogs();
 
         vm.prank(alice);

packages/contracts-bedrock/contracts/test/CrossDomainOwnable2.t.sol

@@ -16,21 +16,23 @@ contract XDomainSetter2 is CrossDomainOwnable2 {
     }
 }
 
-contract CrossDomainOwnable2_Test is Messenger_Initializer {
+contract CrossDomainOwnable2_TestInit is Messenger_Initializer {
     XDomainSetter2 setter;
 
     function setUp() public override {
         super.setUp();
         vm.prank(alice);
         setter = new XDomainSetter2();
     }
+}
 
-    function test_revertNotSetOnlyOwner() external {
+contract CrossDomainOwnable2_CheckOwner_TestFail is CrossDomainOwnable2_TestInit {
+    function test_checkOwner_notMessenger_reverts() external {
         vm.expectRevert("CrossDomainOwnable2: caller is not the messenger");
         setter.set(1);
     }
 
-    function test_revertNotSetOnlyOwner2() external {
+    function test_checkOwner_notOwner_reverts() external {
         // set the xdomain messenger storage slot
         bytes32 key = bytes32(uint256(204));
         bytes32 value = Bytes32AddressLib.fillLast12Bytes(address(setter));
@@ -41,7 +43,7 @@ contract CrossDomainOwnable2_Test is Messenger_Initializer {
         setter.set(1);
     }
 
-    function test_revertOnlyOwner() external {
+    function test_checkOwner_portalIsCaller_fails() external {
         uint240 nonce = 0;
         address sender = bob;
         address target = address(setter);
@@ -75,8 +77,10 @@ contract CrossDomainOwnable2_Test is Messenger_Initializer {
 
         assertEq(setter.value(), 0);
     }
+}
 
-    function test_onlyOwner() external {
+contract CrossDomainOwnable2_Test is CrossDomainOwnable2_TestInit {
+    function test_checkOwner_succeeds() external {
         address owner = setter.owner();
 
         // Simulate the L2 execution where the call is coming from

packages/contracts-bedrock/contracts/test/DeployerWhitelist.t.sol

@@ -12,12 +12,12 @@ contract DeployerWhitelist_Test is CommonTest {
     }
 
     // The owner should be address(0)
-    function test_owner() external {
+    function test_owner_succeeds() external {
         assertEq(list.owner(), address(0));
     }
 
     // The storage slot for the owner must be the same
-    function test_storageSlots() external {
+    function test_storageSlots_succeeds() external {
         vm.prank(list.owner());
         list.setOwner(address(1));
 

packages/contracts-bedrock/contracts/test/Encoding.t.sol

@@ -5,19 +5,31 @@ import { CommonTest } from "./CommonTest.t.sol";
 import { Types } from "../libraries/Types.sol";
 import { Encoding } from "../libraries/Encoding.sol";
 
-contract Encoding_Test is CommonTest {
+contract Encoding_TestInit is CommonTest {
     function setUp() external {
         _setUp();
     }
+}
+
+contract Encoding_EncodeVersionedNonce_TestFail is Encoding_TestInit {
+    // none
+}
 
-    function test_nonceVersioning(uint240 _nonce, uint16 _version) external {
+contract Encoding_EncodeVersionedNonce_Test is Encoding_TestInit {
+    function test_encodeVersionedNonce_succeeds(uint240 _nonce, uint16 _version) external {
         (uint240 nonce, uint16 version) = Encoding.decodeVersionedNonce(
             Encoding.encodeVersionedNonce(_nonce, _version)
         );
         assertEq(version, _version);
         assertEq(nonce, _nonce);
     }
+}
 
+contract Encoding_DecodeVersionedNonce_TestFail is Encoding_TestInit {
+    // none
+}
+
+contract Encoding_DecodeVersionedNonce_Test is Encoding_TestInit {
     function test_decodeVersionedNonce_differential(uint240 _nonce, uint16 _version) external {
         uint256 nonce = uint256(Encoding.encodeVersionedNonce(_nonce, _version));
         (uint256 decodedNonce, uint256 decodedVersion) = ffi.decodeVersionedNonce(nonce);
@@ -26,7 +38,25 @@ contract Encoding_Test is CommonTest {
 
         assertEq(_nonce, uint240(decodedNonce));
     }
+}
 
+contract Encoding_EncodeCrossDomainMessage_TestFail is Encoding_TestInit {
+    function test_encodeCrossDomainMessage_invalidVersion_reverts() external {
+        uint8 version = 3;
+        uint256 nonce = Encoding.encodeVersionedNonce(101, version);
+        vm.expectRevert("Encoding: unknown cross domain message version");
+        Encoding.encodeCrossDomainMessage(
+            nonce,
+            alice,
+            bob,
+            NON_ZERO_VALUE,
+            NON_ZERO_GASLIMIT,
+            NON_ZERO_DATA
+        );
+    }
+}
+
+contract Encoding_EncodeCrossDomainMessage_Test is Encoding_TestInit {
     function test_encodeCrossDomainMessage_differential(
         uint240 _nonce,
         uint8 _version,
@@ -59,7 +89,13 @@ contract Encoding_Test is CommonTest {
 
         assertEq(encoding, _encoding);
     }
+}
+
+contract Encoding_EncodeDepositTransaction_TestFail is Encoding_TestInit {
+    // none
+}
 
+contract Encoding_EncodeDepositTransaction_Test is Encoding_TestInit {
     function test_encodeDepositTransaction_differential(
         address _from,
         address _to,

packages/contracts-bedrock/contracts/test/FeeVault.t.sol

@@ -21,12 +21,12 @@ contract FeeVault_Test is Bridge_Initializer {
         vm.etch(Predeploys.L1_FEE_VAULT, address(new L1FeeVault(recipient)).code);
     }
 
-    function test_constructor() external {
+    function test_constructor_succeeds() external {
         assertEq(baseFeeVault.RECIPIENT(), recipient);
         assertEq(l1FeeVault.RECIPIENT(), recipient);
     }
 
-    function test_minWithdrawalAmount() external {
+    function test_minWithdrawalAmount_succeeds() external {
         assertEq(baseFeeVault.MIN_WITHDRAWAL_AMOUNT(), 10 ether);
         assertEq(l1FeeVault.MIN_WITHDRAWAL_AMOUNT(), 10 ether);
     }

packages/contracts-bedrock/contracts/test/GasPriceOracle.t.sol

@@ -6,7 +6,7 @@ import { GasPriceOracle } from "../L2/GasPriceOracle.sol";
 import { L1Block } from "../L2/L1Block.sol";
 import { Predeploys } from "../libraries/Predeploys.sol";
 
-contract GasPriceOracle_Test is CommonTest {
+contract GasPriceOracle_TestInit is CommonTest {
     event OverheadUpdated(uint256);
     event ScalarUpdated(uint256);
     event DecimalsUpdated(uint256);
@@ -50,31 +50,32 @@ contract GasPriceOracle_Test is CommonTest {
         });
     }
 
-    function test_l1BaseFee() external {
+    function test_l1BaseFee_succeeds() external {
         assertEq(gasOracle.l1BaseFee(), basefee);
     }
 
-    function test_gasPrice() external {
+    function test_gasPrice_succeeds() external {
         vm.fee(100);
         uint256 gasPrice = gasOracle.gasPrice();
         assertEq(gasPrice, 100);
     }
 
-    function test_baseFee() external {
+    function test_baseFee_succeeds() external {
         vm.fee(64);
         uint256 gasPrice = gasOracle.baseFee();
         assertEq(gasPrice, 64);
     }
 
-    function test_scalar() external {
+    function test_scalar_succeeds() external {
         assertEq(gasOracle.scalar(), l1FeeScalar);
     }
 
-    function test_overhead() external {
+    function test_overhead_succeeds() external {
         assertEq(gasOracle.overhead(), l1FeeOverhead);
     }
 
-    function test_setGasPriceReverts() external {
+    // Removed in bedrock
+    function test_setGasPrice_doesNotExist_reverts() external {
         (bool success, bytes memory returndata) = address(gasOracle).call(
             abi.encodeWithSignature("setGasPrice(uint256)", 1)
         );
@@ -83,7 +84,8 @@ contract GasPriceOracle_Test is CommonTest {
         assertEq(returndata, hex"");
     }
 
-    function test_setL1BaseFeeReverts() external {
+    // Removed in bedrock
+    function test_setL1BaseFee_doesNotExist_reverts() external {
         (bool success, bytes memory returndata) = address(gasOracle).call(
             abi.encodeWithSignature("setL1BaseFee(uint256)", 1)
         );