-
Notifications
You must be signed in to change notification settings - Fork 419
(Re)use functional_test_utils in lightning_liquidity #3927
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
(Re)use functional_test_utils in lightning_liquidity #3927
Conversation
👋 Thanks for assigning @tnull as a reviewer! |
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.
Thanks! Generally concept ACK.
As mentioned elsewhere I had remembered that there was a reason for not going this way originally, but given that I can't recall and it seems to work out, this shouldn't keep us from switching.
Arc<KeysManager>, | ||
use std::sync::Arc; | ||
|
||
pub(crate) type TestCM<'a, 'b> = channelmanager::ChannelManager< |
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.
nit: CM
is pretty ambiguous in the LDK space (is it ChannelManager
? ChainMonitor
? ChannelMonitor
?). Can we spell it out instead?
|
||
use bitcoin::Network; | ||
use common::get_lsps_message; |
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.
Seems these imports are now a bit chaotic, can you reestablish a consistent grouping?
use common::get_lsps_message; | ||
use lightning::chain::Filter; | ||
|
||
use lightning::chain::BestBlock; |
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.
Same here, please move lightning
imports down to the others (or the other way around if you prefer, but keep it consistent).
best_block: BestBlock::from_network(Network::Testnet), | ||
}; | ||
|
||
let service_lm = LiquidityManager::new( |
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.
Should we still keep the setup in a create_service_and_client_nodes
helper?
) -> (bitcoin::secp256k1::PublicKey, bitcoin::secp256k1::PublicKey, Node, Node, [u8; 32]) { | ||
pub(crate) fn setup_test_lsps2_nodes<'a, 'b, 'c>( | ||
nodes: Vec<Node<'a, 'b, 'c>>, | ||
) -> ( |
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.
Maybe would be a good time to create a struct
for the return value here?
@@ -107,24 +162,49 @@ fn create_jit_invoice( | |||
invoice_builder = invoice_builder.amount_milli_satoshis(amount_msat).basic_mpp(); | |||
} | |||
|
|||
invoice_builder |
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.
Not sure I understand why these test code changes are necessary?
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.
keys_manager from the node that's coming from functional_test_utils does not have a get_node_secret_key method
I could do some changes on functional_tests to introduce this but it was complex. I prefer to do the changes here in lightning_liquidity.
let me know what you think
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.
Hmm, yeah, indeed, that might be easier.
We should clean that code up a bit though:
diff --git a/lightning-liquidity/tests/lsps2_integration_tests.rs b/lightning-liquidity/tests/lsps2_integration_tests.rs
index 67d86beab..1cb40ba5b 100644
--- a/lightning-liquidity/tests/lsps2_integration_tests.rs
+++ b/lightning-liquidity/tests/lsps2_integration_tests.rs
@@ -94,7 +94,6 @@ fn create_jit_invoice(
let payment_hash = sha256::Hash::from_slice(&payment_hash.0).map_err(|e| {
log_error!(node.logger, "Invalid payment hash: {:?}", e);
- ()
})?;
let currency = Network::Bitcoin.into();
@@ -113,15 +112,11 @@ fn create_jit_invoice(
let raw_invoice = invoice_builder.build_raw().map_err(|e| {
log_error!(node.inner.logger, "Failed to build raw invoice: {:?}", e);
- ()
})?;
+ let sign_fn = node.inner.keys_manager.sign_invoice(&raw_invoice, lightning::sign::Recipient::Node);
raw_invoice
- .clone()
- .sign(|_| {
- node.inner.keys_manager.sign_invoice(&raw_invoice, lightning::sign::Recipient::Node)
- })
- .map_err(|_| ())
+ .sign(|_| sign_fn)
.and_then(|signed_raw| {
Bolt11Invoice::from_signed(signed_raw).map_err(|e| {
log_error!(node.inner.logger, "Failed to create invoice from signed raw: {:?}", e);
fn invoice_generation_flow() { | ||
let (service_node_id, client_node_id, service_node, client_node, promise_secret) = | ||
setup_test_lsps2("invoice_generation_flow"); | ||
fn full_lsps2_flow() { |
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.
Please make this test extension at the very least a separate commit. FWIW, it could even be a separate PR.
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
0f98b09
to
2d59afa
Compare
thank you for the review @tnull ! I just pushed a fixup commit with changes addressing your comments. let me know what you think, thanks!! |
2d59afa
to
b5b1d48
Compare
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.
Feel free to squash fixups!
let liquidity_manager = Arc::new(LiquidityManager::new( | ||
Arc::clone(&keys_manager), | ||
Arc::clone(&channel_manager), | ||
pub(crate) fn create_service_and_client_nodes_with_optional_payer_node<'a, 'b, 'c>( |
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.
Why is this called _with_optional_payer_node
if the method doesn't actually touch the payer node at all?
@@ -82,7 +86,7 @@ fn create_jit_invoice( | |||
let route_hint = RouteHint(vec![RouteHintHop { | |||
src_node_id: service_node_id, | |||
short_channel_id: intercept_scid, | |||
fees: RoutingFees { base_msat: 0, proportional_millionths: 0 }, | |||
fees: RoutingFees { base_msat: 1000, proportional_millionths: 0 }, |
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.
Why do we need make this behavioral change? How is this connected to switching test utils?
@@ -107,24 +162,49 @@ fn create_jit_invoice( | |||
invoice_builder = invoice_builder.amount_milli_satoshis(amount_msat).basic_mpp(); | |||
} | |||
|
|||
invoice_builder |
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.
Hmm, yeah, indeed, that might be easier.
We should clean that code up a bit though:
diff --git a/lightning-liquidity/tests/lsps2_integration_tests.rs b/lightning-liquidity/tests/lsps2_integration_tests.rs
index 67d86beab..1cb40ba5b 100644
--- a/lightning-liquidity/tests/lsps2_integration_tests.rs
+++ b/lightning-liquidity/tests/lsps2_integration_tests.rs
@@ -94,7 +94,6 @@ fn create_jit_invoice(
let payment_hash = sha256::Hash::from_slice(&payment_hash.0).map_err(|e| {
log_error!(node.logger, "Invalid payment hash: {:?}", e);
- ()
})?;
let currency = Network::Bitcoin.into();
@@ -113,15 +112,11 @@ fn create_jit_invoice(
let raw_invoice = invoice_builder.build_raw().map_err(|e| {
log_error!(node.inner.logger, "Failed to build raw invoice: {:?}", e);
- ()
})?;
+ let sign_fn = node.inner.keys_manager.sign_invoice(&raw_invoice, lightning::sign::Recipient::Node);
raw_invoice
- .clone()
- .sign(|_| {
- node.inner.keys_manager.sign_invoice(&raw_invoice, lightning::sign::Recipient::Node)
- })
- .map_err(|_| ())
+ .sign(|_| sign_fn)
.and_then(|signed_raw| {
Bolt11Invoice::from_signed(signed_raw).map_err(|e| {
log_error!(node.inner.logger, "Failed to create invoice from signed raw: {:?}", e);
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.
Just a single question about simplicity and the loss of context:
Apologies if this is a silly question, but I wonder if simplifying the test setup, as has been done here, might make onboarding a bit harder. Some developers (myself included) often learn the system by reading the tests and while the previous version was more verbose, it helped expose how and where LiquidityManager
is wired into an LDK
-based node. Do you think we lose some valuable coupling context with this new setup?
b5b1d48
to
377f30d
Compare
I learn the codebase by reading tests too. The point of this refactor is to enable real end-to-end LSPS tests. By reusing functional_test_utils we can now run that entire flow, which gives other devs a much clearer picture of how LSPS works in practice. Also, the LiquidityManager wiring is still visible IMO. Every test still calls |
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.
LGTM, mod some nits. Feel free to squash, and force-push the requested additional changes.
use lightning::ln::peer_handler::CustomMessageHandler; | ||
|
||
use crate::common::{create_service_and_client_nodes, LSPSNodes}; |
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.
nit: why add this additional common
entry if we have a group for it above already?
log_error!(node.logger, "Failed to build and sign invoice: {}", e); | ||
let raw_invoice = invoice_builder.build_raw().map_err(|e| { | ||
log_error!(node.inner.logger, "Failed to build raw invoice: {:?}", e); | ||
() |
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.
Can drop this ()
, see diff posted above.
|
||
raw_invoice.sign(|_| sign_fn).and_then(|signed_raw| { | ||
Bolt11Invoice::from_signed(signed_raw).map_err(|e| { | ||
log_error!(node.inner.logger, "Failed to create invoice from signed raw: {:?}", e); | ||
() |
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.
Same here, I think you should be able to drop the explicit ()
.
Definitely agree with the refactor. Moving between here and |
Instead of lightning_liquidity using its own utils and having to (re)write a ton of code for simple things like opening a channel and other basic node operations, reuse the code from lightning/functional_test_utils. This allows lightning_liquidity to extend its coverage and support real full end-to-end testing. Previously, this was only possible in ldk-node (which has one full end-to-end LSPS2 test), but that setup wasn't ideal. With this change, LSPS2 can now be tested directly in this repo as well.
377f30d
to
074b0a7
Compare
@tnull I forced push the latest requested changes |
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.
LGTM!
Instead of lightning_liquidity using its own utils and
having to (re)write a ton of code for simple things like
opening a channel and other basic node operations, reuse
the code from lightning/functional_test_utils. This allows
lightning_liquidity to extend its coverage and support
real full end-to-end testing.
As a first example of usage, the LSPS2 testinvoice_generation_flow that only tested invoice creation
now pays the invoice, intercepts the HTLC, creates the
channel, and forwards the HTLC.
Previously, this was only possible in ldk-node (which has
one full end-to-end LSPS2 test https://github.com/lightningdevkit/ldk-node/blob/main/tests/integration_tests_rust.rs#L1249), but that setup wasn't
ideal. With this change, LSPS2 can now be tested directly
in this repo, right here.
Motivation: This was originally a prefactor of #3551 which was a simple change by itself, but with the current lightning liquidity test capabilities, it was impossible to create a test that could check that the feature was implemented correctly.
I explored some options on how to test #3551, including rewriting and extending the lightning_liquidity/common/mod code to be able to do simple node operations, but came to the conclusion that long term the best option would be to reuse the code from functional_test_utils (ton of utils code to do node operations and also it's more maintained, so huge win for lightning_liquidity).
This PR does not include #3551, but will come in the future (with the (now possible) test) once this PR is merged.