Actually check, cleanup, notification schemas #8376

rustyrussell · 2025-06-26T03:58:40Z

We have schemas for (some) notifications, which GRPC seems to rely on, but they were completely untested. And thus, of course, wrong.

This does not work: the final change breaks GRPC! Help me @daywalker90 can you review final two commits especially?

Deprecate "null" for channel_state_changed scid.
Don't wrap plugin notifications in "payload", let them look like native.
Make custom plugin notifications follow the "wrap fields in a subobject of same name".
Add dev hook to dump hook & notification io into files in a given dir, and use it to post-process check that they match schemas.

daywalker90 · 2025-06-26T14:09:33Z

Regarding the compile error with ConnectAddressType: Does the rpc method "connect" also have address type "websocket"? Right now the schema does not. The problem here is that both the rpc method and notification have the same path "connect" and so msggen is searching in the same path for the fields. Either the schemas have to match for "connect.address.type" or i have to submit a patch for mssgen.

rustyrussell · 2025-06-27T04:36:57Z

Regarding the compile error with ConnectAddressType: Does the rpc method "connect" also have address type "websocket"? Right now the schema does not. The problem here is that both the rpc method and notification have the same path "connect" and so msggen is searching in the same path for the fields. Either the schemas have to match for "connect.address.type" or i have to submit a patch for mssgen.

Ah. We can't connect out to a web socket, so connect can't return that as type. Not sure what we want to do here; we could include it in connect for completeness, but it will never appear?

daywalker90 · 2025-06-27T05:31:27Z

Ah. We can't connect out to a web socket, so connect can't return that as type. Not sure what we want to do here; we could include it in connect for completeness, but it will never appear?

Either that or https://github.com/daywalker90/lightning/tree/msggen-connect-notification-override

rustyrussell · 2025-06-27T07:39:02Z

Here's the diff:

diff --git a/doc/schemas/notification/connect.json b/doc/schemas/notification/connect.json
index 8aaeb3eb8..1fbe9310f 100644
--- a/doc/schemas/notification/connect.json
+++ b/doc/schemas/notification/connect.json
@@ -53,7 +53,8 @@
               "ipv4",
               "ipv6",
               "torv2",
-              "torv3"
+              "torv3",
+              "websocket"
             ],
             "description": [
               "Type of connection (*torv2*/*torv3* only if **direction** is *out*)"

And here's the result with your commit applied:

rror[E0412]: cannot find type `ConnectAddressType` in this scope
   --> cln-rpc/src/notifications.rs:130:20
    |
130 |     pub item_type: ConnectAddressType,
    |                    ^^^^^^^^^^^^^^^^^^ not found in this scope
    |
help: consider importing this enum
    |
6   + use crate::model::responses::ConnectAddressType;
    |

error[E0412]: cannot find type `ConnectDirection` in this scope
   --> cln-rpc/src/notifications.rs:136:20
    |
47  | pub enum PeerConnectDirection {
    | ----------------------------- similarly named enum `PeerConnectDirection` defined here
...
136 |     pub direction: ConnectDirection,
    |                    ^^^^^^^^^^^^^^^^
    |
help: an enum with a similar name exists
    |
136 |     pub direction: PeerConnectDirection,
    |                    ~~~~~~~~~~~~~~~~~~~~
help: consider importing this enum
    |
6   + use crate::model::responses::ConnectDirection;
    |

For more information about this error, try `rustc --explain E0412`.
error: could not compile `cln-rpc` (lib) due to 2 previous errors
make: *** [plugins/Makefile:312: target/debug/clnrest] Error 101
make: *** Waiting for unfinished jobs....

daywalker90 · 2025-06-27T08:22:24Z

Did you run make gen?

Particularly important since we're going to update the format: this makes sure we don't break them! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

… in channel_state_changed notification We always prefer to omit fields rather than use 'null' (or unknown!). Note that before this, the schema was broken, so we have to put a special exemption in for that case. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Rather than forcing them to wrap their parameters in a "payload" sub-object, copy in params directly. We include the "origin" field one level up, if they care. The next patch restores compatibility for the one place we currently use them, which is the pay plugin. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…ame. All the core notifications changed over to wrapping the notification fields in an object with the name of the notification, but notifications from plugins were missed. Changelog-Added: Plugins: `channel_hint_update`, `pay_failure` and `pay_success` notifications now have objects of the same name containing the expected fields. Changelog-Deprecated: Plugins: `channel_hint_update`, `pay_failure` and `pay_success` notification fields outside the same-named object. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…ugins. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…ion schemas. Note that we need a workaround for deprecated APIs where "channel_state_changed" output "null" which violated the schema. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

…ications. Modern style for notifications is to put everything inside an object of same name as the method. For now this means duplication for backward compatibility. ChatGPT helped me do that. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Changelog-None

…t.json This is done by tests/test_connection.py::test_websocket: ``` { "jsonrpc": "2.0", "method": "connect", "params": { "connect": { "id": "031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f", "direction": "in", "address": { "type": "websocket", "subtype": "ipv4", "address": "127.0.0.1", "port": 59412 } } } } ``` Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

daywalker90 · 2025-06-26T14:44:29Z

plugins/src/lib.rs

+            serde_json::Value::Object(map) => map.clone(),
+            _ => return Err(anyhow::anyhow!("params must be a JSON object")),
+        };
+        params.insert(method.clone(), json!(v));


What is the intended change here? Right now it constructs a json dict with the old style and the new style like this:

{ "foo":"bar", "method":{"foo":"bar"}}

Is that intended? The thing is it only works for params that are dict's (serde_json::Value::Object doesn't mean json object in the sense that it is json but specifically a json dict)

