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

refactor: use simple boolean for parity in signature #776

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

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Oct 20, 2024

Motivation

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

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

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

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?

Comment on lines 90 to 91
/// Decode an RLP-encoded VRS signature.
pub fn decode_rlp_vrs(buf: &mut &[u8]) -> Result<Self, alloy_rlp::Error> {
pub fn decode_rlp_vrs(
Copy link
Member

Choose a reason for hiding this comment

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

this needs more context on the parity decoding closure

@klkvr klkvr force-pushed the klkvr/signature-parity-bool branch from 6c1f0e0 to 85b4ab3 Compare October 24, 2024 17:58
@klkvr klkvr force-pushed the klkvr/signature-parity-bool branch from 85b4ab3 to 5b01993 Compare October 24, 2024 17:59
@klkvr
Copy link
Member Author

klkvr commented Oct 24, 2024

Extracted new signature type to primitive_sig::PrimitiveSignature

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.

2 participants