-
Notifications
You must be signed in to change notification settings - Fork 421
Add concrete NO_*
consts to avoid users having to specify types
#4100
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
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4100 +/- ##
==========================================
- Coverage 88.71% 88.69% -0.02%
==========================================
Files 176 176
Lines 132778 132859 +81
Branches 132778 132859 +81
==========================================
+ Hits 117790 117836 +46
- Misses 12304 12336 +32
- Partials 2684 2687 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. I still think a builder pattern would be the prettier solution here (if we could make it work), but given how few users will actually use the raw background processor going forward, spending much more time on optimizing the API (esp. after we already spent some time over at #3688) probably doesn't make sense for now.
Note that CI is failing, also
f676ce8
to
e76eb2e
Compare
$ git diff-tree -U1 9ee959c533 e76eb2ed5e
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index 1ce0f1dd12..b7e67f1667 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -3476,6 +3476,24 @@ mod tests {
#[test]
- fn test_no_moves() {
- // Make sure the NO_* constants can be moved so that they can be passed to BP methods.
- let no_liq = crate::NO_LIQUIDITY_MANAGER;
- let no_om = crate::NO_ONION_MESSENGER;
+ fn test_no_consts() {
+ // Compile-test the NO_* constants can be used.
+ let (_, nodes) = create_nodes(1, "test_no_consts");
+ let data_dir = nodes[0].kv_store.get_data_dir();
+ let persister = Arc::new(Persister::new(data_dir));
+ let bg_processor = BackgroundProcessor::start(
+ persister,
+ move |_: Event| Ok(()),
+ Arc::clone(&nodes[0].chain_monitor),
+ Arc::clone(&nodes[0].node),
+ crate::NO_ONION_MESSENGER,
+ nodes[0].no_gossip_sync(),
+ Arc::clone(&nodes[0].peer_manager),
+ crate::NO_LIQUIDITY_MANAGER,
+ Some(Arc::clone(&nodes[0].sweeper)),
+ Arc::clone(&nodes[0].logger),
+ Some(Arc::clone(&nodes[0].scorer)),
+ );
+
+ if !std::thread::panicking() {
+ bg_processor.stop().unwrap();
+ }
} |
e76eb2e
to
6761fe8
Compare
Oops, fixed the CI issues $ git diff-tree -U1 e76eb2ed5e eafb72d649
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index b7e67f1667..350908adb7 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -86,2 +86,4 @@ use std::time::Instant;
use alloc::boxed::Box;
+#[cfg(not(feature = "std"))]
+use alloc::sync::Arc;
@@ -3476,2 +3478,3 @@ mod tests {
#[test]
+ #[cfg(not(c_bindings))]
fn test_no_consts() { |
6761fe8
to
eafb72d
Compare
I believe there's still CI issues? |
eafb72d
to
cd7e347
Compare
Yep, stuff that wasn't being checked in a local $ git diff-tree -U1 eafb72d64 cd7e3479f
diff --git a/lightning-background-processor/src/lib.rs b/lightning-background-processor/src/lib.rs
index 350908adb7..dcf433605e 100644
--- a/lightning-background-processor/src/lib.rs
+++ b/lightning-background-processor/src/lib.rs
@@ -86,3 +86,3 @@ use std::time::Instant;
use alloc::boxed::Box;
-#[cfg(not(feature = "std"))]
+#[cfg(all(not(c_bindings), not(feature = "std")))]
use alloc::sync::Arc;
@@ -360,2 +360,13 @@ type DynMessageRouter = lightning::onion_message::messenger::DefaultMessageRoute
+#[cfg(all(not(c_bindings), not(taproot)))]
+type DynSignerProvider = dyn lightning::sign::SignerProvider<
+ EcdsaSigner = lightning::sign::InMemorySigner,
+>;
+
+#[cfg(all(not(c_bindings), taproot))]
+type DynSignerProvider = dyn lightning::sign::SignerProvider<
+ EcdsaSigner = lightning::sign::InMemorySigner,
+ TaprootSigner = lightning::sign::InMemorySigner,
+>;
+
#[cfg(not(c_bindings))]
@@ -366,3 +377,3 @@ type DynChannelManager = lightning::ln::channelmanager::ChannelManager<
&'static dyn lightning::sign::NodeSigner,
- &'static dyn lightning::sign::SignerProvider<EcdsaSigner = lightning::sign::InMemorySigner>,
+ &'static DynSignerProvider,
&'static dyn FeeEstimator, |
When building a background processor without an onion messenger or liquidity manager, users still need to pass `None` into the background processor initialization methods. Sadly, Rust will require a concrete type for the (unused) `Some` variant of the `Option` passed in, which requires specifying out a lot of type gook. Instead, provide some constants which users can pass in trivially. Fixes lightningdevkit#3612
cd7e347
to
0d59d73
Compare
When building a background processor without an onion messenger or liquidity manager, users still need to pass
None
into the background processor initialization methods. Sadly, Rust will require a concrete type for the (unused)Some
variant of theOption
passed in, which requires specifying out a lot of type gook.Instead, provide some constants which users can pass in trivially.
Fixes #3612