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

Unified client for regular and tracing nodes #791

Merged
merged 32 commits into from
Sep 23, 2021

Conversation

nanocryk
Copy link
Contributor

@nanocryk nanocryk commented Sep 8, 2021

What does it do?

This is a follow-up on substitute branches for tracing and removes evm-tracing feature from the client. Instead we now will use the same client for all nodes, and use substitute runtimes built with the evm-tracing enabled.

What important points reviewers should know?

  • DebugRuntimeApi versions are now removed from the client, and this Api is expected to be accessed only through substitutes.
  • There is now a single logic to handle tracing across historical runtime versions.

@nanocryk nanocryk added B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes B3-apinoteworthy Changes should be mentioned in the release notes of the next minor version release. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes A10-evmtracing Pull request includes evm tracing functionality labels Sep 8, 2021
nanocryk and others added 12 commits September 8, 2021 14:40
# Conflicts:
#	.github/workflows/build.yml
#	Cargo.lock
#	node/Cargo.toml
#	node/cli/src/command.rs
#	node/service/Cargo.toml
#	node/service/src/chain_spec/mod.rs
#	node/service/src/lib.rs
#	node/service/src/rpc.rs
#	primitives/ext/Cargo.toml
#	primitives/rpc/debug/Cargo.toml
#	primitives/rpc/evm-tracing-events/src/runtime.rs
#	runtime/common/src/apis.rs
#	runtime/evm_tracer/Cargo.toml
#	runtime/evm_tracer/src/lib.rs
#	runtime/moonbase/Cargo.toml
#	runtime/moonbeam/Cargo.toml
#	runtime/moonriver/Cargo.toml
@tgmichel tgmichel marked this pull request as ready for review September 16, 2021 12:15
@tgmichel tgmichel added the A0-pleasereview Pull request needs code review. label Sep 16, 2021
@tgmichel tgmichel changed the title Tracing client substitutes support Unified client for regular and tracing nodes Sep 16, 2021
@tgmichel tgmichel added A3-inprogress Pull request is in progress. No review needed at this stage. and removed A0-pleasereview Pull request needs code review. labels Sep 16, 2021
@tgmichel tgmichel added A0-pleasereview Pull request needs code review. and removed A3-inprogress Pull request is in progress. No review needed at this stage. labels Sep 17, 2021
Copy link
Collaborator

@crystalin crystalin left a comment

Choose a reason for hiding this comment

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

Only checked the CI, looks good to me.

@crystalin crystalin added A6-seemsok Pull request had a shallow review, but should have a follow up before merge. and removed A0-pleasereview Pull request needs code review. labels Sep 20, 2021
@crystalin
Copy link
Collaborator

Is the runtime changed for that ?

@tgmichel tgmichel force-pushed the jeremy-tracing-client-substitutes-support branch from a2fcc67 to 695b082 Compare September 23, 2021 07:06
@tgmichel tgmichel merged commit 0873b04 into master Sep 23, 2021
@tgmichel tgmichel deleted the jeremy-tracing-client-substitutes-support branch September 23, 2021 08:51
@tgmichel tgmichel mentioned this pull request Sep 23, 2021
Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

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

Sorry for my late review but I couldn't before. I just have some small remarks related to the IC and cargo features, that can be the subject of a new small PR.

.github/workflows/build.yml Show resolved Hide resolved
node/Cargo.toml Show resolved Hide resolved
node/cli/src/command.rs Show resolved Hide resolved
primitives/rpc/debug/src/api/single.rs Show resolved Hide resolved
@@ -101,26 +102,26 @@ pub struct RawStepLog {

#[derive(Clone, Eq, PartialEq, Debug, Encode, Decode)]
#[cfg_attr(feature = "std", derive(Serialize))]
#[cfg_attr(feature = "std", serde(rename_all = "camelCase", tag = "type"))]
#[cfg_attr(feature = "std", serde(rename_all = "lowercase", tag = "type"))]
pub enum CallInner {
#[cfg_attr(feature = "std", serde(rename_all = "camelCase"))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not readable to mix lowercase and camelCase. It is better to use neither of them and to rename specifically the only 2 fields to rename (call_type and SelfDestruct).

Copy link
Contributor

Choose a reason for hiding this comment

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

I will sneak this in #847

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A6-seemsok Pull request had a shallow review, but should have a follow up before merge. A10-evmtracing Pull request includes evm tracing functionality B3-apinoteworthy Changes should be mentioned in the release notes of the next minor version release. B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants