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

feat: add Signature trait #782

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add Signature trait #782

wants to merge 1 commit into from

Conversation

grandima
Copy link

Motivation

Create trait Signature and move all possible methods and traits from struct Signature to trait Signature. Resolve #766

Solution

  • Add trait Signature.
  • Move all non-const functions totrait Signature.
  • Conform trait Signature to all possible supertraits.
  • Rename struct Signature into struct EcdsaSignature.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

emhane
emhane suggested changes Oct 27, 2024
View reviewed changes
Copy link
Contributor

@emhane emhane left a comment

Choose a reason for hiding this comment

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

I like the renaming of Signature to EcdsaSignature.

try and leave as many default trait method implementations as possible, then I will review again.

crates/primitives/src/signature/sig.rs Outdated
Comment on lines 5 to 6
pub trait Signature<'a>:
TryFrom<&'a [u8], Error = SignatureError> + FromStr<Err = SignatureError>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub trait Signature<'a>:
TryFrom<&'a [u8], Error = SignatureError> + FromStr<Err = SignatureError>
pub trait Signature:
for<'a> TryFrom<&'a [u8], Error = SignatureError> + FromStr<Err = SignatureError>

crates/primitives/src/signature/sig.rs Outdated
Comment on lines 15 to 17
fn test_signature() -> Self
where
Self: Sized;
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be in the trait imo. if it is, then with attribute #[cfg(test)] or #[cfg(feature = test-util], but that would change the logic here, but this is a refactor pr, so leave it out, just move it to the EcdsaSignature impl body.

emhane
emhane suggested changes Oct 27, 2024
View reviewed changes
Copy link
Contributor

@emhane emhane left a comment

Choose a reason for hiding this comment

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

half the review sent, my bad

crates/primitives/src/signature/ecdsa_sig.rs Outdated
Comment on lines 24 to 80
/// Instantiate a new signature from `r`, `s`, and `v` values.
#[allow(clippy::missing_const_for_fn)]
pub fn new(r: U256, s: U256, v: Parity) -> Self {
Self { r, s, v }
}

/// Returns the `r` component of this signature.
#[inline]
pub const fn r(&self) -> U256 {
self.r
}

/// Returns the `s` component of this signature.
#[inline]
pub const fn s(&self) -> U256 {
self.s
}

/// Returns the recovery ID as a `u8`.
#[inline]
pub const fn v(&self) -> Parity {
self.v
}
Copy link
Contributor

Choose a reason for hiding this comment

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

please add these as trait methods too, they make sense in reth integration

crates/primitives/src/signature/sig.rs Outdated
Comment on lines 9 to 30
#[cfg(feature = "rlp")]
fn decode_rlp_vrs(buf: &mut &[u8]) -> Result<Self, alloy_rlp::Error>
where
Self: Sized;
Copy link
Contributor

@emhane emhane Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this for example can have a default impl, since the trait method Signature::from_rs_and_parity is accessible

@grandima grandima force-pushed the signature-trait branch 2 times, most recently from 6d23d9f to e9d3a60 Compare October 28, 2024 17:52
emhane
emhane suggested changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@emhane emhane left a comment

Choose a reason for hiding this comment

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

nice!

crates/primitives/src/signature/super_signature.rs Outdated
Comment on lines 25 to 30
pub trait ArbitrarySuperSig:
arbitrary::Arbitrary
+ proptest::arbitrary::Arbitrary<Parameters=(),Strategy=proptest::strategy::FilterMap<
<(U256, U256, Parity) as proptest::arbitrary::Arbitrary>::Strategy,
fn((U256, U256, Parity)) -> Option<Self>,
>> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

should be enough to do

Suggested change
pub trait ArbitrarySuperSig:
arbitrary::Arbitrary
+ proptest::arbitrary::Arbitrary<Parameters=(),Strategy=proptest::strategy::FilterMap<
<(U256, U256, Parity) as proptest::arbitrary::Arbitrary>::Strategy,
fn((U256, U256, Parity)) -> Option<Self>,
>> {}
pub trait ArbitrarySuperSig:
for <'a> arbitrary::Arbitrary<'a> {}

crates/primitives/src/signature/ecdsa_sig.rs
Comment on lines +81 to +51
#[cfg(feature = "rlp")]
fn decode_rlp_vrs(buf: &mut &[u8]) -> Result<Self, alloy_rlp::Error> {
use alloy_rlp::Decodable;

let parity: Parity = Decodable::decode(buf)?;
let r = Decodable::decode(buf)?;
let s = Decodable::decode(buf)?;

Self::from_rs_and_parity(r, s, parity)
.map_err(|_| alloy_rlp::Error::Custom("attempted to decode invalid field element"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

on second thought, would be cool to move all the feature gated methods into their respective XSuperSig trait actually

Copy link
Author

@grandima grandima Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I thought that too! However, I took iterative approach and decided to implement it in the future commits to this PR. I just did not expect you to review it since I did not ping you. Anyway, thanks for valuable suggestions!

@grandima
Copy link
Author

Hey @emhane!
Thanks for your review.
I'm still in progress on this ticket. To not take much of your time, I'll just trigger "Re-request review" when I'm ready with all the changes.

@grandima grandima force-pushed the signature-trait branch 2 times, most recently from a353cf6 to 2ac8cad Compare October 31, 2024 00:40
@grandima grandima requested a review from emhane October 31, 2024 00:44
@grandima
Copy link
Author

@emhane please review

@grandima
feat: define Signature trait
0e5c533 
add ArbitrarySuperSig, K256SuperSig, RlpSuperSig, SerdeSuperSig feature traits to Signature
emhane
emhane approved these changes Nov 10, 2024
View reviewed changes
Copy link
Contributor

@emhane emhane left a comment

Choose a reason for hiding this comment

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

lgtm

@grandima
Copy link
Author

I don't know exactly who from code owners to tag so I'll just pick @mattsse Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emhane emhane emhane approved these changes

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Define Signature trait
2 participants
@grandima @emhane