-
Notifications
You must be signed in to change notification settings - Fork 212
feat: add fields l1_rpc_client and l2_rpc_client to SingleChainHost #3226
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Sentry. |
| .await | ||
| .map_err(|_| SingleChainHostError::Other("Failed to connect to L1 RPC endpoint"))? | ||
| } else { | ||
| return Err(SingleChainHostError::Other("L1 node address must be set")) |
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.
Ideally we wouldn't want to return an error in that case. Instead, can we enforce that the config specifies either an l1 rpc client or an l1 node address (by using an enum)?
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 host binary CLI application arguments.
#[derive(Default, Parser, Serialize, Clone, Debug)]
#[command(styles = cli_styles())]
pub struct SingleChainHost {How about we remove #[derive(Serialize)] and then apply the following change?
Before
/// Address of L1 JSON-RPC endpoint to use (eth and debug namespace required)
#[arg(
long,
visible_alias = "l1",
requires = "l2_node_address",
requires = "l1_beacon_address",
env
)]
pub l1_node_address: Option<String>,
/// Pre-configured L1 RPC client (not settable via CLI, used internally)
#[serde(skip)]
#[arg(skip)]
pub l1_rpc_client: Option<RpcClient>,After
#[arg(
long,
aliases = ["l1", "l1_node_address"]
visible_alias = "l1",
requires = "l2_node_address",
requires = "l1_beacon_address",
value_parser = parse_rpc_client,
env
)]
pub l1_rpc: RpcClient,
fn parse_rpc_client(s: &str) -> Result<RpcClient, String> {
todo!()
}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.
Looks good! Let me know when it's done
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.
Here’s my draft commit: kien-rise@ab470c6
I’ll test it, and once it’s confirmed to work, I’ll update this PR accordingly.
| .await | ||
| .map_err(|_| SingleChainHostError::Other("Failed to connect to L2 RPC endpoint"))? | ||
| } else { | ||
| return Err(SingleChainHostError::Other("L2 node address must be set")) |
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.
Same here
Resolves #3048
These changes are required to introduce throttling and retry logic to handle timeouts and 429 (Too Many Requests) errors from the L1 RPC.