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

refactor(l1,l2): separate L1 and L2 launch logic #2064

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

tomip01
Copy link
Contributor

@tomip01 tomip01 commented Feb 24, 2025

Motivation

This pull request introduces refactors to the ethrex.rs file with the launch of the L1 and L2. Separates both in two functions.

Description

  • We create a launch.rs with both the launcher of the L1 and the L2. Here we add a cfg to only compile the desired function.
  • We extract the functions outside the main() in the ethrex.rs to a new file utils.rs.

Closes #1987

Copy link

github-actions bot commented Feb 24, 2025

| File                                                     | Lines | Diff |
+----------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex/ethrex.rs     | 16    | -414 |
+----------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex/launch/l1.rs  | 332   | +332 |
+----------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex/launch/l2.rs  | 297   | +297 |
+----------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex/launch/mod.rs | 5     | +5   |
+----------------------------------------------------------+-------+------+
| /home/runner/work/ethrex/ethrex/cmd/ethrex/utils.rs      | 97    | +97  |
+----------------------------------------------------------+-------+------+

Total lines added: +731
Total lines removed: 414
Total lines changed: 1145

@tomip01 tomip01 changed the title Refactor/l1 l2 launch refactor(l1,l2): separate L1 and L2 launch logic Feb 24, 2025
@tomip01 tomip01 self-assigned this Feb 24, 2025
@tomip01 tomip01 added L2 Rollup client L1 Ethereum client refactor labels Feb 24, 2025
@tomip01 tomip01 marked this pull request as ready for review February 24, 2025 20:19
@tomip01 tomip01 requested a review from a team as a code owner February 24, 2025 20:19
.map(Iterator::collect)
.unwrap_or_default();

if network == "holesky" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These networks specific checks can be removed for L2. In general, a whole bunch of other initialization stuff related to p2p can be removed, though it might be a bit of work so it's fine to leave for another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd separate the logic into two different files, so it's easy to read each launcher code separately.

There could be a module launcher with l1.rs and l2.rs submodules each containing a launch function. Then the mod.rs would be:

pub mod l1;
#[cfg(feature = "l2")]
pub mod l2;

And its usage in ethrex would be:

l1::launch(...)
l2::launch(...)

Copy link
Contributor

@ilitteri ilitteri Feb 25, 2025

Choose a reason for hiding this comment

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

If you want to keep the names launch_l1 and launch_l2, that is okay too. Do what you think is and looks the best. If you keep launch_l1 and launch_l2, the usage should be:

launch_l2(...)
launch_l1(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's looks better, done 07789f4

@mpaulucci mpaulucci added tech debt Refactors, cleanups, etc and removed refactor labels Feb 25, 2025
let tracker = TaskTracker::new();
let jwt_secret_clone = jwt_secret.clone();
cfg_if::cfg_if! {
if #[cfg(feature = "based")] {
Copy link
Contributor

Choose a reason for hiding this comment

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

The based feature does not take part in L1, so we should keep it for the L2 launch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L1 Ethereum client L2 Rollup client tech debt Refactors, cleanups, etc
Projects
Status: No status
Status: No status
Development

Successfully merging this pull request may close these issues.

L2, L1: Refactor ethrex.rs's logic to have a launcher for L1 and another for L2
4 participants