Unify Node API around Rust types

So far the interface sometimes took `String`s, `&str`s, or actual Rust
types. Moreover we sometimes took the arguments by reference and
sometimes not.

In order to make the interface more uniform, we here take a step towards
the Rust types and taking arguments by reference.

Note that some of the affected parts are due to change in the future,
e.g., once we transition to using upstream's `NetAddress` for peer
addresses.

Author: tnull

README.md

@@ -10,6 +10,7 @@ The primary abstraction of the library is the `Node`, which can be retrieved by
 ```rust
 use ldk_node::Builder;
 use ldk_node::lightning_invoice::Invoice;
+use ldk_node::bitcoin::secp256k1::PublicKey;
 use std::str::FromStr;
 
 fn main() {
@@ -23,13 +24,15 @@ fn main() {
 	let _funding_address = node.new_funding_address();
 
 	// .. fund address ..
-	
+
 	node.sync_wallets().unwrap();
 
-	node.connect_open_channel("NODE_ID@PEER_ADDR:PORT", 10000, false).unwrap();
+	let node_id = PublicKey::from_str("NODE_ID").unwrap();
+	let node_addr = "IP_ADDR:PORT".parse().unwrap();
+	node.connect_open_channel(&node_id, &node_addr, 10000, false).unwrap();
 
 	let invoice = Invoice::from_str("INVOICE_STR").unwrap();
-	node.send_payment(invoice).unwrap();
+	node.send_payment(&invoice).unwrap();
 
 	node.stop().unwrap();
 }

src/lib.rs

@@ -28,6 +28,7 @@
 //! ```no_run
 //! use ldk_node::Builder;
 //! use ldk_node::lightning_invoice::Invoice;
+//! use ldk_node::bitcoin::secp256k1::PublicKey;
 //! use std::str::FromStr;
 //!
 //! fn main() {
@@ -44,10 +45,12 @@
 //!
 //! 	node.sync_wallets().unwrap();
 //!
-//! 	node.connect_open_channel("NODE_ID@PEER_ADDR:PORT", 10000, false).unwrap();
+//! 	let node_id = PublicKey::from_str("NODE_ID").unwrap();
+//! 	let node_addr = "IP_ADDR:PORT".parse().unwrap();
+//! 	node.connect_open_channel(&node_id, &node_addr, 10000, false).unwrap();
 //!
 //! 	let invoice = Invoice::from_str("INVOICE_STR").unwrap();
-//! 	node.send_payment(invoice).unwrap();
+//! 	node.send_payment(&invoice).unwrap();
 //!
 //! 	node.stop().unwrap();
 //! }
@@ -60,8 +63,8 @@
 //! [`send_payment`]: Node::send_payment
 //!
 #![deny(missing_docs)]
-#![deny(broken_intra_doc_links)]
-#![deny(private_intra_doc_links)]
+#![deny(rustdoc::broken_intra_doc_links)]
+#![deny(rustdoc::private_intra_doc_links)]
 #![allow(bare_trait_objects)]
 #![allow(ellipsis_inclusive_range_patterns)]
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
@@ -133,7 +136,7 @@ use bitcoin::BlockHash;
 
 use rand::Rng;
 
-use std::convert::{TryFrom, TryInto};
+use std::convert::TryInto;
 use std::default::Default;
 use std::fs;
 use std::net::SocketAddr;
@@ -167,7 +170,7 @@ pub struct Config {
 	/// The used Bitcoin network.
 	pub network: bitcoin::Network,
 	/// The IP address and TCP port the node will listen on.
-	pub listening_address: Option<String>,
+	pub listening_address: Option<SocketAddr>,
 	/// The default CLTV expiry delta to be used for payments.
 	pub default_cltv_expiry_delta: u32,
 }
@@ -178,7 +181,7 @@ impl Default for Config {
 			storage_dir_path: "/tmp/ldk_node/".to_string(),
 			esplora_server_url: "http://localhost:3002".to_string(),
 			network: bitcoin::Network::Regtest,
-			listening_address: Some("0.0.0.0:9735".to_string()),
+			listening_address: Some("0.0.0.0:9735".parse().unwrap()),
 			default_cltv_expiry_delta: 144,
 		}
 	}
@@ -262,9 +265,8 @@ impl Builder {
 
 	/// Sets the IP address and TCP port on which [`Node`] will listen for incoming network connections.
 	///
-	/// Format: `ADDR:PORT`
 	/// Default: `0.0.0.0:9735`
-	pub fn set_listening_address(&mut self, listening_address: String) -> &mut Self {
+	pub fn set_listening_address(&mut self, listening_address: SocketAddr) -> &mut Self {
 		self.config.listening_address = Some(listening_address);
 		self
 	}
@@ -821,8 +823,8 @@ impl Node {
 		self.channel_manager.get_our_node_id()
 	}
 
-	/// Returns our own listening address and port.
-	pub fn listening_address(&self) -> Option<String> {
+	/// Returns our own listening address.
+	pub fn listening_address(&self) -> Option<SocketAddr> {
 		self.config.listening_address.clone()
 	}
 
@@ -847,7 +849,8 @@ impl Node {
 	///
 	/// Returns a temporary channel id
 	pub fn connect_open_channel(
-		&self, node_pubkey_and_address: &str, channel_amount_sats: u64, announce_channel: bool,
+		&self, pubkey: &PublicKey, address: &SocketAddr, channel_amount_sats: u64,
+		announce_channel: bool,
 	) -> Result<(), Error> {
 		let runtime_lock = self.running.read().unwrap();
 		if runtime_lock.is_none() {
@@ -862,7 +865,7 @@ impl Node {
 			return Err(Error::InsufficientFunds);
 		}
 
-		let peer_info = PeerInfo::try_from(node_pubkey_and_address.to_string())?;
+		let peer_info = PeerInfo { pubkey: pubkey.clone(), address: address.clone() };
 
 		let con_peer_pubkey = peer_info.pubkey.clone();
 		let con_peer_addr = peer_info.address.clone();
@@ -999,7 +1002,7 @@ impl Node {
 	}
 
 	/// Send a payement given an invoice.
-	pub fn send_payment(&self, invoice: Invoice) -> Result<PaymentHash, Error> {
+	pub fn send_payment(&self, invoice: &Invoice) -> Result<PaymentHash, Error> {
 		if self.running.read().unwrap().is_none() {
 			return Err(Error::NotRunning);
 		}
@@ -1055,7 +1058,7 @@ impl Node {
 	/// This can be used to pay a so-called "zero-amount" invoice, i.e., an invoice that leaves the
 	/// amount paid to be determined by the user.
 	pub fn send_payment_using_amount(
-		&self, invoice: Invoice, amount_msat: u64,
+		&self, invoice: &Invoice, amount_msat: u64,
 	) -> Result<PaymentHash, Error> {
 		if self.running.read().unwrap().is_none() {
 			return Err(Error::NotRunning);
@@ -1281,15 +1284,15 @@ async fn do_connect_peer(
 	pubkey: PublicKey, peer_addr: SocketAddr, peer_manager: Arc<PeerManager>,
 	logger: Arc<FilesystemLogger>,
 ) -> Result<(), Error> {
-	log_info!(logger, "connecting to peer: {}@{}", pubkey, peer_addr);
+	log_info!(logger, "Connecting to peer: {}@{}", pubkey, peer_addr);
 	match lightning_net_tokio::connect_outbound(Arc::clone(&peer_manager), pubkey, peer_addr).await
 	{
 		Some(connection_closed_future) => {
 			let mut connection_closed_future = Box::pin(connection_closed_future);
 			loop {
 				match futures::poll!(&mut connection_closed_future) {
 					std::task::Poll::Ready(_) => {
-						log_info!(logger, "peer connection closed: {}@{}", pubkey, peer_addr);
+						log_info!(logger, "Peer connection closed: {}@{}", pubkey, peer_addr);
 						return Err(Error::ConnectionFailed);
 					}
 					std::task::Poll::Pending => {}
@@ -1302,7 +1305,7 @@ async fn do_connect_peer(
 			}
 		}
 		None => {
-			log_error!(logger, "failed to connect to peer: {}@{}", pubkey, peer_addr);
+			log_error!(logger, "Failed to connect to peer: {}@{}", pubkey, peer_addr);
 			Err(Error::ConnectionFailed)
 		}
 	}

src/peer_store.rs

@@ -1,4 +1,3 @@
-use crate::hex_utils;
 use crate::io::{KVStore, PEER_INFO_PERSISTENCE_KEY, PEER_INFO_PERSISTENCE_NAMESPACE};
 use crate::logger::{log_error, Logger};
 use crate::Error;
@@ -8,8 +7,7 @@ use lightning::util::ser::{Readable, ReadableArgs, Writeable, Writer};
 use bitcoin::secp256k1::PublicKey;
 
 use std::collections::HashMap;
-use std::convert::TryFrom;
-use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, ToSocketAddrs};
+use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr};
 use std::ops::Deref;
 use std::sync::RwLock;
 
@@ -195,28 +193,10 @@ impl Writeable for PeerInfo {
 	}
 }
 
-impl TryFrom<String> for PeerInfo {
-	type Error = Error;
-
-	fn try_from(peer_pubkey_and_ip_addr: String) -> Result<Self, Self::Error> {
-		if let Some((pubkey_str, peer_str)) = peer_pubkey_and_ip_addr.split_once('@') {
-			if let Some(pubkey) = hex_utils::to_compressed_pubkey(pubkey_str) {
-				if let Some(peer_addr) =
-					peer_str.to_socket_addrs().ok().and_then(|mut r| r.next()).map(|pa| pa)
-				{
-					return Ok(PeerInfo { pubkey, address: peer_addr });
-				}
-			}
-		}
-		Err(Error::PeerInfoParseFailed)
-	}
-}
-
 #[cfg(test)]
 mod tests {
 	use super::*;
 	use crate::test::utils::{TestLogger, TestStore};
-	use proptest::prelude::*;
 	use std::str::FromStr;
 	use std::sync::Arc;
 
@@ -248,42 +228,4 @@ mod tests {
 		assert_eq!(deser_peer_store.get_peer(&pubkey), Some(expected_peer_info));
 		assert!(!store.get_and_clear_did_persist());
 	}
-
-	#[test]
-	fn peer_info_parsing() {
-		let valid_peer_info_str =
-			"0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993@127.0.0.1:9738"
-				.to_string();
-
-		let pubkey = PublicKey::from_str(
-			"0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993",
-		)
-		.unwrap();
-		let address: SocketAddr = "127.0.0.1:9738".parse().unwrap();
-		let expected_peer_info = PeerInfo { pubkey, address };
-
-		assert_eq!(Ok(expected_peer_info), PeerInfo::try_from(valid_peer_info_str));
-
-		let invalid_peer_info_str1 =
-			"02-76607124-ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993@127.0.0.1:9738"
-				.to_string();
-		assert_eq!(Err(Error::PeerInfoParseFailed), PeerInfo::try_from(invalid_peer_info_str1));
-
-		let invalid_peer_info_str2 =
-			"0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993@333.0.0.1:9738"
-				.to_string();
-		assert_eq!(Err(Error::PeerInfoParseFailed), PeerInfo::try_from(invalid_peer_info_str2));
-
-		let invalid_peer_info_str3 =
-			"0276607124ebe6a6c9338517b6f485825b27c2dcc0b9fc2aa6a4c0df91194e5993@127.0.0.19738"
-				.to_string();
-		assert_eq!(Err(Error::PeerInfoParseFailed), PeerInfo::try_from(invalid_peer_info_str3));
-	}
-
-	proptest! {
-		#[test]
-		fn peer_info_parsing_doesnt_crash(s in "\\PC*") {
-			let _ = PeerInfo::try_from(s.to_string());
-		}
-	}
 }

src/test/functional_tests.rs

@@ -33,8 +33,9 @@ fn channel_full_cycle() {
 	assert_eq!(node_b.on_chain_balance().unwrap().get_spendable(), 100000);
 
 	println!("\nA -- connect_open_channel -> B");
-	let node_b_addr = format!("{}@{}", node_b.node_id(), node_b.listening_address().unwrap());
-	node_a.connect_open_channel(&node_b_addr, 50000, true).unwrap();
+	node_a
+		.connect_open_channel(&node_b.node_id(), &node_b.listening_address().unwrap(), 50000, true)
+		.unwrap();
 
 	let funding_txo = loop {
 		let details = node_a.list_channels();
@@ -76,7 +77,7 @@ fn channel_full_cycle() {
 	let invoice = node_b.receive_payment(invoice_amount, &"asdf", 9217).unwrap();
 
 	println!("\nA send_payment");
-	let payment_hash = node_a.send_payment(invoice.clone()).unwrap();
+	let payment_hash = node_a.send_payment(&invoice).unwrap();
 
 	expect_event!(node_a, PaymentSuccessful);
 	expect_event!(node_b, PaymentReceived);
@@ -88,7 +89,7 @@ fn channel_full_cycle() {
 	assert_eq!(node_b.payment_info(&payment_hash).unwrap().amount_msat, Some(invoice_amount));
 
 	// Assert we fail duplicate outbound payments.
-	assert_eq!(Err(Error::NonUniquePaymentHash), node_a.send_payment(invoice));
+	assert_eq!(Err(Error::NonUniquePaymentHash), node_a.send_payment(&invoice));
 
 	// Test under-/overpayment
 	let invoice_amount = 1000000;
@@ -97,12 +98,12 @@ fn channel_full_cycle() {
 	let underpaid_amount = invoice_amount - 1;
 	assert_eq!(
 		Err(Error::InvalidAmount),
-		node_a.send_payment_using_amount(invoice, underpaid_amount)
+		node_a.send_payment_using_amount(&invoice, underpaid_amount)
 	);
 
 	let invoice = node_b.receive_payment(invoice_amount, &"asdf", 9217).unwrap();
 	let overpaid_amount = invoice_amount + 100;
-	let payment_hash = node_a.send_payment_using_amount(invoice, overpaid_amount).unwrap();
+	let payment_hash = node_a.send_payment_using_amount(&invoice, overpaid_amount).unwrap();
 	expect_event!(node_a, PaymentSuccessful);
 	let received_amount = match node_b.next_event() {
 		ref e @ Event::PaymentReceived { amount_msat, .. } => {
@@ -125,9 +126,9 @@ fn channel_full_cycle() {
 	// Test "zero-amount" invoice payment
 	let variable_amount_invoice = node_b.receive_variable_amount_payment(&"asdf", 9217).unwrap();
 	let determined_amount = 1234567;
-	assert_eq!(Err(Error::InvalidInvoice), node_a.send_payment(variable_amount_invoice.clone()));
+	assert_eq!(Err(Error::InvalidInvoice), node_a.send_payment(&variable_amount_invoice));
 	let payment_hash =
-		node_a.send_payment_using_amount(variable_amount_invoice, determined_amount).unwrap();
+		node_a.send_payment_using_amount(&variable_amount_invoice, determined_amount).unwrap();
 
 	expect_event!(node_a, PaymentSuccessful);
 	let received_amount = match node_b.next_event() {
@@ -195,10 +196,14 @@ fn channel_open_fails_when_funds_insufficient() {
 	assert_eq!(node_b.on_chain_balance().unwrap().get_spendable(), 100000);
 
 	println!("\nA -- connect_open_channel -> B");
-	let node_b_addr = format!("{}@{}", node_b.node_id(), node_b.listening_address().unwrap());
 	assert_eq!(
 		Err(Error::InsufficientFunds),
-		node_a.connect_open_channel(&node_b_addr, 120000, true)
+		node_a.connect_open_channel(
+			&node_b.node_id(),
+			&node_b.listening_address().unwrap(),
+			120000,
+			true
+		)
 	);
 }
 

src/test/utils.rs

@@ -291,8 +291,8 @@ pub fn random_config(esplora_url: &str) -> Config {
 
 	let rand_port = random_port();
 	println!("Setting random LDK listening port: {}", rand_port);
-	let listening_address = format!("127.0.0.1:{}", rand_port);
-	config.listening_address = Some(listening_address);
+	let listening_address_str = format!("127.0.0.1:{}", rand_port);
+	config.listening_address = Some(listening_address_str.parse().unwrap());
 
 	config
 }