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

Modularize or Simplify the eth2 crate #6452

Open
AgeManning opened this issue Oct 1, 2024 · 15 comments
Open

Modularize or Simplify the eth2 crate #6452

AgeManning opened this issue Oct 1, 2024 · 15 comments
Labels
code-quality good first issue Good for newcomers v6.1.0 New release c. Q1 2025

Comments

@AgeManning
Copy link
Member

Description

The common/eth2 crate appears to import the whole world. Other crates that depend on eth2 are compiling and building libp2p and a whole host of other things.

It would be great if we could significantly reduce the dependency chain here. Either by splitting up some of the structs or removing unnecessary imports.

@dapplion
Copy link
Collaborator

dapplion commented Oct 2, 2024

I remember asking myself this when doing some PoC a long time ago that depended on common/eth2

@dapplion
Copy link
Collaborator

dapplion commented Oct 2, 2024

The dependency on libp2p is only for these types

use lighthouse_network::{types::SyncState, PeerInfo};
use lighthouse_network::{ConnectionDirection, Enr, Multiaddr, PeerConnectionStatus};
use lighthouse_network::PeerId;

@hopinheimer
Copy link
Contributor

hi @AgeManning do you think I can swoop in on this?

@dapplion
Copy link
Collaborator

dapplion commented Oct 3, 2024

From an offline chat with @AgeManning we suggested to:

  • Return the serialized ENR as a String to avoid the dependency on discv5
  • Import PeerID from its own crate
  • Move the misc types SyncState, ConnectionDirection, etc to their own crate of only types and make it a common dependency of lighthouse_network and common/eth2

@SkandaBhat
Copy link

If nobody else is working on this, can I take a crack at this? @AgeManning @dapplion

@AgeManning
Copy link
Member Author

Hey @SkandaBhat - Sorry we have been away at a conference.

@hopinheimer are you still looking into this? Or should @SkandaBhat have a crack?

@hopinheimer
Copy link
Contributor

Hey @AgeManning sorry just reached back from the conference.

hi @SkandaBhat please feel free to pick this up I haven't started work on it.

@SkandaBhat
Copy link

SkandaBhat commented Nov 26, 2024

Sure thanks! @AgeManning please feel free to assign this to me! I am available this week and can create smaller PRs towards this issue.

@Gua00va
Copy link
Contributor

Gua00va commented Nov 27, 2024

From an offline chat with @AgeManning we suggested to:

  • Return the serialized ENR as a String to avoid the dependency on discv5
  • Import PeerID from its own crate
  • Move the misc types SyncState, ConnectionDirection, etc to their own crate of only types and make it a common dependency of lighthouse_network and common/eth2

@dapplion for the 3rd point, should a new crate be created or should I put it in an existing one?

@dapplion
Copy link
Collaborator

for the 3rd point, should a new crate be created or should I put it in an existing one?

You can add a new crate network_types

@dknopik
Copy link
Member

dknopik commented Dec 6, 2024

Hey @SkandaBhat and @Gua00va, thanks for picking up this issue!

We are currently modularizing some parts of the Lighthouse code base for use in Anchor, and as the eth2 crate is rather central we depend on it getting modularized soon. How is your progress? Let me know if I should take a look at some work in progress :)

@SkandaBhat
Copy link

Hey @dknopik I should have a draft by Tuesday next week, does that work?

@dknopik
Copy link
Member

dknopik commented Dec 6, 2024

@SkandaBhat Sounds great, thanks!

@dknopik
Copy link
Member

dknopik commented Dec 11, 2024

Hey @SkandaBhat, I already got started a bit today, see PR #6680. Feel free to improve on it if you have some ideas!

@realbigsean realbigsean added the v6.1.0 New release c. Q1 2025 label Dec 18, 2024
@SkandaBhat
Copy link

Unfortunately got held up with something else last week @dknopik, I'll check your PR today and see if I have anything I can improve!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality good first issue Good for newcomers v6.1.0 New release c. Q1 2025
Projects
None yet
Development

No branches or pull requests

8 participants
@realbigsean @AgeManning @SkandaBhat @dapplion @hopinheimer @Gua00va @dknopik and others