lightning icon indicating copy to clipboard operation
lightning copied to clipboard

Fix notification added versions

Open rustyrussell opened this issue 1 year ago • 8 comments

@ShahanaFarooqui points out we that the "added" documentation on the notification schemas is wrong. Fixing it required modifying files manually, however.

Includes a drive-by fix: we weren't jq-formatting the notification schemas, as noticed by the extra whitespace.

rustyrussell avatar Jun 28 '24 03:06 rustyrussell

Versions when these notifications were originally added:

connect: v0.6.3
channel_opened: v0.7.2.1:
channel_state_changed: v0.9.1
channel_open_failed: v0.9.3
block_added: v22.11
custommsg: v24.02

ShahanaFarooqui avatar Jun 28 '24 05:06 ShahanaFarooqui

@ErikDeSmedt After correctly wrapping the notifications response with doc: Wrapping notifications response with <notification-name> object commit, I am getting below 14 errors from rust compilation.

Could you please help in fixing them?

...
error[E0599]: no variant or associated item named `TORV3` found for enum `ConnectConnectAddressType` in the current scope
   --> cln-rpc/src/notifications.rs:109:40
    |
85  | pub enum ConnectConnectAddressType {
    | ---------------------------------- variant or associated item `TORV3` not found for this enum
...
109 |             ConnectConnectAddressType::TORV3 => "TORV3",
    |                                        ^^^^^ variant or associated item not found in `ConnectConnectAddressType`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `cln-rpc` (lib) due to 14 previous errors
make: *** [cln-rpc/Makefile:15: target/debug/examples/cln-plugin-startup] Error 101

ShahanaFarooqui avatar Jun 28 '24 05:06 ShahanaFarooqui

@ShahanaFarooqui

Can you give a bit more context on why you wrapped all notifications? It feels more complicated, requires rework in msggen and I fail to see any benefit.

In the cln-rpc crate I introduced the following semantics

  • Notification: To summarize the entire message send by cln
  • BlockAddedNotification: Which contains the details related to that event-type.

ErikDeSmedt avatar Jun 28 '24 13:06 ErikDeSmedt

Can you give a bit more context on why you wrapped all notifications? It feels more complicated, requires rework in msggen and I fail to see any benefit.

@ErikDeSmedt Hey, these notifications were always wrapped with their notification names. I did not change anything in the notifications, I just edited the documentation to keep it consistent with the real notification response.

ShahanaFarooqui avatar Jul 02 '24 22:07 ShahanaFarooqui

I'm not convinced that 02e4fa9c1f234d5e60596699c496ab6d25112acd solves a problem that needs fixing. I did not include the notification-name in the JSON-schema by design. I wanted to reduces clutter and simplify the implementation of language bindings.

All notifications have the following structure.

{ 
  <notification_name>: <notification_body>
}

It seems fair to only define the <notification_body> in the json-schema.

ErikDeSmedt avatar Jul 03 '24 11:07 ErikDeSmedt

  • This commit is not to fix any problem but just to correct the definition in the documentation. Without this commit, the documentation and notifications are inconsistent.

  • If wrappers are increasing the clutter then wrappers should be removed from the notifications code first. Why should we keep the notifications wrapped but not document it?

  • These wrappers were originally added to uniquely identify the notification. For example, if a plugin subscribes to all notifications ("*"), how would it differentiate between them without the name of the notification?

ShahanaFarooqui avatar Jul 03 '24 19:07 ShahanaFarooqui

In the JSON-RPC notification spec there is the method in the JSON-RPC envelope telling us what the payload notification type is. No need to wrap further. It gets lost in the dispatch code in pyln-client (where before the wildcard support you'd know from the subscription what types will end up in the handler). That however is an issue with the way the client code handles it, not with the payload of the notification. Please don't wrap and add redundancies.

cdecker avatar Jul 04 '24 08:07 cdecker

In the JSON-RPC notification spec there is the method in the JSON-RPC envelope telling us what the payload notification type is. No need to wrap further. It gets lost in the dispatch code in pyln-client (where before the wildcard support you'd know from the subscription what types will end up in the handler). That however is an issue with the way the client code handles it, not with the payload of the notification. Please don't wrap and add redundancies.

Yuck, didn't we go through and change them all?

rustyrussell avatar Jul 05 '24 09:07 rustyrussell

As suggested by @cdecker and @rustyrussell, removed the notification wrapper from doc schema. Whenever we will add notification validation in CLN, it will require unwrapping the notification first to avoid the validation failure.

ShahanaFarooqui avatar Jul 12 '24 19:07 ShahanaFarooqui