Skip to content
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

Send to route #952

Merged
merged 32 commits into from May 20, 2019

Conversation

Projects
None yet
3 participants
@araspitzu
Copy link
Member

commented Apr 17, 2019

Implement sending a payment to a pre-computed route, the route input format is compatible with the output of the /findroute API. As the route input format is a simple list of nodeId(s) it is mandatory that all the channels involved in the pre-computed route are public, thus sending to a private channel is not supported in this API. The behavior is very similar to other payments API, in a fire-and-forget fashion the endpoint immediately returns a UUID of the payment attempt and more information can then be retrieved via the websocket or payments APIs, the payment will not be retried with different routes (maxAttempts=1).

araspitzu added some commits Apr 17, 2019

Use local random pamentHash for each test in paymentlifecyclespec, us…
…e testKeyManager in BaseRouteSpec fixtures
Merge branch 'fix_flaky_paymentlifecyclespec' into send_to_route
# Conflicts:
#	eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala

@araspitzu araspitzu force-pushed the send_to_route branch from aaa75b3 to be56cba Apr 23, 2019

araspitzu added some commits Apr 23, 2019

@araspitzu araspitzu marked this pull request as ready for review Apr 23, 2019

araspitzu added some commits Apr 23, 2019

Merge branch 'master' into send_to_route
# Conflicts:
#	eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala
#	eclair-core/src/main/scala/fr/acinq/eclair/api/FormParamExtractors.scala
#	eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala
#	eclair-core/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala

@araspitzu araspitzu requested review from sstone and pm47 and removed request for sstone Apr 23, 2019

araspitzu added some commits Apr 17, 2019

Use local random pamentHash for each test in paymentlifecyclespec, us…
…e testKeyManager in BaseRouteSpec fixtures, intercept the route request before the router.
Merge branch 'fix_flaky_paymentlifecyclespec' into send_to_route
# Conflicts:
#	eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala
Merge branch 'master' into send_to_route
# Conflicts:
#	eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala
@pm47
Copy link
Member

left a comment

Wouldn't that be possible to add a goingThroughNodes attributes to the existing SendPayment? Or have a Hint parameter that can be NoHint, GoesThrough(nodeId: PublicKey) or StaticRoute(nodeIds: Seq[PublicKey])

@@ -57,4 +57,10 @@ object FormParamExtractors {
Timeout(str.toInt.seconds)
}

implicit val pubkeyListUnmarshaller: Unmarshaller[String, List[PublicKey]] = Unmarshaller.strict { str =>
serialization.read[List[String]](str).map { el =>

This comment has been minimized.

Copy link
@pm47

pm47 May 13, 2019

Member

Does that parse URL-encoded json?

This comment has been minimized.

Copy link
@araspitzu

araspitzu May 13, 2019

Author Member

There is no URL-encoding here, this only parses the form data parameter (as json array of strings) into a list of public keys.

Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentInitiator.scala Outdated
Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentInitiator.scala Outdated
Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala Outdated
Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentLifecycle.scala Outdated
Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentInitiator.scala Outdated

araspitzu and others added some commits May 13, 2019

Update eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentInit…
…iator.scala

Co-Authored-By: Pierre-Marie Padiou <pm47@users.noreply.github.com>
Update eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentInit…
…iator.scala

Co-Authored-By: Pierre-Marie Padiou <pm47@users.noreply.github.com>

araspitzu added some commits May 13, 2019

@araspitzu

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Wouldn't that be possible to add a goingThroughNodes attributes to the existing SendPayment? Or have a Hint parameter that can be NoHint, GoesThrough(nodeId: PublicKey) or StaticRoute(nodeIds: Seq[PublicKey])

Yes that was an option but adding "hints" to SendPayment feels wrong because it would have a double meaning, on one hand it would refer to the BOLT11 hints found on the payment request, on the other we would store an entire static route in there. This would lead to some branching code where we take different action depending on the value of the hints, and i believe it's not the best approach in term of readability. The two use cases are captured by the duo SendPayment/SendPaymentToRoute.

@codecov-io

This comment has been minimized.

Copy link

commented May 13, 2019

Codecov Report

Merging #952 into master will increase coverage by 0.1%.
The diff coverage is 89.47%.

@@            Coverage Diff            @@
##           master     #952     +/-   ##
=========================================
+ Coverage   80.23%   80.33%   +0.1%     
=========================================
  Files          98       98             
  Lines        7527     7546     +19     
  Branches      297      291      -6     
=========================================
+ Hits         6039     6062     +23     
+ Misses       1488     1484      -4
Impacted Files Coverage Δ
...ala/fr/acinq/eclair/payment/PaymentInitiator.scala 100% <ø> (ø) ⬆️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 50.79% <0%> (-0.82%) ⬇️
...e/src/main/scala/fr/acinq/eclair/api/Service.scala 69.73% <100%> (-1.21%) ⬇️
...ala/fr/acinq/eclair/payment/PaymentLifecycle.scala 88.59% <100%> (+0.41%) ⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala 85.78% <100%> (+0.13%) ⬆️
...cala/fr/acinq/eclair/api/FormParamExtractors.scala 80% <83.33%> (+1.42%) ⬆️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 74.91% <0%> (-0.34%) ⬇️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 83.3% <0%> (ø) ⬆️
...nq/eclair/blockchain/electrum/ElectrumWallet.scala 79.58% <0%> (+0.78%) ⬆️
... and 3 more
@pm47
Copy link
Member

left a comment

Just minor things, otherwise LGTM

Show resolved Hide resolved eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala Outdated
sender.send(router, PeerRoutingMessage(null, remoteNodeId, buggy_ann_a))
sender.expectMsg(TransportHandler.ReadAck(buggy_ann_a))
sender.expectMsg(InvalidSignature(buggy_ann_a))
val buggy_ann_b = ann_b.copy(signature = ann_c.signature, timestamp = ann_b.timestamp + 1)

This comment has been minimized.

Copy link
@pm47

pm47 May 17, 2019

Member

Why did you need to change that?

This comment has been minimized.

Copy link
@araspitzu

araspitzu May 17, 2019

Author Member

Good catch that was some leftover from development, in 3aa79f7 i reverted the changes to RouterSpec and BaseRouterSpec (along with updating the copyright header).

araspitzu and others added some commits May 17, 2019

Update eclair-core/src/main/scala/fr/acinq/eclair/router/Router.scala
Co-Authored-By: Pierre-Marie Padiou <pm47@users.noreply.github.com>
@araspitzu

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

TODO: update doc

Merge branch 'master' into send_to_route
# Conflicts:
#	eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentLifecycle.scala
@pm47

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Just realized there is no API-level test, ideally it would test both input formats (json and comma-separated):

  • ["nodeid1", "nodeid2", ... ]
  • nodeid1,nodeid2
Merge branch 'master' into send_to_route
# Conflicts:
#	eclair-core/src/test/scala/fr/acinq/eclair/api/ApiServiceSpec.scala
@pm47

pm47 approved these changes May 20, 2019

@araspitzu araspitzu merged commit 101bcd7 into master May 20, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.