-
Notifications
You must be signed in to change notification settings - Fork 19
Dw/rpc flag handler #795
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
Dw/rpc flag handler #795
Conversation
nick1udwig
left a comment
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.
Style comment and a few nits. Correct the nits if you agree -- then feel free to merge after removing debugging stuff and confirming it still builds. Thanks for the nice work 🚀
| rpc_config.display() | ||
| ); | ||
|
|
||
| match std::fs::read_to_string(&rpc_config) { |
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.
match is a great construct, but style-wise it should be used sparingly when other control flow is available because of the double-indentation it causes. Especially when there are two main outcomes we care about, we have two good alternatives:
alternative 1:
if let Ok(contents) = std::fs::read_to_string(&rpc_config) {
...
}
...or (if we care about the non-Ok case, but don't need to unpack it)
if let Ok(...) ... {
} else {
}
...alternative 2 (my preference when it works (e.g. when you want to exit early in a non-Ok case):
let Ok(contents) = std::fs::read_to_string(&rpc_config) else {
...
};
...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.
Updated, and because there were nested matches it avoided the dreaded quadruple indentation.
|
|
||
| panic!( | ||
| "Error: runtime could not connect to Base ETH RPCs {rpc_urls:?}\n\ | ||
| "Error: runtime could not connect to any ETH RPC providers\n\ |
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.
Base is significant here because we need the Base RPC to bootstrap our node. Would prefer this went back to Base from any
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.
There doesn't seem to be anything in the original code that limits the connections to Base eth RPCs, and my code maintains the status quo in the that regard. I did update the message, but perhaps I should be enforcing some additional checks for chain ID 8453. I'll look at that after the current work is done.
| This is necessary in order to verify node identity onchain.\n\ | ||
| Please make sure you are using a valid WebSockets URL if using \ | ||
| the --rpc flag, and you are connected to the internet." | ||
| Please check your configured providers and internet connection." |
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.
We now have 2 or 3 ways users can configure RPC so idk that it makes sense to list them all here. Maybe we should link to docs about how to set RPC?
I suppose this means deciding on and maintaining a canonical discussion somewhere. Two options I can think of are
https://book.hyperware.ai/kit/boot-real-node.html?highlight=rpc#--rpc
https://github.com/hyperware-ai/hyperdrive?tab=readme-ov-file#configuring-the-eth-rpc-provider
but it would ultimately be nice to give users as helpful an error message as possible.
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.
Created an issue to address this.
|
All very good points! Because they aren't critical, I'll merge this branch (which has pulled in Tobias' Z index work as well) and clean these up in the PR for Issue #729 which should be done later today. |
Problem
inconsistent handling of --rpc vs --rpc-config flags
Solution
The handling of these flags has now been rationalized (and also made consistent with runtime handling as well). We start by recognizing that --rpc is just a special case (one provider, no auth) of what can be specified in --rpc-config. So only one or the other should be specified (if any), not both.
eth_provider_config will be potentially modified by these flags, adding ProviderConfigs at the top of the vector (after deduplicating them if needed), and preserving the order if multiple ProviderConfigs are specified in the --rpc-config file.
The calls to serve_register_fe and login_with_password now accept this whole eth_provider_config, not merely an RPC URL string. This propagates as required, eventually calling the new function connect_to_provider_from_config, which replaces the previous connect_to_provider function. Now it is possible for an authenticated connection to serve register and login with password. It will also attempt to use all of the saved/configured RPC providers, only falling back to the default if no others succeed.
Notes
I'll remove tagged debug logging before merging