Skip to content

cln-grpc Expose NotificationStream #8220

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nepet
Copy link
Collaborator

@nepet nepet commented Apr 8, 2025

This PR exposes the NotificationStream of the grpc server implementation. This is required for wrapping services to be able to pass down any notification stream.

@nepet nepet requested a review from cdecker as a code owner April 8, 2025 13:34
@nepet nepet marked this pull request as draft April 8, 2025 13:34
@nepet nepet force-pushed the cln-grpc-make-notificiation-stream-public branch from 8202b0b to 423df26 Compare April 8, 2025 13:35
@nepet nepet marked this pull request as ready for review April 8, 2025 13:35
nepet added 2 commits April 8, 2025 16:35
The `cfg-if` macro allows us to organise feature flag related
exports in a more concise way.

Signed-off-by: Peter Neuroth <[email protected]>
Services that want to wrap the server implementation will need
access to the `NotificationStream` struct in order to pass
the stream through.

Changelog-Added: `cln-grpc` Exposed NotificationStream in the server module

Signed-off-by: Peter Neuroth <[email protected]>
@nepet nepet force-pushed the cln-grpc-make-notificiation-stream-public branch from 423df26 to 3a35c2c Compare April 8, 2025 14:35
#[cfg(feature = "server")]
pub use crate::server::Server;
cfg_if::cfg_if! {
if #[cfg(feature = "server")] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry wat? Just make it available as a public re-export, independently of features. Having too many features around complicates the test matrix considerably. Or make it a client feature, negating a feature is super weird

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a missunderstanding. I am not negating some feature, cfg_if! is a macro that just cleans up three rows of #[cfg(feature = "server")] into one bracket.
https://docs.rs/cfg-if/latest/cfg_if/
But of course I can revert that if we don't like that.
Also, I did not see much advantage in exporting NotificationStream to clients as it implements the tokio_stream::Stream trait which makes it accessible further down the line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I see. Wouldn't this be equivalent to

#[cfg(feature="server")]
{
mod a;
mod b;
mod c;
}

Or does the lexical scoping result in a b and c not being accessible at the module then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this worked, but unfortunately the compiler does not accept it. Rust seems to force you into declaring the feature check for every line (in lib.rs).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trick to do this is to have a big submodule file, and you will have something like

#[cfg(feature="server")]
mod server;

where server.rs

mod a;
mod b;
mod c;

IIRC, this should avoid also collision and IMHO is an elegant solution

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