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

Implement Transaction type on Either type #2097

Merged
merged 3 commits into from
Mar 1, 2025

Conversation

neithanmo
Copy link
Contributor

@neithanmo neithanmo commented Feb 21, 2025

Motivation

See #2051

Solution

A new module called either is added, this is useful to handle different transactions types
as it implements Transaction trait. The Either type here is a bit similar to this, however a custom one might be easier to customize.

PR Checklist

  • [x ] Added Tests
  • [x ] 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.

we can simplify this a lot if we reuse the existing Either type from the either crate and implement our Tx traits for that type

@neithanmo
Copy link
Contributor Author

neithanmo commented Feb 28, 2025

we can simplify this a lot if we reuse the existing Either type from the either crate and implement our Tx traits for that typeD

@mattsse
I think, the wrapper approach sacrifices Rust's pattern matching ergonomics:

// With wrapper (cumbersome):
if let Some(tx) = self.as_left() {
    tx.authorization_list()
} else if let Some(tx) = self.as_right() {
    tx.authorization_list()
} else {
    unreachable!()
}

With direct Either enum (clean):

match tx {
    Either::Left(t) => t.authorization_list(),
    Either::Right(t) => t.authorization_list(),
}

While reusing either crate saves implementation code, it significantly impacts usability. Unless we make the inner field public (breaking encapsulation) or implement our own enum directly(as it is currently implemented here), we lose the clean pattern matching that makes working with sum types handy and easy to use and clean.

@yash-atreya yash-atreya added c-consensus Pertaining to the consensus crate enhancement New feature or request labels Feb 28, 2025
@DaniPopes
Copy link
Member

What do you mean? https://docs.rs/either is exactly the same as what you defined

@neithanmo
Copy link
Contributor Author

What do you mean? https://docs.rs/either is exactly the same as what you defined

Yep, it is the same to take advantage of the ergonomics of sum types.
defining it as a wrapper:

pub enum Either(pub either::Either);

would require an indirection layer, assuming inner field is defined as a public:

match tx_either.0 {
       either::Either::Left(..)
       either::Either::Right(..)
}

if defined as private would be even less ergonomic.

Alright, i will change this to use a wrapper type with the inner field public

@neithanmo
Copy link
Contributor Author

What would be a good name here?

  • Either could cause name collision with the original either crates in users code
  • TransactionEither seems good but?

What do you think? @DaniPopes @mattsse

@DaniPopes
Copy link
Member

You shouldn't need a wrapper type at all, all the traits you want to implement are defined in this repository so you can just implement them on the foreign type

@neithanmo
Copy link
Contributor Author

You shouldn't need a wrapper type at all, all the traits you want to implement are defined in this repository so you can just implement them on the foreign type

Done !!

@neithanmo neithanmo changed the title Add custom Either type for transactions Implement Transaction type on Either type Feb 28, 2025
@mattsse mattsse merged commit 6450f05 into alloy-rs:main Mar 1, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-consensus Pertaining to the consensus crate enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants