Send to channel route #1537
Merged
Send to channel route #1537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1537 +/- ##
==========================================
- Coverage 87.19% 87.17% -0.03%
==========================================
Files 139 139
Lines 10871 10882 +11
==========================================
+ Hits 9479 9486 +7
- Misses 1392 1396 +4
|
pm47
reviewed
Sep 29, 2020
Comment on lines
+445
to
+451
| case class PredefinedNodeRoute(nodes: Seq[PublicKey]) extends PredefinedRoute { | ||
| override def isEmpty = nodes.isEmpty | ||
| override def targetNodeId: PublicKey = nodes.last | ||
| } | ||
| case class PredefinedChannelRoute(targetNodeId: PublicKey, channels: Seq[ShortChannelId]) extends PredefinedRoute { | ||
| override def isEmpty = channels.isEmpty | ||
| } |
There was a problem hiding this comment.
The isEmpty method seems to have a different meaning between PredefinedNodeRoute and PredefinedChannelRoute. There could be a valid empty channel route which would be a direct payment.
There was a problem hiding this comment.
I think it works the way it is, a direct payment needs one channel to be specified.
Comment on lines
+247
to
+260
| val route = hops match { | ||
| case Left(shortChannelIds) => PredefinedChannelRoute(invoice.nodeId, shortChannelIds) | ||
| case Right(nodeIds) => PredefinedNodeRoute(nodeIds) | ||
| } |
There was a problem hiding this comment.
Here there is an inconsistency that is related to my other comment. Should we define the node route as nodeIds :+ invoice.nodeId?
There was a problem hiding this comment.
There was an inconsistency in that channel routes used the invoice's nodeId as targetNodeId, whereas in node routes we implied that the last nodeId would match the invoice's nodeId.
I added an explicit check in e802bc2 to reject node routes that are blatantly invalid.
While `sendtoroute` was letting you provide a custom route as a list of nodes, it made it difficult to have fine-grain control over the channels used by payments. The API now allows choosing the exact channels that will be used for the payment; in particular, this will be helpful when consolidating and rebalancing multiple channels to a given node. Fixes #1472
#1520 introduced a regression in the serialization of channel data, impacted the `channels` and `channel` APIs. We restrict the custom command response serializer to revert to the previous behavior.
t-bast
force-pushed
the
send-to-channel-route
branch
from
1250103
to
2c32723
Compare
Codecov Report
@@ Coverage Diff @@
## master #1537 +/- ##
==========================================
- Coverage 87.30% 87.11% -0.19%
==========================================
Files 139 139
Lines 10896 10915 +19
Branches 489 451 -38
==========================================
- Hits 9513 9509 -4
- Misses 1383 1406 +23
|
This reverts commit e802bc2.
pm47
approved these changes
Oct 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While
sendtoroutewas letting you provide a custom route as a list of nodes, it made it difficult to have fine-grain control over the channels used by payments.The API now allows choosing the exact channels that will be used for the payment; in particular, this will be helpful when consolidating and rebalancing multiple channels to a given node.
Fixes #1472
We also fix a serialization regression introduced in #1520