Skip to content

Improve ChannelDetails readability significantly. #988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

After the merge of #984, Jeff pointed out that ChannelDetails has
become a bit of a "bag of variables", and that a few of the variable
names in #984 were more confusing than necessary in context.

This addresses several issues by:

  • Splitting counterparty parameters into a separate
    ChannelCounterpartyParameters struct,
  • using the name unspendable_punishment_reserve for both outbound
    and inbound channel reserves, differentiating them based on their
    position in the counterparty parameters struct or not,
  • Using the name force_close_spend_delay instead of
    spend_csv_on_our_commitment_funds to better communicate what
    is occurring.

@TheBlueMatt TheBlueMatt added this to the 0.0.99 milestone Jul 6, 2021
@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #988 (ceca6f4) into main (431f807) will decrease coverage by 0.01%.
The diff coverage is 90.24%.

❗ Current head ceca6f4 differs from pull request most recent head 2b08a47. Consider uploading reports for the commit 2b08a47 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #988      +/-   ##
==========================================
- Coverage   90.73%   90.72%   -0.02%     
==========================================
  Files          60       60              
  Lines       30702    30656      -46     
==========================================
- Hits        27857    27812      -45     
+ Misses       2845     2844       -1     
Impacted Files Coverage Δ
lightning-invoice/src/utils.rs 84.26% <0.00%> (ø)
lightning/src/ln/channelmanager.rs 84.72% <94.11%> (-0.02%) ⬇️
lightning/src/routing/router.rs 95.93% <95.45%> (-0.18%) ⬇️
lightning/src/ln/functional_tests.rs 97.30% <0.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 431f807...2b08a47. Read the comment docs.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this change!

After the merge of lightningdevkit#984, Jeff pointed out that `ChannelDetails` has
become a bit of a "bag of variables", and that a few of the variable
names in lightningdevkit#984 were more confusing than necessary in context.

This addresses several issues by:
 * Splitting counterparty parameters into a separate
   `ChannelCounterpartyParameters` struct,
 * using the name `unspendable_punishment_reserve` for both outbound
   and inbound channel reserves, differentiating them based on their
   position in the counterparty parameters struct or not,
 * Using the name `force_close_spend_delay` instead of
   `spend_csv_on_our_commitment_funds` to better communicate what
   is occurring.
@TheBlueMatt TheBlueMatt force-pushed the 2021-07-chan-details-usability branch from ceca6f4 to 2b08a47 Compare July 8, 2021 16:47
@TheBlueMatt
Copy link
Collaborator Author

Squashed with no changes, only changes since Jeff's ACK are below. Will merge after CI.

$ git diff-tree -U1 a44b282 2b08a47e
diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs
index f51c9ab7..baa32312 100644
--- a/fuzz/src/router.rs
+++ b/fuzz/src/router.rs
@@ -15,3 +15,3 @@ use lightning::chain;
 use lightning::chain::transaction::OutPoint;
-use lightning::ln::channelmanager::{ChannelDetails, ChannelCounterpartyParameters};
+use lightning::ln::channelmanager::{ChannelDetails, ChannelCounterparty};
 use lightning::ln::features::InitFeatures;
@@ -209,3 +209,3 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 								channel_id: [0; 32],
-								counterparty: ChannelCounterpartyParameters {
+								counterparty: ChannelCounterparty {
 									node_id: *rnid,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ceae6d10..e38b232e 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -607,3 +607,5 @@ const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_G
 #[derive(Clone, Debug, PartialEq)]
-pub struct ChannelCounterpartyParameters {
+pub struct ChannelCounterparty {
+	/// The node_id of our counterparty
+	pub node_id: PublicKey,
 	/// The Features the channel counterparty provided upon last connection.
@@ -612,4 +614,2 @@ pub struct ChannelCounterpartyParameters {
 	pub features: InitFeatures,
-	/// The node_id of our counterparty
-	pub node_id: PublicKey,
 	/// The value, in satoshis, that must always be held in the channel for our counterparty. This
@@ -636,3 +636,3 @@ pub struct ChannelDetails {
 	/// Parameters which apply to our counterparty. See individual fields for more information.
-	pub counterparty: ChannelCounterpartyParameters,
+	pub counterparty: ChannelCounterparty,
 	/// The Channel's funding transaction output, if we've negotiated the funding transaction with
@@ -1180,3 +1180,3 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					channel_id: (*channel_id).clone(),
-					counterparty: ChannelCounterpartyParameters {
+					counterparty: ChannelCounterparty {
 						node_id: channel.get_counterparty_node_id(),
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
index 0faee7be..6c14ebeb 100644
--- a/lightning/src/routing/router.rs
+++ b/lightning/src/routing/router.rs
@@ -1206,3 +1206,3 @@ mod tests {
 			channel_id: [0; 32],
-			counterparty: channelmanager::ChannelCounterpartyParameters {
+			counterparty: channelmanager::ChannelCounterparty {
 				features,
$

@TheBlueMatt TheBlueMatt merged commit 12253e5 into lightningdevkit:main Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants