Notifications over GRPC #7084
Merged
Notifications over GRPC #7084
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ErikDeSmedt
force-pushed
the
notifications-in-grpc
branch
4 times, most recently
from
72a2d5e
to
36d2c5d
Compare
cdecker
approved these changes
Feb 19, 2024
ErikDeSmedt
force-pushed
the
notifications-in-grpc
branch
2 times, most recently
from
9a25c87
to
d6e90cc
Compare
|
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. |
ErikDeSmedt
force-pushed
the
notifications-in-grpc
branch
from
d6e90cc
to
772f228
Compare
|
This still had a couple failing tests, but the logs have now expired. Do you want help getting this PR through CI @ErikDeSmedt? |
ErikDeSmedt
force-pushed
the
notifications-in-grpc
branch
8 times, most recently
from
09e4d38
to
2919e3c
Compare
|
@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
force-pushed
the
notifications-in-grpc
branch
from
2919e3c
to
e8df8d2
Compare
|
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
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
force-pushed
the
notifications-in-grpc
branch
from
e8df8d2
to
28a0186
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.proto-file that defines the grpc-interfaceI've updated the
grpc-pluginto ensure it can support notifications.