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

Refactor/make peer info match geth #7750

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

obasekiosa
Copy link
Contributor

@obasekiosa obasekiosa commented Nov 12, 2024

Fixes #7808

Changes

  • Restructures the admin_peers response

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes
  • No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

@obasekiosa obasekiosa marked this pull request as ready for review November 26, 2024 14:30
@benaadams benaadams requested a review from Copilot March 29, 2025 17:11
@benaadams
Copy link
Member

Need rebase

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the admin_peers response to better match geth by restructuring the PeerInfo response and adding protocol-related details.

  • Introduces a new optional Enr property with JSON serialization handling.
  • Adds properties for Caps, Network (with a new NetworkInfo class), and Protocols in PeerInfo.
  • Makes minor adjustments in AdminRpcModule to update node info.

Reviewed Changes

Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/Nethermind/Nethermind.JsonRpc/Modules/Admin/PeerInfo.cs Refactored PeerInfo with additional protocol and network details and minor comment improvements
src/Nethermind/Nethermind.JsonRpc/Modules/Admin/AdminRpcModule.cs Removed a redundant blank line and added a comment about a repeated call in the constructor
Files not reviewed (2)
  • src/bench_precompiles: Language not supported
  • src/tests: Language not supported
Comments suppressed due to low confidence (1)

src/Nethermind/Nethermind.JsonRpc/Modules/Admin/PeerInfo.cs:69

  • Using the null-forgiving operator on peer.OutSession may cause a runtime exception if both InSession and OutSession are null. Consider explicitly handling the case where both sessions might be null.
LastSignal = (peer.InSession ?? peer.OutSession!).LastPingUtc.ToString(CultureInfo.InvariantCulture);

// ProtocolInfo [or Protocol] with sub-classes Eth, Snap etc...or just


// keep extra info not availibale in get?
Copy link
Preview

Copilot AI Mar 29, 2025

Choose a reason for hiding this comment

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

Typo found: 'availibale' should be corrected to 'available'.

Suggested change
// keep extra info not availibale in get?
// keep extra info not available in get?

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have admin_peers response structure match geth's response structure.
2 participants