Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DEPLOY-512]: Adds L2EP Foundry Tests #11683

Merged
merged 26 commits into from
Jan 20, 2024
Merged

Conversation

chris-de-leon-cll
Copy link
Contributor

@chris-de-leon-cll chris-de-leon-cll commented Jan 4, 2024

Overview

  • This PR adds Foundry test cases for the L2EP contracts (previously we were relying on Hardhat tests)
  • This PR is one of a few related to the L2EP modernization effort - the effort consists of the following components:
    • Migrate old L2EP hardhat tests to Foundry tests
    • Refactor the L2EP contracts such that:
      • They comply with the style guide
      • They are more extensible to other chains
      • They include the gas optimizations and other QoL improvements made for Scroll

Important Note(s)

  • The Foundry tests have been written such that they test the same scenarios as the Hardhat tests - the intention behind this was to reduce overhead and avoid having to reinvent the wheel
  • The Scroll and Optimism Foundry tests are almost identical - the only changes include the imports and contract names. Arbitrum is slightly different, and the test cases are not identical to Scroll and Optimism
  • If we expect future L2EP solutions will be similar to that of Scroll and Optimism, we may want to consider revising the these test cases such that we don't need to copy-paste the same code each time we integrate a new chain
  • The Hardhat testing suite included some test scenarios that manually measured gas usage. These have also been migrated to Foundry, but they might be unnecessary since Foundry already collects gas usage metrics. Any clarity on whether we should still include gas measuring tests in Foundry would be much appreciated!

Primary Change(s)

Here's a breakdown of some of the primary changes included in this PR:

  • Adds Foundry configs for L2EP
  • Adds ScrollValidator test
  • Adds ScrollSequencerUptimeFeed test
  • Adds ScrollCrossDomainGovernor test
  • Adds ScrollCrossDomainForwarder test
  • Adds OptimismValidator test
  • Adds OptimismSequencerUptimeFeed test
  • Adds OptimismCrossDomainGovernor test
  • Adds OptimismCrossDomainForwarder test
  • Adds ArbitrumValidator test
  • Adds ArbitrtumSequencerUptimeFeed test
  • Adds ArbitrumDomainGovernor test
  • Adds ArbitrumDomainForwarder test
  • Adds additional testing mocks
  • Adds documentation (see the README.md file in contracts/src/v0.8/l2ep/README.md)
  • Ensures Foundry tests comply with style guide

Copy link
Contributor

github-actions bot commented Jan 4, 2024

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@chris-de-leon-cll chris-de-leon-cll removed in progress making changes PR is not ready for review labels Jan 6, 2024
@RensR
Copy link
Contributor

RensR commented Jan 9, 2024

You probably want to add l2ep to the Foundry-using-projects list at the top of contracts/GNUmakefile and in CI .github/workflows/solidity-foundry.yml line 35. This will make CI run your new tests. You should also create a snapshot with

make snapshot

with export FOUNDRY_PROFILE=l2ep set. With that set you can run make snapshot and make wrappers for just l2ep, so it's very quick.

contract ArbitrumCrossDomainForwarderTest is L2EPTest {
/// Helper variables
address internal s_strangerAddr = vm.addr(0x1);
/// Helper variable(s)
address internal s_l1OwnerAddr = vm.addr(0x2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also move this one, just like the stranger

address internal s_mockL1OwnerAddr = vm.addr(0x1);
address internal s_strangerAddr = vm.addr(0x2);
/// Helper variable(s)
address internal s_mockL1OwnerAddr = vm.addr(0x2);
address internal s_deployerAddr = vm.addr(0x3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deployer address also seems to be useful in the higher level one

@RensR RensR added this pull request to the merge queue Jan 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2024
@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@RensR RensR added this pull request to the merge queue Jan 20, 2024
Merged via the queue into develop with commit e4bde64 Jan 20, 2024
95 checks passed
@RensR RensR deleted the feat/DEPLOY-512-l2ep-foundry branch January 20, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants