-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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(rpc): remove use of extensible transaction + receipt types #9774
Conversation
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.
+1 on the direction
equivalent to main in hive tests https://github.com/paradigmxyz/reth/actions/runs/10928588345/job/30337917567 |
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.
ty for suffering through this.
I get now why we need the compat trait, although I believe once we have primitive traits we can simplify a few things here. I'm fine with this trait since this unlocks a few things and further separates op from eth.
only have a few questions
@@ -54,13 +51,14 @@ use crate::{ | |||
}; | |||
|
|||
/// Alias for [`reth_rpc_eth_types::EthApiBuilderCtx`], adapter for [`FullNodeComponents`]. | |||
pub type EthApiBuilderCtx<N> = reth_rpc_eth_types::EthApiBuilderCtx< | |||
pub type EthApiBuilderCtx<N, Eth> = reth_rpc_eth_types::EthApiBuilderCtx< | |||
<N as FullNodeTypes>::Provider, | |||
<N as FullNodeComponents>::Pool, | |||
<N as FullNodeComponents>::Evm, | |||
<N as FullNodeComponents>::Network, | |||
TaskExecutor, |
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'd actually like to remove the Tasks generic everywhere and use Box dyn (followup)
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.
it's an unnecessary perf downgrade to use dynamic dispatch for types that are called (i) often and (ii) always return a trait object originating from the same type. I'd try to reserve dynamic dispatch for builders that are called once. or where trait objects are returned that originate from different types for each call.
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.
cool,
this gets us at least closer to where we want to go, the transactioncompat is necessary, tmp weirdness that will improve shortly
Ref #8780. Closes #8988. Closes #10632.
optimism
feature fromreth-rpc-types-compat
reth-rpc-types-compat
transaction type conversion, which was previously optimism-feature gated, into new traitTransactionCompat
. These are called many times during the program, hence are not used as trait objects.EthApiTypes::TransactionCompat
, allowing different networks to use same transaction type but with different reth compatibility methods.EthApiTypes::TransactionCompat
, allows for implementation of conversion reth types ->op-alloy-rpc-types
to be implemented in reth repo as opposed to op-alloy repo.Alt. solution:
alloy_network::Network
associated types must provide reth compatibility viaTransactionResponse
,ReceiptResponse
, etc. traits, then no additional associated typeEthApiTypes::TransactionCompat
is needed: alloy-rs/alloy#1173.