tests: basic cli/server integration tests#171
tests: basic cli/server integration tests#171orbitalturtle wants to merge 6 commits intolndk-org:masterfrom
Conversation
Initially a RefCell was added to the LndNodeSigner because the original version of tonic_lnd returns a mutable reference to the signer. However this reference doesn't need to be mutable: orbitalturtle/tonic_lnd#4 For this PR, this was needed so we could spin up the onion messenger and server in a separate thread in the integration tests. But it's a change that could help with future efficiency as well because the onion messenger can now be used in a multi-threaded way.
It's just a more suitable location.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #171 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 1 1
Lines 126 92 -34
======================================
+ Misses 126 92 -34 ☔ View full report in Codecov by Sentry. |
| pub(crate) struct LndNodeSigner { | ||
| pubkey: PublicKey, | ||
| secp_ctx: Secp256k1<secp256k1::All>, | ||
| signer: RefCell<&'a mut tonic_lnd::SignerClient>, |
kannapoix
left a comment
kannapoix
left a comment
There was a problem hiding this comment.
I'm still catching up architecture of the project, so my comments may wrong.
| Ok(macaroon) | ||
| } | ||
|
|
||
| pub async fn setup_server( |
There was a problem hiding this comment.
Naming it as setup_and_run or just run feels more concrete to me because this function actually runs the server, not just setup.
How about implementing this functions as a method of LNDKServer the same as the run method of LndkOnionMessenger to maintains consistency across projects.
| .tls_config(ServerTlsConfig::new().identity(identity)) | ||
| .expect("couldn't configure tls") | ||
| .add_service(OffersServer::new(server)) | ||
| .serve(addr); |
There was a problem hiding this comment.
Is it good to use serve instead of serve_with_shutdown, which the original code used?
| }; | ||
| let lnd_cfg = LndCfg::new(self.address.clone(), creds); | ||
| let lnd_cfg = LndCfg::new(self.lnd_address.clone(), creds); | ||
| let mut client = get_lnd_client(lnd_cfg) |
There was a problem hiding this comment.
How about pass this client to LNDKServer?
In each LNDKServer's handler methods such as pay_offer, get_invoice, we initialize lnd client but it seems redundant. It seems this is out of scope of this PR, but I would like to know if there are any reasons for this.
3 participants
This PR puts a basic test of the server in place, testing that the cli + server can properly make a payment.
To do so required a little refactoring: