move static channelmanager functions into their own file by Evanfeenstra · Pull Request #2739 · lightningdevkit/rust-lightning

Evanfeenstra · 2023-11-16T23:30:43Z

new ln::onion_payment file with static functions from channelmanager.rs.
cleaned up test_peel_payment_onion and moved into ln::onion_payment.
made PendingHTLCInfo.payment_hash public (mistake from peel_payment_onion static fn in channelmanager #2700).
made UpdateAddHTLC.onion_routing_packet public, so that peel_payment_onion can be used externally.
some better comments on newly public fields.

jkczyz · 2023-11-17T01:38:19Z

I think channelmanager_utils is too broad. Let's use a more restrictive name lest it become another grab bag of utilities, even if only related to ChannelManager. Examples of other modules that we've refactored out are inbound_payment and outbound_payment. Maybe onion_payment in contrast to onion_message? Open to other ideas.

shaavan

Concept ACK

The refactoring looks fantastic on my initial review, and the additional comments are really helpful! I'll take another pass at the added code to see if there are any further improvements we can make to this pull request. Superb work!

shaavan · 2023-11-19T14:59:06Z

lightning/src/ln/channelmanager.rs

 		incoming_cltv_expiry: u32,
-		/// Optional shared secret for phantom node.
+		/// Shared secret derived using a phantom node secret key. If this field is Some, the
+		/// payment was sent to a phantom node (one hop behond the current node), but can be


small nit:

Suggested change

/// payment was sent to a phantom node (one hop behond the current node), but can be

/// payment was sent to a phantom node (one hop behind the current node), but can be

hehe thanks for catching that!

Evanfeenstra · 2023-11-20T18:48:27Z

I think channelmanager_utils is too broad. Let's use a more restrictive name lest it become another grab bag of utilities, even if only related to ChannelManager. Examples of other modules that we've refactored out are inbound_payment and outbound_payment. Maybe onion_payment in contrast to onion_message? Open to other ideas.

Good point, I renamed the module to onion_payment

shaavan

ACK mod nit.

This is a refactoring PR that moves static channelmanger functions to their own file.

I have used the following line command to notice the essential differences introduced by refactoring:

git diff HEAD^ --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra

Essential Changes in the PR:

Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
Modified the visibility of moved code to pub(super). Since these are utilities of channelManager and are not meant to be used in the rest of the codebase, this visibility is the most apt one for it.
Removed create_payment_onion function. Instead, use the create_payment_onion function defined in ln::onion_utils::create_payment_onion;

The refactoring looks clean and clear, and other than the small nits, this PR is good to be merged.

shaavan · 2023-11-23T13:40:43Z

lightning/src/ln/channelmanager.rs

@@ -118,14 +119,19 @@ pub enum PendingHTLCRouting {
 		short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
 	/// An HTLC paid to an invoice we generated.


Small nit:

In the following line, we are saying we don't know yet if the invoice was generated by us. So adding a "supposedly" will make the comments more coherent here.

Suggested change

/// An HTLC paid to an invoice we generated.

/// An HTLC paid to an invoice (supposedly) generated by us.

shaavan · 2023-11-23T13:41:36Z

lightning/src/ln/onion_payment.rs

+/// acceptance. If the payment is to be received, and the amount matches the expected amount for
+/// a given invoice, this indicates the [`msgs::UpdateAddHTLC`], once fully committed in the
+/// channel, will generate an [`Event::PaymentClaimable`].
+/// 


nit by linter:

Suggested change

///

///

shaavan · 2023-11-23T13:41:58Z

lightning/src/ln/onion_payment.rs

@@ -0,0 +1,499 @@
+//! Utilities for channelmanager.rs
+//! 


nit by linter:

Suggested change

//!

//!

codecov-commenter · 2023-11-25T21:47:07Z

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (70ea110) 88.55% compared to head (8c588cf) 88.51%.
Report is 29 commits behind head on main.

❗ Current head 8c588cf differs from pull request most recent head 9f4dcd0. Consider uploading reports for the commit 9f4dcd0 to get more accurate results

Files	Patch %	Lines
lightning/src/ln/onion_payment.rs	90.10%	29 Missing and 7 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2739      +/-   ##
==========================================
- Coverage   88.55%   88.51%   -0.04%     
==========================================
  Files         113      114       +1     
  Lines       89323    89305      -18     
  Branches    89323    89305      -18     
==========================================
- Hits        79097    79052      -45     
- Misses       7860     7882      +22     
- Partials     2366     2371       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Evanfeenstra · 2023-11-25T21:54:38Z

ok, rebased to main and incorporated the nits by @shaavan

shaavan

Code review ACK

valentinewallace

Fine to save these for follow-up if we want to get the initial code move in.

lightning/src/ln/onion_payment.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/onion_payment.rs

lightning/src/ln/channelmanager.rs

Evanfeenstra · 2023-11-28T20:07:16Z

Fine to save these for follow-up if we want to get the initial code move in.

Ok I went ahead and incorporated these nits. Thanks!

valentinewallace · 2023-11-28T20:54:47Z

Will wait til @TheBlueMatt ACK's the docs to merge.

valentinewallace · 2023-11-29T20:41:23Z

Going to land this since it's largely a code move and we can continue iterating on the docs if necessary in #2752.

Evanfeenstra force-pushed the channelmanager-utils branch from f96fa30 to 3df33e1 Compare November 16, 2023 23:37

shaavan reviewed Nov 19, 2023

View reviewed changes

Evanfeenstra force-pushed the channelmanager-utils branch from bfeded7 to 2a63f5f Compare November 20, 2023 18:47

shaavan reviewed Nov 23, 2023

View reviewed changes

Evanfeenstra force-pushed the channelmanager-utils branch from 2a63f5f to 8c588cf Compare November 25, 2023 21:46

shaavan approved these changes Nov 26, 2023

View reviewed changes

valentinewallace previously approved these changes Nov 27, 2023

View reviewed changes

Evanfeenstra dismissed valentinewallace’s stale review via 264f367 November 28, 2023 20:05

move static channelmanager functions into their own file

9f4dcd0

Evanfeenstra force-pushed the channelmanager-utils branch from 264f367 to 9f4dcd0 Compare November 28, 2023 20:06

valentinewallace approved these changes Nov 28, 2023

View reviewed changes

valentinewallace mentioned this pull request Nov 28, 2023

Large final onion payload fixes #2752

Merged

valentinewallace merged commit f07f4b9 into lightningdevkit:main Nov 29, 2023

	/// payment was sent to a phantom node (one hop behond the current node), but can be
	/// payment was sent to a phantom node (one hop behind the current node), but can be

	/// An HTLC paid to an invoice we generated.
	/// An HTLC paid to an invoice (supposedly) generated by us.

Comments

Conversation

Evanfeenstra commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz commented Nov 17, 2023

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan Nov 19, 2023

Choose a reason for hiding this comment

Uh oh!

Evanfeenstra Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

Evanfeenstra commented Nov 20, 2023

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

shaavan Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

shaavan Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Evanfeenstra commented Nov 25, 2023

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Evanfeenstra commented Nov 28, 2023

Uh oh!

valentinewallace commented Nov 28, 2023

Uh oh!

valentinewallace commented Nov 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Evanfeenstra commented Nov 16, 2023 •

edited

Loading

codecov-commenter commented Nov 25, 2023 •

edited

Loading