fix(swaps): set cltvLimit correctly

This fixes a bug whereby the `makerCltvDelta` was erroneously used for
the `cltvLimit` for the payment from maker to taker. `makerCltvDelta` is
irrelevent to the payment to the taker. Instead, this value is set to
the time lock of the route found from maker to taker for lnd, raiden
currently has no equivalent to `cltvLimit`.

In tests, payments would still fail occasionally due to no route found.
Adding 3 blocks to the `cltvLimit` value consistently resolved these
failures - any fewer than 3 and the payments would still fail. More
investigation is warranted into why this is necessary, it's possible
that there is a bug in the `cltvLimit` implementation within lnd.

Fixes #1158.

Author: sangaman

lib/db/types.ts

@@ -29,7 +29,7 @@ export type CurrencyInstance = CurrencyAttributes & Sequelize.Instance<CurrencyA
 
 /* SwapDeal */
 export type SwapDealFactory = Pick<SwapDeal, Exclude<keyof SwapDeal,
-  'makerToTakerRoutes' | 'price' | 'pairId' | 'isBuy' | 'takerUnits' | 'makerUnits'>>;
+  'takerMaxTimeLock' | 'price' | 'pairId' | 'isBuy' | 'takerUnits' | 'makerUnits'>>;
 
 export type SwapDealAttributes = SwapDealFactory & {
   /** The internal db node id of the counterparty peer for this swap deal. */

lib/lndclient/LndClient.ts

@@ -361,10 +361,11 @@ class LndClient extends SwapClient {
         rHash: deal.rHash,
         destination: deal.takerPubKey!,
         amount: deal.takerAmount,
+        finalCltvDelta: deal.takerCltvDelta,
         // Enforcing the maximum duration/length of the payment by
         // specifying the cltvLimit.
-        finalCltvDelta: deal.takerCltvDelta,
-        cltvLimit: deal.makerCltvDelta,
+        // TODO: investigate why we need to add 3 blocks - if not lnd says route not found
+        cltvLimit: deal.takerMaxTimeLock! + 3,
       });
     }
     const preimage = await this.executeSendRequest(request);
