-
Notifications
You must be signed in to change notification settings - Fork 267
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
Build Bolt12 invoices with provided intermediary nodes #2499
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind the use of pipeTo
, it's a bit ugly but it removes the need for boilerplate actor management.
My other comments from #2471 still apply though: it would be useful to refactor most of what is happening inside the handler to dedicated functions that can be called in tests.
Also, test coverage is really low, you must test the cases where one of the FinalizeRoute
fails, when one of them times out, when edge cases are hit, etc. Please spend more time on this and think about what makes sense to test.
I've put the blinded route creation and the aggregation of fees in separate functions. |
This commit contains no logical changes, it just moves some code to places where it makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic looks good and the test coverage is nice, I have made two follow-up commits in #2504
One of them contains a change to provide more flexibility in building the dummy hops (to make them credible) and the other just moves the code around.
eclair-core/src/main/scala/fr/acinq/eclair/router/RouteCalculation.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/payment/receive/MultiPartHandler.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/payment/receive/MultiPartHandler.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartHandlerSpec.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartHandlerSpec.scala
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2499 +/- ##
==========================================
+ Coverage 84.94% 84.96% +0.02%
==========================================
Files 199 200 +1
Lines 15802 15827 +25
Branches 668 693 +25
==========================================
+ Hits 13423 13448 +25
Misses 2379 2379
|
Allows creating arbitrary blinded routes for BOLT12 invoices.