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

feat: NetworkPrimitives #12435

Merged
merged 1 commit into from
Nov 11, 2024
Merged

feat: NetworkPrimitives #12435

merged 1 commit into from
Nov 11, 2024

Conversation

klkvr
Copy link
Collaborator

@klkvr klkvr commented Nov 10, 2024

Introduces NetworkPrimitives trait defining primitive types for eth network messages. For now just adds generics to reth-eth-wire-types, introducing those reth-network would be a more massive change

Comment on lines +77 to +83
impl NetworkPrimitives for EthNetworkPrimitives {
type BlockHeader = reth_primitives::Header;
type BlockBody = reth_primitives::BlockBody;
type Block = reth_primitives::Block;
type BroadcastedTransaction = reth_primitives::TransactionSigned;
type PooledTransaction = reth_primitives::PooledTransactionsElement;
}
Copy link
Member

Choose a reason for hiding this comment

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

this is basically double up of NodePrimitives, why the redundancy?

Copy link
Collaborator Author

@klkvr klkvr Nov 11, 2024

Choose a reason for hiding this comment

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

I think we can always make NodePrimitives a supertrait if those actually end up same

the initial motivation was that transaction type is different so it would have to be a separate trait

Copy link
Collaborator

Choose a reason for hiding this comment

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

it is very similar and we could perhaps try to unify them but I feel like we should keep them separate for now because that makes it easier to transition the network component

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.

great!

@mattsse mattsse added this pull request to the merge queue Nov 11, 2024
@mattsse mattsse added the A-networking Related to networking in general label Nov 11, 2024
Merged via the queue into main with commit 365f6a1 Nov 11, 2024
40 checks passed
@mattsse mattsse deleted the klkvr/network-generics branch November 11, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants