feat(rpc-types): implement serialization for LocalizedTransactionTrace #5446

estensen · 2023-11-15T18:09:36Z

I'm a novice Rust developer and although I have working code I would like to improve it a bit and need some feedback.

I've added two test cases. One from Manually impl Serialize for LocalizedTransactionTrace #5412 and one from feat(rpc-type): implementation of Serialization for LocalizedTransactionTrace #5417. Do you have any other test cases that I could use for selfdestruct and reward?
To make it easier for myself I reordered the structs. I'm not too happy with that and I'm considering changing it to do it inside the serialize function. What do you think?

mattsse

this looks pretty good!
some smol nits.

and i'd like to see a few serde json roundtrip tests against reference data: #5417 (comment)
basically
check equality: from_str -> to_string

mattsse · 2023-11-16T14:08:01Z

crates/rpc/rpc-types/src/eth/trace/parity.rs

+            Action::Call(_call_action) => {
+                s.serialize_field("action", _call_action)?;
+            }
+            Action::Create(_create_action) => {
+                s.serialize_field("action", _create_action)?;
+            }
+            Action::Selfdestruct(_selfdestruct_action) => {
+                s.serialize_field("action", _selfdestruct_action)?;
+            }
+            Action::Reward(_reward_action) => {
+                s.serialize_field("action", _reward_action)?;
+            }
+        }


no _ prefix for fields we're using

mattsse · 2023-11-16T14:09:40Z

crates/rpc/rpc-types/src/eth/trace/parity.rs

+        match &self.trace.action {
+            Action::Call(_call_action) => {
+                s.serialize_field("type", "call")?;
+            }
+            Action::Create(_create_action) => {
+                s.serialize_field("type", "create")?;
+            }
+            Action::Selfdestruct(_selfdestruct_action) => {
+                s.serialize_field("type", "selfdestruct")?;
+            }
+            Action::Reward(_reward_action) => {
+                s.serialize_field("type", "reward")?;
+            }
+        }


I'd like to add an enum for this, like enum ActionKind and then add a function to action fn kind(&self) -> ActionKind

estensen · 2023-11-17T16:45:15Z

this looks pretty good! some smol nits.

and i'd like to see a few serde json roundtrip tests against reference data: #5417 (comment) basically check equality: from_str -> to_string

Thanks for the review!

I'm having a bit of trouble with the json roundtrip test. Is this the serializer not pretty printing maybe?

eb7babd

assertion `left == right` failed
  left: "{\"action\":{\"from\":\"0x4f4495243837681061c4743b74b3eedf548d56a5\",\"gas\":\"0x388b4f\",\"init\":\"0x6080604052600160005534801561001557600080fd5b50610324806100256000396000f3fe608060405234801561001057600080fd5b50600436106100355760003560e01c8062f55d9d1461003a5780631cff79cd1461004f575b600080fd5b61004d6100483660046101da565b610079565b005b61006261005d3660046101fc565b6100bb565b60405161007092919061027f565b60405180910390f35b6002600054141561009d5760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff8116ff5b60006060600260005414156100e35760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff85163b610136576040517f6f7c43f100000000000000000000000000000000000000000000000000000000815260040160405180910390fd5b8473ffffffffffffffffffffffffffffffffffffffff16848460405161015d9291906102de565b6000604051808303816000865af19150503d806000811461019a576040519150601f19603f3d011682016040523d82523d6000602084013e61019f565b606091505b50600160005590969095509350505050565b803573ffffffffffffffffffffffffffffffffffffffff811681146101d557600080fd5b919050565b6000602082840312156101ec57600080fd5b6101f5826101b1565b9392505050565b60008060006040848603121561021157600080fd5b61021a846101b1565b9250602084013567ffffffffffffffff8082111561023757600080fd5b818601915086601f83011261024b57600080fd5b81358181111561025a57600080fd5b87602082850101111561026c57600080fd5b6020830194508093505050509250925092565b821515815260006020604081840152835180604085015260005b818110156102b557858101830151858201606001528201610299565b818111156102c7576000606083870101525b50601f01601f191692909201606001949350505050565b818382376000910190815291905056fea264697066735822122032cb5e746816b7fac95205c068b30da37bd40119a57265be331c162cae74712464736f6c63430008090033\",\"value\":\"0x0\"},\"blockHash\":\"0xd5ac5043011d4f16dba7841fa760c4659644b78f663b901af4673b679605ed0d\",\"blockNumber\":18557272,\"result\":{\"address\":\"0x0292ddd2c4d72bf26602fe5d55de16f50dc535e1\",\"code\":\"0x608060405234801561001057600080fd5b50600436106100355760003560e01c8062f55d9d1461003a5780631cff79cd1461004f575b600080fd5b61004d6100483660046101da565b610079565b005b61006261005d3660046101fc565b6100bb565b60405161007092919061027f565b60405180910390f35b6002600054141561009d5760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff8116ff5b60006060600260005414156100e35760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff85163b610136576040517f6f7c43f100000000000000000000000000000000000000000000000000000000815260040160405180910390fd5b8473ffffffffffffffffffffffffffffffffffffffff16848460405161015d9291906102de565b6000604051808303816000865af19150503d806000811461019a576040519150601f19603f3d011682016040523d82523d6000602084013e61019f565b606091505b50600160005590969095509350505050565b803573ffffffffffffffffffffffffffffffffffffffff811681146101d557600080fd5b919050565b6000602082840312156101ec57600080fd5b6101f5826101b1565b9392505050565b60008060006040848603121561021157600080fd5b61021a846101b1565b9250602084013567ffffffffffffffff8082111561023757600080fd5b818601915086601f83011261024b57600080fd5b81358181111561025a57600080fd5b87602082850101111561026c57600080fd5b6020830194508093505050509250925092565b821515815260006020604081840152835180604085015260005b818110156102b557858101830151858201606001528201610299565b818111156102c7576000606083870101525b50601f01601f191692909201606001949350505050565b818382376000910190815291905056fea264697066735822122032cb5e746816b7fac95205c068b30da37bd40119a57265be331c162cae74712464736f6c63430008090033\",\"gasUsed\":\"0x2cb4a\"},\"subtraces\":0,\"traceAddress\":[0,6,0,0],\"transactionHash\":\"0x54160ddcdbfaf98a43a43c328ebd44aa99faa765e0daa93e61145b06815a4071\",\"transactionPosition\":102,\"type\":\"create\"}"
 right: "{\n    \"action\": {\n        \"from\": \"0x4f4495243837681061c4743b74b3eedf548d56a5\",\n        \"gas\": \"0x388b4f\",\n        \"init\": \"0x6080604052600160005534801561001557600080fd5b50610324806100256000396000f3fe608060405234801561001057600080fd5b50600436106100355760003560e01c8062f55d9d1461003a5780631cff79cd1461004f575b600080fd5b61004d6100483660046101da565b610079565b005b61006261005d3660046101fc565b6100bb565b60405161007092919061027f565b60405180910390f35b6002600054141561009d5760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff8116ff5b60006060600260005414156100e35760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff85163b610136576040517f6f7c43f100000000000000000000000000000000000000000000000000000000815260040160405180910390fd5b8473ffffffffffffffffffffffffffffffffffffffff16848460405161015d9291906102de565b6000604051808303816000865af19150503d806000811461019a576040519150601f19603f3d011682016040523d82523d6000602084013e61019f565b606091505b50600160005590969095509350505050565b803573ffffffffffffffffffffffffffffffffffffffff811681146101d557600080fd5b919050565b6000602082840312156101ec57600080fd5b6101f5826101b1565b9392505050565b60008060006040848603121561021157600080fd5b61021a846101b1565b9250602084013567ffffffffffffffff8082111561023757600080fd5b818601915086601f83011261024b57600080fd5b81358181111561025a57600080fd5b87602082850101111561026c57600080fd5b6020830194508093505050509250925092565b821515815260006020604081840152835180604085015260005b818110156102b557858101830151858201606001528201610299565b818111156102c7576000606083870101525b50601f01601f191692909201606001949350505050565b818382376000910190815291905056fea264697066735822122032cb5e746816b7fac95205c068b30da37bd40119a57265be331c162cae74712464736f6c63430008090033\",\n        \"value\": \"0x0\"\n    },\n    \"blockHash\": \"0xd5ac5043011d4f16dba7841fa760c4659644b78f663b901af4673b679605ed0d\",\n    \"blockNumber\": 18557272,\n    \"result\": {\n        \"address\": \"0x0292ddd2c4d72bf26602fe5d55de16f50dc535e1\",\n        \"code\": \"0x608060405234801561001057600080fd5b50600436106100355760003560e01c8062f55d9d1461003a5780631cff79cd1461004f575b600080fd5b61004d6100483660046101da565b610079565b005b61006261005d3660046101fc565b6100bb565b60405161007092919061027f565b60405180910390f35b6002600054141561009d5760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff8116ff5b60006060600260005414156100e35760405163caa30f5560e01b815260040160405180910390fd5b600260005573ffffffffffffffffffffffffffffffffffffffff85163b610136576040517f6f7c43f100000000000000000000000000000000000000000000000000000000815260040160405180910390fd5b8473ffffffffffffffffffffffffffffffffffffffff16848460405161015d9291906102de565b6000604051808303816000865af19150503d806000811461019a576040519150601f19603f3d011682016040523d82523d6000602084013e61019f565b606091505b50600160005590969095509350505050565b803573ffffffffffffffffffffffffffffffffffffffff811681146101d557600080fd5b919050565b6000602082840312156101ec57600080fd5b6101f5826101b1565b9392505050565b60008060006040848603121561021157600080fd5b61021a846101b1565b9250602084013567ffffffffffffffff8082111561023757600080fd5b818601915086601f83011261024b57600080fd5b81358181111561025a57600080fd5b87602082850101111561026c57600080fd5b6020830194508093505050509250925092565b821515815260006020604081840152835180604085015260005b818110156102b557858101830151858201606001528201610299565b818111156102c7576000606083870101525b50601f01601f191692909201606001949350505050565b818382376000910190815291905056fea264697066735822122032cb5e746816b7fac95205c068b30da37bd40119a57265be331c162cae74712464736f6c63430008090033\",\n        \"gasUsed\": \"0x2cb4a\"\n    },\n    \"subtraces\": 0,\n    \"traceAddress\": [\n        0,\n        6,\n        0,\n        0\n    ],\n    \"transactionHash\": \"0x54160ddcdbfaf98a43a43c328ebd44aa99faa765e0daa93e61145b06815a4071\",\n    \"transactionPosition\": 102,\n    \"type\": \"create\"\n}"

