Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rpc): runtime addcurrency for lnd & connext #1746

Merged
merged 13 commits into from
Nov 5, 2020
Merged

Conversation

sangaman
Copy link
Collaborator

@sangaman sangaman commented Jul 23, 2020

This PR allows for adding Connext and Lnd currencies via the AddCurrency rpc call and have them available for trading immediately without requiring a restart of xud.

In the case of Connext it merely registers the tokenaddress and currency symbol for the new currency with the existing ConnextClient, as we had previously done with Raiden.

In the case of Lnd, we read from the configuration and use that to instantiate and initialize a new LndClient for the specified currency.

Closes #1111.

EDIT by @kilrau : closes #1538

This PR allows for adding Connext and Lnd currencies via the
`AddCurrency` rpc call and have them available for trading immediately
without requiring a restart of xud.

In the case of Connext it merely registers the tokenaddress and currency
symbol for the new currency with the existing ConnextClient, as we had
previously done with Raiden.

In the case of Lnd, we read from the configuration and use that to
instantiate and initialize a new `LndClient` for the specified currency.

Closes #1111.
@sangaman sangaman added the grpc gRPC API label Jul 23, 2020
@sangaman sangaman requested review from a user, raladev and LePremierHomme July 23, 2020 04:46
@sangaman sangaman self-assigned this Jul 23, 2020
Copy link
Contributor

@raladev raladev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (invalid) connext currency adding corrupts order acceptance/removing from remote peers
    Screenshot from 2020-07-23 15-32-55

@sangaman
Copy link
Collaborator Author

@raladev Would you mind rerunning your test with the latest from this branch? It should print the error as to why peer orders aren't getting removed, I don't understand why that's happening right now and that would help a lot.

this.swapClients.set(currency.id, this.connextClient);
this.connextClient.tokenAddresses.set(currency.id, currency.tokenAddress);
this.emit('connextUpdate', this.connextClient.tokenAddresses, this.connextClient.address);
} else if (currency.swapClient === SwapClientType.Raiden) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should remove the raiden part since it's never going to be used and it would make the function more readable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sangaman
Copy link
Collaborator Author

I'm also trying to work through why simulation tests are failing, so no rush. Moving this to draft status until I get to the bottom of it.

@sangaman sangaman marked this pull request as draft July 24, 2020 12:10
@raladev
Copy link
Contributor

raladev commented Jul 24, 2020

@sangaman errors are occurred because of ETH disabling:

  1. addcurrency XUC Connext 18 0xgogen as taker (13:03:24.286 taker)
  2. maker is deactivating ETH for the taker and stopping BTC/ETH order sending to him (13:03:24.571 maker - deactivation, 13:04:06.900 maker - order placing after deactivation (no sending to ZooSave))
  3. but maker is sending invalidation packets to taker (13:04:30.637 maker - order invalidation (sending to ZooSave))
  4. as a result we got remove order error for taker (13:04:30.858)

xud_maker_addcur.log
xud_taker_addcur.log

@kilrau kilrau added the P2 mid priority label Aug 10, 2020
@kilrau
Copy link
Contributor

kilrau commented Aug 13, 2020

Can we push this one over the finish line too? @sangaman

@kilrau
Copy link
Contributor

kilrau commented Sep 30, 2020

This just needs simulation tests to pass. Want to have a look? @rsercano

@rsercano rsercano self-assigned this Sep 30, 2020
@LePremierHomme
Copy link
Contributor

@kilrau @rsercano I'd rather work on the simtest fix myself, because it might be non-trivial, and require some refactoring.

@kilrau
Copy link
Contributor

kilrau commented Oct 2, 2020

@kilrau @rsercano I'd rather work on the simtest fix myself, because it might be non-trivial, and require some refactoring.

Makes sense, go ahead!

@kilrau
Copy link
Contributor

kilrau commented Oct 2, 2020

Assigned both issues this will close to @LePremierHomme :
#1888
#1111

Le Premier Homme added 3 commits October 6, 2020 14:25
@kilrau kilrau requested a review from raladev October 21, 2020 10:10
@kilrau
Copy link
Contributor

kilrau commented Oct 21, 2020

Nice! Ready4Review and end-to-end testing?

@LePremierHomme
Copy link
Contributor

