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

fix(op): ensure EthApiServer helper trait method default impls, call OpEthApi overrides #9879

Merged
merged 8 commits into from
Jul 30, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Jul 29, 2024

Closes #9868

  • Replaces inner type on OpEthApi with EthApiInner. This ensure that the default trait method impls kept by OpEthApi, will actually invoke the OpEthApi overrides of other trait methods.
  • Inlines getters in EthApiServer helper trait impls for OpEthApi
  • Adds getter trait methods for EthState and EthApiSpec, and moves impl of trait methods for EthApi, into default impl, to simplify impl for re-worked OpEthApi type

@emhane emhane added C-bug An unexpected or incorrect behavior A-rpc Related to the RPC implementation A-op-reth Related to Optimism and op-reth labels Jul 29, 2024
@emhane emhane requested a review from clabby July 29, 2024 21:31
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensure that the default trait method impls kept by OpEthApi, will actually invoke the OpEthApi overrides of other trait methods.

can you please elaborate on this, not obvious to me what this pr fixes.

Adds getter trait methods for EthState and EthApiSpec, and moves impl of trait methods for EthApi, into default impl, to simplify impl for re-worked OpEthApi type

isn't the approach then implement everything that's required on opapi directly and not move more stuff to default impls or did I misunderstood this?

@emhane
Copy link
Member Author

emhane commented Jul 29, 2024

@mattsse
Copy link
Collaborator

mattsse commented Jul 29, 2024

gotcha, where do we have this problem rn?

this sounds more like a general design problem due to the many helper traits with default impls

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, now that I had some time to think about this, this makes perfect sense and we need to wrap the inner type as you highlighted

only have naming suggestion

crates/optimism/rpc/src/eth/mod.rs Outdated Show resolved Hide resolved
Comment on lines +48 to +54
pub type EthApiBuilderCtx<N> = reth_rpc_eth_types::EthApiBuilderCtx<
<N as FullNodeTypes>::Provider,
<N as FullNodeComponents>::Pool,
<N as FullNodeComponents>::Evm,
NetworkHandle,
TaskExecutor,
<N as FullNodeTypes>::Provider,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now your pr about the AT network type makes perfect sense to me,

perhaps we start by removing the taskexecutor generic and replace with Box<dyn>? don't think the generic is actually useful

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I'd like to replace both consensus trait object and task executor trait object with generics eventually. trait objects are great for things that aren't in scope for the whole program, but for consensus and task executor that isn't the case and we can make the smol perf gain by resolving them at compile time. also because we already have a great way to pass the components down to the FullNode type, which is via the FullNodeComponents trait.

made an issue #9872, will tackle it before continuing with #9774

@emhane emhane force-pushed the emhane/fix-op-ethapi branch from 6e78ab2 to 84265d2 Compare July 30, 2024 15:23
@emhane emhane enabled auto-merge July 30, 2024 15:23
@emhane emhane added this pull request to the merge queue Jul 30, 2024
Merged via the queue into main with commit e4ae2a7 Jul 30, 2024
33 checks passed
@emhane emhane deleted the emhane/fix-op-ethapi branch July 30, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-reth Related to Optimism and op-reth A-rpc Related to the RPC implementation C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework OpEthApi type
2 participants