Skip to content
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

[wip] feat: embed TxEnvelope into rpc_types::Transaction #1169

Closed
wants to merge 14 commits into from

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Aug 21, 2024

Motivation

Closes #1165

Solution

  • Added Envelope generic for rpc transaction which is consensus::TxEnvelope by default. For AnyNetwork we need a catch-all envelope, so I've extracted current rpc_types_eth::Transaction without block-related fields into AnyTxEnvelope type.
  • Added Serialize and Deserialize for TxType. It deserializes as Option<U8> with None representing legacy and serializes as U8.
  • Updated TxEnvelope to use custom deserialize impl to handle case of missing type field as legacy tx.

After this PR, Transaction will only implement arbitrary if k256 feature is enabled because of similar requirement on SignedAuthorization. I think this makes sense and believe we also want to add similar restriction for Arbitrary impl on Signed<T> which will only produce valid transactions.

At this point we are maintaining 6 very similar serde schemes + rlp encoding implementations for different tx holding types (5 tx types + AnyTxEnvelope). Wondering if we should add a macro for generating those so that serde attrs for e.g. gas are only defined once?

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@@ -205,7 +204,7 @@ mod tests {
"type": "0x2",
"accessList": [],
"chainId": "0x1",
"v": "0x0",
"yParity": "0x0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v on non-legacy transactions is deprecated, so that's a serialization change, deser should be fine

though we've decided to test this before merging

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klkvr
Copy link
Member Author

klkvr commented Aug 28, 2024

The only breaking change this PR adds is for serde roundrip for rpc_types::Transaction. Earlier we wouldn't modify the serialized payload, but with this PR we will convert "v": "0x0 | 0x1" into yParity key with the same value.

This might only result in incorrect payload for legacy transactions as for them v is required. However, per EIP-155 spec:

then v continues to be set to {0,1} + 27 as previously.

Which likely means that it is safe to expect v on legacy transactions to always equal to 0x1c | 0x1b, thus never resulting in incorrect deserialization on our side

I've changed test fixtures because they contained v key with the same value yParity should contain. The change was required because during roundrip v was becoming yParity, thus failing assertions. Deserialization is still fine for previous fixtures, so there're no breaking changes for deserialization in this PR.

@klkvr
Copy link
Member Author

klkvr commented Oct 29, 2024

Closing in favor of #1460

@klkvr klkvr closed this Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] embed consensus transaction types in eth_getTransaction
3 participants