[Merge] Add serde impls for Transactions type
#2649
Conversation
paulhauner
force-pushed
the
merge-tx-encoding
branch
from
0d17372 to
5a56104
Compare
| 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)
| // 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
left a comment
michaelsproul
left a comment
There was a problem hiding this comment.
Yeah ok cool. I thought it seemed strange that the exec engines weren't using the selectors. Merge at will
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
* Start implemented serde for transactions * Revise serde impl * Add tests for transaction decoding
2 participants
Issue Addressed
NA
Proposed Changes
Add
serdeimpls and tests for theexecution_payload.transactionsfield.Additional Info
NA