-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore(sdk): improve usability tx primitive traits #12437
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
c1e7aaa
Implement Transaction for SignedTransaction types
emhane 8e0da8f
Improve usability transaction primitive traits
emhane 68108f8
Add blanket impl for TxType
emhane 99c9943
Fix conflicts
emhane ae6d4b8
Fix arbitrary feature gate
emhane 421f5fc
Merge branch 'main' into emhane/signed-tx
emhane e77c3f1
Merge branch 'main' into emhane/signed-tx
emhane d0cbfc9
Add todo
emhane ffe638a
Fix merge conflicts
emhane 02f1be9
Merge branch 'main' into emhane/signed-tx
emhane 385f5d6
Fix lint
emhane df5bde0
Merge branch 'main' into emhane/signed-tx
emhane 9c973ae
Fix merge conflicts
emhane fcda0dc
Remove redundant trait method
emhane 203ba81
Merge branch 'main' into emhane/signed-tx
emhane 5816e45
Rename AlloyTransactionExt to TransactionExt
emhane a9349fb
Merge branch 'main' into emhane/signed-tx
emhane c83ac9c
Merge branch 'emhane/signed-tx' of github.com:paradigmxyz/reth into e…
emhane 1e42d01
Fix merge conflicts
emhane 4c3a52c
Add todo
emhane 85dc28b
Merge branch 'main' into emhane/signed-tx
emhane 7a33d52
Merge branch 'main' into emhane/signed-tx
emhane da6a573
Fix merge conflicts
emhane File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,17 +2,18 @@ | |
|
||
use alloc::fmt; | ||
use core::hash::Hash; | ||
use reth_codecs::Compact; | ||
|
||
use alloy_consensus::Transaction; | ||
use alloy_eips::eip2718::{Decodable2718, Encodable2718}; | ||
use alloy_primitives::{keccak256, Address, PrimitiveSignature as Signature, TxHash, B256}; | ||
use alloy_primitives::{keccak256, Address, PrimitiveSignature, TxHash, B256}; | ||
use reth_codecs::Compact; | ||
use revm_primitives::TxEnv; | ||
|
||
use crate::{transaction::TransactionExt, FullTransaction, MaybeArbitrary, Transaction}; | ||
|
||
/// Helper trait that unifies all behaviour required by block to support full node operations. | ||
pub trait FullSignedTx: SignedTransaction<Transaction: Compact> + Compact {} | ||
pub trait FullSignedTx: SignedTransaction<Transaction: FullTransaction> + Compact {} | ||
|
||
impl<T> FullSignedTx for T where T: SignedTransaction<Transaction: Compact> + Compact {} | ||
impl<T> FullSignedTx for T where T: SignedTransaction<Transaction: FullTransaction> + Compact {} | ||
|
||
/// A signed transaction. | ||
pub trait SignedTransaction: | ||
|
@@ -31,6 +32,8 @@ pub trait SignedTransaction: | |
+ alloy_rlp::Decodable | ||
+ Encodable2718 | ||
+ Decodable2718 | ||
+ TransactionExt | ||
+ MaybeArbitrary | ||
{ | ||
/// Transaction type that is signed. | ||
type Transaction: Transaction; | ||
|
@@ -42,7 +45,7 @@ pub trait SignedTransaction: | |
fn transaction(&self) -> &Self::Transaction; | ||
|
||
/// Returns reference to signature. | ||
fn signature(&self) -> &Signature; | ||
fn signature(&self) -> &PrimitiveSignature; | ||
|
||
/// Recover signer from signature and hash. | ||
/// | ||
|
@@ -65,8 +68,10 @@ pub trait SignedTransaction: | |
/// Create a new signed transaction from a transaction and its signature. | ||
/// | ||
/// This will also calculate the transaction hash using its encoding. | ||
fn from_transaction_and_signature(transaction: Self::Transaction, signature: Signature) | ||
-> Self; | ||
fn from_transaction_and_signature( | ||
transaction: Self::Transaction, | ||
signature: PrimitiveSignature, | ||
) -> Self; | ||
|
||
/// Calculate transaction hash, eip2728 transaction does not contain rlp header and start with | ||
/// tx type. | ||
|
@@ -77,3 +82,11 @@ pub trait SignedTransaction: | |
/// Fills [`TxEnv`] with an [`Address`] and transaction. | ||
fn fill_tx_env(&self, tx_env: &mut TxEnv, sender: Address); | ||
} | ||
|
||
impl<T: SignedTransaction> TransactionExt for T { | ||
type Type = <T::Transaction as TransactionExt>::Type; | ||
|
||
fn signature_hash(&self) -> B256 { | ||
self.transaction().signature_hash() | ||
} | ||
Comment on lines
+89
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo we can add |
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this?
unclear why we need 3 traits now if this trait is now just a default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about moving the rlp encoding traits to
FullTransaction
...to make primitive traits more lightweight. I'm also good with removing theFull
-traits for the data primitive traits and addingCompact
as a super trait always. wdyt? @mattsseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshieDo ?