-
Notifications
You must be signed in to change notification settings - Fork 43
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
base: main
Are you sure you want to change the base?
Conversation
|
cmd/ethrex/launch.rs
Outdated
.map(Iterator::collect) | ||
.unwrap_or_default(); | ||
|
||
if network == "holesky" { |
There was a problem hiding this comment.
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
cmd/ethrex/launch.rs
Outdated
There was a problem hiding this comment.
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(...)
There was a problem hiding this comment.
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(...)
There was a problem hiding this comment.
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
This reverts commit 86418b0.
let tracker = TaskTracker::new(); | ||
let jwt_secret_clone = jwt_secret.clone(); | ||
cfg_if::cfg_if! { | ||
if #[cfg(feature = "based")] { |
There was a problem hiding this comment.
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.
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
launch.rs
with both the launcher of the L1 and the L2. Here we add a cfg to only compile the desired function.main()
in theethrex.rs
to a new fileutils.rs
.Closes #1987