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

Migrate from TransactionSigned to alloy_consensus::Signed<T> #11510

Open
emhane opened this issue Oct 5, 2024 · 2 comments
Open

Migrate from TransactionSigned to alloy_consensus::Signed<T> #11510

emhane opened this issue Oct 5, 2024 · 2 comments
Labels
C-debt Refactor of code section that is hard to understand or maintain

Comments

@emhane
Copy link
Member

emhane commented Oct 5, 2024

Describe the feature

Remove reth type

/// Signed transaction.
#[cfg_attr(any(test, feature = "reth-codec"), reth_codecs::add_arbitrary_tests(rlp))]
#[derive(Debug, Clone, PartialEq, Eq, Hash, AsRef, Deref, Serialize, Deserialize)]
pub struct TransactionSigned {
/// Transaction hash
pub hash: TxHash,
/// The transaction signature values
pub signature: Signature,
/// Raw transaction info
#[deref]
#[as_ref]
pub transaction: Transaction,
}

in favour of alloy_consensus::Signed<T, S>. In order to limit scope, it's recommended to introduce type alias

pub type TransactionSigned = Signed<reth_primitives::Transaction, alloy_primitives::Signature>;

blocked by #11253

Additional context

No response

@emhane emhane added S-blocked This cannot more forward until something else changes C-debt Refactor of code section that is hard to understand or maintain labels Oct 5, 2024
@emhane
Copy link
Member Author

emhane commented Oct 5, 2024

Starting with turning TransactionSigned into a wrapper of alloy type should unblock this

#[derive(derive_more::Deref)]
pub struct TransactionSigned {
    #[deref]
    inner: Signed<Transaction, Signature>
}

@emhane emhane removed the S-blocked This cannot more forward until something else changes label Oct 5, 2024
@klkvr
Copy link
Collaborator

klkvr commented Oct 5, 2024

current TransactionSigned is a bit different from how alloy handles transaction enums: https://github.com/alloy-rs/alloy/blob/de2a13340303db6dde1296f68df57078a5b1fdd8/crates/consensus/src/transaction/envelope.rs#L93-L115

there might be valid transactions with no signatures, eg right now we keep a dummy signature for deposit tx which is never encoded

should we consider refactoring TransactionSigned into an enum or embedding alloy's envelope right away?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-debt Refactor of code section that is hard to understand or maintain
Projects
Status: Todo
Development

No branches or pull requests

2 participants