-
Notifications
You must be signed in to change notification settings - Fork 461
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
Add client id to admin_peers
RPC API
#7706
Conversation
Where else should we check? |
You use
|
Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com>
Nah lets leave P2P messages out of this, it is a different context. |
@@ -36,7 +38,8 @@ public PeerInfo(Peer peer, bool includeDetails) | |||
$"{nameof(PeerInfo)} cannot be created for a {nameof(Peer)} with an unknown {peer.Node}"); | |||
} | |||
|
|||
ClientId = peer.Node.ClientId; | |||
Name = peer.Node.ClientId; | |||
Id = CalculateClientId(peer.Node.Id); |
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.
Btw do we need to calculate it here, we have already peer.Node.IdHash
?
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 see, let me check again.
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 see that IdHash is calculated by PrefixedBytes
, this isn’t the clientId.
IdHash = Keccak.Compute(PublicKey.PrefixedBytes);
I think we should use new CalculateClientId
function, right?
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.
Not sure, it is your PR ;)
Best to check with Geth code.
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.
Yeah IdHash
should be fine
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.
Change to IdHash
@LukaszRozmej nethermind/src/Nethermind/Nethermind.JsonRpc/Modules/Admin/AdminRpcModule.cs Lines 49 to 61 in 458266e
|
Closing in favor of #7717 |
Add client id to
admin_peers
RPC APIRelated #7536
Changes
admin_peers
RPC APIclientId
->name
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Screenshots
Notes
Regarding updating the
clientId
parameter toname
, I believe that refers to the client name. However, I've only updated it in the admin_peers RPC API, keepingclientId
unchanged in other functions.