eth_sendRawTransactionConditional L2 RPC endpoint#13762
eth_sendRawTransactionConditional L2 RPC endpoint#13762oac1771 wants to merge 3 commits intoparadigmxyz:mainfrom
Conversation
|
Here's an example of the api implementation in op-geth: https://github.com/ethereum-optimism/op-geth/blob/optimism/internal/sequencerapi/api.go How about i'll create the backing implementation for this feature in the block builder and you can finish off the api in the file I referenced. I would recommend familiarizing yourself with that handler |
|
Sounds great |
| #[async_trait::async_trait] | ||
| impl<T> L2EthApiExtServer for T | ||
| where | ||
| T: FullEthApi, | ||
| jsonrpsee_types::error::ErrorObject<'static>: From<T::Error>, | ||
| { |
There was a problem hiding this comment.
we can't implement this default like this because we need to actually handle the conditionals.
can we start with a new helper type in
reth/crates/optimism/rpc/Cargo.toml
Line 2 in 5db0129
instead, that wraps P: Pool?
like struct L2TransactionConditional<P>{ pool: P}
There was a problem hiding this comment.
Based on the impl in op-geth, we don't check the conditional in the pool since that means repeated state checks as transactions are reinserted in the pool.
Instead we check the conditional at the rpc layer preemptively to the pool and at the block building process. The transaction has a mutable rejected field, set by the that is used to drive eviction in the mempool (also being used in the interop feature)
still wrapping my head around the different modules integrate with each other in this code. If a similar mutable approach is possible with OpTransactionSigned or if there is a nicer idiomatic way for the payload builder to better surface a list of non-included tx that should be evicted the mempool
There was a problem hiding this comment.
let's start with my suggestion and I'll leave more instructions once we have that placeholder
There was a problem hiding this comment.
oh nice I didn't see #13806, I was making the same exact change to OpPooledTransaction to manage some external state. Let me pull that in.
I'll create a separate PR (hopefully tomorrow) so we can discuss from there separately from there. Can you briefly elaborate on why we'd need L2TransactionConditional<P> { pool: P }? I'm not fully following a change to the api. In the draft I have I simply have an Option<TransactionConditional> field on OpTransactionSigned so afaik pool api should have to be wrapped at all since the pool transactions are already typed.
Unless you were thinking OpSignedTransaction is untouched and the pool has a separate submission api for these txs.
I think a draft a small diff in the optimism crate will help clarify. Will get this out super soon!
There was a problem hiding this comment.
Unless you were thinking OpSignedTransaction is untouched and the pool has a separate submission api for these txs.
Sounds good, can you link a draft pr?
There was a problem hiding this comment.
as @mattsse says, the conditionals need to be handled and they are specific of the L2, so it won't be implemented in this place of the code anyway. perhaps you would like to close this pr and give it another go by implementing https://github.com/paradigmxyz/reth/pull/13762/files#r1921021197 @oac1771 , unless @hamdiallam has already started with it?
emhane
added
A-op-reth
|
sorry, just saw this is superseded by #14311, nvm @hamdiallam |
4 participants
This is a draft pr for 13488.
So far I just implemented a blanket implementation of
L2EthApiExtServerwithout using theConditionalOptions. Im not exactly sure how to extract the information from thebytesargument to check againstcondition. I think I am missing something and would definitely love a push in the right direction.