@kilrau yes.

ghost
ghost previously approved these changes Oct 22, 2020
@raladev raladev self-requested a review October 23, 2020 12:01
@raladev
Copy link
Contributor

raladev commented Oct 23, 2020

  • xud is crashed after removing pre-defined pair

Steps:

  1. bash xud.sh -b cu (xud-docker branch with addcurrency PR)
  2. remove USDT/DAI

Actual result:
Xud is crashed
Screenshot from 2020-10-23 15-01-38

23/10/2020 11:59:51.215 [P2P] verbose: Verified reachability of advertised address: rkwdw765jyuo3wbr4w74pyyq625bo2ihyppw4wbb6b45qwpscacpspad.onion:28885
{ message: 'trading pair USDT/DAI does not exist', code: '3.1' }

/app/dist/bootstrap.js:13
        throw err;
        ^
{ message: 'trading pair USDT/DAI does not exist', code: '3.1' }

Note:
it may be crashed only if u did not add new pairs before that, but if u did that, all will be fine.
Screenshot from 2020-10-23 15-14-27

Copy link
Contributor

@raladev raladev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above

@kilrau
Copy link
Contributor

kilrau commented Oct 23, 2020

Thanks for catching this @raladev , ping us when to test again @LePremierHomme

@kilrau
Copy link
Contributor

kilrau commented Oct 27, 2020

Does this resolve issue 1-3 of #1932 ? @raladev

@raladev
Copy link
Contributor

raladev commented Oct 28, 2020

@kilrau @LePremierHomme

Weirdness no 1:

mainnet > getbalance SER
Error: 9 FAILED_PRECONDITION: swapClient for currency SER not found

This error is legit, it appeared because SER was added using Lnd as a swapClient, but Lnd does not support this currency, so -> swapClient for currency SER not found. Maybe we should add check for addcurrency command and Lnd Client (lnd client can be used only for LTC and BTC currencies)

testnet > addcurrency SER Lnd --decimal_places 7 --token_address 0xB561fEF0d624C0826ff869946f6076B7c4f2ba42
success
testnet > getbalance SER
Error: 9 FAILED_PRECONDITION: swapClient for currency SER not found

Weirdness no 2:

mainnet > xucli removecurrency SER
Error: 3 INVALID_ARGUMENT: currency must consist of 2 to 5 upper case English letters or numbers

Cant reproduce it with addcurrency branch. Marked as Fixed.

Weirdness no 3:

mainnet > getbalance

Balance:
┌──────────┬───────────────┬────────────────────────────┬───────────────────────────────┐
│ Currency │ Total Balance │ Channel Balance (Tradable) │ Wallet Balance (Not Tradable) │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ BTC │ 0 │ 0 │ 0 │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ DAI │ 0 │ 0 │ 0 │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ ETH │ 0 │ 0 │ 0 │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ LTC │ 0 │ 0 │ 0 │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ USDT │ 0 │ 0 │ 0 │
└──────────┴───────────────┴────────────────────────────┴───────────────────────────────┘

getbalance command above doesn't show SER balance.

Reproduced, it is a bug.

Steps:

  1. addcurrency LINK Connext 18 0x01be23585060835e02b77ef475b0cc51aa1e0709 on testnet
  2. getbalance
  3. getbalance LINK

Actual result:

  1. No Link in get balance table
testnet > getbalance LINK
Error: 2 UNKNOWN: cannot convert LINK units of 0 to amount because units per currency was not found in the database

Note:
Xud log is full of following errors:

28/10/2020 16:13:52.353 [CONNEXT] error: failed to update total outbound capacity: Error: cannot convert LINK units of 0 to amount because units per currency was not found in the database
    at UnitConverter.unitsToAmount (/app/dist/utils/UnitConverter.js:30:23)
    at ConnextClient.<anonymous> (/app/dist/connextclient/ConnextClient.js:503:62)
    at Generator.next (<anonymous>)
    at fulfilled (/app/dist/connextclient/ConnextClient.js:24:58)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
