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

Notifications over GRPC #7084

Merged
merged 24 commits into from
May 16, 2024

Conversation

ErikDeSmedt
Copy link
Contributor

This PR implements notifications over the grpc-interface.

The first commit introduces the json-schema's that are used to serialize notifications.

I've introduced notifications to msggen. This PR also introduces code that allows msggen to generate

  • structs that can deserialize JSON-notifications
  • the .proto-file that defines the grpc-interface
  • methods to convert notifications from JSON to protobuf
  • a function grpc-server

I've updated the grpc-plugin to ensure it can support notifications.

@ErikDeSmedt ErikDeSmedt force-pushed the notifications-in-grpc branch 4 times, most recently from 72a2d5e to 36d2c5d Compare February 19, 2024 10:52
contrib/msggen/msggen/model.py Outdated Show resolved Hide resolved
@ErikDeSmedt ErikDeSmedt force-pushed the notifications-in-grpc branch 2 times, most recently from 9a25c87 to d6e90cc Compare February 21, 2024 10:03
@ErikDeSmedt ErikDeSmedt marked this pull request as ready for review February 21, 2024 10:13
@ErikDeSmedt
Copy link
Contributor Author

I've marked this PR as ready for review.

I am explicitly looking for review on the choose better when subfield is optional commit.

The proposed change also change generated code for schema's introduced before v24.05.
To my understanding this is safe for protobuf as the optional-keyword will not affect the wire-format.
For the cln_rpc-structs library users will have to update their code. However, the updated code will still work with older versions of Core Lightning.

@endothermicdev
Copy link
Collaborator

This still had a couple failing tests, but the logs have now expired. Do you want help getting this PR through CI @ErikDeSmedt?

@ErikDeSmedt ErikDeSmedt force-pushed the notifications-in-grpc branch 8 times, most recently from 09e4d38 to 2919e3c Compare May 14, 2024 17:36
@ErikDeSmedt
Copy link
Contributor Author

@endothermicdev : I've rebased it on top of master and the tests are mostly successful again. I assume the current failure is a flake.

Let me know if have any question or need help to merge this request

@cdecker
Copy link
Member

cdecker commented May 15, 2024

Rebased on top of #7274 for immediate inclusion.

I've added the schema for a couple of notifications.
Included a `notifications`-field in the `Service` and wrote the code
to parse them.

Add block-added to utils.py
No, this isn't my last resort.

The file to generate `gprc`-bindings got quite long.
I've put it in a separate folder and cut it into smaller pieces
In Core Lightning notifications are JSON-messages. This commit
introduces structs that can be used to parse the notification
messages.

Using `msggen` all required tructs are automatically generated
ErikDeSmedt and others added 19 commits May 16, 2024 10:57
In the next commit I'll change the behavior of `OptionalPatch`.
The changes require me to have access to the `parent` of a field.

Splitting it up in a separate commit makes it easier to review.
You can run `msggen` against this version and the previous `version`.

I've tested it. It returns exactly the same output.
**Problem Description**

In previous commits I introduced some new fields to `msggen`.
One example is `CustomMsgResponse` in `cln-grpc/src/notification.rs`.

```rust
pub struct CustomMsgResponse {
    #[serde(skip_serializing_if = "Option::is_none")]
    pub peer_id: Option<PublicKey>,
    #[serde(skip_serializing_if = "Option::is_none")]
    pub payload: Option<String>,
}
```

The `peer_id` and `payload` are required parameters.
However, the generated code is still marking them as `Optional`.

This is a choice made by `msggen`. It does this because `payload` and
`peer_id` are recently added fields. By marking the field as optional
the language bindings would also work when used on an older version of
Core-Lightning.

In this scenario. Marking them as optional is overkill.
The `CustomMsgStruct` and `payload` field are created in the same
version of CoreLightning.

This commit solves this behavior.
I'm working to expose a stream of notifications over grpc.
This requries me to define structs that can be used to request
a stream of notifications.

These structs are all empty.
Use the changes in the `cln-grpc`-crate to support
notifications.
The schema in the docs for the `ConnectNotification` was faulty.
I've already fix this in ElementsProject#7085

The new schema is

```
{
  "connect" :  {
    "address" : {
      "address" : "127.0.0.1",
      "port" : 38012,
      "type" : "ipv4"
  },
  "direction" : "in",
  "id" : "022d223620a359a47ff7f7ac447c85c46c923da53389221a0054c11c1e3ca31d59"
  }
}
```

The `address` in the `connect` field will be encoded in protobuf as
`ConnectAddress`. However, this collides with the `ConnectAddress` that
is defined in the `connect` rpc-method.

This commit
- Updates the schema in `doc/schemas/notification/connect.json`
- Changes `msggen` to include an override to `PeerConnect` for any
  notification typename that starts with `Connect`

Both have an `address` field which is a composite type. This results in
naming collisions

schema's: Updated schema for connect-notification

schema for connect notification + overrides

Override ConnectAddress to `PeerConnectAddress` in protobuf
for notifications
In rust enum are expected to be CamelCase.
However, we are generating enum's using CAPITAL_SNAKE_CASE.

This generates a warning and breaks CI.

See `cln_rpc::notifications::ConnectAddressType::LOCAL_SOCKET`
I forgot to add a `break` clause at the end of the test.

Without the break the test will keep waiting forever
on a `custommsg` that will never arrive
When running `make check-python` we'll expclicitly include
`pyln-grpc-proto` in the `PYTHONPATH`.

This ensures we always run the tests using the version installed in
`./contrib/pyln-grpc-proto`
@cdecker cdecker merged commit 934b85a into ElementsProject:master May 16, 2024
33 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants