fix: make a sensible encoding api by prestwich · Pull Request #1496 · alloy-rs/alloy

prestwich · 2024-10-16T17:15:12Z

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

crates/consensus/src/transaction/envelope.rs

prestwich · 2024-10-16T17:35:14Z

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

mattsse

only looked briefly because rlp gives me headaches

wdyt @Rjected

crates/consensus/src/transaction/rlp.rs

prestwich · 2024-10-17T15:54:42Z

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 · 2024-10-19T01:01:12Z

now closes #1510 as well

prestwich · 2024-10-21T13:09:55Z

this has been rebased onto main after #1528 and is ready for review. Note that 1528 included a version of this API in the blob sidecar rlp impl. I'm also prepping a followup PR to move other RLP impls to this style

prestwich · 2024-10-22T16:39:53Z

Could we get a patch release of alloy-rlp? alloy-rs/rlp#28 would let us reduce code complexity

onbjerg · 2024-10-23T15:32:56Z

@prestwich alloy-rlp 0.3.9 is now released with alloy-rs/rlp#28

prestwich · 2024-10-23T17:19:51Z

@prestwich alloy-rlp 0.3.9 is now released with alloy-rs/rlp#28

Thank you, this PR is updated, rebased, and ready for review 👍

mattsse

cool,
I think this a lot cleaner now.

would like one more sanity rlp review from @klkvr

Rjected

I really like the API, one concern about the header type when this is implemented on legacy txs - I'm thinking legacy txs should probably override all of the default impls for network_ fns

I think we should also specify the type of RLP header when possible (list v string)

crates/consensus/src/transaction/rlp.rs

crates/consensus/src/transaction/legacy.rs

crates/consensus/src/transaction/eip2930.rs

crates/consensus/src/transaction/eip4844.rs

prestwich · 2024-10-24T14:18:07Z

fixes up and ready for re-review

klkvr

lgtm, just a nit

crates/consensus/src/transaction/eip4844.rs

## Motivation Bumps alloy, alloy-core and alloy-eip7702. Code changes: - alloy-rs/alloy#1496 changed transaction encoding API, encoding functions are now prefixed with `rlp`,`eip2718` or `network`. I've updated `TxDeposit` to have methods following this pattern - alloy-rs/core#776 updated signature type to avoid using `Parity` generic and instead just keep a boolean for parity byte. The motivation is to have more correctness in encoding and data structure. Before that it was possible to construct `Signed<TxLegacy>` where transaction chain_id is `Some`, but the parity would not follow EIP-155. This required a few changes to `SpanBatchTransactions` type: - `encode_y_parity_bits` and `encode_tx_sigs_rs` are merged into single `encode_tx_sigs` which firstly encodes parity bits and then `rs` values. Same was done for decoding methods - `recover_v` method is removed. instead, we now pass the `is_protected` flag down to `SpanBatchLegacyTransactionData` and only set `chain_id` to `Some` when `is_protected` is true Blocked by minor releases for alloy, core and revm ## Solution  ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes

@clabby

* chore(consensus): Small Cleanup (alloy-rs#180) ### Description Cleans up `op-alloy-consensus`, moving tests to their respective modules. * feat(genesis): EIP 1559 System Config Accessor (alloy-rs#179) ### Description Introduces an accessor method for the `SystemConfig` to get the encoded EIP 1559 params. * chore: bump alloy (alloy-rs#178) * add arbitrary attr (alloy-rs#182) close alloy-rs#158 * feat(rpc-types): `{Try}From` impl for `OpTransactionReceipt` + `Transaction` -> consensus types (alloy-rs#183) ## Overview Adds `{Try}From` implementations for the RPC receipt / transaction type into the `op-alloy-consensus` `OpReceiptEnvelope` + `OpTxEnvelope` counterparts. closes alloy-rs#181 * chore: release 0.5.1 (alloy-rs#184) ### Description Releases `0.5.1`. Unblocks @clabby from refactoring `kona-host` to remove the `debug_*` endpoints since alloy-rs#183 was merged. * feat(protocol): Batch Type (alloy-rs#186) ### Description Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Batch Validity (alloy-rs#187) ### Description Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). This PR introduces the `BatchValidity` type. * feat(protocol): SingleBatch Type (alloy-rs#188) ### Description Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). This PR introduces the `SingleBatch` type. * feat(protocol): BatchValidationProvider (alloy-rs#189) ### Description Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). This PR introduces a validation trait that is used for span batch validity checks. * feat(protocol): Batch Error Types (alloy-rs#190) ### Description Ports batch error types to `op-alloy-protocol`. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Batch Utilities (alloy-rs#191) ### Description Ports batch utilities to `op-alloy-protocol`. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Span Batch Element (alloy-rs#193) ### Description Ports the span batch element type to `op-alloy-protocol`. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * chore(consensus): Upstream Receipt Constructor (alloy-rs#165) ### Description Small PR to upstream a [receipt constructor from kona](https://github.com/anton-rs/kona/blob/main/crates/executor/src/util.rs#L11). * feat(protocol): Span Batch Bits (alloy-rs#194) ### Description Ports the span batch bits to `op-alloy-protocol`. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Batch TX Data (alloy-rs#195) ### Description Ports the batch transaction data types to `op-alloy-protocol`. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Span Batch Transactions (alloy-rs#196) ### Description Ports the batch transactions to `op-alloy-protocol`. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Span Batch Type (alloy-rs#197) ### Description Ports the Span Batch Type to `op-alloy-protocol`. A follow-on pr will implement batch validation and the prefix check. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Span Batch Validity Checks (alloy-rs#198) ### Description Ports span batch validity checks to `op-alloy-protocol`. ### Provenance Part of a port migrating the batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * feat(protocol): Batch (alloy-rs#200) ### Description Ports the remaining batch types to `op-alloy-protocol`. ### Provenance Last PR in the stack to migrate batch types from `kona-derive` to `op-alloy`. See [`kona#695`](op-rs/kona#695). * chore(workspace): Import Touchups (alloy-rs#199) ### Description Small touchups to dependencies, hoisting dep versioning to the workspace root. * chore: Release 0.5.2 (alloy-rs#201) ### Description Release `op-alloy-*` `v0.5.2` including ported batch types, improved crate hygiene, and more. * feat: Book (alloy-rs#206) ### Description Introduces an mdbook hosted at https://alloy-rs.github.io/op-alloy. Also adds a github action workflow to publish the book. * feat: README (alloy-rs#207) ### Description Updates the `op-alloy` readme, following the layout of [alloy](https://github.com/alloy-rs/alloy). * feat: Book Setup (alloy-rs#208) ### Description Sets up the initial structure for the `op-alloy` book. * feat(book): Genesis - Rollup Config (alloy-rs#209) ### Description Outlines the `RollupConfig` type in `op-alloy-genesis`. * feat: `op-alloy` meta crate (alloy-rs#210) ### Description Introduces a meta crate like `alloy` that re-exports all `op-alloy-*` crates behind respective feature flags. * feat(book): Genesis - System Config (alloy-rs#211) ### Description Adds docs for the `SystemConfig` and structures the book out a bit. * feat(book): Consensus (alloy-rs#212) ### Description Documents the `op-alloy-consensus` crate in the mdbook. * fix(book): Missing READMEs (alloy-rs#213) ### Description Fixes the mdbook missing `READMEs` * feat: `OpTransactionRequest` (alloy-rs#215)   ## Motivation Closes alloy-rs#27 To be able to replace `UnsingedTx` and `TxEnvelope` on Optimism network with op-consensus types, we need a op-specific tx builder. ## Solution Add `OpTransactionBuilder` which is a wrapper around `TransactionBuilder`. It intentionally does not have any direct methods allowing to set EIP-4844 fields, however they can still be set through `AsMut` or during deserialization. When building typed transaction, eip-4844 transactions are getting converted to eip-1559 ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes * fix: ci powerset (alloy-rs#214) ### Description Fixes the feature powerset in ci to exclude `op-alloy` due to combinatorial overflow. * fix(ci): Remove Docs gh-page publish (alloy-rs#216) ### Description Removes the publishing of basic docs to gh-pages. There's now a nice mdbook that's published by the books.yml workflow. * chore(book): Fix Doc Links (alloy-rs#218) ### Description Fixes links doc links. * feat: add jsonrpsee trait for SuperchainSignal (alloy-rs#217) related issues paradigmxyz/reth#12275   ## Motivation  ## Solution  ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes --------- Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> * chore(ci): Use Justfile Targets in Github Actions (alloy-rs#219) ### Description Small PR that re-jiggers github actions to use justfile targets for easier local reproducibility. * chore(book): Consensus Docs (alloy-rs#222) ### Description Re-work the `op-alloy-consensus` docs. * chore(book): Genesis Docs (alloy-rs#223) ### Description Touches up the genesis docs. * fix(book): Small Book Touchups (alloy-rs#220) ### Description Touches up the mdbook and the repo readme to include an active development section. * chore(book): Load Rollup Config Example (alloy-rs#224) ### Description Loading a rollup config example. * [wip] feat: bump alloy (alloy-rs#205)   ## Motivation Bumps alloy, alloy-core and alloy-eip7702. Code changes: - alloy-rs/alloy#1496 changed transaction encoding API, encoding functions are now prefixed with `rlp`,`eip2718` or `network`. I've updated `TxDeposit` to have methods following this pattern - alloy-rs/core#776 updated signature type to avoid using `Parity` generic and instead just keep a boolean for parity byte. The motivation is to have more correctness in encoding and data structure. Before that it was possible to construct `Signed<TxLegacy>` where transaction chain_id is `Some`, but the parity would not follow EIP-155. This required a few changes to `SpanBatchTransactions` type: - `encode_y_parity_bits` and `encode_tx_sigs_rs` are merged into single `encode_tx_sigs` which firstly encodes parity bits and then `rs` values. Same was done for decoding methods - `recover_v` method is removed. instead, we now pass the `is_protected` flag down to `SpanBatchLegacyTransactionData` and only set `chain_id` to `Some` when `is_protected` is true Blocked by minor releases for alloy, core and revm ## Solution  ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes * feat: add holocene extradata fn (alloy-rs#233) ~~blocked by alloy-rs#205~~ ported from reth * chore: release 0.6.0 * chore(book): Protocol Docs (alloy-rs#225) ### Description Makes progress on the book section for working with `op-alloy-protocol` types. * fix(book): Links Imports (alloy-rs#227) ### Description Fixes links doc import. * chore(book): RPC Types (alloy-rs#228) ### Description Adds small docs for `op-alloy-rpc-types` * feat(book): Engine RPC Types (alloy-rs#229) ### Description Adds `op-alloy-rpc-types-engine` docs to the book. * chore(book): Consolidate Links (alloy-rs#230) ### Description Consolidates links into the `links.md` doc. * chore(rpc-types): Clean up Exports (alloy-rs#231) ### Description Small PR to clean up the exports of `op-alloy-rpc-types`. * fix(protocol): Batch Decoding (alloy-rs#235) ### Description Breaks out the small batch decoding fix from alloy-rs#234. * feat: add fn for decoded 1559 params (alloy-rs#236) smol helper * chore: move eip1559 impls (alloy-rs#237) move to consensus, make them reusable * Release: v0.6.1 (alloy-rs#238) * chore: release 0.6.2 * chore: Bump Alloy Deps (alloy-rs#239) ### Description Bumps alloy deps to `0.6.1`. * feat: bump alloy (alloy-rs#240)   ## Motivation  ## Solution  ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes * chore: release 0.6.3 * fix(consensus): fix arbitrary impl for `OpTxType` (alloy-rs#242)   ## Motivation Bug arbitrary impl ## Solution Add missing `OpTxType::Eip7702` to `OpTxType::ALL` list ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes * fix(consensus): add conversion for `OpTxType::Eip7702` (alloy-rs#244)   ## Motivation Broken decoding for eip7702 ## Solution Adds conversion from u8 to `OpTxType::Eip7702` ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes * chore: add is dynamic fee (alloy-rs#245) * chore: release 0.6.4 * feat: add nonce to RPC transaction (alloy-rs#246)   ## Motivation ref paradigmxyz/reth#12474 We need a separate `deposit_nonce` to account for deposit transaction responses which have it while it's not present in inner envelope. ## Solution Adds `nonce` field to `OptionalFields` helper. It would get deserialized only if envelope did not consume it and serialized only if present and inner envelope is a deposit ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes * feat: wrap `TxDeposit` into `Sealed` in `OpTxEnvelope` (alloy-rs#247)   ## Motivation ref https://t.me/paradigm_reth/36099 Makes envelope more consistend by making all variants hold a hash ## Solution  ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes * chore: add deserde test (alloy-rs#248) * chore: bump alloy 064 (alloy-rs#249) * chore: release 0.6.5 * refactor: support mantle * fix: correct token ratio field name and update L1 origin type * update alloy version to 0.11.1 * fix: sepolia batch_inbox_address (#6) --------- Co-authored-by: refcell <abigger87@gmail.com> Co-authored-by: Oliver <onbjerg@users.noreply.github.com> Co-authored-by: Joseph Zhao <65984904+programskillforverification@users.noreply.github.com> Co-authored-by: clabby <ben@clab.by> Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com> Co-authored-by: Hoa Nguyen <hoa@notional.ventures> Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com> Co-authored-by: PinelliaC <wylbzc4928@gmail.com>

## Motivation Bumps alloy, alloy-core and alloy-eip7702. Code changes: - alloy-rs/alloy#1496 changed transaction encoding API, encoding functions are now prefixed with `rlp`,`eip2718` or `network`. I've updated `TxDeposit` to have methods following this pattern - alloy-rs/core#776 updated signature type to avoid using `Parity` generic and instead just keep a boolean for parity byte. The motivation is to have more correctness in encoding and data structure. Before that it was possible to construct `Signed<TxLegacy>` where transaction chain_id is `Some`, but the parity would not follow EIP-155. This required a few changes to `SpanBatchTransactions` type: - `encode_y_parity_bits` and `encode_tx_sigs_rs` are merged into single `encode_tx_sigs` which firstly encodes parity bits and then `rs` values. Same was done for decoding methods - `recover_v` method is removed. instead, we now pass the `is_protected` flag down to `SpanBatchLegacyTransactionData` and only set `chain_id` to `Some` when `is_protected` is true Blocked by minor releases for alloy, core and revm ## Solution  ## PR Checklist - [ ] Added Tests - [ ] Added Documentation - [ ] Breaking changes

prestwich requested review from DaniPopes, emhane, gakonst, mattsse and onbjerg as code owners October 16, 2024 17:15

prestwich commented Oct 16, 2024

View reviewed changes

crates/consensus/src/transaction/envelope.rs Show resolved Hide resolved

mattsse reviewed Oct 16, 2024

View reviewed changes

crates/consensus/src/transaction/rlp.rs Show resolved Hide resolved

prestwich marked this pull request as draft October 17, 2024 01:12

prestwich marked this pull request as ready for review October 17, 2024 15:53

prestwich force-pushed the prestwich/improve-enc-api branch from 539cf91 to 95844bf Compare October 17, 2024 15:53

prestwich self-assigned this Oct 17, 2024

prestwich requested a review from mattsse October 17, 2024 19:45

prestwich force-pushed the prestwich/improve-enc-api branch from 95844bf to bb3c510 Compare October 19, 2024 00:29

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

Closed

This was referenced Oct 19, 2024

refactor: move hash memoization out of signed #1485

Closed

fix: correct implementations of Encodable and Decodable for sidecars #1528

Merged

[Bug] trie_hash impl incorrect for tx envelope #1537

Closed

yash-atreya mentioned this pull request Oct 21, 2024

chore(deps): bumps alloy, revm, fork-db foundry-rs/foundry#9150

Merged

prestwich force-pushed the prestwich/improve-enc-api branch from 86e2c5e to 21f7aef Compare October 21, 2024 12:55

prestwich added 5 commits October 23, 2024 13:19

fix: make a sensible encoding api

e72ce42

fix: k256 test

c0f30a6

fix: single doc hidden

f99d477

fix: missing doc

b7dd75a

nit: undo changed list

f1bd89a

prestwich added 4 commits October 23, 2024 13:19

lint: clippy

cc7f064

fix: use corrected impls

1216123

nit: remove bug comments

cf6fb4f

refactor: use length_with_payload

1257307

prestwich force-pushed the prestwich/improve-enc-api branch from 8de9c9f to 1257307 Compare October 23, 2024 17:19

mattsse approved these changes Oct 23, 2024

View reviewed changes

Rjected reviewed Oct 23, 2024

View reviewed changes

crates/consensus/src/transaction/rlp.rs Outdated Show resolved Hide resolved

crates/consensus/src/transaction/rlp.rs Outdated Show resolved Hide resolved

crates/consensus/src/transaction/rlp.rs Outdated Show resolved Hide resolved

klkvr requested changes Oct 23, 2024

View reviewed changes

klkvr reviewed Oct 23, 2024

View reviewed changes

crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved

prestwich added 4 commits October 24, 2024 08:05

fix: legacy network headers are transparent

a977339

fix: restore signature v check

7b03e1a

fix: check for leftoverbytes

a7aba88

fix: corrected RLP decoding of eip4844 variant

72169eb

prestwich force-pushed the prestwich/improve-enc-api branch from d9c7bd5 to 72169eb Compare October 24, 2024 12:26

fix: list check in 4844 decoding

2a024ee

klkvr approved these changes Oct 24, 2024

View reviewed changes

crates/consensus/src/transaction/eip4844.rs Outdated Show resolved Hide resolved

fix: add map or

fe75cf0

mattsse approved these changes Oct 28, 2024

View reviewed changes

mattsse merged commit f34a2dd into main Oct 28, 2024

mattsse deleted the prestwich/improve-enc-api branch October 28, 2024 13:23

prestwich mentioned this pull request Oct 28, 2024

Fix: move type flag encoding back into encode_2718 #1483

Closed

klkvr mentioned this pull request Oct 30, 2024

[wip] feat: bump alloy alloy-rs/op-alloy#205

Merged

3 tasks

Conversation

prestwich commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

PR Checklist

Uh oh!

Uh oh!

prestwich commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattsse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prestwich commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestwich commented Oct 19, 2024

Uh oh!

prestwich commented Oct 21, 2024

Uh oh!

prestwich commented Oct 22, 2024

Uh oh!

onbjerg commented Oct 23, 2024

Uh oh!

prestwich commented Oct 23, 2024

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Rjected left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prestwich commented Oct 24, 2024

Uh oh!

klkvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

prestwich commented Oct 16, 2024 •

edited

Loading

prestwich commented Oct 16, 2024 •

edited

Loading

mattsse left a comment •

edited

Loading

prestwich commented Oct 17, 2024 •

edited

Loading

Rjected left a comment •

edited

Loading