-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(rpc): add Other(String) variant to RethRpcModule for custom RPC modules #17758
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
Conversation
634edcd to
7c0cd10
Compare
4c8b6a3 to
c231143
Compare
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.
introducing a new string based variant makes sense,
however we need a few additional cli level parse tests here that ensure that this behaves as expected,
and the validation part is a bit to restrict, if we skip ::Other then we likely dont need this?
however validation here is missing, the problematic part is something like --http.api eht which is clearly a typo that we currently dont catch,
since this is CLI specific, we need to make this behaviour somehow configurable.
my suggestion here would be introducing something liek
RethCliParsers {
type ChainSpec:
type RpcModules
}
and replacing C: ChainSpecParser with that
reth/crates/ethereum/cli/src/interface.rs
Line 30 in 6166209
| pub struct Cli<C: ChainSpecParser = EthereumChainSpecParser, Ext: clap::Args + fmt::Debug = NoArgs> |
then we can configure how rpc modules are parsed
makes total sense, done here 68f8abc |
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.
after reviewing I understood why we don't integrate an rpcmoduleparser, we can't really propagate the module parser down to the rpcserverargs because then we leak an unconstrained generic all over the place, right?
so perhaps we enforce this either when we do cli::parse or we enforce this in the Nodecommand::run somewhere early
so we do need to instead validate the parsed modules after somewhere, unsure where the proper location for that is.
maybe this should actually be enforced by the parser impl for the Cli itself?
| type ChainSpecParser: reth_cli::chainspec::ChainSpecParser + std::fmt::Debug; | ||
|
|
||
| /// The RPC module validator type. | ||
| type RpcModuleValidator: reth_rpc_server_types::RpcModuleValidator; |
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.
should we follow the same convention as ChainSpecParser here and move the parse_rpc_modules in the trait?
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.
makes sense, moved
| /// Check if two strings are likely typos using edit distance. | ||
| fn is_likely_typo(input: &str, target: &str) -> bool { | ||
| // Quick check: if lengths differ by more than 2, unlikely to be a typo | ||
| if input.len().abs_diff(target.len()) > 2 { | ||
| return false; | ||
| } | ||
|
|
||
| let distance = edit_distance(input, target); | ||
|
|
||
| // Consider it a typo if: | ||
| // - Distance is 1 (single char difference) | ||
| // - Distance is 2 and strings are at least 3 chars (for transpositions like "eth" -> "eht") | ||
| match distance { | ||
| 1 => true, | ||
| 2 => input.len() >= 3 && target.len() >= 3, | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| /// Calculate edit distance between two strings. | ||
| fn edit_distance(s1: &str, s2: &str) -> usize { |
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.
dont think we need this
either the user input is valid, or it's not, no need to check for typos
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.
ok, removed
crates/rpc/rpc-builder/src/lib.rs
Outdated
| // Skip Other variants - they should be registered via extend_rpc_modules | ||
| if matches!(namespace, RethRpcModule::Other(_)) { |
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 prefer introducing is_other functions than adding matches! for checks like
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.
yep, much beter, done
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 perhaps we enforce this either when we do cli::parse or we enforce this in the Nodecommand::run somewhere early
thx, great catch! now this is done after cli parsing, in with_runner_and_components during NodeCommand::run, ptal
| type ChainSpecParser: reth_cli::chainspec::ChainSpecParser + std::fmt::Debug; | ||
|
|
||
| /// The RPC module validator type. | ||
| type RpcModuleValidator: reth_rpc_server_types::RpcModuleValidator; |
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.
makes sense, moved
crates/rpc/rpc-builder/src/lib.rs
Outdated
| // Skip Other variants - they should be registered via extend_rpc_modules | ||
| if matches!(namespace, RethRpcModule::Other(_)) { |
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.
yep, much beter, done
| /// Check if two strings are likely typos using edit distance. | ||
| fn is_likely_typo(input: &str, target: &str) -> bool { | ||
| // Quick check: if lengths differ by more than 2, unlikely to be a typo | ||
| if input.len().abs_diff(target.len()) > 2 { | ||
| return false; | ||
| } | ||
|
|
||
| let distance = edit_distance(input, target); | ||
|
|
||
| // Consider it a typo if: | ||
| // - Distance is 1 (single char difference) | ||
| // - Distance is 2 and strings are at least 3 chars (for transpositions like "eth" -> "eht") | ||
| match distance { | ||
| 1 => true, | ||
| 2 => input.len() >= 3 && target.len() >= 3, | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| /// Calculate edit distance between two strings. | ||
| fn edit_distance(s1: &str, s2: &str) -> usize { |
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.
ok, removed
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.
great, I like this
I'd like for @klkvr to take a look as well before we merge this
since this is breaking we should time merging this appropriately
| vergen_build_timestamp: Cow::Owned(env!("VERGEN_BUILD_TIMESTAMP").to_string()), | ||
| vergen_cargo_target_triple: Cow::Owned(env!("VERGEN_CARGO_TARGET_TRIPLE").to_string()), | ||
| vergen_cargo_features: Cow::Owned(env!("VERGEN_CARGO_FEATURES").to_string()), | ||
| vergen_cargo_features: Cow::Owned(String::new()), |
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.
hmm not sure about this
| #[command(author, version =version_metadata().short_version.as_ref(), long_version = version_metadata().long_version.as_ref(), about = "Reth", long_about = None)] | ||
| pub struct Cli<C: ChainSpecParser = EthereumChainSpecParser, Ext: clap::Args + fmt::Debug = NoArgs> | ||
| { | ||
| pub struct Cli<P: RethCliParsers = EthereumCliParsers, Ext: clap::Args + fmt::Debug = NoArgs> { |
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'm wondering if a less invasive change would be to do Cli<C: ChainSpecParser = EthereumChainSpecParser, Ext: clap::Args + fmt::Debug = NoArgs, R: RpcModuleValidator = DefaultRpcModuleValidator>
That way we won't need to change most of the cli code
|
closing in favor of #18160 |
Adds support for custom RPC modules by introducing an
Other(String)variant toRethRpcModule. This allows users to configure and use RPC modules beyond the predefined set, enabling greater extensibility for custom implementations.The PR also includes validation to warn users when they configure RPC modules that haven't been registered with the RPC builder, helping catch configuration errors early.