-
Notifications
You must be signed in to change notification settings - Fork 464
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
Implement admin_peerEvents for rpc module #7999
base: master
Are you sure you want to change the base?
Implement admin_peerEvents for rpc module #7999
Conversation
Allow real time pub-sub through websocket on peer_added and peer_removed events.
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.
Potentially we also need msgsend
and msgrecv
subscriptions, but they will be a bit harder to add as I believe we don't expose events yet.
ResultWrapper<string> admin_subscribe(string subscriptionName, string? args = null); | ||
[JsonRpcMethod(Description = "Unsubscribes from a subscription.", IsImplemented = true, IsSharable = false, Availability = RpcEndpoint.All & ~RpcEndpoint.Http)] | ||
ResultWrapper<bool> admin_unsubscribe(string subscriptionId); |
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 think those methods need to be moved to AdminModule
as we need to guarantee that they are available only when admin module is enabled? Or we can try adding that logic explicitly for subscriptions in RpcModuleProvider
?
|
||
namespace Nethermind.JsonRpc.Modules.Subscribe | ||
{ | ||
public struct AdminSubscriptionType |
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.
public struct AdminSubscriptionType | |
public static class AdminSubscriptionType |
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.
We should change it on SubscriptionType
too.
@@ -28,7 +28,7 @@ private void OnEvicted(object? sender, TxEventArgs e) | |||
{ | |||
ScheduleAction(async () => | |||
{ | |||
using JsonRpcResult result = CreateSubscriptionMessage(e.Transaction.Hash); | |||
using JsonRpcResult result = CreateSubscriptionMessage(e.Transaction.Hash, "eth_subscription"); |
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.
move "eth_subscription" to some global constant
[JsonPropertyName("type")] | ||
public string Type { get; set; } | ||
|
||
[JsonPropertyName("peer")] | ||
public string PeerId { get; set; } | ||
|
||
[JsonPropertyName("local")] | ||
public string LocalAddr { get; set; } | ||
|
||
[JsonPropertyName("remote")] | ||
public string RemoteAddr { get; set; } | ||
|
||
[JsonPropertyName("error")] | ||
public string? Err { get; set; } |
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.
Imo name properties as they should be in response and you can drop JsonPropertyName
attributes. Casing will be handled by default.
|
||
} | ||
|
||
[SkipLocalsInit] |
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.
no locals - not needed.
@@ -39,7 +39,7 @@ private void OnBlockAddedToMain(object? sender, BlockReplacementEventArgs e) | |||
{ | |||
ScheduleAction(async () => | |||
{ | |||
using JsonRpcResult result = CreateSubscriptionMessage(new BlockForRpc(e.Block, _includeTransactions, _specProvider)); | |||
using JsonRpcResult result = CreateSubscriptionMessage(new BlockForRpc(e.Block, _includeTransactions, _specProvider), "eth_subscription"); |
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.
make eth_subscription
default value of that parameter, so we don't have to always pass it. As it will be dominant one.
PeerInfo peerInfo = new(peerEventArgs.Peer, false); | ||
ScheduleAction(async () => | ||
{ | ||
using JsonRpcResult result = CreateSubscriptionMessage(new PeerAddDropResponse(peerInfo, "Add", null), "admin_subscription"); |
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.
admin_subscription
to a const
These changes allow real time pub-sub through websocket on peer-related events (
peerEvents
), specifically,peerAdded
andpeerRemoved
events. Once a client subscribes forpeerEvents
, A JSON-RPC notification with event payload and subscription id is sent to this client for every event matching the subscription topic (peerAdded
orpeerRemoved
).Users subscribe to peerEvents by sending a request as such:
{"jsonrpc": "2.0", "id": 1, "method": "admin_subscribe", "params": ["peerEvents"]}
Resolves #7816
Changes
peerEvents
.IPeerPool.peerAdded
andIPeerPool.peerRemoved
events when requested.eth_subscribe
andadmin_subscribe
inJsonRpcSubscriptionResponse
class.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?