@@ -408,6 +409,7 @@ class LndClient extends SwapClient {
   private executeSendRequest = async (
     request: lndrpc.SendRequest,
   ): Promise<string> => {
+    this.logger.trace(`sending payment with ${JSON.stringify(request.toObject())}`);
     let sendPaymentResponse: lndrpc.SendResponse;
     try {
       sendPaymentResponse = await this.sendPaymentSync(request);

lib/raidenclient/RaidenClient.ts

@@ -165,26 +165,28 @@ class RaidenClient extends SwapClient {
   public sendPayment = async (deal: SwapDeal): Promise<string> => {
     assert(deal.state === SwapState.Active);
     assert(deal.destination);
-    let amount = 0;
-    let tokenAddress;
+    let amount: number;
+    let tokenAddress: string;
+    let lock_timeout: number | undefined;
     if (deal.role === SwapRole.Maker) {
       // we are the maker paying the taker
       amount = deal.takerUnits;
-      tokenAddress = this.tokenAddresses.get(deal.takerCurrency);
+      tokenAddress = this.tokenAddresses.get(deal.takerCurrency)!;
     } else {
       // we are the taker paying the maker
       amount = deal.makerUnits;
-      tokenAddress = this.tokenAddresses.get(deal.makerCurrency);
+      tokenAddress = this.tokenAddresses.get(deal.makerCurrency)!;
+      lock_timeout = deal.makerCltvDelta!;
     }
     if (!tokenAddress) {
       throw(errors.TOKEN_ADDRESS_NOT_FOUND);
     }
     const tokenPaymentResponse = await this.tokenPayment({
       amount,
+      lock_timeout,
       token_address: tokenAddress,
       target_address: deal.destination!,
       secret_hash: deal.rHash,
-      lock_timeout: deal.makerCltvDelta,
     });
     return this.sanitizeTokenPaymentResponse(tokenPaymentResponse);
   }

lib/swaps/Swaps.ts

@@ -9,7 +9,7 @@ import SwapRepository from './SwapRepository';
 import { OwnOrder, PeerOrder } from '../orderbook/types';
 import assert from 'assert';
 import { SwapDealInstance } from '../db/types';
-import { SwapDeal, SwapSuccess, SanitySwap, ResolveRequest } from './types';
+import { SwapDeal, SwapSuccess, SanitySwap, ResolveRequest, Route } from './types';
 import { generatePreimageAndHash, setTimeoutPromise } from '../utils/utils';
 import { PacketType } from '../p2p/packets';
 import SwapClientManager from './SwapClientManager';
@@ -519,8 +519,9 @@ class Swaps extends EventEmitter {
       return false;
     }
 
+    let makerToTakerRoutes: Route[];
     try {
-      deal.makerToTakerRoutes = await takerSwapClient.getRoutes(takerUnits, takerIdentifier, deal.takerCurrency, deal.takerCltvDelta);
+      makerToTakerRoutes = await takerSwapClient.getRoutes(takerUnits, takerIdentifier, deal.takerCurrency, deal.takerCltvDelta);
     } catch (err) {
       this.failDeal(deal, SwapFailureReason.UnexpectedClientError, err.message);
       await this.sendErrorToPeer({
@@ -533,7 +534,7 @@ class Swaps extends EventEmitter {
       return false;
     }
 
-    if (deal.makerToTakerRoutes.length === 0) {
+    if (makerToTakerRoutes.length === 0) {
       this.failDeal(deal, SwapFailureReason.NoRouteFound, 'Unable to find route to destination');
       await this.sendErrorToPeer({
         peer,
@@ -563,10 +564,11 @@ class Swaps extends EventEmitter {
     if (height) {
       this.logger.debug(`got block height of ${height} for ${takerCurrency}`);
 
-      const routeAbsoluteTimeLock = deal.makerToTakerRoutes[0].getTotalTimeLock();
+      const routeAbsoluteTimeLock = makerToTakerRoutes[0].getTotalTimeLock();
       this.logger.debug(`choosing a route with total time lock of ${routeAbsoluteTimeLock}`);
       const routeTimeLock = routeAbsoluteTimeLock - height;
       this.logger.debug(`route time lock: ${routeTimeLock}`);
+      deal.takerMaxTimeLock = routeTimeLock;
 
       const makerClientLockBuffer = this.swapClientManager.get(makerCurrency)!.lockBuffer;
       this.logger.debug(`maker client lock buffer: ${makerClientLockBuffer}`);

lib/swaps/types.ts

@@ -56,8 +56,8 @@ export type SwapDeal = {
   rHash: string;
   /** The hex-encoded preimage. */
   rPreimage?: string;
-  /** The routes the maker should use to send to the taker. */
-  makerToTakerRoutes?: Route[];
+  /** The maximum time lock from the maker to the taker in blocks. */
+  takerMaxTimeLock?: number;
   /** The identifier for the payment channel network node we should pay to complete the swap.  */
   destination?: string;
   /** The time when we created this swap deal locally. */

test/jest/LndClient.spec.ts

@@ -204,7 +204,7 @@ describe('LndClient', () => {
         amount: deal.takerAmount,
         destination: deal.takerPubKey,
         rHash: deal.rHash,
-        cltvLimit: deal.makerCltvDelta,
+        cltvLimit: deal.takerMaxTimeLock + 3,
         finalCltvDelta: deal.takerCltvDelta,
       });
     });

test/jest/integration/Swaps.spec.ts

@@ -149,7 +149,7 @@ describe('Swaps', () => {
       expect(dealAccepted).toEqual(false);
     });
 
-    test('it rejects upon 0 makerToTakerRoutes found', async () => {
+    test('it rejects upon 0 maker to taker routes found', async () => {
       lndBtc.getRoutes = jest.fn().mockReturnValue([]);
       swapClientManager.get = jest.fn().mockImplementation((currency) => {
         if (currency === takerCurrency) {

test/utils.ts

@@ -115,6 +115,6 @@ export const getValidDeal = () => {
     state: 0,
     role: 1,
     createTime: 1559120485138,
-    makerToTakerRoutes: [{ getTotalTimeLock: () => {} }],
+    takerMaxTimeLock: 100,
   };
 };