refactor(l1, l2): use `Path` instead of `String` for path cli args #3770

mpaulucci · 2025-07-22T15:46:27Z

Motivation
Lets use Paths that are a higher abstraction for dealing with paths.

Description

Removed String paths in favor of Path and PathBuf
Renamed set_datadir to init_datadir.
Added default_datadir add added it to the default cli option so that you can see it when running ethrex --help
If jwt_secret_path is relative path, set it as part of the default path.

github-actions · 2025-07-22T15:48:42Z

Lines of code report

Total lines added: 38
Total lines removed: 8
Total lines changed: 46

Detailed view

+----------------------------------------------------+-------+------+
| File                                               | Lines | Diff |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/bench/import_blocks_benchmark.rs | 37    | -1   |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs                           | 451   | -4   |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/ethrex.rs                        | 134   | +1   |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs                  | 320   | +18  |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/command.rs                    | 474   | +3   |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/lib.rs                           | 6     | -2   |
+----------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/utils.rs                         | 162   | +16  |
+----------------------------------------------------+-------+------+
| ethrex/tooling/archive_sync/src/main.rs            | 287   | -1   |
+----------------------------------------------------+-------+------+

Copilot

Pull Request Overview

This PR refactors CLI path arguments to use Rust's Path and PathBuf types instead of raw strings, improving type safety and path handling. It also improves the datadir initialization process and JWT secret path resolution.

Replaces String path arguments with PathBuf across CLI options and function signatures
Renames set_datadir to init_datadir and adds default_datadir function for better path management
Updates error handling to use proper error types instead of panics

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
tooling/archive_sync/src/main.rs	Updates datadir argument to use PathBuf and new path utility functions
docs/l1/dev-setup-l1.md	Updates documentation to show expanded default datadir path
crates/storage/store.rs	Changes panic to proper error handling for genesis mismatch
cmd/ethrex/utils.rs	Adds path-based functions and improves JWT secret path handling
cmd/ethrex/lib.rs	Removes constant definitions that are now handled by functions
cmd/ethrex/l2/command.rs	Updates L2 commands to use PathBuf and new path utilities
cmd/ethrex/initializers.rs	Updates function signatures to accept Path types and improves logging
cmd/ethrex/ethrex.rs	Updates main ethrex binary to use new path handling
cmd/ethrex/cli.rs	Updates CLI argument types and path handling throughout
cmd/ethrex/bench/import_blocks_benchmark.rs	Updates benchmark to use new path utilities

Copilot · 2025-07-24T17:00:17Z

cmd/ethrex/l2/command.rs

-                let rollup_store = l2::initializers::init_rollup_store(&rollup_store_dir).await;
+                let store = init_store(&datadir, genesis).await;
+                let rollup_store =
+                    l2::initializers::init_rollup_store(rollup_store_dir.to_str().unwrap()).await;


Using .to_str().unwrap() can panic if the path contains invalid UTF-8. Consider using a more robust approach or handling the error case explicitly.

Suggested change

l2::initializers::init_rollup_store(rollup_store_dir.to_str().unwrap()).await;

l2::initializers::init_rollup_store(

rollup_store_dir

.to_str()

.ok_or_else(|| eyre::eyre!("Invalid UTF-8 in rollup_store_dir path"))?

).await;

Copilot · 2025-07-24T17:00:18Z

cmd/ethrex/l2/command.rs

@@ -518,15 +520,16 @@ impl Command {
                    info!("Private key not given, not updating contract.");
                }
                info!("Updating store...");
-                let rollup_store = l2::initializers::init_rollup_store(&rollup_store_dir).await;
+                let rollup_store =
+                    l2::initializers::init_rollup_store(rollup_store_dir.to_str().unwrap()).await;


Using .to_str().unwrap() can panic if the path contains invalid UTF-8. Consider using a more robust approach or handling the error case explicitly.

Copilot · 2025-07-24T17:00:18Z

cmd/ethrex/initializers.rs

