reth icon indicating copy to clipboard operation
reth copied to clipboard

Relax Trait Bounds on TransactionPool::Transaction and EthPoolTransaction

Open 0xForerunner opened this issue 1 year ago • 9 comments

I'm working on the World Coin Block builder and ran into a problem implementing TransactionPool which currently requires

    PoolTransaction<
        Consensus = TransactionSignedEcRecovered,
        Pooled = PooledTransactionsElementEcRecovered
    >

which is unnecessarily restrictive. I've created a super trait which looks like this:

/// Super trait for transactions that can be converted to and from Eth transactions
pub trait EthPoolTransaction:
    PoolTransaction<
        Consensus: From<TransactionSignedEcRecovered> + Into<TransactionSignedEcRecovered>,
        Pooled: From<PooledTransactionsElementEcRecovered>
                    + Into<PooledTransactionsElementEcRecovered>,
    > + IntoRecoveredTransaction
{
    /// Extracts the blob sidecar from the transaction.
    fn take_blob(&mut self) -> EthBlobTransactionSidecar;

    /// Returns the number of blobs this transaction has.
    fn blob_count(&self) -> usize;

    /// Validates the blob sidecar of the transaction with the given settings.
    fn validate_blob(
        &self,
        blob: &BlobTransactionSidecar,
        settings: &KzgSettings,
    ) -> Result<(), BlobTransactionValidationError>;

    /// Returns the number of authorizations this transaction has.
    fn authorization_count(&self) -> usize;
}

which should be far more versatile for anyone else looking to implement the trait.

I was considering removing IntoRecoveredTransaction since it seems unnecessary since now you can simply call .into_consensus().into() in every case where you would have called .to_recovered_transaction But I decided to leave it since I wasn't sure if there were any other use cases for it. If you guys want to remove it we can do that in another PR.

Additionally perhaps we should consider renaming trait TransactionPool to trait EthTransactionPool to better illustrate it's bounds.

Would love your feedback, and looking forward to getting this merged in. Thanks!

0xForerunner avatar Sep 20 '24 17:09 0xForerunner