mattsse

nice, last tiny nit

mattsse · 2023-11-18T07:49:11Z

crates/rpc/rpc-types/src/eth/trace/parity.rs

@@ -394,17 +411,17 @@ impl Serialize for LocalizedTransactionTrace {
        s.serialize_field("subtraces", &self.trace.subtraces)?;
        s.serialize_field("traceAddress", &self.trace.trace_address)?;

-        match &self.trace.action {
-            Action::Call(_call_action) => {
+        match &self.trace.action.kind() {


this can be simplified with a single s.serialize_field("type", kind())?; call if ActionKind serializes accordingly

mattsse · 2023-11-18T07:49:29Z

crates/rpc/rpc-types/src/eth/trace/parity.rs

@@ -190,6 +199,14 @@ pub enum ActionType {
    Reward,
 }

+#[derive(Debug)]


Suggested change

#[derive(Debug)]

#[derive(Debug, Copy, Clone, PartialEq, Eq)]

mattsse

nice

test nit

mattsse · 2023-11-20T12:42:23Z

crates/rpc/rpc-types/src/eth/trace/parity.rs

+        let trace: Value = serde_json::from_str(&reference_data).unwrap();
+        let serialized = serde_json::to_string(&trace).unwrap();
+
+        assert_eq!(serialized, reference_data);


this check is redundant,

this needs to deserialize into trace object not Value

Some issues with this. Is there an easier way to test this than implement deserialize?

let trace: LocalizedTransactionTrace = serde_json::from_str(&reference_data).unwrap(); let _serialized = serde_json::to_string(&trace).unwrap(); // thread 'eth::trace::parity::tests::test_deserialize_serialize' panicked at crates/rpc/rpc-types/src/eth/trace/parity.rs:685:86: // called `Result::unwrap()` on an `Err` value: Error("missing field `trace`", line: 25, column: 1)

possible that I gave you incorrect test data.

will check, but this looks great now!

mattsse

nice, ty.

feat(rpc-types): implement serialization for LocalizedTransactionTrace

b19e2b1

estensen requested review from mattsse and Rjected as code owners November 15, 2023 18:09

mattsse requested changes Nov 16, 2023

View reviewed changes

estensen added 4 commits November 17, 2023 10:52

feat(rpc-types): remove _ prefix from used fields

ffa69c5

chore(rpc-types): add enum for action kind

97b293f

fix(rpc-types): fix serialization test

d4a8ca9

chore(rpc-types): test deserialize then serialize

eb7babd

mattsse requested changes Nov 18, 2023

View reviewed changes

fix(rpc-types): fix serialization test

bba12d8

mattsse requested changes Nov 20, 2023

View reviewed changes

fix(rpc-types): remove redundant test check

19b29da

estensen requested a review from Evalir as a code owner November 24, 2023 07:07

fix serialize

4992e54

mattsse approved these changes Nov 24, 2023

View reviewed changes

mattsse added this pull request to the merge queue Nov 24, 2023

Merged via the queue into paradigmxyz:main with commit 1e1cdce Nov 24, 2023
27 checks passed

estensen deleted the estensen/serialize-localized-transaction-trace branch November 24, 2023 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rpc-types): implement serialization for LocalizedTransactionTrace #5446

feat(rpc-types): implement serialization for LocalizedTransactionTrace #5446

estensen commented Nov 15, 2023

mattsse left a comment

mattsse Nov 16, 2023

mattsse Nov 16, 2023

estensen commented Nov 17, 2023

mattsse left a comment

mattsse Nov 18, 2023

mattsse Nov 18, 2023

mattsse left a comment

mattsse Nov 20, 2023

estensen Nov 23, 2023

mattsse Nov 23, 2023

mattsse left a comment

	#[derive(Debug)]
	#[derive(Debug, Copy, Clone, PartialEq, Eq)]

feat(rpc-types): implement serialization for LocalizedTransactionTrace #5446

feat(rpc-types): implement serialization for LocalizedTransactionTrace #5446

Conversation

estensen commented Nov 15, 2023

mattsse left a comment

Choose a reason for hiding this comment

mattsse Nov 16, 2023

Choose a reason for hiding this comment

mattsse Nov 16, 2023

Choose a reason for hiding this comment

estensen commented Nov 17, 2023

mattsse left a comment

Choose a reason for hiding this comment

mattsse Nov 18, 2023

Choose a reason for hiding this comment

mattsse Nov 18, 2023

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

mattsse Nov 20, 2023

Choose a reason for hiding this comment

estensen Nov 23, 2023

Choose a reason for hiding this comment

mattsse Nov 23, 2023

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment