[SHIP-3521] Add tests to L2EP contracts to improve test coverage (#14341

)

* Add tests trying to improve test coverage

* Increate testing and clean up

* prettier

* Update MockBaseSequencerUptimeFeed.sol

* Create stale-cougars-approve.md

* Update l2ep.gas-snapshot

* [Bot] Update changeset file with jira issue

* Revert "Update l2ep.gas-snapshot"

This reverts commit 6e0d211.

* Update l2ep.gas-snapshot

* fix comments and clean up scroll tests

* Update l2ep.gas-snapshot

* Update ZKSyncSequencerUptimeFeed.t.sol

* fix gas snapshot

* Update ScrollSequencerUptimeFeed.t.sol

* Update ScrollSequencerUptimeFeed.t.sol

* Update l2ep.gas-snapshot

* rename l2ep -> shared to l2ep -> base

* cleanup

* address feedback

* Renames tests to follow Solidity convetions

* [Bot] Update changeset file with jira issues

---------

Co-authored-by: app-token-issuer-infra-releng[bot] <120227048+app-token-issuer-infra-releng[bot]@users.noreply.github.com>
Co-authored-by: Mohamed Mehany <7327188+mohamed-mehany@users.noreply.github.com>

.github/workflows/solidity-foundry.yml

@@ -33,7 +33,7 @@ jobs:
             { "name": "ccip", "setup": { "run-coverage": true, "min-coverage": 98.8, "extra-coverage-params": "--no-match-path='*End2End*'", "run-gas-snapshot": true, "run-forge-fmt": true }},
             { "name": "functions", "setup": { "run-coverage": false, "min-coverage": 98.5, "run-gas-snapshot": true, "run-forge-fmt": false }},
             { "name": "keystone", "setup": { "run-coverage": true, "min-coverage": 72.8, "run-gas-snapshot": false, "run-forge-fmt": false }},
-            { "name": "l2ep", "setup": { "run-coverage": true, "min-coverage": 61.0, "run-gas-snapshot": true, "run-forge-fmt": false }},
+            { "name": "l2ep", "setup": { "run-coverage": true, "min-coverage": 65.0, "run-gas-snapshot": true, "run-forge-fmt": false }},
             { "name": "liquiditymanager", "setup": { "run-coverage": true, "min-coverage": 44, "run-gas-snapshot": true, "run-forge-fmt": false }},
             { "name": "llo-feeds", "setup": { "run-coverage": true, "min-coverage": 49.3, "run-gas-snapshot": true, "run-forge-fmt": false }},
             { "name": "operatorforwarder", "setup": { "run-coverage": true, "min-coverage": 55.7, "run-gas-snapshot": true, "run-forge-fmt": false }},

contracts/.changeset/thirty-rules-rule.md

@@ -0,0 +1,10 @@
+---
+'@chainlink/contracts': patch
+---
+
+[L2EP] Refactor tests and fix file exclusion for coverage
+
+
+PR issue: SHIP-3521
+
+Solidity Review issue: SHIP-4050

contracts/gas-snapshots/l2ep.gas-snapshot

@@ -34,6 +34,18 @@ ArbitrumSequencerUptimeFeed_UpdateStatus:test_RevertIfNotL2CrossDomainMessengerA
 ArbitrumSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndNoTimeChange() (gas: 114527)
 ArbitrumSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndTimeChange() (gas: 114610)
 ArbitrumValidator_Validate:test_PostSequencerOffline() (gas: 69009)
