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): add helper trait to node API to simplify type definition #10616

Merged
merged 28 commits into from
Oct 26, 2024

Conversation

emhane
Copy link
Member

@emhane emhane commented Aug 29, 2024

Ref #9555

Building unit tests for any type that inherits from FullNodeComponents is too costly. Replacing the trait bound N: FullNodeComponents with N: NodeCore in type definitions, enables testing without importing a massive framework of unused mock node components. This, without sacrificing DevX of aggregate generic N for a long list of generics.

  • Adds helper trait reth_node_api::NodeCore, with blanket implementation for all types that impl reth_node_api::FullNodeComponents
  • Replaces reth_optimism_rpc::OpEthApi<N: FullNodeComponents> with OpEthApi<N: NodeCore>

@emhane emhane added C-debt Refactor of code section that is hard to understand or maintain A-op-reth Related to Optimism and op-reth A-sdk Related to reth's use as a library labels Aug 29, 2024
@emhane emhane requested review from klkvr and shekhirin August 29, 2024 17:34
@emhane emhane requested a review from gakonst as a code owner September 4, 2024 14:34
@github-actions github-actions bot added S-stale This issue/PR is stale and will close with no further activity and removed S-stale This issue/PR is stale and will close with no further activity labels Sep 26, 2024
@emhane
Copy link
Member Author

emhane commented Oct 24, 2024

it's not possible to move the helper traits NodeCore and NodeTy into rpc crates reth-rpc-eth-api because it creates a cyclical dependency in blanket impl of helper traits for all types that impl reth-node-api traits @mattsse . which crate do you want it moved to?

@emhane
Copy link
Member Author

emhane commented Oct 25, 2024

it's not possible to move the helper traits NodeCore and NodeTy into rpc crates reth-rpc-eth-api because it creates a cyclical dependency in blanket impl of helper traits for all types that impl reth-node-api traits @mattsse . which crate do you want it moved to?

removed re-export from reth-rpc-eth-api and reth-rpc-api from reth-node-core and reth-node-api, in order to move RpcNodeCore trait into reth-rpc-eth-api @mattsse

not sure what's the root of the cargo hack check bug @Rjected

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.

lgtm!
nice work, combined with #12050 this will become a lot easier.

Comment on lines 5 to 6
/// Helper trait to relax trait bounds on [`FullNodeComponents`], when defining types.
pub trait RpcNodeCore: Clone {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this a lot as a non-restricted version of our components.

This is specifically tailored towards rpc, which is great because then we can extend this with additional things that we need for rpc, for example a trace executor type #10616

Comment on lines 5 to 6
/// Helper trait to relax trait bounds on [`FullNodeComponents`], when defining types.
pub trait RpcNodeCore: Clone {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this contains all components used by the rpc impl.
I think naming makes sense

@mattsse
Copy link
Collaborator

mattsse commented Oct 26, 2024

pub enum JwtError {
       | ----------------- doesn't satisfy `_: Into<Box<dyn Error + Send + Sync>>`

ah I think the alloy-rpc-types-engine dep in node-core just lacks the "std" feature, imo this is actually an alloy issue

@emhane emhane requested a review from rkrasiuk as a code owner October 26, 2024 09:55
@emhane emhane force-pushed the emhane/def-op-ethapi branch from 35c4130 to d550a27 Compare October 26, 2024 10:05
@emhane emhane enabled auto-merge October 26, 2024 10:40
@emhane emhane added this pull request to the merge queue Oct 26, 2024
Merged via the queue into main with commit 44e4c47 Oct 26, 2024
41 checks passed
@emhane emhane deleted the emhane/def-op-ethapi branch October 26, 2024 11:11
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-sdk Related to reth's use as a library C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants