feat(connext): lazy collateralization #1916

sangaman · 2020-09-24T05:58:57Z

Closes #1896. Branched off of and dependant on #1885. WIP for now but this adds functionality to track reserved outbound & inbound balances each time an own order is added to or removed from the order book. Every time an order is added (and reserved amount increases) we check if the inbound capacity for that currency is sufficient to cover all orders. In Connext's case, if it's not sufficient then we make a collateral request to cover all orders +3% (unless we already have a pending collateral request waiting). We don't do anything on the lnd side since lnd can't dynamically increase its inbound capacity (although we could consider doing something like logging a message).

Related to #1584, which can use the reserved amounts that are being tracked in this PR and expose them over rpc.

Tests still need to be updated to mock the new methods that track reserved amounts and/or add test cases for the reserved amount tracking logic.

sangaman · 2020-09-29T07:49:40Z

I finished adding comprehensive tests across the OrderBook, SwapClientManager, and ConnextClient classes. This should be ready for testing/review.

raladev · 2020-09-30T13:34:15Z

we still have order placing collaterization, should it be removed?

simnet > buy 0.5 ETH/BTC 0.1 2
9 FAILED_PRECONDITION: channel collateralization in progress, please try again in ~1 minute

ghost

Looking very good. Left some minor comments + simulation tests are still failing.

ghost · 2020-09-30T13:20:25Z

lib/connextclient/ConnextClient.ts

+      // if not start a new request, and when it completes call channelBalance to refresh our inbound capacity
+      // we don't await this request - instead we allow for "lazy collateralization" to complete since
+      // we don't expect all orders to be filled at once, we can be patient
+      const requestCollateralPromise = this.requestCollateralPromises.get(currency) ?? this.sendRequest('/request-collateral', 'POST', {


This logic below seems to be the same as the one defined in public checkInboundCapacity. Can we extract this to a function?

ghost · 2020-09-30T13:59:42Z

* we still have order placing collaterization, should it be removed?

simnet > buy 0.5 ETH/BTC 0.1 2
9 FAILED_PRECONDITION: channel collateralization in progress, please try again in ~1 minute

No, this feature is complementary to the order placing collateralization.

For example:

0 inbound liquidity
buy 0.5 ETH -> placeorder will request collateral and not let the order enter the orderbook
buy 0.5 ETH -> placeorder will succeed immediately and request the additional 0.5 ETH collateral in the background (this feature)

raladev

Approved but need to fix tests

sangaman · 2020-10-01T06:22:29Z

I finally came up with a way to get the simulation instability tests to pass, as connext payments were inexplicably failing for certain cases. I couldn't figure out why the regular collateral calls on PlaceOrder calls weren't causing any issues, until eventually I realized that all simulation tests use nobalancechecks which skips the collateral on order calls. This PR actually triggers collateral requests in the simulation tests, and for a reason I still don't understand it caused payments to fail in certain cases.

To work around this, I made it so the custom-xud patch code removes the lazy collateral calls logic, and tests are passing now.

lib/connextclient/ConnextClient.ts

ghost · 2020-10-01T06:41:04Z

lib/connextclient/ConnextClient.ts

+            });
+
+            this.requestCollateralPromises.set(currency, requestCollateralPromise);
+            return from(requestCollateralPromise);


Maybe we can also remove the above lines and replace it with return from(this.requestCollateralInBackground(currency, minCollateralRequestUnits)) as it seems to be doing the same thing (with extra logging)?

return from(this.requestCollateralInBackground(currency, minCollateralRequestUnits)) doesn't work exactly because requestCollateralInBackground doesn't return a promise, by design the promise is resolved/handled in the background. I could just call the method and return nothing? that should be fine right?

kilrau · 2020-10-01T10:36:49Z

This PR actually triggers collateral requests in the simulation tests, and for a reason I still don't understand it caused payments to fail in certain cases.

Shouldn't we find out why these fail though?

sangaman · 2020-10-01T15:13:00Z

This PR actually triggers collateral requests in the simulation tests, and for a reason I still don't understand it caused payments to fail in certain cases.

Shouldn't we find out why these fail though?

Yes, I reached out to the connext team and I've spent a lot of time trying to figure it out myself but it's still a myster. I was thinking it could be that payments to a node that is in the middle of recollateralization would fail, but that's not supposed to be the case according to connext.

This ads functionality to track reserved outboudn & inbound balances each tmie an own order is added to or removed from the order book. Every time an order is added (and reserved amount increases) we check if the inbound capaacity for that currency is sufficient to cover all orders. In Connext's case, if it's not sufficient then we make a collateral request to cover all orders +3% (unless we already have a pending collateral request waiting). We don't do anything on the lnd side since lnd can't dynamically increase its inbound capacity. Closes #1896.

This restores the logic to request collateral in the custom xud builds used for instability tests by adding a one second wait after adding an order on the maker side (who then requests collateralization) but before placing a matching order on the taker side. This appears to work around a brief, transient inability to send a connext payment to a node immediately after that node requests collateral. Related PR #1916 - specifically the persistent simulation test failures that were eventually avoided by removing the collateral requests from custom-xud, which is being restored here.

sangaman added order book connext labels Sep 24, 2020

sangaman requested review from a user, kilrau and raladev September 24, 2020 05:58

sangaman self-assigned this Sep 24, 2020

sangaman marked this pull request as draft September 24, 2020 05:59

sangaman changed the title ~~Lazy collateral~~ WIP: Lazy collateral Sep 24, 2020

sangaman force-pushed the lazy-collateral branch from f8b98d9 to bab5f81 Compare September 29, 2020 07:48

sangaman marked this pull request as ready for review September 29, 2020 07:49

sangaman changed the title ~~WIP: Lazy collateral~~ feat(connext): lazy collateralization Sep 29, 2020

ghost reviewed Sep 30, 2020

View reviewed changes

raladev previously approved these changes Sep 30, 2020

View reviewed changes

sangaman dismissed raladev’s stale review via 8644547 September 30, 2020 15:45

sangaman force-pushed the lazy-collateral branch 2 times, most recently from e1c4924 to e8f7690 Compare October 1, 2020 05:28

sangaman mentioned this pull request Oct 1, 2020

feat(rpc): expose reserved balance for GetBalance #1925

Merged

sangaman requested a review from a user October 1, 2020 06:23

ghost reviewed Oct 1, 2020

View reviewed changes

sangaman force-pushed the lazy-collateral branch from e8f7690 to c4ef81f Compare October 1, 2020 15:11

sangaman requested a review from a user October 1, 2020 15:11

sangaman added 2 commits October 1, 2020 11:13

test(sim): name order ids after test cases

9d34b3a

sangaman force-pushed the lazy-collateral branch from c4ef81f to 9d34b3a Compare October 1, 2020 15:14

ghost approved these changes Oct 1, 2020

View reviewed changes

raladev self-requested a review October 2, 2020 11:12

raladev approved these changes Oct 2, 2020

View reviewed changes

raladev merged commit 0f2b841 into master Oct 2, 2020

kilrau mentioned this pull request Oct 5, 2020

Reserved Balance (show balance reserved by open orders separately) #1584

Closed

sangaman mentioned this pull request Oct 7, 2020

test(sim): connext collateralization with wait #1926

Merged

sangaman deleted the lazy-collateral branch October 14, 2020 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(connext): lazy collateralization #1916

feat(connext): lazy collateralization #1916

sangaman commented Sep 24, 2020 •

edited

Loading

sangaman commented Sep 29, 2020

raladev commented Sep 30, 2020

ghost left a comment

ghost Sep 30, 2020

sangaman Sep 30, 2020

ghost commented Sep 30, 2020

raladev left a comment

sangaman commented Oct 1, 2020

ghost Oct 1, 2020

sangaman Oct 1, 2020 •

edited

Loading

kilrau commented Oct 1, 2020

sangaman commented Oct 1, 2020

feat(connext): lazy collateralization #1916

feat(connext): lazy collateralization #1916

Conversation

sangaman commented Sep 24, 2020 • edited Loading

sangaman commented Sep 29, 2020

raladev commented Sep 30, 2020

ghost left a comment

Choose a reason for hiding this comment

ghost Sep 30, 2020

Choose a reason for hiding this comment

sangaman Sep 30, 2020

Choose a reason for hiding this comment

ghost commented Sep 30, 2020

raladev left a comment

Choose a reason for hiding this comment

sangaman commented Oct 1, 2020

ghost Oct 1, 2020

Choose a reason for hiding this comment

sangaman Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

kilrau commented Oct 1, 2020

sangaman commented Oct 1, 2020

sangaman commented Sep 24, 2020 •

edited

Loading

sangaman Oct 1, 2020 •

edited

Loading