28/10/2020 16:13:55.340 [CONNEXT] error: failed to update total outbound capacity: Error: cannot convert LINK units of 0 to amount because units per currency was not found in the database
    at UnitConverter.unitsToAmount (/app/dist/utils/UnitConverter.js:30:23)
    at ConnextClient.<anonymous> (/app/dist/connextclient/ConnextClient.js:503:62)
    at Generator.next (<anonymous>)
    at fulfilled (/app/dist/connextclient/ConnextClient.js:24:58)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
28/10/2020 16:13:58.339 [CONNEXT] error: failed to update total outbound capacity: Error: cannot convert LINK units of 0 to amount because units per currency was not found in the database
    at UnitConverter.unitsToAmount (/app/dist/utils/UnitConverter.js:30:23)
    at ConnextClient.<anonymous> (/app/dist/connextclient/ConnextClient.js:503:62)
    at Generator.next (<anonymous>)
    at fulfilled (/app/dist/connextclient/ConnextClient.js:24:58)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
Trace: getTradingPair
    at OrderBook.getTradingPair (/app/dist/orderbook/OrderBook.js:176:25)
    at OrderBook.removePeerOrder (/app/dist/orderbook/OrderBook.js:814:33)
    at Pool.OrderBook.handleOrderInvalidation (/app/dist/orderbook/OrderBook.js:956:47)
    at Pool.emit (events.js:314:20)
    at Pool.<anonymous> (/app/dist/p2p/Pool.js:633:34)
    at Generator.next (<anonymous>)
    at /app/dist/p2p/Pool.js:27:71
    at new Promise (<anonymous>)
    at __awaiter (/app/dist/p2p/Pool.js:23:12)
    at Pool.handlePacket (/app/dist/p2p/Pool.js:621:47)
Trace: getTradingPair
    at OrderBook.getTradingPair (/app/dist/orderbook/OrderBook.js:176:25)
    at OrderBook.removePeerOrder (/app/dist/orderbook/OrderBook.js:814:33)
    at Pool.OrderBook.handleOrderInvalidation (/app/dist/orderbook/OrderBook.js:956:47)
    at Pool.emit (events.js:314:20)
    at Pool.<anonymous> (/app/dist/p2p/Pool.js:633:34)
    at Generator.next (<anonymous>)
    at /app/dist/p2p/Pool.js:27:71
    at new Promise (<anonymous>)
    at __awaiter (/app/dist/p2p/Pool.js:23:12)
    at Pool.handlePacket (/app/dist/p2p/Pool.js:621:47)
28/10/2020 16:14:01.343 [CONNEXT] error: failed to update total outbound capacity: Error: cannot convert LINK units of 0 to amount because units per currency was not found in the database
    at UnitConverter.unitsToAmount (/app/dist/utils/UnitConverter.js:30:23)
    at ConnextClient.<anonymous> (/app/dist/connextclient/ConnextClient.js:503:62)
    at Generator.next (<anonymous>)
    at fulfilled (/app/dist/connextclient/ConnextClient.js:24:58)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

My own Weirdness
Even after LINK currency removing (it was successful) xud continues throw the errors and ask connext for LINK balance.

@LePremierHomme
Copy link
Contributor

LePremierHomme commented Nov 1, 2020

@raladev the issue described in #1746 (comment) was fixed by e52ba4f so this branch can be re-tested.

Regarding the described issue 3 in #1932 and #1746 (comment) - looks like the currency add is fine, but it is expected to already be defined statically in the unit converter. Any ideas? @erkarl @sangaman

@ghost
Copy link

ghost commented Nov 2, 2020

Regarding the described issue 3 in #1932 and #1746 (comment) - looks like the currency add is fine, but it is expected to already be defined statically in the unit converter. Any ideas? @erkarl @sangaman

Once #1938 is merged we'll have access to the ethprovider via connext proxy. We can then query the contract address and get the decimal count dynamically. I suggest to hold off on adding more currencies to the currently static unit converter list.

@LePremierHomme
Copy link
Contributor

Ok, so looks like this PR just awaits final approval regarding the e52ba4f fix for #1746 (comment), from both @raladev and @erkarl.

@raladev
Copy link
Contributor

raladev commented Nov 2, 2020

Current state:

0. Alice has BTC/USDT pairs and order for this pair that were placed by bob
1. removepair BTC/USDT as Alice
2. addpair BTC/USDT as Alice