+BaseSequencerUptimeFeed_AggregatorV3Interface:test_AggregatorV3Interface_ReturnsValidData() (gas: 69197)
+BaseSequencerUptimeFeed_AggregatorV3Interface:test_getAnswer_ReturnsValidAnswer() (gas: 57300)
+BaseSequencerUptimeFeed_AggregatorV3Interface:test_getTimestamp_ReturnsValidTimestamp() (gas: 57151)
+BaseSequencerUptimeFeed_Constructor:test_Constructor_InitialState() (gas: 22050)
+BaseSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_ProtectReads_AllowWhen_Whitelisted() (gas: 589517)
+BaseSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_ProtectReads_DisallowWhen_NotWhitelisted() (gas: 562342)
+BaseSequencerUptimeFeed_UpdateStatus:test_updateStatus_IgnoreOutOfOrderUpdates() (gas: 69490)
+BaseSequencerUptimeFeed_UpdateStatus:test_updateStatus_UpdateWhen_NoStatusChangeSameTimestamp() (gas: 77166)
+BaseSequencerUptimeFeed_UpdateStatus:test_updateStatus_UpdateWhen_StatusChangeAndNoTimeChange() (gas: 96021)
+BaseSequencerUptimeFeed_UpdateStatus:test_updateStatus_UpdateWhen_StatusChangeAndTimeChange() (gas: 96100)
+BaseSequencerUptimeFeed_transferL1Sender:test_transferL1Sender_CorrectlyTransfersL1Sender() (gas: 1479310)
+BaseValidator_GetAndSetGasLimit:test_GetAndSetGasLimit_CorrectlyHandlesGasLimit() (gas: 20119)
 OptimismCrossDomainForwarder_AcceptL1Ownership:test_CallableByPendingL1Owner() (gas: 47110)
 OptimismCrossDomainForwarder_AcceptL1Ownership:test_NotCallableByNonPendingOwners() (gas: 22135)
 OptimismCrossDomainForwarder_Constructor:test_InitialState() (gas: 21947)
