lndk-org / lndk

MIT License
76 stars 19 forks source link

Upgrade to LDK 0.0.123 #119

Closed jbesraa closed 2 weeks ago

jbesraa commented 1 month ago

should resolve #118

Among the changes from previous LDK version:

  1. No need to pass Secp256k1Error to SignError
  2. OnionMessenger takes extra argument which should implement NL referring to NodeIdLookUp trait. This required adding new struct LndkNodeIdLookUp to implement the above trait.
  3. path.introduction_node is replaced in favor of path.introduction_node()
  4. OfferBuilder signature change. OfferBuilder::new takes now pubkey instead of disc.
  5. Replace sign_closure with LndkSigner struct and implemented SignInvoiceRequestFn trait for it

Edit: sorry @orbitalturtle I just noticed you assigned yourself to #118!

orbitalturtle commented 1 month ago

Hey @jbesraa, thanks for picking this up! I'll give it a review later this week.

orbitalturtle commented 1 month ago

@jbesraa Oh and one other thing to note here... When you run make itest the integration tests will fail because the pinned branch we're using of our fork of ldk-sample is using a different version of LDK.

I just merged a PR that updates ldk-sample to v123, so if you can change Cargo.toml to point there, that should fix the compiling errors: https://github.com/lndk-org/ldk-sample/commits/main/

The forward_onion_message test with fail because OnionMessenger's send_onion_message function doesn't allow providing a path anymore. But that test is kind of irrelevant now anyway so you can just delete it

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 0.00%. Comparing base (80fcd0c) to head (aae7541). Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #119 +/- ## ====================================== Coverage 0.00% 0.00% ====================================== Files 1 1 Lines 96 96 ====================================== Misses 96 96 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jbesraa commented 3 weeks ago

@orbitalturtle This should be ready for a review

jbesraa commented 3 weeks ago

@jbesraa Oh and one other thing to note here... When you run make itest the integration tests will fail because the pinned branch we're using of our fork of ldk-sample is using a different version of LDK.

I just merged a PR that updates ldk-sample to v123, so if you can change Cargo.toml to point there, that should fix the compiling errors: https://github.com/lndk-org/ldk-sample/commits/main/

The forward_onion_message test with fail because OnionMessenger's send_onion_message function doesn't allow providing a path anymore. But that test is kind of irrelevant now anyway so you can just delete it

Did you mean wait_to_receive_onion_message test? cant find forward_onion_message

jbesraa commented 2 weeks ago

@orbitalturtle Thanks for the review. Addressed your comments + rebased.

jbesraa commented 2 weeks ago

@orbitalturtle Fixed up the requested changes. Changed the function name as well as the previous one didnt seem accurate.

diff --git a/src/lndk_offers.rs b/src/lndk_offers.rs
index b20c06a..7dada0f 100644
--- a/src/lndk_offers.rs
+++ b/src/lndk_offers.rs
@@ -123,7 +123,8 @@ impl OfferHandler {
             Destination::BlindedPath(ref path) => match path.introduction_node {
                 IntroductionNode::NodeId(pubkey) => connect_to_peer(client.clone(), pubkey).await?,
                 IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                    Self::get_chan_info(client.clone(), scid, direction).await?;
+                    let pubkey = get_node_id(client.clone(), scid, direction).await?;
+                    connect_to_peer(client.clone(), pubkey).await?
                 }
             },
         };