Actual result:
No orders of bob in alice's orderbook. 
  • I got strange behavior when i did not get buy order from remote peer after removing and returning the currency. Why? IDK, I tried to reproduce it but no luck, only thing that i have is one screenshot:) And im not sure that it is connected with pair removing/adding
    Screenshot from 2020-11-02 16-51-03

@kilrau
Copy link
Contributor

kilrau commented Nov 2, 2020

Removed the closing statement for #1888 . We don't need vector for this, we just need #1054 and just bumped prio for this, @sangaman already has a PR up that fixes it: #1904

As just discussed, let's leave the closing statement here and please open a new bug issue @raladev .

* I got strange behavior when i did not get buy order from remote peer after removing and returning the currency. Why? IDK, I tried to reproduce it but no luck, only thing that i have is one screenshot:) And im not sure that it is connected with pair removing/adding

As just discussed, this is most likely an unrelated bug. @raladev please open a bug issue with as many details as you can get from your env. I observed the same behaviour a while ago on one of my environments and was not able to reproduce it ever since. Must be some edge case bug.

@raladev raladev requested review from raladev and a user November 3, 2020 13:21
@sangaman
Copy link
Collaborator Author

sangaman commented Nov 5, 2020

The code looks good and passes some quick manual tests for me. No reason to hold this up any longer. Thanks to @LePremierHomme for figuring some of these lingering issues out.

@sangaman sangaman merged commit fc83823 into master Nov 5, 2020
@sangaman sangaman deleted the addcurrency-runtime branch November 5, 2020 18:03
raladev added a commit that referenced this pull request Nov 19, 2020
* feat: removeorder output changed to a more meaningful message (#1526)

* fix(p2p): don't reconnect peers when pool closed (#1965)

This ensures that we don't attempt to reconnect to peers that have
disconnected from us after we have started closing the p2p pool. This
may help prevent scenarios where we unintentionally attempt to
reconnect to peers after shutting down xud.

> Should be tested against [#1668 (comment)](#1668 (comment)) @raladev

re-connection after shutdown is disappeared, but my xud still can not be gracefully  terminated, it waits something:

```
28/10/2020 05:17:43.164 [CONNEXT] trace: sending request to /balance/0x69C3d485623bA3f382Fc0FB6756c4574d43C1618
^C28/10/2020 05:17:44.087 [GLOBAL] info: XUD is shutting down
28/10/2020 05:17:44.088 [LND-BTC] info: new status: Disconnected
28/10/2020 05:17:44.089 [LND-LTC] info: new status: Disconnected
28/10/2020 05:17:44.090 [CONNEXT] info: new status: Disconnected
28/10/2020 05:17:44.093 [P2P] debug: Peer 03ece33a30db1dbce4b62fa96a5e9541138a24997ef5672eebed2d332270e39542 (OzoneYellow): closing socket. reason: Shutdown
28/10/2020 05:17:44.095 [HTTP] info: http server has closed
28/10/2020 05:17:44.096 [RPC] info: gRPC server completed shutdown
28/10/2020 05:17:44.097 [P2P] trace: Sent Disconnecting packet to 03ece33a30db1dbce4b62fa96a5e9541138a24997ef5672eebed2d332270e39542 (OzoneYellow): "{\"body\":{\"reason\":9},\"header\":{\"id\":\"95133be0-1917-11eb-b75b-73d0f0278756\"}}"
28/10/2020 05:17:44.109 [ORDERBOOK] debug: removed all orders for peer 03ece33a30db1dbce4b62fa96a5e9541138a24997ef5672eebed2d332270e39542 (OzoneYellow)
28/10/2020 05:17:44.118 [GLOBAL] info: XUD shutdown gracefully
```

* feat(lnd): change gRPC client options

* fix(connext): not enough balance for closechannel (#1963)

This introduces better error handling for Connext when using
`closeChannel` to remove funds from a Connext channel and specifying an
amount to remove that is greater than the available balance.

* feat: reserved capacity checks on PlaceOrder (#1949)

This rejects orders that would put our total reserved balance over our
total capacity for either the outbound or inbound currency. The sum of
the inbound & outbound amounts for a newly placed order are added to
the amounts reserved by open orders, and if either of these amounts
exceed the corresponding capacity then the request to place the order is
rejected.

An exception to this are inbound limits for Connext currencies, since we
have the ability to dynamically request additional inbound collateral
via our "lazy collateral" approach.

It is still possible for market orders to cause our open orders to
exceed our capacity. This is a difficult problem to avoid entirely, as
the price that market orders will execute at is unknown until the
execution is complete. Even if we simulate the matching routine, we
won't know which matches will succeed until we attempt a swap.

Instead, we generously assume that market orders will execute at the
best quoted price for purposes of these capacity checks. For users that
simultaneously place limit orders and market orders for the same
currencies, it should be made clear that market orders may use up their
available balance needed for their limit orders to succeed.

Closes #1947.

* fix(cli): openchannel assertion error for string amount (#1950)

Fixes #1643.

* feat(swapclient): auto init wallets on xud unlock (#1973)

This adds a new feature to xud to automatically attempt to create a
wallet for any new swap client configured after an xud node has been
created. Effectively this only changes the behavior for lnd clients, as
this is already the existing behavior for Connext. The process for
initializing has now been standardized instead of the ad hoc approach
used previously.

If xud tries to unlock an lnd node and gets an error message indicating
that the wallet has not been created, then it will generate a client &
currency specific seed mnemonic using seedutil and call `InitWallet`
with that seed and the existing xud password, such that the wallet
funds and node identity for the new lnd client can be unlocked and
restored along with the rest of lnd.

Closes #1929.

* feat(rpc): runtime addcurrency for lnd & connext (#1746)

Co-authored-by: Le Premier Homme <interjoint1@gmail.com>

* refactor(rpc): rename reserved TradingLimits fields

This renames the `reservedOutbound` and `reservedInbound` fields on the
`TradingLimits` call to `reservedSell` and `reservedBuy` respectively.

* fix(rpc): no success if no channels to close (#1689) (#1942)

Co-authored-by: rsercano <ozdemirsercan27@gmail.com>
Co-authored-by: Daniel McNally <mcnallydp@gmail.com>

* fix: tls certificate check on startup (#1510)

* fix: alias missing in streamorders (#1725) (#1962)

* fix(lnd): handling hold invoice check errors (#1969)

This adds better error handling for when the test calls to verify lnd
hold invoices are available fail due to connectivity reasons. Previously
any error that occurred at this step would cause us to set lnd's status
to `NoHoldInvoiceSupport` including connection issues. There was also a
bug that caused us to try to set the status to connected even when a
hold invoice status check failed.

This could result in the unusual behavior of status going to
`Disconnected` upon a call failing due to the grpc `UNAVAILABLE` error
status, then being set to `NoHoldInvoiceSupport` and then to
`ConnectionVerified`. Now we only set `NoHoldInvoiceSupport` when the
test calls fail for a reason other than `UNAVAILABLE`, and we only set
the status to `ConnectionVerified` when the hold invoice calls succeed.

Closes #1968.

* feat(lnd): SendPaymentV2

This migrates the call we use to send payments with lnd from the
deprecated `SendPaymentSync` to `SendPaymentV2` which allows for multi
path payments, among other improvements. As part of this change, the
lnd proto files have been updated to their v0.11.x versions and the
version of lnd used in simulation tests has been updated to v0.11.1 as
well.

Closes #1590.

* Revert "feat: removeorder output changed to a more meaningful message (#1526)"

* fix: use regtest instead of regnet arg

This fixes a bug where the xud flag to set the network was incorrectly
configured as `regnet` when it should be `regtest` to match what xud
expects.

* fix(lnd): don't calculate negative capacities

This fixes a bug in the logic for calculating the inbound & outbound
amount/capacity totals. We subtract the channel reserve amounts from the
balances when determining how large a payment the channel could support,
however we should not end up with a negative number.

* feat: new grpc call for subscribring alerts such as low balance (#864)

* fix: changes removeorder output to a more meaningful message (#1526) (#1986)

Co-authored-by: rsercano <ozdemirsercan27@gmail.com>
Co-authored-by: Daniel McNally <mcnallydp@gmail.com>
Co-authored-by: Karl Ranna <karl@karlranna.com>
Co-authored-by: Le Premier Homme <interjoint1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grpc gRPC API P2 mid priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No active orders from peers after adding new pair addpair/addcurrency requires xud restart
5 participants