daywalker90 · 2025-07-25T06:07:00Z

Omg today i learned why noone reacts to my GitHub Reviews: i never finalize them and keep them in pending...

rustyrussell added this to the v25.09 milestone Jun 26, 2025

madelinevibes added the Status::Assigned The issue has been given to a team member for resolution. label Jun 26, 2025

pytest: simple test for pay's custom notifications.

6c2591e

Particularly important since we're going to update the format: this makes sure we don't break them! Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

rustyrussell force-pushed the guilt/check-notification-json branch from 6603b1b to de61108 Compare July 25, 2025 01:57

ShahanaFarooqui linked an issue Jul 25, 2025 that may be closed by this pull request

Notifications and hooks schemas should be validated by test infrastructure #8368

Open

rustyrussell and others added 8 commits July 25, 2025 11:30

lightningd: add dev option to save hooks and notifications to/from pl…

601e2e7

…ugins. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

pyln-testing: check plugin notifications against any extant notificat…

2cb2b88

…ion schemas. Note that we need a workaround for deprecated APIs where "channel_state_changed" output "null" which violated the schema. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

msggen: add override for connect notification in cln-rpc aswell

e1709e6

Changelog-None

rustyrussell marked this pull request as ready for review July 25, 2025 02:01

rustyrussell requested a review from cdecker as a code owner July 25, 2025 02:01

rustyrussell force-pushed the guilt/check-notification-json branch from de61108 to bf111ad Compare July 25, 2025 02:01

daywalker90 reviewed Jul 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Actually check, cleanup, notification schemas #8376

Actually check, cleanup, notification schemas #8376

Uh oh!

rustyrussell commented Jun 26, 2025

Uh oh!

daywalker90 commented Jun 26, 2025 •

edited

Loading

Uh oh!

rustyrussell commented Jun 27, 2025

Uh oh!

daywalker90 commented Jun 27, 2025

Uh oh!

rustyrussell commented Jun 27, 2025 •

edited

Loading

Uh oh!

daywalker90 commented Jun 27, 2025

Uh oh!

daywalker90 Jun 26, 2025

Uh oh!

daywalker90 commented Jul 25, 2025

Uh oh!

Uh oh!

Actually check, cleanup, notification schemas #8376

Are you sure you want to change the base?

Actually check, cleanup, notification schemas #8376

Uh oh!

Conversation

rustyrussell commented Jun 26, 2025

Uh oh!

daywalker90 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustyrussell commented Jun 27, 2025

Uh oh!

daywalker90 commented Jun 27, 2025

Uh oh!

rustyrussell commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daywalker90 commented Jun 27, 2025

Uh oh!

daywalker90 Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

daywalker90 commented Jul 25, 2025

Uh oh!

Uh oh!

daywalker90 commented Jun 26, 2025 •

edited

Loading

rustyrussell commented Jun 27, 2025 •

edited

Loading