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

support for --relaxed-rpc in anvil #5490

Closed
wants to merge 4 commits into from

Conversation

aathan
Copy link
Contributor

@aathan aathan commented Jul 27, 2023

Motivation

Sometimes you have no choice but to deal with an upstream rpc provider that does not strictly conform to the standards. It would be nice to be able to tell anvil to be a little more forgiving in those scenarios.

Solution

Ultimately, this means having Providers that can vary behavior based on parameters. I've submitted an upstream patch to ethers-rs to support this (with an initial implementation that has a single const bool generic type param for "strictness") here: gakonst/ethers-rs#2527, which is necessary to compile this patch.

@aathan
Copy link
Contributor Author

aathan commented Jul 27, 2023

I'm new to Rust. There may be better ways to achieve the behavior polymorphism. I didn't want to use a dyn trait because that would introduce dynamic dispatch carelessly, and I got compiler warnings that impl traits are unstable, so this uses a ProviderTypes enum.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this doesn't look too bad.

supportive, because this comes up regularly, especially on new testnet nodes.

a few nits

common/src/provider.rs Outdated Show resolved Hide resolved
common/src/provider.rs Outdated Show resolved Hide resolved
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

some comments, supportive to have parity with hardhat's support of these non-compliant rpc providers

anvil/src/cmd.rs Outdated Show resolved Hide resolved
common/src/provider.rs Outdated Show resolved Hide resolved
common/src/provider.rs Outdated Show resolved Hide resolved
common/src/provider.rs Outdated Show resolved Hide resolved
anvil/src/config.rs Outdated Show resolved Hide resolved
@aathan
Copy link
Contributor Author

aathan commented Jul 28, 2023

I've implemented your suggestions. If you want more changes to the docs/comments please do so as you please ... note that I kept the original comments vague in most locations "downstream" of clap precisely because this option is likely to become vague down the road. Right now it only affects whether "method" can appear in the rpc json response dict, but in the future it might be 10 other things. Do we really want to update every comment everywhere every time there is a new "relaxation"? Maybe. Up to you...

@aathan
Copy link
Contributor Author

aathan commented Jul 28, 2023

Should #5458 be reopened?

@aathan
Copy link
Contributor Author

aathan commented Aug 24, 2023

Do I need to refactor the ethers-rs feature into a non-generic runtime-bool to make this feature happen? I left a comment over on that project.

Meanwhile, I brought this PR up to date, hopefully the changes to ethers-rs can be approved and then this can be merged.

This is an imperfect PR that does not address some bigger problems I see in the codebase. Specifically, that remote addresses (urls, ips, whatever) are stored basically bare, and then, providers are instantiated from them.

When those new providers are created, there's no attempt to mimic the connectivity features relevant to successfully communicating with that far end. Timeouts, relaxed/strict rpc handling, ssl certs, dialects, versions, whatever.

Every place that stores a "fork_url" w/ or w/o separate connectivity parameters should probably instead store a "RemoteConnInfo" struct that either contains or references enough info from which to build connectivity providers. I did notice there are both ForkUrl and RpcUrl types floating around.

@Evalir
Copy link
Member

Evalir commented Sep 18, 2023

Will also close this as stale, mainly because we are now knee deep in the larger ecosystem migration to Alloy, and eventually we'll move to replacing provider code.

We should set as an objective to support this flag—albeit doing more work on this PR to bring it to a mergeable state is probably not worth it anymore as it will soon be changed. Thanks for the work though! It was good quality—sorry for taking so long.

@Evalir Evalir closed this Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants