Improve tests with external Lightning implementations #653
Conversation
|
I've assigned @tnull as a reviewer! |
|
CI is failing due to the issue described in #654 |
| expect_event, generate_blocks_and_wait, premine_blocks, random_config, wait_for_tx, | ||
| }; | ||
|
|
||
| pub trait ExternalLightningNode { |
There was a problem hiding this comment.
Hmm, I'm not convinced DRYing up this code across implementations makes sense given their different feature sets. In particular, we'll want to add integration tests for splicing/BOLT12/etc going forward, i.e., features that LND doesn't implement. Wouldn't we then need to still workaround the ExternalLightningNode? At this point the trait would then just be an unnecessary complication?
There was a problem hiding this comment.
I understand your point about different feature sets, but I still think using a trait for ExternalLightningNode makes sense. Most of the test logic is the same across implementations; the only difference is in the API calls. The trait allows us to unify these common tests while letting each implementation handle its own API calls.
Regarding features like BOLT12, I agree LND doesn’t support them, but Core Lightning and Eclair do. The ExternalLightningNode doesn’t force all implementations to implement BOLT12 tests. Core Lightning can have the BOLT12 tests now, and when Eclair support is added, it can reuse the same tests by just implementing the ExternalLightningNode API methods.
In short, the ExternalLightningNode isn’t meant to enforce uniformity for all features. It’s meant to reduce code duplication for shared logic (like channel open, sending/receiving payments, channel close). For feature-specific tests, if multiple implementations support a feature (for example, BOLT12 in Core Lightning and Eclair), it makes sense to include it in the ExternalLightningNode so tests can be shared. If only one implementation supports a feature (for example, splicing only in Eclair), it should remain outside the ExternalLightningNode, as there’s no need for a shared test.
- Add `ExternalLightningNode ` trait to standardize methods for external Lightning nodes in tests.
- Create a generic test function that handles the test logic, requiring only an initialized external node.
- This simplifies adding tests for other implementations by just initializing the node and passing it to the test.
- Update CLN version for testing(new version v25.05).
- Remove unnecessary tokio dependency: `tokio = { version = "1.37", features = ["fs"] }`
Introduce `do_external_node_opens_channel_simple_transactions_with_ldk` to test external nodes initiating channel opening and performing simple transactions with LDK. Extend ExternalLightningNode trait with `open_channel` and `create_new_address` methods for better support in integration tests.
Introduce `do_bolt12_cycle_with_external_node` to test BOLT12 payments between LDK and external nodes, including offers without and with specified amounts, as well as receiving payments. Extend ExternalLightningNode trait with `generate_offer` and `pay_offer`.
moisesPompilio
force-pushed
the
improve-external-ln-integration-tests
branch
from
63a8b55 to
0291f2a
Compare
Introduces the
ExternalLightningNodetrait to standardize how tests interact with external Lightning nodes and adds a generic test function that handles common test logic, requiring only an initialized node.Adds new test targets: external nodes opening channels and performing simple transactions with LDK, and a bolt12 target to cover BOLT12 offer/request flows.
Minor cleanup: update CLN version and remove an unused dependency.
Nota: Tests with LND opening channels to LDK were skipped due to dust exposure HTLC errors. I think recent changes in the rust-lightning channel logic might have resolved this issue.