-
Notifications
You must be signed in to change notification settings - Fork 100
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
RPC-JSON over WebSockets #847
Conversation
I think community will love two absolutely different websocket interfaces. |
This would make 3? Because |
What this feature offers, is MUST to have. My other reasoning is that with |
added uility log added applicationengine.log added connection check
@roman-khimov @superboyiii We would like to use this feature in |
{ | ||
internal class WebSocketClient : IDisposable, IEquatable<WebSocketClient> | ||
{ | ||
public WebSocket Socket { get; init; } |
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.
required and remove null checks?
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.
It is not null there. Hints the reason why no ?
and init
on property. We don't want null.
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.
It is not null there. Hints the reason why no
?
andinit
on property. We don't want null.
Yep, but it's used with ? inside the class
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.
Right and that reason, is for if client disconnect and the class becomes disposed
. But you should never pass in
a null
WebSocket
.
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 marked the project as nullable and I detected it, I was going to fix it, I think that new projects should be nullable
@superboyiii If this PR can you on your list ASAP for testing. I need this PR to update the UI for |
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.
Please fix @shargon
@@ -13,10 +13,9 @@ | |||
|
|||
namespace Neo.Plugins | |||
{ | |||
internal class WebSocketConnection<TClient> : IDictionary<Guid, TClient>, IDisposable | |||
where TClient : WebSocketClient, new() |
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.
Need to change this back. WebSocketClient
is a base
class. You can extend the class by inheriting it. For example if you want to add more properties/fields or methods to it. Like a tracking number or Client's login information.
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 program dynamically and make things reuse able.
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.
Need to change this back.
WebSocketClient
is abase
class. You can extend the class by inheriting it. For example if you want to add more properties/fields or methods to it. Like a tracking number or Client's login information.
We can extend WebSocketClient, we don't need more types of WebSocketClients, sometimes simpler is better
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.
Yes in theory. But this also limits the API or makes more work in the future. If the code is already there or built for reusability, than the code should stay.
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.
If the code is already there or built for reusability, than the code should stay.
I'm not agree, we don't need two WebSocketClients
This reverts commit 074649f.
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.
Please set the project as nullable, I tried to change it, but it was reverted one of my commits
{ | ||
internal class WebSocketClient : IDisposable, IEquatable<WebSocketClient> | ||
{ | ||
public WebSocket? Socket { get; init; } |
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 don't need this with ?, only is needed because you want a T type that we don't need
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 don't understand. Referring to T
type; we do need it! This a BASE
CLASS. NOT DOING SO WILL LIMIT AND FLAW EXPANDABLE OF AND POINT OF THIS LIBRARY. If we want to keep limiting the api
we can do that. Just look at RpcServer
is outdated and cannot be expanded or updated easily. Which is why no one touches it and in the state it's in now.
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.
remainder to check #859, remove websocket
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.
@shargon, @Jim8y, @vncoelho since the work on this PR is continuing, I'd like to pay your attention one more time to the problem mentioned by @roman-khimov in #847 (comment) and to the comment of @cschuchardt88 in #847 (comment).
The case is that NeoGo node already has WS server that handles all standard RPC methods exactly the same way as regular RPC server does (using exactly the same response structures and without handlers code duplication, see the comment below). Also, NeoGo WS server has subscriptions interface and notifications subsystem implemented for more than 3 years (check out the https://github.com/nspcc-dev/neo-go/blob/master/docs/notifications.md and code connected with https://github.com/nspcc-dev/neo-go/blob/d90169761595933febb8a25d1abde2e46154b96d/pkg/services/rpcsrv/server.go#L265). I know that there are community users of the existing NeoGo WS system (and, in particular, WS notification subsystem), and another WS interface implementation would be a problem for our users. We need a single unified and aligned websocket interface. What do you think?
2edeeaf
to
23d9a34
Compare
Repository moved to neo, please re-open there |
Description will be updated soon....
Some things to know
OnRequest
event to get custom paths/headers and query params)ws://127.0.0.1:10340/
ws://127.0.0.1:10340/?MyCustomParam=HelloWorld
ws://127.0.0.1:10340/MyCustomPath/FakePage.html?say=Hello%20Bob
Neo.Json
neo-core
fails)WebSocket
Events and their IDs
Request information
config.json
Developers API (all static)
Events
Methods
Properties
Creating a method in
Code
Video of Syncing (Blockchain)
Recording.2023-11-25.233845.mp4
Screenshots
Block Event (on commit)
Invoke Response (Echo Method)
Contract Response (on syncing/new block)
Transaction Response (on syncing/new block)
Block Response (on syncing/new block)
Change Log
WebSocketServer
closes #716