Normalise rpc debug trace format #839

AmosAidoo · 2025-03-23T17:56:29Z

Which issue does this PR close?

Closes Normalise JSON-RPC format for rpcDebugTrace #783

Changes

Moved normalisation that Hardhat does on debug trace into EDR.
Added new types RpcDebugTraceResult and RpcDebugTraceLogItem that conform to expected structure.
Added tests to validate trace structure against expected normalised structure.

How was this tested?

Compared trace manually with output from Geth.
Wrote test to validate normalised structure.
Ran tests and verified all tests pass including old ones.

Geth is used to generate the desired JSON trace format and these tests validates the results against them.

…ation

changeset-bot · 2025-03-23T17:56:32Z

🦋 Changeset detected

Latest commit: 090729a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@nomicfoundation/edr	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

AmosAidoo · 2025-03-23T18:02:02Z

@nebasuke This is my PR for the issue.
I am available anytime to make the necessary changes.

nebasuke · 2025-03-24T12:25:12Z

Great, thank you @AmosAidoo, we appreciate the effort!

It will probably be @Wodann or @agostbiro that can take a look.

AmosAidoo · 2025-03-25T11:04:07Z

cd hardhat-tests && pnpm test:ci
Noticed I missed these tests. I'm taking a look.

nebasuke · 2025-03-25T11:27:36Z

Noticed I missed these tests. I'm taking a look.

Great, thanks! I'll re-run CI when needed.

additionally added pass and gasUsed fields and leading 0x for storage and memory as the hardhat tests still expects them

AmosAidoo · 2025-03-25T12:59:06Z

the tests still expect the pass, gasUsed and opName fields so I added them in addition to the new format. Also, the tests still assume the memory and storage strings will have the leading 0x string.
So I guess this PR will need a follow-up issue after this is integrated into Hardhat. The hardhat tests will then have to be updated afterwards.

AmosAidoo · 2025-03-25T13:10:17Z

Great, thanks! I'll re-run CI when needed.

You can re-run the CI

AmosAidoo · 2025-03-25T14:06:54Z

Can you kindly re-run

Wodann

Thank you for taking a stab at this. I left some requests to try and move the changes into edr_provider as the current approach goes through multiple steps of serialization and deserialization.

Wodann · 2025-03-25T14:58:54Z

crates/edr_napi/src/debug_trace.rs

+pub struct RpcDebugTraceResult {
+    pub failed: bool,
+    pub gas: BigInt,
+    pub pass: bool,
+    pub gas_used: BigInt,
+    pub return_value: String,
+    pub struct_logs: Vec<RpcDebugTraceLogItem>,
+}


Could you please add documentation for the struct and its pub member fields?

crates/edr_napi/src/debug_trace.rs

Wodann · 2025-03-25T14:59:49Z

crates/edr_napi/src/provider.rs

