Skip to content

Conversation

@ManuelBilbao
Copy link
Contributor

Motivation

Url is the correct type for the EthClient

Description

Replace all String uses with Url when used for EthClient

@ManuelBilbao ManuelBilbao self-assigned this Oct 27, 2025
Copilot AI review requested due to automatic review settings October 27, 2025 15:54
@ManuelBilbao ManuelBilbao requested a review from a team as a code owner October 27, 2025 15:54
@ManuelBilbao ManuelBilbao added the tech debt Refactors, cleanups, etc label Oct 27, 2025
@github-actions github-actions bot added the L2 Rollup client label Oct 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the EthClient to use Url type instead of String for RPC URLs, improving type safety and making the API more explicit about expecting valid URLs.

Key Changes:

  • Updated EthClient::new() and related constructors to accept Url instead of &str/String
  • Removed internal URL parsing logic from EthClient::new_with_config() since URLs are now pre-validated
  • Updated all call sites across the codebase to parse URLs before passing to EthClient

Reviewed Changes

Copilot reviewed 21 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/networking/rpc/clients/eth/mod.rs Modified EthClient constructors to accept Url type and removed internal URL parsing
crates/l2/sequencer/configs.rs Changed EthConfig.rpc_url field type from Vec<String> to Vec<Url>
cmd/ethrex/l2/options.rs Updated EthOptions.rpc_url to Vec<Url> and modified default initialization
cmd/ethrex/l2/deployer.rs Changed DeployerOptions.rpc_url to Url type
cmd/ethrex/l2/initializers.rs Added URL parsing when creating L2 RPC URL from address and port
crates/l2/sequencer/*.rs Updated multiple sequencer components to work with Url type
crates/l2/tests/*.rs Added URL parsing for test client initialization
tooling//src/.rs Updated tooling code to parse URLs before creating EthClient
*/Cargo.toml Added url workspace dependency where needed

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +176 to +177
Url::parse(&format!("http://{}:{}", opts.http_addr, opts.http_port))
.expect("Error parsing RPC URL")
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The error message 'Error parsing RPC URL' should include the actual URL that failed to parse for easier debugging. Consider using unwrap_or_else to include the formatted URL in the panic message.

Suggested change
Url::parse(&format!("http://{}:{}", opts.http_addr, opts.http_port))
.expect("Error parsing RPC URL")
let url_str = format!("http://{}:{}", opts.http_addr, opts.http_port);
Url::parse(&url_str).unwrap_or_else(|e| panic!("Error parsing RPC URL '{}': {}", url_str, e))

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Oct 27, 2025

Lines of code report

Total lines added: 27
Total lines removed: 7
Total lines changed: 34

Detailed view
+-------------------------------------------------+-------+------+
| File                                            | Lines | Diff |
+-------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/deployer.rs                | 1080  | +2   |
+-------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs            | 368   | +2   |
+-------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/options.rs                 | 1028  | +2   |
+-------------------------------------------------+-------+------+
| ethrex/crates/l2/monitor/app.rs                 | 496   | +10  |
+-------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_watcher.rs        | 422   | +4   |
+-------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/metrics.rs           | 165   | +1   |
+-------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/mod.rs               | 235   | +2   |
+-------------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/utils.rs             | 186   | +1   |
+-------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/clients/eth/mod.rs | 608   | -7   |
+-------------------------------------------------+-------+------+
| ethrex/tooling/load_test/src/main.rs            | 358   | +1   |
+-------------------------------------------------+-------+------+
| ethrex/tooling/reorgs/src/simulator.rs          | 470   | +2   |
+-------------------------------------------------+-------+------+

@ManuelBilbao ManuelBilbao added this pull request to the merge queue Oct 28, 2025
Merged via the queue into main with commit 963889b Oct 28, 2025
44 checks passed
@ManuelBilbao ManuelBilbao deleted the eth_client_url branch October 28, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L2 Rollup client tech debt Refactors, cleanups, etc

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants