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

sangaman · 2020-10-01T06:19:14Z

This adds a field to the GetBalance response to return the balance that is reserved for open orders.

Related issue #1584 - before closing it though I think we'll need another PR that enforces the reserved outbound amount before accepting new orders. As is currently it's possible to have more outbound balance reserved than you have available if you keep adding orders in excess of your balance.

This is in draft status until #1916 is merged. I haven't tested this manually yet but it should be ready for testing.

kilrau · 2020-10-01T10:26:00Z

Related issue #1584 - before closing it though I think we'll need another PR that enforces the reserved outbound amount before accepting new orders.

Agreed and good thinking, I didn't actually mention this in the issue 👍 Can you take care of this PR too?

sangaman · 2020-10-01T13:55:09Z

Agreed and good thinking, I didn't actually mention this in the issue +1 Can you take care of this PR too?

Yes that's what I'm thinking, but it probably makes sense to keep it separate.

raladev · 2020-10-05T08:56:48Z

@sangaman can u please fix conflicts?

for now it does not work -_-
Screenshot from 2020-10-05 16-24-18
Screenshot from 2020-10-05 16-24-00

sangaman · 2020-10-09T06:51:24Z

@sangaman can u please fix conflicts?

* [ ]  for now it does not work -_-
  [Screenshot from 2020-10-05 16-24-18](https://user-images.githubusercontent.com/29906866/95085013-510f2c80-0727-11eb-9d6a-15887e158057.png)
  [Screenshot from 2020-10-05 16-24-00](https://user-images.githubusercontent.com/29906866/95085018-52405980-0727-11eb-8aa4-a9aea62a4bf8.png)

I fixed the conflicts, but I'm pretty stumped as to why this didn't work for you. I added some test cases as well as a simulation test assertion that verifies that GetBalance returns the expected reserved amount after placing an order and then after removing it.

Could you try testing again real quick and if you get the same result could you upload your log? I added some temporary logging statements that might be helpful to figure out what's going on.

raladev · 2020-10-09T13:36:26Z

i see reserve log lines but nothing changed in getbalance response
Screenshot from 2020-10-09 16-33-14
Screenshot from 2020-10-09 16-33-10

reserve.log

09/10/2020 13:31:13.073 [ORDERBOOK] trace: order added: {"pairId":"BTC/USDT","price":11000,"quantity":5000000,"isBuy":false,"localId":"1","id":"b32eca10-0a33-11eb-9376-bbbedc4e2d67","initialQuantity":5000000,"hold":0,"createdAt":1602250273073}
09/10/2020 13:31:13.074 [ORDERBOOK] trace: added 5000000 to BTC reserved amount, new amount is 5000000
09/10/2020 13:31:13.076 [RPC] trace: call /xudrpc.Xud/PlaceOrder succeeded


09/10/2020 13:32:23.268 [ORDERBOOK] trace: order added: {"pairId":"BTC/USDT","price":10000,"quantity":3000000,"isBuy":true,"localId":"2","id":"dd05b240-0a33-11eb-9376-bbbedc4e2d67","initialQuantity":3000000,"hold":0,"createdAt":1602250343268}
09/10/2020 13:32:23.268 [ORDERBOOK] trace: added 30000000000 to USDT reserved amount, new amount is 30000000000

raladev

above

sangaman · 2020-10-09T15:32:54Z

OK I figured it out, I was setting the reserved balance properly when a single currency was specified for the GetBalance call - but when retrieving the balance for all currencies it wasn't being set. My tests were only using the single currency case and yours was using multi-currency, which is why only you were getting any problems. I've added a test case for fetching reserved balances for multiple currencies and fixed the underlying issue, it should work for you now. Thanks again for the tests @raladev.

raladev

Approved, but in follow up PR with channel balance check we need to:

add in order amount in GetBalance table;
think about behavoiur with closed channel - should we remove orders if channel was partially or fully closed?

kilrau · 2020-10-12T13:56:02Z

* should we remove orders if channel was partially or fully closed?

I would say no for now, sounds like this could lead to unintended behaviour. In the case of arby, the next order issuance will account for the reduced balance, so shouldn't be a problem.

sangaman · 2020-10-12T13:59:27Z

* add `in order` amount in GetBalance table;

Is it not there for you currently in the parentheses? Or are you suggesting it should be in a separate column? I can definitely change the formatting of the table in this PR if we'd like, I just tried to follow the example in the original issue.

* should we remove orders if channel was partially or fully closed?
I would say no for now, sounds like this could lead to unintended behaviour. In the case of arby, the next order issuance will account for the reduced balance, so shouldn't be a problem.

I agree, auto cancelling orders would be fairly complicated and I'm not sure it would be desirable.

raladev · 2020-10-12T14:16:21Z

Is it not there for you currently in the parentheses?

for now there is definitely nothing in getbalance table.

kilrau · 2020-10-12T14:30:45Z

Is it not there for you currently in the parentheses?

* [ ]  for now there is definitely nothing in getbalance table.
  ![Screenshot from 2020-10-12 17-15-05](https://user-images.githubusercontent.com/29906866/95756586-a19a0300-0cae-11eb-944e-1d6c198248fe.png)

As just discussed, we want this to be in for this PR

sangaman · 2020-10-12T16:57:03Z

I suspect this may have to do with the xucli you're running in docker not using the same branch as what xud is running - could that be the case @raladev? Based on the code and some quick tests it should be outputting like 0.0098 (0.0002 in orders) in the table.

raladev · 2020-10-13T11:51:47Z

I suspect this may have to do with the xucli you're running in docker not using the same branch as what xud is running - could that be the case @raladev? Based on the code and some quick tests it should be outputting like 0.0098 (0.0002 in orders) in the table.

not sure how it can be in docker env, but now i also checked clean native fresh compiled xud and got same result.

ghost

utACK. Looks good code wise - nice test coverage 👍

This adds a field to the `GetBalance` response to return the balance that is reserved for open orders.

sangaman · 2020-10-13T17:27:11Z

I finally figured out why it wasn't displaying properly, so I fixed that and as per our discussions I moved the reserved In Orders balance to a separate columns. I also added a suite of tests to make sure the getbalance formatted CLI output looks proper.

┌──────────┬───────────────┬──────────────────────────────────────────┬────────────────────────────┬───────────┐
│ Currency │ Total Balance │ Wallet Balance (Not Tradable)            │ Channel Balance (Tradable) │ In Orders │
├──────────┼───────────────┼──────────────────────────────────────────┼────────────────────────────┼───────────┤
│ BTC      │ 0.005         │ 0.004 (0.00075 pending | 0.001 inactive) │ 0.001 (0.00025 pending)    │ 0.0008    │
└──────────┴───────────────┴──────────────────────────────────────────┴────────────────────────────┴───────────┘

raladev

May be merged after simulation tests passing

sangaman added the grpc gRPC API label Oct 1, 2020

sangaman requested review from a user and kilrau October 1, 2020 06:19

sangaman self-assigned this Oct 1, 2020

sangaman force-pushed the rpc/reserved-amounts branch from 4cca27b to b08502d Compare October 1, 2020 06:25

kilrau requested a review from raladev October 1, 2020 10:25

kilrau marked this pull request as ready for review October 5, 2020 08:02

sangaman force-pushed the rpc/reserved-amounts branch from 43b0051 to 5641d1d Compare October 9, 2020 06:49

raladev requested changes Oct 9, 2020

View reviewed changes

sangaman force-pushed the rpc/reserved-amounts branch from 5641d1d to 2be8d45 Compare October 9, 2020 15:31

sangaman requested a review from raladev October 9, 2020 17:16

raladev previously approved these changes Oct 12, 2020

View reviewed changes

raladev self-requested a review October 12, 2020 14:07

ghost reviewed Oct 13, 2020

View reviewed changes

feat(rpc): expose reserved balance for GetBalance

165546e

This adds a field to the `GetBalance` response to return the balance that is reserved for open orders.

sangaman dismissed raladev’s stale review via 165546e October 13, 2020 17:24

sangaman force-pushed the rpc/reserved-amounts branch from 2be8d45 to 165546e Compare October 13, 2020 17:24

raladev approved these changes Oct 13, 2020

View reviewed changes

raladev merged commit 8b18dd7 into master Oct 13, 2020

sangaman mentioned this pull request Oct 14, 2020

values of wallet and channel columns are mixed up (GetBalance call) #1930

Closed

sangaman deleted the rpc/reserved-amounts branch October 14, 2020 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sangaman commented Oct 1, 2020

kilrau commented Oct 1, 2020

sangaman commented Oct 1, 2020

raladev commented Oct 5, 2020 •

edited

Loading

sangaman commented Oct 9, 2020

raladev commented Oct 9, 2020 •

edited

Loading

raladev left a comment

sangaman commented Oct 9, 2020

raladev left a comment

kilrau commented Oct 12, 2020

sangaman commented Oct 12, 2020

raladev commented Oct 12, 2020

kilrau commented Oct 12, 2020

sangaman commented Oct 12, 2020

raladev commented Oct 13, 2020

ghost left a comment

sangaman commented Oct 13, 2020

raladev left a comment

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

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

Conversation

sangaman commented Oct 1, 2020

kilrau commented Oct 1, 2020

sangaman commented Oct 1, 2020

raladev commented Oct 5, 2020 • edited Loading

sangaman commented Oct 9, 2020

raladev commented Oct 9, 2020 • edited Loading

raladev left a comment

Choose a reason for hiding this comment

sangaman commented Oct 9, 2020

raladev left a comment

Choose a reason for hiding this comment

kilrau commented Oct 12, 2020

sangaman commented Oct 12, 2020

raladev commented Oct 12, 2020

kilrau commented Oct 12, 2020

sangaman commented Oct 12, 2020

raladev commented Oct 13, 2020

ghost left a comment

Choose a reason for hiding this comment

sangaman commented Oct 13, 2020

raladev left a comment

Choose a reason for hiding this comment

raladev commented Oct 5, 2020 •

edited

Loading

raladev commented Oct 9, 2020 •

edited

Loading