@@ -60,23 +72,10 @@ OptimismCrossDomainGovernor_TransferL1Ownership:test_CallableByL1Owner() (gas: 4
 OptimismCrossDomainGovernor_TransferL1Ownership:test_CallableByL1OwnerOrZeroAddress() (gas: 28733)
 OptimismCrossDomainGovernor_TransferL1Ownership:test_NotCallableByL2Owner() (gas: 16456)
 OptimismCrossDomainGovernor_TransferL1Ownership:test_NotCallableByNonOwners() (gas: 11044)
-OptimismSequencerUptimeFeed_AggregatorV3Interface:test_AggregatorV3Interface() (gas: 72286)
-OptimismSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetAnswerWhenRoundDoesNotExistYet() (gas: 17639)
-OptimismSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetRoundDataWhenRoundDoesNotExistYet() (gas: 17875)
-OptimismSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetTimestampWhenRoundDoesNotExistYet() (gas: 17628)
-OptimismSequencerUptimeFeed_Constructor:test_InitialState() (gas: 22002)
-OptimismSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_AggregatorV2V3InterfaceAllowReadsIfConsumingContractIsWhitelisted() (gas: 589475)
-OptimismSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_AggregatorV2V3InterfaceDisallowReadsIfConsumingContractIsNotWhitelisted() (gas: 562336)
-OptimismSequencerUptimeFeed_UpdateStatus:test_IgnoreOutOfOrderUpdates() (gas: 67804)
-OptimismSequencerUptimeFeed_UpdateStatus:test_RevertIfNotL2CrossDomainMessengerAddr() (gas: 13109)
-OptimismSequencerUptimeFeed_UpdateStatus:test_RevertIfNotL2CrossDomainMessengerAddrAndNotL1SenderAddr() (gas: 23523)
-OptimismSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenNoChange() (gas: 77205)
-OptimismSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndNoTimeChange() (gas: 96089)
-OptimismSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndTimeChange() (gas: 96172)
-OptimismValidator_SetGasLimit:test_CorrectlyUpdatesTheGasLimit() (gas: 18636)
-OptimismValidator_Validate:test_PostSequencerOffline() (gas: 74764)
-OptimismValidator_Validate:test_PostSequencerStatusWhenThereIsNotStatusChange() (gas: 74798)
-OptimismValidator_Validate:test_RevertsIfCalledByAnAccountWithNoAccess() (gas: 15556)
+OptimismSequencerUptimeFeed_Constructor:test_Constructor_InitialState() (gas: 1530881)
+OptimismSequencerUptimeFeed_ValidateSender:test_ValidateSender_UpdateStatusWhen_StatusChangeAndNoTimeChange() (gas: 17874)
+OptimismValidator_Validate:test_Validate_PostSequencerOffline() (gas: 74773)
+OptimismValidator_Validate:test_Validate_PostSequencerStatus_NoStatusChange() (gas: 74788)
 ScrollCrossDomainForwarder_AcceptL1Ownership:test_CallableByPendingL1Owner() (gas: 47196)
 ScrollCrossDomainForwarder_AcceptL1Ownership:test_NotCallableByNonPendingOwners() (gas: 22185)
 ScrollCrossDomainForwarder_Constructor:test_InitialState() (gas: 21623)
@@ -103,40 +102,12 @@ ScrollCrossDomainGovernor_TransferL1Ownership:test_CallableByL1Owner() (gas: 489
 ScrollCrossDomainGovernor_TransferL1Ownership:test_CallableByL1OwnerOrZeroAddress() (gas: 28791)
 ScrollCrossDomainGovernor_TransferL1Ownership:test_NotCallableByL2Owner() (gas: 16456)
 ScrollCrossDomainGovernor_TransferL1Ownership:test_NotCallableByNonOwners() (gas: 11044)
-ScrollSequencerUptimeFeed_AggregatorV3Interface:test_AggregatorV3Interface() (gas: 72309)
-ScrollSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetAnswerWhenRoundDoesNotExistYet() (gas: 17639)
-ScrollSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetRoundDataWhenRoundDoesNotExistYet() (gas: 17875)
-ScrollSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetTimestampWhenRoundDoesNotExistYet() (gas: 17628)
-ScrollSequencerUptimeFeed_Constructor:test_InitialState() (gas: 174353)
-ScrollSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_AggregatorV2V3InterfaceAllowReadsIfConsumingContractIsWhitelisted() (gas: 589475)
-ScrollSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_AggregatorV2V3InterfaceDisallowReadsIfConsumingContractIsNotWhitelisted() (gas: 562336)
-ScrollSequencerUptimeFeed_UpdateStatus:test_IgnoreOutOfOrderUpdates() (gas: 67850)
-ScrollSequencerUptimeFeed_UpdateStatus:test_RevertIfNotL2CrossDomainMessengerAddr() (gas: 13109)
-ScrollSequencerUptimeFeed_UpdateStatus:test_RevertIfNotL2CrossDomainMessengerAddrAndNotL1SenderAddr() (gas: 23523)
-ScrollSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenNoChange() (gas: 77251)
-ScrollSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndNoTimeChange() (gas: 96135)
-ScrollSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndTimeChange() (gas: 96218)
-ScrollValidator_SetGasLimit:test_CorrectlyUpdatesTheGasLimit() (gas: 18770)
-ScrollValidator_Validate:test_PostSequencerOffline() (gas: 78299)
-ScrollValidator_Validate:test_PostSequencerStatusWhenThereIsNotStatusChange() (gas: 78339)
-ScrollValidator_Validate:test_RevertsIfCalledByAnAccountWithNoAccess() (gas: 15556)
-ZKSyncSequencerUptimeFeed_AggregatorV3Interface:test_AggregatorV3Interface() (gas: 67090)
-ZKSyncSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetAnswerWhenRoundDoesNotExistYet() (gas: 17639)
-ZKSyncSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetRoundDataWhenRoundDoesNotExistYet() (gas: 17875)
-ZKSyncSequencerUptimeFeed_AggregatorV3Interface:test_RevertGetTimestampWhenRoundDoesNotExistYet() (gas: 17628)
-ZKSyncSequencerUptimeFeed_Constructor:test_InitialState() (gas: 22006)
-ZKSyncSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_AggregatorV2V3InterfaceAllowReadsIfConsumingContractIsWhitelisted() (gas: 589495)
-ZKSyncSequencerUptimeFeed_ProtectReadsOnAggregatorV2V3InterfaceFunctions:test_AggregatorV2V3InterfaceDisallowReadsIfConsumingContractIsNotWhitelisted() (gas: 562342)
-ZKSyncSequencerUptimeFeed_UpdateStatus:test_IgnoreOutOfOrderUpdates() (gas: 61883)
-ZKSyncSequencerUptimeFeed_UpdateStatus:test_RevertIfNotL2CrossDomainMessengerAddr() (gas: 13055)
-ZKSyncSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenNoChange() (gas: 71321)
-ZKSyncSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndNoTimeChange() (gas: 90176)
-ZKSyncSequencerUptimeFeed_UpdateStatus:test_UpdateStatusWhenStatusChangeAndTimeChange() (gas: 90259)
-ZKSyncValidator_Constructor:test_ConstructingRevertedWithInvalidChainId() (gas: 104069)
-ZKSyncValidator_Constructor:test_ConstructingRevertedWithZeroL1BridgeAddress() (gas: 81784)
-ZKSyncValidator_Constructor:test_ConstructingRevertedWithZeroL2UpdateFeedAddress() (gas: 81796)
-ZKSyncValidator_GetChainId:test_CorrectlyGetsTheChainId() (gas: 8346)
-ZKSyncValidator_GetSetL2GasPerPubdataByteLimit:test_CorrectlyGetsAndUpdatesTheGasPerPubdataByteLimit() (gas: 18896)
-ZKSyncValidator_Validate:test_PostSequencerOffline() (gas: 52210)
-ZKSyncValidator_Validate:test_PostSequencerStatusWhenThereIsNotStatusChange() (gas: 52279)
-ZKSyncValidator_Validate:test_RevertsIfCalledByAnAccountWithNoAccess() (gas: 15623)
+ScrollSequencerUptimeFeed_Constructor:test_Constructor_InitialState_WhenValidL2XDomainMessenger() (gas: 1511902)
+ScrollSequencerUptimeFeed_ValidateSender:test_ValidateSender_UpdateStatusWhen_StatusChangeAndNoTimeChange() (gas: 17864)
+ScrollValidator_Validate:test_Validate_PostSequencerOffline() (gas: 78317)
+ScrollValidator_Validate:test_Validate_PostSequencerStatus_NoStatusChange() (gas: 78338)
+ZKSyncSequencerUptimeFeed_ValidateSender:test_ValidateSender_SuccessWhen_SenderIsValid() (gas: 12611)
+ZKSyncValidator_GetChainId:test_GetChainId_CorrectlyGetsTheChainId() (gas: 8369)
+ZKSyncValidator_GetSetL2GasPerPubdataByteLimit:test_GetSetL2GasPerPubdataByteLimit_CorrectlyHandlesGasPerPubdataByteLimit() (gas: 18918)
+ZKSyncValidator_Validate:test_Validate_PostSequencerOffline() (gas: 52208)
+ZKSyncValidator_Validate:test_Validate_PostSequencerStatus_NoStatusChange() (gas: 52280)

contracts/src/v0.8/l2ep/shared/BaseSequencerUptimeFeed.sol → contracts/src/v0.8/l2ep/base/BaseSequencerUptimeFeed.sol

@@ -59,7 +59,7 @@ abstract contract BaseSequencerUptimeFeed is
   /// @param l1SenderAddress Address of the L1 contract that is permissioned to call this contract
   /// @param initialStatus The initial status of the feed
   constructor(address l1SenderAddress, bool initialStatus) {
-    // We neet to allow l1SenderAddress to be zero because this contract is deployed first
+    // We need to allow l1SenderAddress to be zero because this contract is deployed first
     // After deploying the validator contract, this contract will be updated with the correct L1 sender address
     _setL1Sender(l1SenderAddress);
 
@@ -81,9 +81,9 @@ abstract contract BaseSequencerUptimeFeed is
 
   /// @notice Set the allowed L1 sender for this contract to a new L1 sender
   /// @dev Can be disabled by setting the L1 sender as `address(0)`. Accessible only by owner.
-  /// @param to new L1 sender that will be allowed to call `updateStatus` on this contract
-  function transferL1Sender(address to) external virtual onlyOwner {
-    _setL1Sender(to);
+  /// @param newSender new L1 sender that will be allowed to call `updateStatus` on this contract
+  function transferL1Sender(address newSender) external virtual onlyOwner {
+    _setL1Sender(newSender);
   }
 
   /// @notice internal method that stores the L1 sender
@@ -164,13 +164,10 @@ abstract contract BaseSequencerUptimeFeed is
     return s_rounds[uint80(roundId)].startedAt;
   }
 
-  /**
-   * @notice Record a new status and timestamp if it has changed since the last round.
-   * @dev This function will revert if not called from `l1Sender` via the L1->L2 messenger.
-   *
-   * @param status Sequencer status
-   * @param timestamp Block timestamp of status update
-   */
+  /// @notice Record a new status and timestamp if it has changed since the last round.
+  /// @dev This function will revert if not called from `l1Sender` via the L1->L2 messenger.
+  /// @param status Sequencer status
+  /// @param timestamp Block timestamp of status update
   function updateStatus(bool status, uint64 timestamp) external override {
     _validateSender(s_l1Sender);
 

contracts/src/v0.8/l2ep/shared/BaseValidator.sol → contracts/src/v0.8/l2ep/base/BaseValidator.sol

@@ -24,7 +24,7 @@ abstract contract BaseValidator is SimpleWriteAccessController, AggregatorValida
   uint32 internal s_gasLimit;
 
   /// @param l1CrossDomainMessengerAddress address the L1CrossDomainMessenger contract address
-  /// @param l2UptimeFeedAddr the address of the SequencerUptimeFeed contract address
+  /// @param l2UptimeFeedAddr the address of the L2 SequencerUptimeFeed contract address
   /// @param gasLimit the gasLimit to use for sending a message from L1 to L2
   constructor(address l1CrossDomainMessengerAddress, address l2UptimeFeedAddr, uint32 gasLimit) {
     if (l1CrossDomainMessengerAddress == address(0)) {

contracts/src/v0.8/l2ep/optimism/OptimismSequencerUptimeFeed.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.19;
 
-import {BaseSequencerUptimeFeed} from "../shared/BaseSequencerUptimeFeed.sol";
+import {BaseSequencerUptimeFeed} from "../base/BaseSequencerUptimeFeed.sol";
 
 import {IL2CrossDomainMessenger} from "@eth-optimism/contracts/L2/messaging/IL2CrossDomainMessenger.sol";
 

contracts/src/v0.8/l2ep/optimism/OptimismValidator.sol

@@ -3,7 +3,7 @@ pragma solidity ^0.8.19;
 
 import {ISequencerUptimeFeed} from "./../interfaces/ISequencerUptimeFeed.sol";
 
-import {BaseValidator} from "../shared/BaseValidator.sol";
+import {BaseValidator} from "../base/BaseValidator.sol";
 
 import {IL1CrossDomainMessenger} from "@eth-optimism/contracts/L1/messaging/IL1CrossDomainMessenger.sol";
 

contracts/src/v0.8/l2ep/scroll/ScrollSequencerUptimeFeed.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.24;
 
-import {BaseSequencerUptimeFeed} from "../shared/BaseSequencerUptimeFeed.sol";
+import {BaseSequencerUptimeFeed} from "../base/BaseSequencerUptimeFeed.sol";
 
 import {IL2ScrollMessenger} from "@scroll-tech/contracts/L2/IL2ScrollMessenger.sol";
 

contracts/src/v0.8/l2ep/scroll/ScrollValidator.sol

@@ -3,7 +3,7 @@ pragma solidity 0.8.24;
 
 import {ISequencerUptimeFeed} from "../interfaces/ISequencerUptimeFeed.sol";
 
-import {BaseValidator} from "../shared/BaseValidator.sol";
+import {BaseValidator} from "../base/BaseValidator.sol";
 
 import {IL1MessageQueue} from "@scroll-tech/contracts/L1/rollup/IL1MessageQueue.sol";
 import {IL1ScrollMessenger} from "@scroll-tech/contracts/L1/IL1ScrollMessenger.sol";

contracts/src/v0.8/l2ep/test/mocks/MockBaseSequencerUptimeFeed.sol

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import {BaseSequencerUptimeFeed} from "../../base/BaseSequencerUptimeFeed.sol";
+
+contract MockBaseSequencerUptimeFeed is BaseSequencerUptimeFeed {
+  string public constant override typeAndVersion = "MockSequencerUptimeFeed 1.1.0-dev";
+
+  /// @dev this will be used for internal testing
+  bool private s_validateSenderShouldPass;
+
+  constructor(
+    address l1SenderAddress,
+    bool initialStatus,
+    bool validateSenderShouldPass
+  ) BaseSequencerUptimeFeed(l1SenderAddress, initialStatus) {
+    s_validateSenderShouldPass = validateSenderShouldPass;
+  }
+
+  function _validateSender(address /* l1Sender */) internal view override {
+    if (!s_validateSenderShouldPass) {
+      revert InvalidSender();
+    }
+  }
+}

contracts/src/v0.8/l2ep/test/mocks/MockBaseValidator.sol

@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import {BaseValidator} from "../../base/BaseValidator.sol";
+
+contract MockBaseValidator is BaseValidator {
+  string public constant override typeAndVersion = "MockValidator 1.1.0-dev";
+
+  constructor(
+    address l1CrossDomainMessengerAddress,
+    address l2UptimeFeedAddr,
+    uint32 gasLimit
+  ) BaseValidator(l1CrossDomainMessengerAddress, l2UptimeFeedAddr, gasLimit) {}
+
+  function validate(
+    uint256 /* previousRoundId */,
+    int256 /* previousAnswer */,
+    uint256 /* currentRoundId */,
+    int256 /* currentAnswer */
+  ) external view override checkAccess returns (bool) {
+    return true;
+  }
+}