Plugable EVM tracing #883
Plugable EVM tracing #883
Conversation
| @@ -31,6 +31,7 @@ pub struct Cli { | |||
| pub run: cumulus_client_cli::RunCmd, | |||
|
|
|||
| /// Enable EVM tracing module on a non-authority node. | |||
| #[cfg(feature = "evm-tracing")] | |||
There was a problem hiding this comment.
Instead of adding options one by one, and having #[cfg(feature = "evm-tracing")]] over each, you should create a struct and flatten it. Perfect example is few lines above for the cumulus_client_cli.
There was a problem hiding this comment.
Preprocessor conditions makes it possible to exclude tracing dependencies from production client.
There was a problem hiding this comment.
That is correct. However, I didn't comment that at all.
My comment relates to having many feature checks and I suggest to add only one instead and use the flatten macro.
| @@ -168,6 +176,7 @@ impl RelayChainCli { | |||
| } | |||
|
|
|||
| /// EVM tracing CLI flags. | |||
| #[cfg(feature = "evm-tracing")] | |||
There was a problem hiding this comment.
Why not move all evm-tracing related type into a separate module?
The code would be much cleaner.
There was a problem hiding this comment.
I didn’t change anything here, just add few preprocessor conditions; all comments are fair for 5.0 without this PR.
There was a problem hiding this comment.
I commented what you changed - adding the feature check 🤷♂️.
|
|
||
| #[cfg(not(feature = "evm-tracing"))] | ||
| if config.chain_spec.is_astar() { | ||
| start_astar_node(config, polkadot_config, collator_options, para_id, cli.enable_evm_rpc) | ||
| .await | ||
| .map(|r| r.0) | ||
| .map_err(Into::into) |
There was a problem hiding this comment.
This isn't clean nor is it scalable - why not instead introduce a new struct/type, like AdditionalConfiguration which would aggregate all additional config types?
That way you keep a single entry point for the start_xyz_node function. If we need to add additional ones in the future, how will that look like otherwise?
There was a problem hiding this comment.
Good point for refactoring the service.
| }) | ||
| ) | ||
| }).await | ||
| } |
There was a problem hiding this comment.
We've discussed in the team that service code is already too complex as it is. Adding additional 1000+ lines of C/P code with slight modification looks like another giant leap in the wrong direction.
We should carefully rethink this approach - the solution which you're basing this PR on didn't include this code in production for a reason.
Merging this, as it is, makes further development and maintenance of this code even more difficult than before.
There was a problem hiding this comment.
Up to me it not grown complexity, good point is have full refactoring instead of trying to deal with complexity in this PR.
There was a problem hiding this comment.
Ok, then we have differing opinions. Mine is based on doing the uplifts roughly for the last year.
And with this code, it looks like that will be even more complex now that it was before.
Pull Request Summary
This PR introduce EVM tracing as optional feature. Using runtime overriding it makes possible to enable tracing on runtimes that don't have tracing runtime API.
New rust features:
evm-tracingon runtimes: enable tracing runtime API when build with specified feature - used for tracing runtimesevm-tracingon collator: enable tracing RPC and host functions required for tracing runtimes.Usage:
tracingnode and runtime--wasm-runtime-overridesflag to substitute tracing runtime instead of productionCheck list