-
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(net): extract NetworkHandle
methods for launching node to traits
#9966
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.
looks like a lot of the changes are unrelated and due to moved use statements, please try to avoid that in a pr that also moves stuff around because this makes it a lot harder to review
I'd like to break this down, can we start this process with a pr that introduces the new traits and perhaps (multiple) prs that move the types to network-types as prep?
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.
did anything change here?
all of this looks unrelated, hard to tell
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.
hm, sometimes when bases switches, changes on main look like new changes. I checked this out from emhane/network-types
originally.
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.
the module preludes I crossed, I cleaned up, is this a problem?
7552a4e
to
321032f
Compare
7552a4e
to
f2c08cb
Compare
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 would still prefer if we could do this incrementally, because it's a bit to keep an overview about why certain things are moved where.
I still think we can move the types that we need first and then introduce the traits
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.
hmm, this would be more appropriate in network-api I believe
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.
this is needed in PeersHandle
type, so needs to stay too. we should try and keep types in the designated types crate and traits in the designated api crate, just like it's cleanly done for reth-rpc-eth-api
and reth-rpc-eth-types
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.
these are all network api related types so I don't understand why we can't move this to the API crate, types is intended for standalone network related types, types we only used for interacting with network API functions don't fit here like command types
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.
we can re-export them in reth-network-api
, but it's much more intentional design if we keep types in types and api in api
I think that argument is contradictory, since the traits and cyclical deps are the reason why the types need to move |
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.
okay convinced but please keep these smaller moving forward
Ref #9872
BlockDownloaderProvider
in order to use generic network in node builder, instead of concrete typeNetworkHandle
FullNetwork
which unifies behaviour needed to launch nodePeersHandleProvider
to integrate generic network in testingreth-network
toreth-network-types
in order to extractNetworkHandle
methods into new traitsNote: In order to move
BlockDownloaderProvider
toreth-network-api
, theFetchClient
type would have to move toreth-network-types
, andreth_network_p2p::EthResponseValidator
andreth_network_p2p::HeadersRequest
would have to move out ofreth-network-p2p
. The latter, is to remove dep onreth-network-types
fromreth-network-p2p
, sinceFetchClient
depends onreth-network-p2p