@@ -144,7 +145,7 @@ impl OfferHandler {
             let reply_path_intro_node_id = match reply_path.introduction_node {
                 IntroductionNode::NodeId(pubkey) => pubkey.to_string(),
                 IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                    Self::get_chan_info(client.clone(), scid, direction)
+                    get_node_id(client.clone(), scid, direction)
                         .await?
                         .to_string()
                 }
@@ -322,28 +323,6 @@ impl OfferHandler {
             .await
             .map_err(|_| OfferError::PaymentFailure)
     }
-
-    async fn get_chan_info(
-        client: Client,
-        scid: u64,
-        direction: Direction,
-    ) -> Result<PublicKey, OfferError> {
-        let get_info_request = ChanInfoRequest { chan_id: scid };
-        let channel_info = client
-            .lightning_read_only()
-            .get_chan_info(get_info_request)
-            .await
-            .map_err(OfferError::GetChannelInfo)?
-            .into_inner();
-        let pubkey = match direction {
-            Direction::NodeOne => channel_info.node1_pub,
-            Direction::NodeTwo => channel_info.node2_pub,
-        };
-        PublicKey::from_slice(pubkey.as_bytes()).map_err(|e| {
-            error!("Could not parse pubkey. {e}");
-            OfferError::IntroductionNodeNotFound
-        })
-    }
 }

 pub struct PayInvoiceParams {
@@ -591,22 +570,11 @@ impl InvoicePayer for Client {
         let introduction_node = match path.introduction_node {
             IntroductionNode::NodeId(pubkey) => pubkey,
             IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                let get_info_request = ChanInfoRequest { chan_id: scid };
-                let channel_info = self
-                    .clone()
-                    .lightning_read_only()
-                    .get_chan_info(get_info_request)
-                    .await?
-                    .into_inner();
-                let pubkey = match direction {
-                    Direction::NodeOne => channel_info.node1_pub,
-                    Direction::NodeTwo => channel_info.node2_pub,
-                };
-                match PublicKey::from_slice(pubkey.as_bytes()) {
+                match get_node_id(self.clone(), scid, direction).await {
                     Ok(pubkey) => pubkey,
                     Err(e) => {
-                        error!("Could not parse pubkey. {e}");
-                        return Err(Status::unknown("Could not parse pubkey"));
+                    let pubkey = get_node_id(client.clone(), scid, direction).await?;
diff --git a/src/lndk_offers.rs b/src/lndk_offers.rs
index b20c06a..7dada0f 100644
--- a/src/lndk_offers.rs
+++ b/src/lndk_offers.rs
@@ -123,7 +123,8 @@ impl OfferHandler {
             Destination::BlindedPath(ref path) => match path.introduction_node {
                 IntroductionNode::NodeId(pubkey) => connect_to_peer(client.clone(), pubkey).await?,
                 IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                    Self::get_chan_info(client.clone(), scid, direction).await?;
+                    let pubkey = get_node_id(client.clone(), scid, direction).await?;
+                    connect_to_peer(client.clone(), pubkey).await?
                 }
             },
         };
@@ -144,7 +145,7 @@ impl OfferHandler {
             let reply_path_intro_node_id = match reply_path.introduction_node {
                 IntroductionNode::NodeId(pubkey) => pubkey.to_string(),
                 IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                    Self::get_chan_info(client.clone(), scid, direction)
+                    get_node_id(client.clone(), scid, direction)
                         .await?
                         .to_string()
                 }
@@ -322,28 +323,6 @@ impl OfferHandler {
             .await
             .map_err(|_| OfferError::PaymentFailure)
     }
-
-    async fn get_chan_info(
-        client: Client,
-        scid: u64,
-        direction: Direction,
diff --git a/src/lndk_offers.rs b/src/lndk_offers.rs
index b20c06a..7dada0f 100644
--- a/src/lndk_offers.rs
+++ b/src/lndk_offers.rs
@@ -123,7 +123,8 @@ impl OfferHandler {
             Destination::BlindedPath(ref path) => match path.introduction_node {
                 IntroductionNode::NodeId(pubkey) => connect_to_peer(client.clone(), pubkey).await?,
                 IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                    Self::get_chan_info(client.clone(), scid, direction).await?;
+                    let pubkey = get_node_id(client.clone(), scid, direction).await?;
+                    connect_to_peer(client.clone(), pubkey).await?
                 }
             },
         };
@@ -144,7 +145,7 @@ impl OfferHandler {
             let reply_path_intro_node_id = match reply_path.introduction_node {
                 IntroductionNode::NodeId(pubkey) => pubkey.to_string(),
                 IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                    Self::get_chan_info(client.clone(), scid, direction)
+                    get_node_id(client.clone(), scid, direction)
                         .await?
                         .to_string()
                 }
@@ -322,28 +323,6 @@ impl OfferHandler {
             .await
             .map_err(|_| OfferError::PaymentFailure)
     }
-
-    async fn get_chan_info(
-        client: Client,
-        scid: u64,
-        direction: Direction,
-    ) -> Result<PublicKey, OfferError> {
-        let get_info_request = ChanInfoRequest { chan_id: scid };
-        let channel_info = client
-            .lightning_read_only()
-            .get_chan_info(get_info_request)
-            .await
-            .map_err(OfferError::GetChannelInfo)?
-            .into_inner();
-        let pubkey = match direction {
-            Direction::NodeOne => channel_info.node1_pub,
-            Direction::NodeTwo => channel_info.node2_pub,
-        };
-        PublicKey::from_slice(pubkey.as_bytes()).map_err(|e| {
-            error!("Could not parse pubkey. {e}");
-            OfferError::IntroductionNodeNotFound
-        })
-    }
 }

 pub struct PayInvoiceParams {
@@ -591,22 +570,11 @@ impl InvoicePayer for Client {
         let introduction_node = match path.introduction_node {
             IntroductionNode::NodeId(pubkey) => pubkey,
             IntroductionNode::DirectedShortChannelId(direction, scid) => {
-                let get_info_request = ChanInfoRequest { chan_id: scid };
-                let channel_info = self
-                    .clone()
-                    .lightning_read_only()
-                    .get_chan_info(get_info_request)
-                    .await?
-                    .into_inner();
-                let pubkey = match direction {
-                    Direction::NodeOne => channel_info.node1_pub,
-                    Direction::NodeTwo => channel_info.node2_pub,
-                };
-                match PublicKey::from_slice(pubkey.as_bytes()) {
+                match get_node_id(self.clone(), scid, direction).await {
                     Ok(pubkey) => pubkey,
                     Err(e) => {
-                        error!("Could not parse pubkey. {e}");
-                        return Err(Status::unknown("Could not parse pubkey"));
+                        error!("{e}");
+                        return Err(Status::unknown("Could not get node id."));
                     }
                 }
             }
@@ -678,6 +646,28 @@ impl InvoicePayer for Client {
     }
 }

+async fn get_node_id(
+    client: Client,
+    scid: u64,
+    direction: Direction,
+) -> Result<PublicKey, OfferError> {
+    let get_info_request = ChanInfoRequest { chan_id: scid };
+    let channel_info = client
+        .lightning_read_only()
+        .get_chan_info(get_info_request)
+        .await
+        .map_err(OfferError::GetChannelInfo)?
+        .into_inner();
+    let pubkey = match direction {
+        Direction::NodeOne => channel_info.node1_pub,
+        Direction::NodeTwo => channel_info.node2_pub,
+    };
+    PublicKey::from_slice(pubkey.as_bytes()).map_err(|e| {
+        error!("Could not parse pubkey. {e}");
+        OfferError::IntroductionNodeNotFound
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;