-
Notifications
You must be signed in to change notification settings - Fork 9
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
implementing life cycle rpc method like getmanifest to be able to run the plugin #26
Conversation
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com> # Title: Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com> # Title: Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
c6b5c23
to
fff8a38
Compare
.toList(); | ||
response["subscriptions"] = plugin.subscriptions; | ||
response["hooks"] = plugin.hooks.toList(); | ||
response["notifications"] = []; |
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.
Why is this []
and not plugin.notifications
?
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.
Because the notification it is not a list of string but a list of object! So we need to find a way of how to do that!
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.
So like a list of notification objects, similar to JRPCLightning?
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.
the two concept are analogous, are implementing the same stuff
@@ -46,12 +49,12 @@ class Plugin implements CLNPlugin { | |||
required String def, | |||
required String description, | |||
required bool deprecated}) { | |||
options = Option( | |||
options.add(Option( |
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.
Here we are maintaining a List<Option>
so shouldn't we just add
the new option onto the list?
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.
the option are the value that the user can put with --some=hello
, so this is a list of option and not a single one!
We need to add inside the list the options, and I was unable to understand way you was doing options = Option
I assumed it was a mistake
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.
So this is correct?
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.
Why should be wrong? this code the code that I removed is also wrong! you are casting a option into a List
.toList(); | ||
response["subscriptions"] = plugin.subscriptions; | ||
response["hooks"] = plugin.hooks.toList(); | ||
response["notifications"] = []; |
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.
response["notifications"] = []; | |
response["notifications"] = []; |
We need to check what type of object it is required by cln here @swaptr
response["options"] = plugin.options.map((opt) => opt.toMap()).toList(); | ||
response["rpcmethods"] = plugin.rpcMethods.values | ||
.where((rpc) => rpc.name != "init" && rpc.name != "getmanifest") | ||
.map((rpc) => rpc.toMap()) | ||
.toList(); | ||
response["subscriptions"] = plugin.subscriptions; | ||
response["hooks"] = plugin.hooks.toList(); | ||
response["notifications"] = []; |
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 need to check this with attention with the core lightning docs, maybe we can write some units tests @swaptr
@@ -138,7 +144,7 @@ class Plugin implements CLNPlugin { | |||
var result = HashMap<String, Object>(); | |||
result["level"] = level; | |||
result["message"] = message; | |||
stdout.write(jsonDecode(Response(result: result).toJson())); | |||
stdout.write(Response(result: result).toJson()); |
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.
Sure that this works?
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.
The error on IDE disappears but when I try to log the messageSocket
in start() method, this is returned 2022-06-16T16:00:01.315Z UNUSUAL plugin-cln_plugin_example.exe: Killing plugin: Malformed JSON-RPC notification missing "method" or "params": {"jsonrpc":"2.0","result":{"level":"debug","message":"Instance of 'Request'"}}
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.
maybe you need to put the also the id inside the constructor?
Somethings like Response(id: 40, result: result).toJson()
.toList(); | ||
response["subscriptions"] = plugin.subscriptions; | ||
response["hooks"] = plugin.hooks.toList(); | ||
response["notifications"] = []; |
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.
Because the notification it is not a list of string but a list of object! So we need to find a way of how to do that!
21f683f
to
c5505d4
Compare
Co-authored-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
c5505d4
to
dc9922b
Compare
This should unlock you! this time for real!
Fixes #19