@@ -102,6 +105,9 @@ impl Provider {
    #[doc = "Handles a JSON-RPC request and returns a JSON-RPC response."]
    #[napi]
    pub async fn handle_request(&self, json_request: String) -> napi::Result<Response> {
+        let method_name = serde_json::from_str::<JsonRpcMethodExtractor>(&json_request)
+            .map_or_else(|_| "".to_string(), |m| m.method);


Suggested change

.map_or_else(|_| "".to_string(), |m| m.method);

.map_or_else(|_| String::new(), |m| m.method);

Removed this as well

Wodann · 2025-03-25T15:01:23Z

crates/edr_napi/src/provider.rs

+                    let parsed_data: Result<DebugTraceResult, _> = match &data {
+                        Either::A(json_string) => {
+                            serde_json::from_str::<JsonRpcResponse<DebugTraceResult>>(json_string)
+                                .map(|r| r.result)
+                        }
+                        Either::B(json_value) => serde_json::from_value::<
+                            JsonRpcResponse<DebugTraceResult>,
+                        >(json_value.clone())
+                        .map(|r| r.result),
+                    };
+
+                    match parsed_data {
+                        Ok(trace_result) => {
+                            let transformed = normalise_rpc_debug_trace(trace_result)
+                                .map_err(|e| napi::Error::new(Status::GenericFailure, e))?;
+
+                            let transformed = JsonRpcResponse {
+                                result: transformed,
+                            };
+                            Either::B(serde_json::to_value(transformed)?)
+                        }
+                        Err(_) => data,
+                    }


Could you please split this into a function with a descriptive name?

No need for these lines now after moving normalisation to edr_provider.

Wodann · 2025-03-25T15:04:21Z

crates/edr_napi/src/provider.rs

+                    let parsed_data: Result<DebugTraceResult, _> = match &data {
+                        Either::A(json_string) => {
+                            serde_json::from_str::<JsonRpcResponse<DebugTraceResult>>(json_string)
+                                .map(|r| r.result)
+                        }
+                        Either::B(json_value) => serde_json::from_value::<
+                            JsonRpcResponse<DebugTraceResult>,
+                        >(json_value.clone())
+                        .map(|r| r.result),
+                    };


It is incredibly inefficient to:

serialize the result to JSON (string or object)

deserialize the result from JSON (string or object)

normalize result

serialize the result

Could you please investigate making this change one level up, before we first serialize the result? That would be in edr_provider

I have moved the normalisation into edr_provider

AmosAidoo · 2025-03-25T15:19:43Z

Thank you for taking a stab at this. I left some requests to try and move the changes into edr_provider as the current approach goes through multiple steps of serialization and deserialization.

Perfect, thank you for the review. I'll make the changes.

AmosAidoo · 2025-03-26T09:44:10Z

I made the necessary changes. Tests pass when I run them locally. Could you kindly re-trigger the ci.

AmosAidoo · 2025-03-26T10:13:44Z

Seems to be looking good so far regarding the tests. Looks like the remaining failures are because of environmental variables not set as I am an external contributor.

Wodann

This is going in the right direction, but quite a few things are still unclear to me about the chosen solution. I'd appreciate if you could clarify.

Wodann · 2025-03-28T21:40:39Z

crates/edr_napi/src/debug_trace.rs

+/// together because Hardhat still depends on them but `pass` and `gas_used`
+/// should likely to be removed after a while.
+#[napi(object)]
+pub struct RpcDebugTraceResult {


As this type is no longer part of the API, do we really need to expose it?

I assume Hardhat has its own definition, given that it's a standard.

cc: @fvictorio

You are right, Hardhat has its own definition

Wodann · 2025-03-28T21:40:48Z

crates/edr_napi/src/debug_trace.rs

+/// Debug logs after normalising the EIP-3155 debug logs.
+/// This is the format Hardhat expects
+#[napi(object)]
+pub struct RpcDebugTraceLogItem {


As this type is no longer part of the API, do we really need to expose it?

I assume Hardhat has its own definition, given that it's a standard.

cc: @fvictorio

Wodann · 2025-03-28T21:42:17Z

crates/edr_napi/test/provider.ts

@@ -415,3 +515,48 @@ function assertEqualMemory(stepMemory: Buffer | undefined, expected: Buffer) {

  assert.isTrue(stepMemory.equals(expected));
 }
+
+function assertJsonRpcFormatNormalised(trace: RpcDebugTraceResult) {


I think these assertions can happen without knowing the type RpcDebugTraceResult, but am not 100%. If so, we can just remove the type from EDR's N-API, reducing our API surface.

Wodann · 2025-03-28T21:42:30Z

crates/edr_napi/test/provider.ts

+        // assert.isFalse(key.startsWith("0x"));
+        // assert.isFalse(value.startsWith("0x"));


Why are these two disabled?

I disabled these two because at this moment, the normalisation is still being done in Hardhat so it is still correct to have 0x at the start of the strings.
Here is the reference in Hardhat: https://github.com/NomicFoundation/hardhat/blob/024d72b09c6edefb00c012e9514a0948c255d0ab/v-next/hardhat/src/internal/builtin-plugins/network-manager/edr/utils/convert-to-edr.ts#L197-L203

The Hardhat tests also operate on this assumption so I believe these two asserts should be disabled until the normalisation is completely removed from Hardhat.

There is a process for incorporating changes to Hardhat into your PR. It's explained here. That's what would be required in order to test your changes in Hardhat and make sure everything is working as expected.

Ah perfect. Thank you

Wodann · 2025-03-28T21:43:15Z

crates/edr_provider/src/lib.rs

@@ -380,10 +382,12 @@ impl<LoggerErrorT: Debug + Send + Sync + 'static, TimerT: Clone + TimeSinceEpoch
            // debug_* methods
            MethodInvocation::DebugTraceTransaction(transaction_hash, config) => {
                debug::handle_debug_trace_transaction(data, transaction_hash, config)
+                    .and_then(normalise_rpc_debug_trace)


Please move this call inside the handle_debug_trace_transaction call, following the same pattern as all other functions. The handle_* functions determine the result type, not the provider.

Wodann · 2025-03-28T21:43:59Z

crates/edr_provider/src/lib.rs

+
+// Rust port of https://github.com/NomicFoundation/hardhat/blob/024d72b09c6edefb00c012e9514a0948c255d0ab/v-next/hardhat/src/internal/builtin-plugins/network-manager/edr/utils/convert-to-edr.ts#L176
+/// This normalization is done because this is the format Hardhat expects
+fn normalise_rpc_debug_trace<LoggerErrorT: Debug>(


Please move this to the same file as the handle_ function I mentioned in my other comment.

Wodann · 2025-03-28T21:44:31Z

crates/edr_provider/src/lib.rs

+                    // Removing this trim temporarily as the Hardhat test assumes 0x is there
+                    // .map(|value| value.trim_start_matches("0x").to_string())


If Hardhat requires the 0x, then why do you believe we should trim the 0x?

Hardhat doesn't require the 0x but assumes they are there. Hardhat later on trims the 0x when normalising. Here is the relevant code:
https://github.com/NomicFoundation/hardhat/blob/024d72b09c6edefb00c012e9514a0948c255d0ab/v-next/hardhat/src/internal/builtin-plugins/network-manager/edr/utils/convert-to-edr.ts#L192-L195

When running the hardhart-tests for example, I confirmed this expected format from the trace generated from Geth which the tests depend on:

https://github.com/NomicFoundation/edr/blob/main/hardhat-tests/test/fixture-debug-traces/traceCall/contractAGetMessage.ts

https://github.com/NomicFoundation/edr/blob/main/hardhat-tests/test/fixture-debug-traces/traceCall/contractAGetMessageDebugConfig.ts

Wodann · 2025-03-28T21:44:37Z

crates/edr_provider/src/lib.rs

+            storage: log.storage.map(|storage| {
+                storage
+                    .into_iter()
+                    // Removing this trim temporarily as the Hardhat test assumes 0x is there


If Hardhat requires the 0x, then why do you believe we should trim the 0x?

Same as the reasons stated for the two related comments above. Hardhat doesn't require 0x but assumes it is there and so it get removed during normalisation.

Wodann · 2025-03-28T21:45:30Z

crates/edr_provider/src/requests/debug.rs

@@ -120,3 +121,48 @@ impl From<DebugTraceConfig> for edr_evm::DebugTraceConfig {
        }
    }
 }
+
+/// This is the JSON-RPC Debug trace format
+#[derive(Debug, Clone, serde::Deserialize, serde::Serialize)]


Do we need both Deserialize and Serialize? Based on the code, it seems that serde::Serialize is sufficient.

Yes you are right, thanks for pointing it out. I'll remove it.

Wodann · 2025-03-28T21:47:05Z

crates/edr_provider/src/requests/debug.rs

+#[serde(rename_all = "camelCase")]
+pub struct RpcDebugTraceResult {
+    pub failed: bool,
+    pub gas: u64,


Did the old format require a 0x... representation? What made you decide to go with normal u64 serialization?

Do you have a reference to support this choice?

If we should support a hex representation of a quantity, you should use alloy_serde::quantity.

This applies to all u64 usages here.

Yes so I used u64 because gas_used in DebugTraceResult is also u64:
https://github.com/NomicFoundation/edr/blob/main/crates/edr_evm/src/debug_trace.rs#L220

Noted the point on alloy_serde::quantity for hex quantities.

Remove unused serde::Deserialize from rpc debug structs and move normalization into debug.rs, following the pattern of other functions in the provider

AmosAidoo · 2025-04-01T09:45:03Z

I have made some changes and replied to the questions

Wodann · 2025-04-10T17:56:29Z

I added some information to the original issue on how to create test vectors for your code. You can essentially use geth to create some expected output formats and do string comparison between your generated output and the expected output.

Most of the info about debug_traceCall is here, but note that they use a js lib, not the raw rpc, in the snippets.
To be able to port it with confidence, we should create a few test cases using geth, and making sure we match their format.

AmosAidoo · 2025-04-10T20:22:02Z

Noted. Thank you

AmosAidoo · 2025-04-23T08:11:48Z

I added some information to the original issue on how to create test vectors for your code. You can essentially use geth to create some expected output formats and do string comparison between your generated output and the expected output.

Most of the info about debug_traceCall is here, but note that they use a js lib, not the raw rpc, in the snippets.
To be able to port it with confidence, we should create a few test cases using geth, and making sure we match their format.

@Wodann is it sufficient to use geth in dev mode to generate the test cases?

AmosAidoo added 6 commits March 21, 2025 15:51

test: add tests to validate normalised JSON format

d3206e9

Geth is used to generate the desired JSON trace format and these tests validates the results against them.

test: update test to check structure of json format

8882efc

feat: add json rpc versions of debug trace types

745b094

feat: ports trace debug normalisation from hardhat to edr

17948b3

json rpc format for trace debug structs used in provider for normalis…

aa51ae6

…ation

updated pnpm-lock.yaml

d7a49de

nebasuke requested a review from a team March 24, 2025 12:23

revert pnpm-lock update

75638d1

nebasuke and others added 3 commits March 25, 2025 11:29

Create famous-ties-know.md

f346eb0

fix: hardhat tests expects certain data format

567fef3

additionally added pass and gasUsed fields and leading 0x for storage and memory as the hardhat tests still expects them

fix: applied linter formatting

746f8bf

fix: do not assert leading 0x string

3af4ced

Wodann requested changes Mar 25, 2025

View reviewed changes

AmosAidoo added 2 commits March 25, 2025 18:35

fix: move normalisation from edr_nap to edr_provider

6725885

fix: add documentation to new structs

eafe808

feat: update index.d.ts with documentation

1c0f167

nebasuke requested a review from Wodann March 27, 2025 09:38

Wodann requested changes Mar 28, 2025

View reviewed changes

fix: remove deserialize and move normalisation

68d1998

Remove unused serde::Deserialize from rpc debug structs and move normalization into debug.rs, following the pattern of other functions in the provider

AmosAidoo added 2 commits April 1, 2025 11:37

fix: reduce napi api surface

d4ffe6f

fix: apply cargo fmt

090729a

	.map_or_else(\|_\| "".to_string(), \|m\| m.method);
	.map_or_else(\|_\| String::new(), \|m\| m.method);

		// assert.isFalse(key.startsWith("0x"));
		// assert.isFalse(value.startsWith("0x"));

		// Removing this trim temporarily as the Hardhat test assumes 0x is there
		// .map(\|value\| value.trim_start_matches("0x").to_string())

Normalise rpc debug trace format #839

Are you sure you want to change the base?

Normalise rpc debug trace format #839

Conversation

AmosAidoo commented Mar 23, 2025

Which issue does this PR close?

Changes

How was this tested?

changeset-bot bot commented Mar 23, 2025 • edited Loading

🦋 Changeset detected

AmosAidoo commented Mar 23, 2025

nebasuke commented Mar 24, 2025

AmosAidoo commented Mar 25, 2025

nebasuke commented Mar 25, 2025

AmosAidoo commented Mar 25, 2025

AmosAidoo commented Mar 25, 2025

AmosAidoo commented Mar 25, 2025

Wodann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmosAidoo Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmosAidoo commented Mar 25, 2025

AmosAidoo commented Mar 26, 2025

AmosAidoo commented Mar 26, 2025

Wodann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmosAidoo commented Apr 1, 2025

Wodann commented Apr 10, 2025 • edited Loading

AmosAidoo commented Apr 10, 2025

AmosAidoo commented Apr 23, 2025

changeset-bot bot commented Mar 23, 2025 •

edited

Loading

AmosAidoo Mar 25, 2025 •

edited

Loading

Wodann commented Apr 10, 2025 •

edited

Loading