-
Notifications
You must be signed in to change notification settings - Fork 943
Actually check, cleanup, notification schemas #8376
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
base: master
Are you sure you want to change the base?
Actually check, cleanup, notification schemas #8376
Conversation
Particularly important since we're going to update the format: this makes sure we don't break them! Signed-off-by: Rusty Russell <[email protected]>
… 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 <[email protected]>
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 <[email protected]>
…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 <[email protected]>
…ugins. Signed-off-by: Rusty Russell <[email protected]>
…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 <[email protected]>
…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 <[email protected]>
…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 } } } } ``` Totally breaks all the GRPC code though. Signed-off-by: Rusty Russell <[email protected]>
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 |
Either that or https://github.com/daywalker90/lightning/tree/msggen-connect-notification-override |
Here's the diff:
And here's the result with your commit applied:
|
Did you run |
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?