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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2ae9e2f
feat: add timeout flag and override default rpc timeout value
PanGan21 Oct 6, 2024
2a4d68b
Merge branch 'master' into cast-add-rpc-timeout-flag
PanGan21 Oct 6, 2024
99d442e
fix clippy
PanGan21 Oct 6, 2024
1666ac4
fix: move timeout to rpc args
PanGan21 Oct 6, 2024
eced274
Merge branch 'master' into cast-add-rpc-timeout-flag
PanGan21 Oct 6, 2024
285ea8c
Merge branch 'master' into cast-add-rpc-timeout-flag
PanGan21 Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions crates/cast/bin/cmd/rpc.rs
View file
Open in desktop
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.

cc @mattsse

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 •
edited
Loading

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 •
edited
Loading

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


#[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