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

Implement registering and sending custom notifications to lightningd #6135

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

chrisguida
Copy link
Contributor

@chrisguida chrisguida commented Mar 30, 2023

The ability to send custom notifications from plugins is needed for a new rust plugin, smaug, which, once finished, will watch a descriptor wallet for updates and sends on-chain movements to the bookkeeper plugin using custom notifications.

This PR updates the cln-plugin create with a new send_custom_notification method on the Plugin object, which allows plugins to send custom notifications during normal operation. It also expands the getmanifest rpc response with a notifications array, which is needed in order for the plugin to register which notifications it wants to send with lightningd.

Sorry for all the formatting changes, my editor does this automatically on save and I'm unsure how to easily revert these, but I will remove if requested.

Edit: hmm, all my formatting changes are gone. No idea how that happened. Magic!

@vincenzopalazzo vincenzopalazzo added plugin rust Issue related to rust labels May 9, 2023
@rustyrussell rustyrussell added this to the v23.11 milestone Aug 21, 2023
@rustyrussell
Copy link
Contributor

Can we have a test? Would be nice to have a rust plugin in tests/plugins/ that we can use to test this?

@chrisguida
Copy link
Contributor Author

@rustyrussell hmm, I'm actually not seeing any examples of testers for the cln-plugin rust code. There are examples, but I'm not seeing a test module for cln-plugin.

@cdecker Is there a test module I'm missing? Do you need me to create one? Or do you guys think augmenting one or both example plugins would be sufficient?

@chrisguida
Copy link
Contributor Author

Ah yes, actually it does look like the examples are used in pytest, eg 2605e11

I'll get started on this!

@chrisguida
Copy link
Contributor Author

chrisguida commented Sep 15, 2023

@rustyrussell test added! Let me know what else you need on this :)

@chrisguida
Copy link
Contributor Author

@rustyrussell I'm noticing just now that I misread your comment asking for a new plugin. I instead implemented this as an additional feature in the example plugin. I can reimplement the test as its own plugin in tests/plugins if you think it's worth doing! Just let me know :)

Copy link
Collaborator

@nepet nepet left a comment

Choose a reason for hiding this comment

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

There is still a WIP commit in this PR, and a Changelog line would also be nice!

cln-rpc/Makefile Outdated Show resolved Hide resolved
plugins/src/lib.rs Outdated Show resolved Hide resolved
plugins/src/messages.rs Show resolved Hide resolved
… lightningd

This functionality already exists in the Python framework; this feature
enables it for Rust plugins as well.

Changelog-Added: cln-plugin: Implement send_custom_notification to allow sending custom notifications to other plugins.
Also fix Makefile for rust plugin examples

Also add in a missing assert in the test_plugin_start test
…example files themselves are changed

and to rebuild all the examples when the dependencies are changed (`CLN_RPC_SOURCES`).
@chrisguida
Copy link
Contributor Author

chrisguida commented Oct 27, 2023

Fixed / squashed / edited commit messages!

Let me know what else is missing :)

@chrisguida chrisguida requested a review from nepet October 27, 2023 19:06
@nepet nepet merged commit 4f667e8 into ElementsProject:master Oct 30, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin rust Issue related to rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants