Addition of API calls checkinvoice and findroute by n1bor · Pull Request #475 · ACINQ/eclair

n1bor · 2018-03-05T20:31:14Z

checkinvoice parses an invoice/PaymentRequest and returns the JSON object. Usecase is that you need to be 100% sure that Eclair is paying the amount you expect! Easy to be factor of 1000 out if someone has a bug!

checkroute takes an invoice and checks if we have a route to pay at that moment. Is useful when you do not want to accept a request for payment you know you can not make.

Includes tests for checkroute.

n1bor · 2018-03-05T22:36:27Z

fixed bug in help docs and squashed commits.
And travis worked this time too!

dpad85 · 2018-03-06T13:42:12Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

We should reject the method if the paymentRequest argument is missing, with something like:

case _ => reject(UnknownParamsRejection(req.id, "[payment_request]"))

dpad85 · 2018-03-06T13:43:01Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

Same as checkinvoice, the method should be rejected if the paymentRequest argument is missing

dpad85 · 2018-03-06T13:45:09Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

An amount is not required for a payment request (donation cases) and is not needed to find a route

Arh - was hoping check route also checked the route had enough funds. Guess it does not.
Fixed

dpad85 · 2018-03-06T13:54:05Z

Great stuff, thanks for the PR!

I noticed that the failures field in the PaymentFailed is not propertly serialized to JSON and, when a route can not be found, we get an empty failure:

{
  "paymentHash": "ff75b8778563b85d525569a8259862ee9bc50ea7b5d781015dd957c17eaca496",
  "failures": [
    {
      "t": {}
    }
  ]
}

This can be fixed with a ThrowableSerializer, something like:

class ThrowableSerializer extends CustomSerializer[Throwable](format => ({ null }, {
  case t: Throwable if t.getMessage != null => JString(t.getMessage)
  case t: Throwable => JString(t.getClass.getSimpleName)
}))

which then gives

...
"failures": [
  {
    "t": "route not found"
  }

Could you add it to the JsonSerializers file (and to the formats field in Service?)

pm47

I understand the use case for checkinvoice, although I'm not sure just printing out the payment request will help, because there are usually a lot of zeroes. FWIW, the human-readable part of the payment request was designed to do just that, even if it is arguably difficult to read.

I think having a checkroute (or maybe findroute) method is nice, but you don't need to go through PaymentLifecycle (which role is to manage payment failures), instead just query the router directly, like allnodes/allchannels/allupdates do: send a RouteRequest and you'll get a RouteResponse.

edit: I also think this findroute method should take either a nodeid or a payment request as input.

pm47 · 2018-03-06T14:31:36Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

nitpick: remove unnecessary braces around completeRpc

Arh - using the router is much simpler! Updated.
Use-case for both these is another app using the API - in checkinvoice case is to check amount is what we are expecting - before paying with send. So zeros fine.

dpad85 · 2018-03-10T19:56:37Z

Do you mind if I push 1 or 2 commits on your branch to fix some indentation issues? I also would like to add a serializer for the object returned by the findroute call, so that it returns something like:

{
  "hops": "node1 -> node2 -> node3"
  "channels": "shortid1 -> shortid2"
}

n1bor · 2018-03-12T18:57:55Z

@dpad85 - sure change however you want. Just want to be able to identify if there is a route, and get from the invoice the amount.
Think the more detail in the API the better. As then future proof for other use-cases.

n1bor · 2018-03-23T09:05:05Z

rebased.. any chance of getting this merged as have some other api changes to come.

…ll. Updated after review comments.

pm47

I left a few nitpick comments, please use proper identation and formatting (Ctrl+Shift+L on IntelliJ)

Also, please merge against master as there has been conflicting changes.

Thanks!

pm47 · 2018-03-23T15:30:17Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

+                      case "checkinvoice" => req.params match {
+                        case JString(paymentRequest) :: Nil  => Try(PaymentRequest.read(paymentRequest)) match {
+                          case Success(pr) => {
+                            completeRpc(req.id,pr)


superfluous { } around completeRpc(req.id,pr)

pm47 · 2018-03-23T15:31:34Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

+                        case JString(nodeId) :: Nil if nodeId.length()==66 =>
+                          Try(PublicKey(nodeId)) match {
+                            case Success(pk) =>  completeRpcFuture(req.id, (router ? RouteRequest(appKit.nodeParams.nodeId, pk)  ).mapTo[RouteResponse]  )
+                            case (Failure(_)) => reject(RpcValidationRejection(req.id, s"invalid nodeId hash: '$nodeId'"))


Remove () around Failure(_)

pm47 · 2018-03-23T15:33:17Z

eclair-core/src/main/scala/fr/acinq/eclair/api/Service.scala

+                          }
+
+                        case JString(paymentRequest) :: Nil  => Try(PaymentRequest.read(paymentRequest)) match {
+                          case pr@Success(PaymentRequest(_,_,_,_,_,_)) =>


use case Success(pr: PaymentRequest) => instead

dpad85 · 2018-03-23T19:54:46Z

@pm47 can you update your review with the latest commits?

pm47

API doc needs to be updated in README.md

n1bor · 2018-03-23T21:59:37Z

Updated README - sorry about commit message - thought I changed that before I pushed??

n1bor · 2018-03-23T22:36:56Z

annoying timeouts... can we add akka.test.timefactor=10 or something? As expecting to use travis as a performance tester is not a good idea?

pm47 · 2018-03-24T12:56:40Z

@n1bor flaky tests were actually due to a regression fixed in 64c15b4. Believe it or not sometimes tests work as expected! 😁

n1bor force-pushed the api-updates branch from dd9e5a3 to eadd608 Compare March 5, 2018 22:15

dpad85 reviewed Mar 6, 2018

View reviewed changes

pm47 requested changes Mar 6, 2018

View reviewed changes

n1bor force-pushed the api-updates branch from eadd608 to 536865a Compare March 6, 2018 23:15

add api call to check invoice/paymentRequest and return details in json

18eb051

n1bor force-pushed the api-updates branch from 536865a to 35834f7 Compare March 23, 2018 09:04

Adding in API call to allow user to check route before making send ca…

b537a32

…ll. Updated after review comments.

n1bor force-pushed the api-updates branch from 35834f7 to b537a32 Compare March 23, 2018 11:35

pm47 requested changes Mar 23, 2018

View reviewed changes

Fixed some indentation issues and nitpicks

f290085

pm47 changed the title ~~Addition of API calls checkinvoice and checkroute~~ Addition of API calls checkinvoice and findroute Mar 23, 2018

Added a serializer for route response

274a453

dpad85 force-pushed the api-updates branch from 9ef8e1d to 274a453 Compare March 23, 2018 19:44

Merge branch 'master' into api-updates

74cab1a

pm47 requested changes Mar 23, 2018

View reviewed changes

pm47 previously approved these changes Mar 23, 2018

View reviewed changes

Update README to include new checkinvoice and findroute API calls

c94bcec

n1bor dismissed pm47’s stale review via c94bcec March 23, 2018 22:01

n1bor force-pushed the api-updates branch from 59132c1 to c94bcec Compare March 23, 2018 22:01

pm47 approved these changes Mar 24, 2018

View reviewed changes

pm47 merged commit 1a8cb2a into ACINQ:master Mar 24, 2018

Conversation

n1bor commented Mar 5, 2018

Uh oh!

n1bor commented Mar 5, 2018

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!

Choose a reason for hiding this comment

Uh oh!

dpad85 commented Mar 6, 2018

Uh oh!

pm47 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dpad85 commented Mar 10, 2018

Uh oh!

n1bor commented Mar 12, 2018

Uh oh!

n1bor commented Mar 23, 2018

Uh oh!

pm47 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!

dpad85 commented Mar 23, 2018

Uh oh!

pm47 left a comment

Choose a reason for hiding this comment

Uh oh!

n1bor commented Mar 23, 2018

Uh oh!

n1bor commented Mar 23, 2018

Uh oh!

pm47 commented Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pm47 left a comment •

edited

Loading

pm47 commented Mar 24, 2018 •

edited

Loading