feat: serde for consensus tx types #361
Conversation
| @@ -62,12 +62,17 @@ impl TryFrom<u8> for TxType { | |||
| /// | |||
| /// [EIP-2718]: https://eips.ethereum.org/EIPS/eip-2718 | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
| #[cfg_attr(feature = "serde", serde(tag = "type"))] | |||
There was a problem hiding this comment.
not sure if we want the type as tag here,
I think for readability this makes sense, and untagged could be a mess
wdyt @prestwich
There was a problem hiding this comment.
I think that type may be omitted for legacy txns, and any objects generated in the past and stored somewhere will definitely have type omitted. So this would prevent us from handling old receipts at the very least
There was a problem hiding this comment.
@prestwich so those should be untagged instead?
There was a problem hiding this comment.
ideally we would write custom impls here I think? but I don't feel super strongly about it. can you check ethers-rs and its issues for how we used to do this? as long as we're not worse than ethers, we should be fine to move forward
There was a problem hiding this comment.
I didn't find any issues related to this type tag, likely because ethers users couldn't really have issues with deserializing txses serialized by ethers itself. And deserialization of external JSON-encoded transactions was likely done through Transaction which is more like an RPC type without much restrictions.
imo ideal impl here would be the one which is able to deserialize any valid eth_getTransactionByHash response into TxEnvelope and per execution spec all RPC responses must include "type"
There was a problem hiding this comment.
I thin i'm comfortable leaving as typed, as supporting outdated nodes or long-term stored responses is not a compelling usecase.
imo ideal impl here would be the one which is able to deserialize any valid eth_getTransactionByHash response into TxEnvelope and per execution spec all RPC responses must include "type"
agree, tho as stated elsewhere, i think the "right" way to achieve this is by embedding TxEnvelope in the transaction rpc response type
There was a problem hiding this comment.
agree, tho as stated elsewhere, i think the "right" way to achieve this is by embedding TxEnvelope in the transaction rpc response type
perhaps this is the next step once network abstraction is generic enough to allow us such strict constraints on RPC responses without breaking stuff for projects using Ethereum network for everything?
There was a problem hiding this comment.
the way to enforce it as a bound on network (if we even want to do that) would be to have type TransactionResponse: RpcObject + AsRef<Self::TxEnvelope>, basically. That would ensure that any response object contained an envelope
I think that the longer we go without embedding consensus objects in the RPC types, the worse of a time we'll have making the change
My primary target list is
- Log
- TxEnvelope
- Receipt
klkvr
force-pushed
the
klkvr/consensus-serde
branch
from
254a7b7
to
6deea34
Compare
| @@ -62,12 +62,17 @@ impl TryFrom<u8> for TxType { | |||
| /// | |||
| /// [EIP-2718]: https://eips.ethereum.org/EIPS/eip-2718 | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
| #[cfg_attr(feature = "serde", serde(tag = "type"))] | |||
There was a problem hiding this comment.
I think that type may be omitted for legacy txns, and any objects generated in the past and stored somewhere will definitely have type omitted. So this would prevent us from handling old receipts at the very least
klkvr
force-pushed
the
klkvr/consensus-serde
branch
from
674b677
to
9c8ee02
Compare
| @@ -3,8 +3,11 @@ use alloy_primitives::{Signature, B256}; | |||
|
|
|||
| /// A transaction with a signature and hash seal. | |||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
There was a problem hiding this comment.
is there followup work to dedupe the rpc_types signature here?
| @@ -914,6 +954,7 @@ impl Transaction for TxEip4844WithSidecar { | |||
| /// This represents a set of blobs, and its corresponding commitments and proofs. | |||
| #[derive(Debug, Default, Clone, PartialEq, Eq, Hash)] | |||
| #[repr(C)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
There was a problem hiding this comment.
Can we proactively dedupe this as part of this PR?
current code in RPC types is as follows
/// This represents a set of blobs, and its corresponding commitments and proofs.
#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, Serialize, Deserialize)]
#[repr(C)]
pub struct BlobTransactionSidecar {
/// The blob data.
pub blobs: Vec<Blob>,
/// The blob commitments.
pub commitments: Vec<Bytes48>,
/// The blob proofs.
pub proofs: Vec<Bytes48>,
}
There was a problem hiding this comment.
@prestwich rpc-engine-types relies on Blob and Bytes48 having ssz support and c-kzg types does not have it, what's the desired way to handle this?
There was a problem hiding this comment.
manually impl ssz trait with a helper struct? If it's getting that complicated, it should go in a separate PR though
Motivation
Adds
SerializeandDeserializeimpls forTypedTransaction,TxEnvelope, etc.Bumps core
Solution
I don't really like the way we manage
typeon tagged tx enums, current version is from ethers, perhaps there is a way we can do better here?PR Checklist