fix: use enveloped encoding for typed transactions #239
Conversation
Rjected
requested review from
DaniPopes,
Evalir,
gakonst and
prestwich
as code owners
|
fixing conflicts and adding blob tx with sidecar impl... |
There was a problem hiding this comment.
Awesome! Agreed we need to iron these out—but I believe enveloped encoding is already handled by TxEnvelope & Decodable2718/Encodable2718? (see envelope.rs & alloy_eips::eip2718)
I do think allowing txs to be encoded alone, not in an envelope, is a bit of a footgun and we've seen this in the telegram chat a few times. Maybe we should disallow it?
Ah I didn't see that at all, I'll check those out. I'll add some docs, in that case |
|
encode_signed and decode_signed probably should be documented as "dont call this, you want to call TxEnvelope methods instead" |
Rjected
force-pushed
the
dan/fix-send-raw-transaction-4844
branch
from
4e4a972 to
09e4eea
Compare
| pub fn payload_len_with_signature(&self, signature: &Signature) -> usize { | ||
| /// Returns what the encoded length should be, if the transaction were RLP encoded with the | ||
| /// given signature. | ||
| pub(crate) fn encoded_len_with_signature(&self, signature: &Signature) -> usize { |
There was a problem hiding this comment.
side note: i consider these all tech debt that is paid down by a refactor of rlp derives
There was a problem hiding this comment.
yes, rlp(flatten) etc will usher in a new era of derived and correct encodings
|
@Rjected Plans here? Flagging that it seems like foundry-rs/foundry#7242 (comment) is blocked on this |
I can get this rfr / done today to unblock |
Rjected
force-pushed
the
dan/fix-send-raw-transaction-4844
branch
2 times, most recently
from
7714411 to
40468f0
Compare
|
Marked this as RFR, the code is ready to review, i plan on adding more tests today |
Rjected
force-pushed
the
dan/fix-send-raw-transaction-4844
branch
2 times, most recently
from
239737c to
7991659
Compare
remove redundant method fix: remove encode_enveloped * improve docs for `encode_signed` * fix vec with_capacity and `into_signed` for 4844 with sidecar fix more raw tx encodings, need to fix length hmm this is all network encoding? still TODO: clear up api boundaries remove encode_signed, decode_signed large refactors to encoding logic todo: analogous methods on Signed, fix tests fix tests fix more tests and lints remove incorrect doc remove unnecessary printlns remove deref, remove tagged legacy fix tests fix Encodable length and add test
Rjected
force-pushed
the
dan/fix-send-raw-transaction-4844
branch
from
7991659 to
6b99d2a
Compare
|
|
||
| /// Encode with a signature. This encoding is usually RLP, but may be | ||
| /// different for future EIP-2718 transaction types. | ||
| fn encode_signed(&self, signature: &Signature, out: &mut dyn BufMut); |
There was a problem hiding this comment.
encode_signed and decode_signed are not specific enough for eth transactions, it's ambiguous whether or not you want the network or RPC RLP format for transactions. Encodable2718 provides this distinction
There was a problem hiding this comment.
ok, makes sense to me. need to document this, but can be a follow up, would create an issue
Motivation
The encoding for
eth_sendRawTransactionfor typed transactions was not correct, because it was encoding a RLP header for typed transactions.Solution
This completely overhauls transaction encoding and decoding in alloy.
There is no longer
encode_signedanddecode_signedinTransaction.Signed<T>transactions cannot be encoded or decoded directly. Now, the main way to encode and decode transactions is throughEncodable2718andDecodable2718, specifically throughTxEnvelope.The
TaggedLegacyvariant is removed.impl<T> std::ops::Deref for Signed<T>is removed.impl From<Signed<TxEip4844WithSidecar>> for TxEnvelopeis added.impl From<Signed<TxEip4844>> for TxEnvelopeis added.The 2718 trait methods now use
alloy_rlp::Resultinstead ofResult<_, Eip2718Error>Raw RLP of a blob transaction with sidecar is added to
consensus/testdata, and is used in a roundtrip test.PR Checklist