fix(swaps): ensure correct takerCltvDelta and makerCltvDelta usage

[ghost]

This PR fixes several bugs around Swaps.acceptDeal - maker's opportunity to either accept or reject the SwapRequest, more specifically the logic around setting the CLTV values.

• include takerCltvDelta as finalCltvDelta when querying for makerToTakerRoutes. This is necessary to ensure we calculate makerCltvDelta correctly.
• LndClient.addInvoice now requires cltvExpiry so that we can force the minimum duration of the route by specifying the makerCltvDelta. Previously, it was possible for the first leg to be shorter than the second one as described in [WIP] refactor(lnd): use SendPayment instead of SendToRoute #1025 (comment)
• remove unnecessary logging added by chore: add additional swaps logging #1035 and feat(logger): add more detailed swaps logging #1033
• send error to peer when accepting the deal failed because something went wrong while creating the invoice. Previously we only rejected the deal and logged an error.

Closes #1028
Closes #984

[offerm]

Why this.cltvDelta should be used here?
argument should not be optional IMHO

[ghost]

I originally thought about making the argument mandatory, but the second usage of getRoutes happens within the context of maker at the beginning of the deal execution process when taker queries routes to maker.

At this point taker has the following information:

const makerOrder = {
  id: 'fb1708f0-90d7-11e9-8836-9bf81440c6fa',
  pairId: 'LTC/BTC',
  price: 0.1,
  quantity: 10000,
  isBuy: true,
  peerPubKey:
   '038d48a01d25735cd7bd26ac8cbd7583e4c202718848ab63b08b378dc840a24e81',
  createdAt: 1560759291688,
  initialQuantity: 10000,
};

const takerOrder = {
  pairId: 'LTC/BTC',
  price: 0.1,
  quantity: 10000,
  isBuy: false,
  localId: 'fd6da320-90d7-11e9-9e9d-0f0eb626ffe9',
  hold: 0,
  id: 'fd6da320-90d7-11e9-9e9d-0f0eb626ffe9',
  initialQuantity: 10000,
  createdAt: 1560759295570,
};

What would you set the finalCltvDelta to? Furthermore, the value of takerToMakerRoutes is being used as a light sanity check (error when 0 routes found). Alternative option is to remove the getRoutes check from the taker side.

[ghost]

In order to be more explicit I changed getRoutes to require finalCltvDelta.

[sangaman]

Fwiw I found it a bit odd to pass an object's own property to one of its method calls as in routes = await swapClient.getRoutes(makerAmount, destination, swapClient.cltvDelta);.

Another way to consider writing this is:

public getRoutes =  async (amount: number, destination: string, finalCltvDelta = this.cltvDelta) => {

That way it's clear what the value defaults to if its not specified. Ultimately not a big deal though so it definitely be left as is.

[ghost]

After thinking about it for a while I agree, it's weird to pass an object's own property to one of its method calls. I changed it back.

[offerm]

This should be handled in the same way. The first leg should include the second leg.

[ghost]

In the case of SanitySwap and SanitySwapAck we're currently not exchanging CLTV delta information.

If it's OK with you I'd like to handle everything related to the sanity swaps in a separate issue/PR.

[sangaman]

I also think it's better to handle sanity swaps in a separate PR.

[kilrau]

Does this fix #977 ?

[ghost]

I doubt it will fix #977 because this PR didn't touch sanity swaps' flow/logic. In the case of SanitySwap and SanitySwapAck we're currently not exchanging CLTV delta information and due to that it can still fail with FinalExpiryTooSoon.

[sangaman]

Looks good to me, especially the new test cases. Thanks!

[sangaman]

I also think it's better to handle sanity swaps in a separate PR.