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

feat(cast): add --timeout option #9044

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
19 changes: 17 additions & 2 deletions crates/cast/bin/cmd/rpc.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::time::Duration;

use cast::Cast;
use clap::Parser;
use eyre::Result;
Expand Down Expand Up @@ -29,16 +31,29 @@ pub struct RpcArgs {
#[arg(long, short = 'w')]
raw: bool,

/// Timeout for the rpc request
///
/// The timeout will be used to override the default timeout for RPC.
/// Default value is 45s
#[arg(long, env = "ETH_RPC_TIMEOUT")]
pub timeout: Option<u64>,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add this to Config and RpcOpts, so that it's always available in get_provider across the codebase

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this as first option in previous commit and it was conflicting with --timeout for other commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it would be possible but the flag should be named differently

Copy link
Member

@DaniPopes DaniPopes Oct 7, 2024

Choose a reason for hiding this comment

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

The other commands should also use the config's and RpcOpts timeout then

Copy link
Contributor Author

@PanGan21 PanGan21 Oct 7, 2024

Choose a reason for hiding this comment

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

They map thought to a different Config property. (example Config::transaction_timeout)

Here is an example of failing tests when timeout was in RpcOpts:
https://github.com/foundry-rs/foundry/actions/runs/11200334838/job/31133848891

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @DaniPopes just following up on this so we can complete the feature.

Do you believe we should rename the flags of the other commands to something else which will allow to add this timeout to RpcOpts? Their functionality seem different to me so it doesn't seem wise to use the same RpcOpts::timeout for everything.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think so, a global timeout flag/config makes sense
cc @mattsse @klkvr for more opinions


#[command(flatten)]
rpc: RpcOpts,
}

impl RpcArgs {
pub async fn run(self) -> Result<()> {
let Self { raw, method, params, rpc } = self;
let Self { raw, method, params, rpc, timeout } = self;

let config = Config::from(&rpc);
let provider = utils::get_provider(&config)?;
let mut builder = utils::get_provider_builder(&config)?;

if let Some(timeout) = timeout {
builder = builder.timeout(Duration::from_secs(timeout));
}

let provider = builder.build()?;

let params = if raw {
if params.is_empty() {
Expand Down
Loading