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

TypeTransaction conversion trait impls #472

Merged
merged 5 commits into from
Apr 8, 2024

Conversation

yash-atreya
Copy link
Member

Motivation

Ref #438

Solution

Add From bounds to UnsignedTx and TransactionRequest types in Network.

Satisfy bounds by implementing the following:

  • From<TypedTransaction> for TransactionRequest
  • From<TxEnvelope> for TransactionRequest
  • From<TypedTransaction> for WithOtherFields<TransactionRequest>
  • From<TxEnvelope> for WithOtherFields<TransactionRequest>

To accomplish the above, implemented

  • From<TxLegacy> for TransactionRequest
  • From<TxEip2930> for TransactionRequest
  • From<TxEip1559> for TransactionRequest
  • From<TxEip4844Variant> for TransactionRequest

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

minor changes

crates/consensus/src/signed.rs Outdated Show resolved Hide resolved

impl From<TxEnvelope> for TransactionRequest {
fn from(envelope: TxEnvelope) -> TransactionRequest {
match envelope {
Copy link
Member

Choose a reason for hiding this comment

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

we could attempt to extract from from the signature here (ignoring any error on the ecdsa recovery)

Copy link
Member Author

Choose a reason for hiding this comment

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

recover_signer is gated behind the k256 feature here. We can make k256 default in consensus or add cfg gate here. But I do not like adding so many cfg gates with different features. It makes the UX cumbersome. Curious to hear your view?

Copy link
Member

Choose a reason for hiding this comment

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

it does make the UX cumbersome, but I also don't like dropping information that we definitely have and the user is likely to want if that makes sense

I wonder if it would make sense to extend the signed struct with signed_by: OnceLock<Address> which can be computed if K256 enabled

Copy link
Member

Choose a reason for hiding this comment

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

Regardless, this decision shouldn't block the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if it would make sense to extend the signed struct with signed_by: OnceLock

which can be computed if K256 enabled

If I'm not wrong, OnceLock enables lazy initialization, so this wouldn't affect performance even if the signer is recovered implicitly.

@yash-atreya
Copy link
Member Author

Ready to merge @prestwich ?

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

little cleanup.

please open a followup issue for memoizing signer address on Signed

TxEnvelope::Legacy(tx) => {
#[cfg(feature = "k256")]
{
if let Ok(signer) = tx.recover_signer() {
Copy link
Member

Choose a reason for hiding this comment

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

let from = tx.recover_signer().ok();
let tx = tx.strip_signature().into()
if let Some(from) = from {
   tx.from(from);
}

bit cleaner

@prestwich prestwich merged commit 84a110c into alloy-rs:main Apr 8, 2024
18 checks passed
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
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