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

feat: Add RethCliExt #3983

Merged
merged 6 commits into from
Jul 31, 2023
Merged

feat: Add RethCliExt #3983

merged 6 commits into from
Jul 31, 2023

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Jul 28, 2023

This provides a way to additional cli args to the reth cli.

This adds RethRpcServerArgsExt that can install additional modules to the configured transports.

Default is a noop.

This is only one part of making the cli/node builder customizable, and is only intended for installing additional rpc components easily.

In order to provide something like a node builder we'd need:

  • an abstraction over Node command,
  • additional traits that setup all the components

@mattsse mattsse requested review from Rjected and onbjerg as code owners July 28, 2023 15:27
@mattsse mattsse added C-enhancement New feature or request M-changelog This change should be included in the changelog A-cli Related to the reth CLI labels Jul 28, 2023
@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #3983 (88bf7b5) into main (1ac2f15) will decrease coverage by 0.10%.
Report is 2 commits behind head on main.
The diff coverage is 32.96%.

Impacted file tree graph

Files Changed Coverage Δ
crates/rpc/rpc-builder/src/lib.rs 64.15% <0.00%> (-1.90%) ⬇️
bin/reth/src/cli/ext.rs 4.16% <4.16%> (ø)
bin/reth/src/args/rpc_server_args.rs 54.42% <68.75%> (-0.18%) ⬇️
bin/reth/src/cli/mod.rs 34.40% <100.00%> (ø)
bin/reth/src/node/mod.rs 12.27% <100.00%> (+0.28%) ⬆️

... and 13 files with indirect coverage changes

Flag Coverage Δ
integration-tests 16.28% <0.00%> (-0.02%) ⬇️
unit-tests 64.12% <32.96%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 25.83% <51.72%> (-0.07%) ⬇️
blockchain tree 83.04% <ø> (ø)
pipeline 89.83% <ø> (ø)
storage (db) 74.27% <ø> (ø)
trie 94.70% <ø> (ø)
txpool 45.40% <ø> (-0.61%) ⬇️
networking 77.65% <ø> (-0.05%) ⬇️
rpc 58.36% <0.00%> (-0.17%) ⬇️
consensus 63.51% <ø> (ø)
revm 33.10% <ø> (ø)
payload builder 6.58% <ø> (ø)
primitives 87.91% <ø> (-0.03%) ⬇️

Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

Going off of what we talked about 1 on 1 -

This makes sense for the time being, but we should try to move towards having an abstraction that reth node uses instead of having reth node be the abstraction. The reasoning is that you might want complete control over the CLI/how the components are configured, and using reth node as the abstraction also suffers the following issues:

  • We'd need to add a flag/option for anything you would want to configure and probably hide most of them
  • It is not easy to integrate reth node into an existing code base with stricter requirements

In general, we would eventually want to support swapping out the builder, txpool, enabling/disabling parts of the pipeline, adding/removing/overriding things in RPC etc and encapsulating that in Clap (or things on top of Clap) is probably quite hard

Sensitive to the fact that building a better abstraction that we can reuse for reth node is a complex task, so this will do for now

Wdyt?

@mattsse
Copy link
Collaborator Author

mattsse commented Jul 31, 2023

yes, I agree,

I think we should try to abstract the node start command (or rather configuring + launching the node) and then the default Cli node command is just one impl of that,
it should be possible however to make this in such a way that even the default cli node commands is configurable to some degree

@mattsse mattsse enabled auto-merge July 31, 2023 14:48
@mattsse mattsse added this pull request to the merge queue Jul 31, 2023
Merged via the queue into main with commit a1e6815 Jul 31, 2023
@mattsse mattsse deleted the matt/cli-ext branch July 31, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Related to the reth CLI C-enhancement New feature or request M-changelog This change should be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants