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

Define trait TransactionEnvelope #1910

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

Conversation

emhane
Copy link
Contributor

@emhane emhane commented Jan 12, 2025

Moves part of implementation for TxEnvelope into new trait TransactionEnvelope.

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.

what's the main motiviation for this?

we haven't seen a usecase where we'd need to deal with various txenvelope types, although imo this trait seems reasonable.
but this currently clashes with txenvelope's native functions which we could then remove

#[auto_impl::auto_impl(&, Arc)]
pub trait TransactionEnvelope: Typed2718 {
/// Returns ref to [`Signed<TxLegacy>`] if transaction is a legacy transaction.
fn as_legacy(&self) -> Option<&Signed<TxLegacy>>;
Copy link
Member

Choose a reason for hiding this comment

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

this bakes in the signed type, which I think is fine because I think this will not be used by reth.

but the function name could be problematic, could we introduce as_signed_* instead?

ah I guess this isn't necessary:

pub const fn as_eip2930(&self) -> Option<&Signed<TxEip2930>> {

@emhane
Copy link
Contributor Author

emhane commented Jan 15, 2025

relying on this trait instead of hardcoded TxEnvelope type, allows for rollups to define their own TxEnvelope type to use in block bodies. it has same benefits as generalising the hardcoded transaction type in reth to T: alloy_consensus::Transaction.

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