-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: master
Are you sure you want to change the base?
Conversation
--timeout
option
/// 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>, |
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.
I think we should add this to Config and RpcOpts, so that it's always available in get_provider across the codebase
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.
cc @mattsse
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.
I tried this as first option in previous commit and it was conflicting with --timeout for other commands.
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.
So it would be possible but the flag should be named differently
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 other commands should also use the config's and RpcOpts timeout then
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.
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
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.
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.
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.
Motivation
Context: #8959
Solution
The following pr adds
--timeout
option to cast command overriding the default rpc timeout.