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

chore(sdk): Use EthApiBuilder instead of FnOnce trait #14442

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

emhane
Copy link
Member

@emhane emhane commented Feb 12, 2025

Closes #12509

  • Removes EthApiBuilderCtx which is no longer needed since the entire EthApi is passed to pubs and filter API builders.

@emhane emhane added C-debt A clean up/refactor of existing code A-sdk Related to reth's use as a library labels Feb 12, 2025
@emhane emhane marked this pull request as draft February 12, 2025 10:50
@github-actions github-actions bot added the A-rpc Related to the RPC implementation label Feb 12, 2025
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 is great, and I think we want this @klkvr ?

the failing test are likely due to a missing trait bound?

@emhane
Copy link
Member Author

emhane commented Feb 13, 2025

this is great, and I think we want this @klkvr ?

the failing test are likely due to a missing trait bound?

ye, the trait bounds in rpc builder are madness

@emhane emhane added the A-op-reth Related to Optimism and op-reth label Feb 13, 2025
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.

@emhane

I made some progress on this but there are a few more things I need to think about first.

@emhane emhane force-pushed the emhane/ethapi-builder branch from 5d23de0 to 4fc1af5 Compare February 15, 2025 21:27
@emhane
Copy link
Member Author

emhane commented Feb 15, 2025

@mattsse lemme know if this is what you want

@emhane emhane marked this pull request as ready for review February 15, 2025 21:53
@emhane emhane requested a review from rakita as a code owner February 15, 2025 21:53
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.

cool, ty. this looks good overall

need a bit more time to check a few things but I consider this complete

@mattsse mattsse self-assigned this Feb 20, 2025
Comment on lines 134 to 143
fn build(
self,
provider: N::Provider,
pool: N::Pool,
network: N::Network,
evm: N::Evm,
config: EthConfig,
executor: impl TaskSpawner + 'static,
cache: EthStateCache<BlockTy<N::Types>, ReceiptTy<N::Types>>,
) -> Self::EthApi {
Copy link
Member

Choose a reason for hiding this comment

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

this could just take an instance of N I think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I think we should add the context container type back here but now generic over N

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I get it now

@emhane emhane enabled auto-merge February 25, 2025 12:38
@emhane emhane requested review from mattsse and klkvr February 25, 2025 12:38
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 A-sdk Related to reth's use as a library C-debt A clean up/refactor of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace DynEthApiBuilder with a trait
3 participants