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(state_network): implement enr and node_id endpoints for state network #1143

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

morph-dev
Copy link
Collaborator

What was wrong?

State network doesn't have any endpoint implemented.

How was it fixed?

I added implementation for the enr and node_id related queries.

Next steps are:

  • Implement StateStorage (at least minimal support for now)
  • Implement the rest of the endpoints

To-Do

@@ -14,42 +14,42 @@ pub enum Discv5Endpoint {
/// State network JSON-RPC endpoints. Start with "portal_state" prefix
#[derive(Debug, PartialEq, Eq, Clone)]
pub enum StateEndpoint {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reordered these so they are grouped by similar logic: enr/node related, content related

Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

@morph-dev Any thoughts on adding some tests to ethportal-peertest? Seems like simply copying some history endpoints in basic.rs is a good place to start. There's certainly some opportunity for refactoring, since I don't believe ethportal-peertest was updated throughout the beacon network implementation. However, I don't feel too strongly about refactoring ethportal-peertest but these seem like endpoints that should be tested. I'm also happy if you want to do it in a following pr

Copy link
Member

@ogenev ogenev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ogenev
Copy link
Member

ogenev commented Feb 7, 2024

However, I don't feel too strongly about refactoring ethportal-peertest but these seem like endpoints that should be tested. I'm also happy if you want to do it in a following pr.

I think we definitely need to revisit and refactor ethportal-peertest to support multiple networks and add beacon and state
tests.

@morph-dev
Copy link
Collaborator Author

I added some tests to ethportal-peertest and did a bit of refactoring.

More refactoring would be needed for future tests. And probably with a different approach (because content key/value types are different).

Copy link
Collaborator

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for the improvements to peertest!

@morph-dev morph-dev merged commit c58b09e into ethereum:master Feb 7, 2024
8 checks passed
@morph-dev morph-dev deleted the enr_state_network branch February 7, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants