refactor: use simple boolean for parity in signature by klkvr · Pull Request #776 · alloy-rs/core

klkvr · 2024-10-20T21:07:40Z

Motivation

Right now we use Parity enum for parity value in signature:

core/crates/primitives/src/signature/parity.rs

Lines 10 to 17 in 9c53778

    
           pub enum Parity { 
        
               /// Explicit V value. May be EIP-155 modified. 
        
               Eip155(u64), 
        
               /// Non-EIP155. 27 or 28. 
        
               NonEip155(bool), 
        
               /// Parity flag. True for odd. 
        
               Parity(bool), 
        
           }

Because of it containing EIP-155 related variants it basically captures transaction-related logic which does not belong to primitives and results in inconsistencies and need for dynamic checks. e.g alloy-rs/alloy#1305 #705 alloy-rs/alloy#1510

Primitive signature representation in context of Ethereum is (U256,U256,bool), this PR changes Signature to have this structure and leaving specialized parity encoding to types holding signatures and having enough context to perform correct encoding.

At this point in any context we know how parity should be encoded and storing a simple boolean is enough to figure this out. i.e legacy transaction knows its chain_id and can adjust parity accordingly.

API changes in this PR:

serde now defaults to serializing/deserializing parity as "yParity": "0x{0,1}"
rlp now defaults to encoding/decoding parity as boolean
write_rlp_vrs now accepts encodable v value as a parameter, allowing callers to provide arbitrary encoding for parity
decode_rlp_vrs now accepts a decode_parity closure allowing callers to provide arbitrary decoding for parity, possibly capturing additional context (e.g legacy chain_id)

This PR is accompanied with PR in alloy to demonstrate/discuss needed API changes alloy-rs/alloy#1540 alloy-rs/eips#12

PR Checklist

Added Tests
Added Documentation
Breaking changes

mattsse

I personally much prefer this over the enum approach.
another option would be just an integer but given that we need parity as bool, this makes more sense.

I wonder if we should remove the rlp trait impls for this entirely to avoid footguns, because en/decoding is tx specific?

regarding rollout, this will require a few changes downstream, so I wonder if we should instead introduce this a new type first, transition everything, then remove the old Signature type?

mattsse · 2024-10-24T10:09:53Z

crates/primitives/src/signature/sig.rs

    /// Decode an RLP-encoded VRS signature.
-    pub fn decode_rlp_vrs(buf: &mut &[u8]) -> Result<Self, alloy_rlp::Error> {
+    pub fn decode_rlp_vrs(


this needs more context on the parity decoding closure

klkvr · 2024-10-24T17:59:41Z

Extracted new signature type to primitive_sig::PrimitiveSignature

mattsse

lgtm

## 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

* wip * fix doc * fmt

@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

klkvr requested review from DaniPopes, gakonst and mattsse as code owners October 20, 2024 21:07

This was referenced Oct 20, 2024

feat: integrate signature with boolean parity alloy-rs/alloy#1540

Merged

feat: integrate signature with boolean parity alloy-rs/eips#12

Merged

mattsse reviewed Oct 24, 2024

View reviewed changes

klkvr force-pushed the klkvr/signature-parity-bool branch from 6c1f0e0 to 85b4ab3 Compare October 24, 2024 17:58

wip

5b01993

klkvr force-pushed the klkvr/signature-parity-bool branch from 85b4ab3 to 5b01993 Compare October 24, 2024 17:59

klkvr added 2 commits October 24, 2024 22:00

fix doc

e3905d9

fmt

59b63a6

klkvr mentioned this pull request Oct 27, 2024

Regression: "Failed to decode transaction" foundry-rs/foundry#9046

Closed

2 tasks

mattsse approved these changes Oct 28, 2024

View reviewed changes

mattsse merged commit 0ee0afe into main Oct 28, 2024

mattsse deleted the klkvr/signature-parity-bool branch October 28, 2024 13:27

klkvr mentioned this pull request Oct 28, 2024

[Feature] Migrate to PrimitiveSignature type #788

Closed

3 tasks

klkvr self-assigned this Oct 28, 2024

This was referenced Oct 30, 2024

chore: replace Signature with PrimitiveSignature #796

Merged

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

Merged

ZelionD pushed a commit to ZelionD/core that referenced this pull request Nov 11, 2024

refactor: use simple boolean for parity in signature (alloy-rs#776)

c7a9564

* wip * fix doc * fmt

moricho mentioned this pull request Dec 3, 2024

feat: Use PrimitiveSignature instead of Signature paradigmxyz/reth#13087

Merged

jenpaff moved this from Done to Completed in Alloy Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use simple boolean for parity in signature#776

refactor: use simple boolean for parity in signature#776
mattsse merged 3 commits intomainfrom
klkvr/signature-parity-bool

klkvr commented Oct 20, 2024 •

edited

Loading

Uh oh!

mattsse left a comment

Uh oh!

mattsse Oct 24, 2024

Uh oh!

klkvr commented Oct 24, 2024

Uh oh!

mattsse left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	pub enum Parity {
	/// Explicit V value. May be EIP-155 modified.
	Eip155(u64),
	/// Non-EIP155. 27 or 28.
	NonEip155(bool),
	/// Parity flag. True for odd.
	Parity(bool),
	}

Conversation

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

Uh oh!

Motivation

PR Checklist

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

mattsse Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

klkvr commented Oct 24, 2024

Uh oh!

mattsse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

klkvr commented Oct 20, 2024 •

edited

Loading