Skip to content

Swap Vec<&RouteHop> parameters for slices #1728

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
merged 2 commits into from
Sep 19, 2022

Conversation

TheBlueMatt
Copy link
Collaborator

In c353c3e, new scorer-updating methods were added to the Router object, however they were passed as a Vec of references. We use the list-of-references pattern to make bindings simpler (by not requiring allocations per entry), however there's no reason prefer to passing a Vec over a slice, given the Vec doesn't hold ownership of the objects anyway.

We also add a second commit which replaces an Option<&u64> with an Option<u64>.

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2022

Codecov Report

Base: 90.73% // Head: 90.71% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (85eb1fd) compared to base (dc28f9b).
Patch coverage: 30.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1728      +/-   ##
==========================================
- Coverage   90.73%   90.71%   -0.02%     
==========================================
  Files          86       86              
  Lines       46675    46679       +4     
  Branches    46675    46679       +4     
==========================================
- Hits        42350    42345       -5     
- Misses       4325     4334       +9     
Impacted Files Coverage Δ
lightning-invoice/src/utils.rs 94.85% <0.00%> (ø)
lightning-invoice/src/payment.rs 89.85% <40.90%> (-0.27%) ⬇️
lightning/src/ln/monitor_tests.rs 99.44% <0.00%> (-0.12%) ⬇️
lightning/src/ln/functional_tests.rs 96.99% <0.00%> (-0.08%) ⬇️
lightning/src/util/events.rs 38.40% <0.00%> (+0.26%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jkczyz
jkczyz previously approved these changes Sep 19, 2022
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, mod above feedback.

In c353c3e, new scorer-updating
methods were added to the `Router` object, however they were
passed as a `Vec` of references. We use the list-of-references
pattern to make bindings simpler (by not requiring allocations per
entry), however there's no reason prefer to passing a `Vec` over
a slice, given the `Vec` doesn't hold ownership of the objects
anyway.
In c353c3e an accessor method was
added which returns an `Option<&u64>`. While this allows Rust to
return an 8-byte object, returning a reference to something
pointer-sized is a somewhat strange API.

Instead, we opt for a straight `Option<u64>`, which is sadly
somewhat larger on the stack, but is simpler and already supported
in the bindings generation.
@TheBlueMatt
Copy link
Collaborator Author

Dropped the excess ampersands and went ahead and squashed since its super trivial:

$ git diff-tree -U1 6d6b9cc4 85eb1fde
diff --git a/lightning-invoice/src/payment.rs b/lightning-invoice/src/payment.rs
index 5af6cb766..3b4fe7092 100644
--- a/lightning-invoice/src/payment.rs
+++ b/lightning-invoice/src/payment.rs
@@ -1847,3 +1847,3 @@ mod tests {
 		fn notify_payment_path_failed(&self, path: &[&RouteHop], short_channel_id: u64) {
-			self.scorer.lock().payment_path_failed(&path, short_channel_id);
+			self.scorer.lock().payment_path_failed(path, short_channel_id);
 		}
@@ -1851,3 +1851,3 @@ mod tests {
 		fn notify_payment_path_successful(&self, path: &[&RouteHop]) {
-			self.scorer.lock().payment_path_successful(&path);
+			self.scorer.lock().payment_path_successful(path);
 		}
@@ -1855,3 +1855,3 @@ mod tests {
 		fn notify_payment_probe_successful(&self, path: &[&RouteHop]) {
-			self.scorer.lock().probe_successful(&path);
+			self.scorer.lock().probe_successful(path);
 		}
@@ -1859,3 +1859,3 @@ mod tests {
 		fn notify_payment_probe_failed(&self, path: &[&RouteHop], short_channel_id: u64) {
-			self.scorer.lock().probe_failed(&path, short_channel_id);
+			self.scorer.lock().probe_failed(path, short_channel_id);
 		}
diff --git a/lightning-invoice/src/utils.rs b/lightning-invoice/src/utils.rs
index 8c96edbb0..5faecbfab 100644
--- a/lightning-invoice/src/utils.rs
+++ b/lightning-invoice/src/utils.rs
@@ -486,3 +486,3 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref> Router for DefaultR
 	fn notify_payment_path_failed(&self, path: &[&RouteHop], short_channel_id: u64) {
-		self.scorer.lock().payment_path_failed(&path, short_channel_id);
+		self.scorer.lock().payment_path_failed(path, short_channel_id);
 	}
@@ -490,3 +490,3 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref> Router for DefaultR
 	fn notify_payment_path_successful(&self, path: &[&RouteHop]) {
-		self.scorer.lock().payment_path_successful(&path);
+		self.scorer.lock().payment_path_successful(path);
 	}
@@ -494,3 +494,3 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref> Router for DefaultR
 	fn notify_payment_probe_successful(&self, path: &[&RouteHop]) {
-		self.scorer.lock().probe_successful(&path);
+		self.scorer.lock().probe_successful(path);
 	}
@@ -498,3 +498,3 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, S: Deref> Router for DefaultR
 	fn notify_payment_probe_failed(&self, path: &[&RouteHop], short_channel_id: u64) {
-		self.scorer.lock().probe_failed(&path, short_channel_id);
+		self.scorer.lock().probe_failed(path, short_channel_id);
 	}

@jkczyz jkczyz merged commit 18ac915 into lightningdevkit:main Sep 19, 2022
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.

4 participants