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

feat: transaction trait #11877

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Conversation

edisontim
Copy link
Contributor

@edisontim edisontim commented Oct 18, 2024

Transaction trait has been partially implemented but it's missing a few trait bounds as well as the behavior of reth_primitives::Transaction
Closes #11533

@edisontim
Copy link
Contributor Author

Hey @emhane ! Could you have a look at this one please?

Copy link
Member

@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.

good job so far

crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

with these things I recommend starting smol, we can always add more functionality later

crates/primitives-traits/Cargo.toml Outdated Show resolved Hide resolved
crates/primitives-traits/Cargo.toml Outdated Show resolved Hide resolved
crates/primitives-traits/Cargo.toml Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
Comment on lines -10 to -13
/// Helper trait that unifies all behaviour required by transaction to support full node operations.
pub trait FullTransaction: Transaction + Compact {}

impl<T> FullTransaction for T where T: Transaction + Compact {}
Copy link
Member

Choose a reason for hiding this comment

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

revert this, we don't want to require Compact be a super trait for Transaction

Copy link
Member

@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.

almost there

crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
crates/primitives-traits/src/transaction/mod.rs Outdated Show resolved Hide resolved
@emhane emhane enabled auto-merge October 26, 2024 14:57
@edisontim edisontim requested a review from mattsse October 26, 2024 17:07
Copy link
Member

@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.

another idea for arbitrary feature, is to do smthg like this in order to use it as super trait independently of FullTransaction, as you initially thought, but without requiring impl of another trait TransactionInner on transaction types alloy-rs/core#766 (comment)
so for this case it would be

#[cfg(not(feature = "arbitrary"))]
trait MaybeArbitrary {}

#[cfg(feature = "arbitrary")]
trait MaybeArbitrary: for<'a> arbitrary::Arbitrary<'a> {}

pub trait Transaction: .. + MaybeArbitrary {
    ..
}

but we can merge this pr as is and make that change in a follow up pr

@edisontim
Copy link
Contributor Author

another idea for arbitrary feature, is to do smthg like this in order to use it as super trait independently of FullTransaction, as you initially thought, but without requiring impl of another trait TransactionInner on transaction types alloy-rs/core#766 (comment) so for this case it would be

#[cfg(not(feature = "arbitrary"))]
trait MaybeArbitrary {}

#[cfg(feature = "arbitrary")]
trait MaybeArbitrary: for<'a> arbitrary::Arbitrary<'a> {}

pub trait Transaction: .. + MaybeArbitrary {
    ..
}

but we can merge this pr as is and make that change in a follow up pr

That makes sense!

Copy link
Collaborator

@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.

lgtm, pending @emhane suggestions

@emhane emhane added this pull request to the merge queue Oct 29, 2024
@mattsse mattsse removed this pull request from the merge queue due to a manual request Oct 29, 2024
@mattsse mattsse added the A-sdk Related to reth's use as a library label Oct 29, 2024
/// Helper trait that unifies all behaviour required by transaction to support full node operations.
pub trait FullTransaction: Transaction + Compact {}

#[cfg(feature = "arbitrary")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(feature = "arbitrary")]

@emhane emhane added this pull request to the merge queue Oct 29, 2024
Merged via the queue into paradigmxyz:main with commit 8278418 Oct 29, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sdk Related to reth's use as a library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define trait Transaction
3 participants