tests: basic cli/server integration tests #171
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. |
| pubkey: PublicKey, | ||
| secp_ctx: Secp256k1<secp256k1::All>, | ||
| signer: RefCell<&'a mut tonic_lnd::SignerClient>, |
| @@ -310,6 +316,64 @@ fn check_auth_metadata(metadata: &MetadataMap) -> Result<String, Status> { | |||
| 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?
| @@ -165,7 +171,7 @@ impl Offers for LNDKServer { | |||
| cert: self.lnd_cert.clone(), | |||
| macaroon, | |||
| }; | |||
| 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.
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: