Add method to count total fees in a Route #999 by galderz · Pull Request #1063 · lightningdevkit/rust-lightning

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

galderz · 2021-08-27T18:28:03Z

Part of #999. Add a member function for Route that returns the total amount of fees paid across all paths.

Further work will be carried out following @valentinewallace's work in #1053, so that the same can be calculated post payment. This implies taking into account which payments were really confirmed and discard what failed or was retried.

codecov · 2021-08-27T18:48:50Z

Codecov Report

Merging #1063 (03bb808) into main (4368b56) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1063 +/- ##
==========================================
- Coverage 90.82% 90.74% -0.08% 
==========================================
Files 65 65 Lines 32890 33948 +1058 ==========================================
+ Hits 29872 30806 +934 - Misses 3018 3142 +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`95.99% <100.00%> (+0.05%)`	⬆️
lightning/src/ln/msgs.rs	`85.70% <0.00%> (-3.41%)`	⬇️
lightning/src/ln/chan_utils.rs	`96.20% <0.00%> (-1.22%)`	⬇️
lightning/src/util/ser_macros.rs	`86.84% <0.00%> (-1.06%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.38% <0.00%> (-1.01%)`	⬇️
lightning/src/ln/channelmanager.rs	`85.00% <0.00%> (-0.85%)`	⬇️
lightning/src/chain/channelmonitor.rs	`91.26% <0.00%> (-0.71%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.86% <0.00%> (-0.51%)`	⬇️
lightning-invoice/src/lib.rs	`88.11% <0.00%> (-0.39%)`	⬇️
lightning/src/util/ser.rs	`91.92% <0.00%> (-0.28%)`	⬇️
... and 22 more

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 4368b56...03bb808. Read the comment docs.

…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz · 2021-09-21T05:46:50Z

Reworked unit tests and added an extra get_total_amount method.

valentinewallace

Just nits, LGTM

valentinewallace · 2021-09-21T17:13:29Z

+	/// This doesn't include any extra payment made to the recipient,
+	/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.


nit: lines should be 100chars each

valentinewallace · 2021-09-21T17:19:02Z

+ .flat_map(|path| path.split_last().unwrap().1)
+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}


nit: newline below

valentinewallace · 2021-09-21T17:19:13Z

+ .map(|hop| &hop.fee_msat)
+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.


nit: ticks around Route and above

valentinewallace · 2021-09-21T17:20:32Z

+ .sum();
+	}
+	/// Returns the total amount paid on this Route, excluding the fees.
+	pub fn get_total_amount(&self) -> u64 {


Idk if it's worth noting for these methods that they'll panic for 0-length routes?

valentinewallace · 2021-09-21T17:22:00Z

+ RouteHop {
+ pubkey: PublicKey::from_slice(&hex::decode("02eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619").unwrap()[..]).unwrap(),
+ channel_features: ChannelFeatures::empty(), node_features: NodeFeatures::empty(),
+ short_channel_id: 0, fee_msat: 100, cltv_expiry_delta: 0 // Test vectors are garbage and not generateble from a RouteHop, we fill in payloads manually


nit here and below: s/generateble/generatable (i think these comments can all be deleted though 😛)

TheBlueMatt · 2021-09-21T18:09:06Z

Gonna land this, will handle val's comments in a new PR.

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

TheBlueMatt reviewed Aug 27, 2021
View reviewed changes

Comment threadlightning/src/routing/router.rs Outdated
Comment threadlightning/src/routing/router.rs
Comment threadlightning/src/routing/router.rs

TheBlueMatt added this to the 0.0.101 milestone Sep 9, 2021

Add methods to count total fees and total amount in a Routelightning…
03bb808
…devkit#999 * Added `get_total_fees` method to route, to calculate all the fees paid accross each path. * Added `get_total_amount` method to route, to calculate the total of actual amounts paid in each path.

galderz force-pushed the t_total_fee_999 branch from 03aaa9d to 03bb808Compare September 21, 2021 05:46

TheBlueMatt approved these changes Sep 21, 2021
View reviewed changes

valentinewallace approved these changes Sep 21, 2021
View reviewed changes

TheBlueMatt merged commit 8f78bbd into lightningdevkit:mainSep 21, 2021

TheBlueMatt mentioned this pull request Sep 21, 2021
Fix a panic in Route's new fee-calculation methods and clean up #1088
Merged

		/// This doesn't include any extra payment made to the recipient,
		/// which can happen in excess of the amount passed to `get_route`'s `final_value_msat`.

Uh oh!

Conversation

galderz commented Aug 27, 2021

Uh oh!

codecovBot commented Aug 27, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

galderz commented Sep 21, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

galderz commented Aug 27, 2021

Uh oh!

codecovBot commented Aug 27, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

galderz commented Sep 21, 2021

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

galderz commented Aug 27, 2021

Uh oh!

codecovBot commented Aug 27, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecovBot commented Aug 27, 2021•
edited
Loading

codecovBot commented Aug 27, 2021•
edited
Loading

codecovBot commented Aug 27, 2021•
edited
Loading

codecovBot commented Aug 27, 2021•
edited
Loading