move static channelmanager functions into their own file

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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

[Evanfeenstra]

• 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]

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]

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

[Evanfeenstra]

hehe thanks for catching that!

[Evanfeenstra]

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:

1. Updated comments for members of PendingHTLCRouting enum. The comments look clean and clear and present a better understanding of the member variables.
2. 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.
3. 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]

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]

nit by linter:

Suggested change

	///
	///

[shaavan]

nit by linter:

Suggested change

	//!
	//!

[codecov-commenter]

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]

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.

[Evanfeenstra]

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]

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

[valentinewallace]

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