+pub fn open_store(data_dir: &Path) -> Store {
+    if data_dir.ends_with("memory") {
+        Store::new(
+            data_dir.to_str().expect("Invalid data directory path"),


Using .to_str().expect() can panic if the path contains invalid UTF-8. Consider using a more robust approach or handling the error case explicitly.

Suggested change

data_dir.to_str().expect("Invalid data directory path"),

data_dir.to_str().ok_or_else(|| {

error!("Invalid data directory path: {:?}", data_dir);

"Invalid data directory path"

})?,

Copilot · 2025-07-24T17:00:18Z

cmd/ethrex/initializers.rs

-        Store::new(data_dir, engine_type).expect("Failed to create Store")
+        info!("Opening store at path: {:?}", data_dir);
+        Store::new(
+            data_dir.to_str().expect("Invalid data directory path"),


Using .to_str().expect() can panic if the path contains invalid UTF-8. Consider using a more robust approach or handling the error case explicitly.

Suggested change

data_dir.to_str().expect("Invalid data directory path"),

match data_dir.to_str() {

Some(path) => path,

None => {

error!("Data directory path contains invalid UTF-8: {:?}", data_dir);

panic!("Invalid data directory path");

}

},

Copilot · 2025-07-24T17:00:19Z

cmd/ethrex/utils.rs

+pub fn init_datadir(datadir: PathBuf) -> PathBuf {
+    if datadir.exists() {
+        if !datadir.is_dir() {
+            panic!("Datadir {:?} exists but is not a directory", datadir);
+        }
+    } else {
+        std::fs::create_dir_all(&datadir).expect("Failed to create data directory");
+    }
+    datadir


Using panic for error handling is not recommended. Consider returning a Result type and handling this error case gracefully.

Suggested change

pub fn init_datadir(datadir: PathBuf) -> PathBuf {

if datadir.exists() {

if !datadir.is_dir() {

panic!("Datadir {:?} exists but is not a directory", datadir);

}

} else {

std::fs::create_dir_all(&datadir).expect("Failed to create data directory");

}

datadir

pub fn init_datadir(datadir: PathBuf) -> io::Result<PathBuf> {

if datadir.exists() {

if !datadir.is_dir() {

return Err(io::Error::new(

io::ErrorKind::InvalidInput,

format!("Datadir {:?} exists but is not a directory", datadir),

));

}

} else {

std::fs::create_dir_all(&datadir)?;

}

Ok(datadir)

Copilot · 2025-07-24T17:00:19Z

cmd/ethrex/cli.rs

@@ -49,14 +48,13 @@ pub struct Options {
    #[arg(
        long = "datadir",
        value_name = "DATABASE_DIRECTORY",
-        help = "If the datadir is the word `memory`, ethrex will use the InMemory Engine",
-        default_value = DEFAULT_DATADIR,
+        default_value = default_datadir("ethrex"),
        help = "Receives the name of the directory where the Database is located.",
        long_help = "If the datadir is the word `memory`, ethrex will use the `InMemory Engine`.",
        help_heading = "Node options",
        env = "ETHREX_DATADIR"
    )]


Calling default_datadir() at compile time in the default_value attribute may not work as expected since it requires runtime evaluation. Consider using a default value function or handling this in the parsing logic.

Copilot · 2025-07-24T17:00:19Z

cmd/ethrex/cli.rs

@@ -223,8 +221,8 @@ impl Default for Options {
 pub enum Subcommand {
    #[command(name = "removedb", about = "Remove the database")]
    RemoveDB {
-        #[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value = DEFAULT_DATADIR, required = false)]
-        datadir: String,
+        #[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value = default_datadir("ethrex"), required = false)]


Calling default_datadir() at compile time in the default_value attribute may not work as expected since it requires runtime evaluation. Consider using a default value function or handling this in the parsing logic.

Suggested change

#[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value = default_datadir("ethrex"), required = false)]

#[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value_t = default_datadir("ethrex"), required = false)]

Copilot · 2025-07-24T17:00:19Z

cmd/ethrex/l2/command.rs

@@ -54,8 +55,8 @@ pub enum Command {
    },
    #[command(name = "removedb", about = "Remove the database", visible_aliases = ["rm", "clean"])]
    RemoveDB {
-        #[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value = DEFAULT_L2_DATADIR, required = false)]
-        datadir: String,
+        #[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value = default_datadir("ethrex"), required = false)]


Calling default_datadir() at compile time in the default_value attribute may not work as expected since it requires runtime evaluation. Consider using a default value function or handling this in the parsing logic.

Suggested change

#[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value = default_datadir("ethrex"), required = false)]

#[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value_t = default_datadir("ethrex"), required = false)]

Copilot · 2025-07-24T17:00:20Z

cmd/ethrex/l2/command.rs

@@ -111,12 +112,12 @@ pub enum Command {
        network: Network,
        #[arg(
            long = "datadir",
+            default_value = default_datadir("ethrex-l2"),


Calling default_datadir() at compile time in the default_value attribute may not work as expected since it requires runtime evaluation. Consider using a default value function or handling this in the parsing logic.

Suggested change

default_value = default_datadir("ethrex-l2"),

default_value_t = default_ethrex_l2_datadir(),

Copilot · 2025-07-24T17:00:20Z

tooling/archive_sync/src/main.rs

@@ -302,22 +302,21 @@ struct Args {
    #[arg(
        long = "datadir",
        value_name = "DATABASE_DIRECTORY",
-        help = "If the datadir is the word `memory`, ethrex will use the InMemory Engine",
-        default_value = DEFAULT_DATADIR,
+        default_value = default_datadir("ethrex"),


Calling default_datadir() at compile time in the default_value attribute may not work as expected since it requires runtime evaluation. Consider using a default value function or handling this in the parsing logic.

Suggested change

default_value = default_datadir("ethrex"),

default_value_t = default_datadir("ethrex"),

fmoletta

This changes the behaviour of the node:
For example, If I run:

cargo run --release --bin ethrex -- --http.addr 0.0.0.0 --network sepolia --authrpc.jwtsecret jwt.hex --datadir example

On main: A directory named example will be created at /Users/username/Library/Application Support(on Mac)
On this branch: A folder named example will be created at the crate's root

mpaulucci · 2025-07-25T17:27:29Z

This changes the behaviour of the node: For example, If I run:
cargo run --release --bin ethrex -- --http.addr 0.0.0.0 --network sepolia --authrpc.jwtsecret jwt.hex --datadir example
On main: A directory named example will be created at /Users/username/Library/Application Support(on Mac) On this branch: A folder named example will be created at the crate's root

good find. I think the PR behaviour is desirable. What do you think?

mpaulucci added 3 commits July 22, 2025 17:35

refactor(l1, l2): use PathBuf instead of String for path cli args.

45e57e9

Rename to init_datadir.

b03d7ee

Rename data_dir to datadir.

0919eea

github-actions bot assigned mpaulucci Jul 22, 2025

github-actions bot added the L2 Rollup client label Jul 22, 2025

github-project-automation bot added this to ethrex_l2 Jul 22, 2025

mpaulucci added 12 commits July 22, 2025 18:24

Roll back some changes.

98b5288

More minor fixes.

274c714

Make jwt a Path.

a45b247

Simplify path conversion.

2d97e80

Fix linting.

f05490d

Fix lint.

3e2805a

Properly init directory and add validations.

094b421

Merge branch 'main' into use-path

35fc4dd

Merge branch 'main' of github.com:lambdaclass/ethrex into use-path

5cb1f3a

Update readme.

7f7b1bf

Fix l2.

a32fd48

Merge branch 'main' into use-path

904db29

mpaulucci force-pushed the use-path branch from b9c5ae5 to e52eba4 Compare July 24, 2025 16:25

Try to revert this change.

7da3f3d

mpaulucci force-pushed the use-path branch from e52eba4 to 7da3f3d Compare July 24, 2025 16:26

mpaulucci changed the title ~~refactor(l1, l2): use PathBuf instead of String for path cli args~~ refactor(l1, l2): use Path instead of String for path cli args Jul 24, 2025

mpaulucci marked this pull request as ready for review July 24, 2025 16:58

Copilot AI review requested due to automatic review settings July 24, 2025 16:58

mpaulucci requested a review from a team as a code owner July 24, 2025 16:58

Copilot AI reviewed Jul 24, 2025

View reviewed changes

fmoletta reviewed Jul 25, 2025

View reviewed changes

mpaulucci mentioned this pull request Jul 29, 2025

refactor(levm): add execution flags to EF tests runner #3843

Merged

ilitteri moved this to In Review in ethrex_l2 Jul 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(l1, l2): use `Path` instead of `String` for path cli args #3770

refactor(l1, l2): use `Path` instead of `String` for path cli args #3770

Uh oh!

mpaulucci commented Jul 22, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 22, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

Copilot AI Jul 24, 2025

Uh oh!

fmoletta left a comment

Uh oh!

mpaulucci commented Jul 25, 2025

Uh oh!

Uh oh!

-            data_dir.to_str().expect("Invalid data directory path"),
+            match data_dir.to_str() {
+                Some(path) => path,
+                None => {
+                    error!("Data directory path contains invalid UTF-8: {:?}", data_dir);
+                    panic!("Invalid data directory path");
+                }
+            },

	#[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value = default_datadir("ethrex"), required = false)]
	#[arg(long = "datadir", value_name = "DATABASE_DIRECTORY", default_value_t = default_datadir("ethrex"), required = false)]

	default_value = default_datadir("ethrex-l2"),
	default_value_t = default_ethrex_l2_datadir(),

	default_value = default_datadir("ethrex"),
	default_value_t = default_datadir("ethrex"),

refactor(l1, l2): use Path instead of String for path cli args #3770

Are you sure you want to change the base?

refactor(l1, l2): use Path instead of String for path cli args #3770

Uh oh!

Conversation

mpaulucci commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Lines of code report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

fmoletta left a comment

Choose a reason for hiding this comment

Uh oh!

mpaulucci commented Jul 25, 2025

Uh oh!

Uh oh!

refactor(l1, l2): use `Path` instead of `String` for path cli args #3770

refactor(l1, l2): use `Path` instead of `String` for path cli args #3770

mpaulucci commented Jul 22, 2025 •

edited

Loading

github-actions bot commented Jul 22, 2025 •

edited

Loading