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.

Sign up for GitHub

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

Jump to bottom

fix: make a sensible encoding api #1496

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

fix: make a sensible encoding api #1496

wants to merge 19 commits into from
+900 −1,036

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented Oct 16, 2024 •
edited
Loading

Spawned from the depths of #1485

Motivation

Fix transaction coding APIs to be usable and clear

  • [Bug] encode_with_signature_fields encodes an invalid transaction when the signature Parity is unexpected #1510 output for encode_with_signature
  • Codebase generally confuses the 2 uses of RLP encoding (tx payload and network format)
  • The docs for encode_with_signature are wrong
  • encode_with_signature produces 2718 output when with_header is false, and RLP in network format when with_header is true. All other encode_ methods produce RLP output in non-network format
  • decode_with_signature is missing entirely
  • EncodableSignature seems to now be redundant as reth migrated to alloy-primitives::Signature

Solution

create a simple API in which encoding and decoding function names specify which format they produce/accept

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@prestwich
Copy link
Member Author

prestwich commented Oct 16, 2024 •
edited
Loading

cc @mattsse @klkvr. I'd like feedback on the new API before applying it to the other tx types

mattsse
mattsse reviewed Oct 16, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment •
edited
Loading

Choose a reason for hiding this comment

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

only looked briefly because rlp gives me headaches

wdyt @Rjected

crates/consensus/src/transaction/rlp.rs
Comment on lines +55 to +67
fn eip2718_encode_with_type(&self, signature: &Signature, ty: u8, out: &mut dyn BufMut) {
out.put_u8(ty);
Copy link
Member

Choose a reason for hiding this comment

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

why would this need a type as argument

Copy link
Member Author

Choose a reason for hiding this comment

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

to demonstrate how cheap and low-effort it is to have abstraction here :)

@prestwich prestwich marked this pull request as draft October 17, 2024 01:12
@prestwich prestwich marked this pull request as ready for review October 17, 2024 15:53
@prestwich
Copy link
Member Author

prestwich commented Oct 17, 2024 •
edited
Loading

this is rebased and ready for review

The main thing it does is make an INTERNAL trait RlpEcdsaTx that captures common encoding behavior across existing tx types. This trait is not intended to cover all future behavior, just to unify codepaths across the 5 current tx types, and make sure that encoding schemes are correct, simple to use, and impossible to confuse

@prestwich prestwich self-assigned this Oct 17, 2024
@prestwich
Copy link
Member Author

now closes #1510 as well

@prestwich prestwich linked an issue Oct 19, 2024 that may be closed by this pull request
[Bug] encode_with_signature_fields encodes an invalid transaction when the signature Parity is unexpected #1510
Open
This was referenced Oct 19, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

@emhane emhane Awaiting requested review from emhane emhane is a code owner

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@prestwich prestwich

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] encode_with_signature_fields encodes an invalid transaction when the signature Parity is unexpected
2 participants
@prestwich @mattsse