fix(swaps): set cltvLimit correctly #1163
Conversation
|
Definitely needs your review @offerm |
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.
sangaman
force-pushed
the
fix/cltv-limit
branch
from
5207197 to
31b41a1
Compare
| 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)!; |
There was a problem hiding this comment.
I think we also need to use lock_timout=deal.takerMaxTimeLock here to ensure that the maker's payment duration.
There was a problem hiding this comment.
I don't think so, because we're using lock_timeout as the delay extended to the recipient of the payment, whereas the takerMaxTimeLock is an upper bound on the total delay for the route. Also when the maker to taker payment is raiden, we use a hardcoded value for the route total time lock of 101 (which is then set to takerMaxTimelock) so I don't think it would be of much use here anyway.
Still, this is a good point to consider going forward, if we're going to be using multihop raiden payments with variable total time locks then we need to know what the maximum time lock will be for properly executing the swap.
There was a problem hiding this comment.
Ah, right - makes sense. I forgot that we're only supporting direct channels for now.
There was a problem hiding this comment.
Fixes #1158.
This fixes a bug whereby the
makerCltvDeltawas erroneously used for thecltvLimitfor the payment from maker to taker.makerCltvDeltais 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 tocltvLimit.In tests, payments would still fail occasionally due to no route found. Adding 3 blocks to the
cltvLimitvalue 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 thecltvLimitimplementation within lnd.This fix makes the new multihop simulation test case in #1160 pass.