[Merge] Add serde impls for Transactions type
#2649
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paulhauner
force-pushed
the
merge-tx-encoding
branch
from
0d17372 to
5a56104
Compare
Comment on lines
+410
to
+428
| fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error> | ||
| where | ||
| A: serde::de::SeqAccess<'a>, | ||
| { | ||
| let mut outer = VariableList::default(); | ||
|
|
||
| while let Some(val) = seq.next_element::<String>()? { | ||
| let inner_vec = hex::decode(&val).map_err(de::Error::custom)?; | ||
| let opaque_transaction = VariableList::new(inner_vec).map_err(|e| { | ||
| serde::de::Error::custom(format!("transaction too large: {:?}", e)) | ||
| })?; | ||
| let transaction = Transaction::OpaqueTransaction(opaque_transaction); | ||
| outer.push(transaction).map_err(|e| { | ||
| serde::de::Error::custom(format!("too many transactions: {:?}", e)) | ||
| })?; | ||
| } | ||
|
|
||
| Ok(outer) | ||
| } |
There was a problem hiding this comment.
I think we can avoid a handwritten visitor implementation by defining a JsonTransaction type that derives (De)Serialize, and then uses serde attributes to tweak the behaviour. This has the advantage of being declarative rather than imperative.
AFAICT you want the serialisation to match the types::Transaction enum, except without the {"selector": 0, "value": value} wrapping, which could be achieved by using the untagged enum representation serde(untagged): https://serde.rs/enum-representations.html#untagged
Everything else could be reused from Transaction:
lighthouse/consensus/types/src/execution_payload.rs
Lines 8 to 20 in f1eec45
(and you don't need the rename = "0" hack because of the untagged repr)
Comment on lines
+440
to
+442
| // It's important to match on the inner values of the transaction. Serializing the | ||
| // entire `Transaction` will result in appending the SSZ union prefix byte. The | ||
| // execution node does not want that. |
There was a problem hiding this comment.
I think this is exactly serde(untagged)
|
Very good points about the
|
michaelsproul
approved these changes
Sep 29, 2021
paulhauner
added a commit
that referenced
this pull request
Oct 1, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
paulhauner
added a commit
that referenced
this pull request
Oct 12, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
paulhauner
added a commit
that referenced
this pull request
Oct 27, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
paulhauner
added a commit
to paulhauner/lighthouse
that referenced
this pull request
Nov 3, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
paulhauner
added a commit
that referenced
this pull request
Nov 11, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
paulhauner
added a commit
that referenced
this pull request
Nov 28, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
paulhauner
added a commit
that referenced
this pull request
Nov 28, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
paulhauner
added a commit
that referenced
this pull request
Dec 2, 2021
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Addressed
NA
Proposed Changes
Add
serdeimpls and tests for theexecution_payload.transactionsfield.Additional Info
NA