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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: serde for consensus tx types #361
feat: serde for consensus tx types #361
Changes from all commits
333a7af
caff7ed
e636c77
2845ad5
b193b4c
daade9a
78ed4e7
543cb79
bd14c32
fa5ab01
ffdd5b1
9c8ee02
33b1d5e
56befff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there followup work to dedupe the rpc_types signature here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we proactively dedupe this as part of this PR?
current code in RPC types is as follows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prestwich rpc-engine-types relies on
Blob
andBytes48
havingssz
support and c-kzg types does not have it, what's the desired way to handle this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manually impl ssz trait with a helper struct? If it's getting that complicated, it should go in a separate PR though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prestwich so those should be untagged instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 intoTxEnvelope
and per execution spec all RPC responses must include"type"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thin i'm comfortable leaving as typed, as supporting outdated nodes or long-term stored responses is not a compelling usecase.
agree, tho as stated elsewhere, i think the "right" way to achieve this is by embedding
TxEnvelope
in the transaction rpc response typeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 envelopeI 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