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

Lsps0 implementation #114

Merged
merged 9 commits into from
Aug 21, 2023
Merged

Lsps0 implementation #114

merged 9 commits into from
Aug 21, 2023

Conversation

JssDWt
Copy link
Collaborator

@JssDWt JssDWt commented Aug 11, 2023

Implement lsps0 server for CLN #104

  • Adds custom message receive support to cln_plugin
  • Adds custom message send support to cln client
  • Adds a server implementation for lsps0, much like grpc.NewServer. You create an lsps0.NewServer(). Then you register the required subservices, in this PR, it contains the ProtocolServer (see lsps0/protocol_server.go). The service has a service implementation and a service description. The service description maps the lsps0 method to the function being called (lsps0.listprotocols). That way, the services can be focused on handling requests and responses, and the lsps0 server wraps these in jsonrpc 2.0 messages over lightning custom messages.
  • Adds an integration test for requesting the supported versions over lightning messaging, through the lsps0 protocol.

Note: This PR points to the lsps2 branch. The idea is to create a feature branch with lsps2 changes.

Order of pull requests:

  1. (this PR) Lsps0 implementation #114
  2. lsps2: implement lsps2.get_versions #115
  3. Implement lsps2.get_info #116
  4. Implement lsps2.buy #120

@JssDWt JssDWt changed the base branch from master to lsps2 August 11, 2023 12:34
@JssDWt JssDWt marked this pull request as ready for review August 11, 2023 12:39
@JssDWt
Copy link
Collaborator Author

JssDWt commented Aug 11, 2023

For an example how to implement a service on top of this, see #115

@JssDWt
Copy link
Collaborator Author

JssDWt commented Aug 14, 2023

Note: The CLN plugin can no longer be dynamic, because we're setting a feature bit now.

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Looks Great!
The infrastructure and the layer separation looks promising to meet the evolving lsp spec requirements.

lsps0/server.go Outdated
// NOTE: The handler is being called synchonously. There's an option to
// do this in a goroutine instead. Also, there's the option to put the
// goroutine in the method desc for specific methods instead.
r, err := m.method.Handler(m.service.serviceImpl, context.TODO(), df)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it should be called in go routine otherwise the requests will block each other (in contrary to the grpc server behavior).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It now runs in a goroutine. On top of this, there's a mechanism to make sure there are not too many simultaneous requests.

s.RegisterService(
&ServiceDesc{
ServiceName: "lsps0",
HandlerType: (*ProtocolServer)(nil),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the HandlerType ? Where do we use it?

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 took it from the grpc service desc. It was used to check the runtime type of the handler passed via RegisterService, whether it implements the interface in the service description. I had removed that check, because I misunderstood it. Reintroduced the check now. Let me know if you think that's a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

I took it from the grpc service desc. It was used to check the runtime type of the handler passed via RegisterService, whether it implements the interface in the service description. I had removed that check, because I misunderstood it. Reintroduced the check now. Let me know if you think that's a good idea.

I see. As long as we have generic RegisterService underneath then I agree the check is a good idea.
I wonder if we can simplify and add the RegisterXXXServer methods (e.g RegisterProtocolServer) directly as a method of the Server struct and have that type safety check as part of the method prototype.
What do yo think?

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 wonder if we can simplify and add the RegisterXXXServer methods (e.g RegisterProtocolServer) directly as a method of the Server struct and have that type safety check as part of the method prototype.

I don't see a way to do that without creating a circular reference between the implementing services and the main server, do you have something in mind?

Copy link
Member

Choose a reason for hiding this comment

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

I see... I now understand that the interfaces are not declared in the base module (lsp0) but every addition introduces its own interface so indeed we can't do it the way I suggested.

@JssDWt
Copy link
Collaborator Author

JssDWt commented Aug 17, 2023

@JssDWt JssDWt requested a review from roeierez August 17, 2023 08:38
@JssDWt JssDWt mentioned this pull request Aug 18, 2023
@JssDWt JssDWt merged commit cc26cc8 into lsps2 Aug 21, 2023
35 checks passed
@JssDWt JssDWt mentioned this pull request Aug 21, 2023
@JssDWt JssDWt deleted the lsps0 branch August 21, 2023 08:03
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