schema: add notification schemas and generated bindings#8938
schema: add notification schemas and generated bindings#8938nepet wants to merge 4 commits intoElementsProject:masterfrom
Conversation
|
I'm somewhat of a notification schemer myself, so i compared this PR with mine and hopefully combined we got it correct (surely !?). I think we should not bump versions in Cargo.toml's in this PR since it will trigger an automatic release to crates.io and i'd like to get some other changes in first. Unless you like to be last or nobody minds multiple version bumps in a short time period. Also CI will complain (check the diff in Also i think you should add the notification subscriptions in |
|
Another thing i just tested to confirm: The documentation and the code of |
|
I also noticed that our (ignore the assert diff) |
|
Very nice changes, just some minor issues that @daywalker90 pointed out ^^ |
nepet
force-pushed
the
schema/add-notifications
branch
from
be286d6 to
0fc831d
Compare
|
@daywalker90 I hope I got all your findings fixed 😅! Thanks a lot for reviewing! |
nepet
force-pushed
the
schema/add-notifications
branch
from
0fc831d to
6dd88f8
Compare
|
|
nepet
force-pushed
the
schema/add-notifications
branch
6 times, most recently
from
38e8801 to
f81c863
Compare
madelinevibes
added
the
26.04 RC
daywalker90
force-pushed
the
schema/add-notifications
branch
from
f81c863 to
f06384c
Compare
Add schema definitions for the documented native notifications which were still only described in prose. This makes the notification payloads available in doc/schemas so msggen can consume them in later commits, while keeping this commit schema-only. The balance_snapshot schema follows the current implementation shape: the notification wrapper contains a single balance_snapshot object whose accounts field is an array of account snapshots. doc: add flat xpay notification schemas Add schema files for the documented xpay notifications using the temporary flat naming convention. These topics are plugin-emitted rather than native lightningd notifications, so keep them separate from msggen generation for now while still recording their subscriber-visible payload shape in doc/schemas.
nepet
force-pushed
the
schema/add-notifications
branch
from
8dfa430 to
fb6b86a
Compare
|
So do I get it correctly, that primitives for both modules, rpc and grpc, aren't generated or am I just blind ^^? In this case I'd also need to add |
Correct, and you already did :) |
nepet
force-pushed
the
schema/add-notifications
branch
from
fb6b86a to
4196075
Compare
|
Amended "0ddd055f08e7" to also include |
|
@daywalker90 I missed the |
Good catch! |
|
We have an actual regression in performance here.
|
Expand the native notification list consumed by msggen and regenerate the Rust and gRPC outputs for the documented notification schemas. This wires the new schema-backed native notifications through cln-rpc, cln-grpc, and the bundled msggen schema metadata. Keep the temporary xpay-prefixed plugin notifications out of generation for now. coin_movement.extra_tags remains a repeated string in the generated gRPC surface for now because msggen does not currently emit enums nested under repeated array items correctly. Changelog-Changed: cln-rpc and cln-grpc now expose notification bindings for balance_snapshot, coin_movement, deprecated_oneshot, disconnect, forward_event, invoice_creation, invoice_payment, log, onionmessage_forward_fail, openchannel_peer_sigs, plugin_started, plugin_stopped, sendpay_failure, sendpay_success, shutdown, and warning. msggen: generate xpay notification bindings Treat xpay as a built-in plugin for notification generation and include its documented notification schemas in the msggen output. This extends the generated cln-rpc and cln-grpc notification surfaces with the flat xpay schema files, while exposing the actual notification names `pay_part_start` and `pay_part_end` in generated APIs. Changelog-Changed: cln-rpc and cln-grpc now expose xpay notification bindings for `pay_part_start` and `pay_part_end`.
cln-grpc: clean up notification tests
Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
daywalker90
force-pushed
the
schema/add-notifications
branch
from
4196075 to
8e51c94
Compare
|
It was because we subscribed to the "shutdown" notification and didn't handle it properly which makes the plugin hang for 30 seconds for every test teardown. I removed the subscription to "shutdown" entirely, since i think it would just be for the cln-grpc plugin itself (where we don't need it if we don't do something special on shutdown) and not to be broadcasted via grpc. |
rustyrussell
left a comment
rustyrussell
left a comment
There was a problem hiding this comment.
There's a bit of back-and-forth which would be helped by combining some commits, and the commit messages are a bit messy from previous combinations, but the result is great.
6 participants
Important
26.04 FREEZE March 11th: Non-bugfix PRs not ready by this date will wait for 26.06.
RC1 is scheduled on March 23rd
The final release is scheduled for April 15th.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgradeAdd JSON schemas for 18 notification topics (native and xpay), generate Rust and gRPC bindings from them via msggen, and include round-trip deserialization tests for the most complex types.