feat: adding new flag to trade all possible quantity for buy/sell #1922
Conversation
rsercano
added
the
command line (CLI)
There was a problem hiding this comment.
I'm not able to issue a market buy all order when the peer order's offered quantity is higher than we can fill:
➜ xud git:(feature/buy-sell-all-grpc) ✗ ./bin/xucli orderbook BTC/DAI
Trading pair: BTC/DAI
┌───────────────────────────────────────┬───────────────────────────────────────┐
│ Buy │ Sell │
├───────────────────┬───────────────────┼───────────────────┬───────────────────┤
│ Quantity │ Price │ Price │ Quantity │
├───────────────────┼───────────────────┼───────────────────┼───────────────────┤
│ │ │ 100000 │ 0.02 │
└───────────────────┴───────────────────┴───────────────────┴───────────────────┘
➜ xud git:(feature/buy-sell-all-grpc) ✗ ./bin/xucli getbalance
Balance:
┌──────────┬───────────────┬────────────────────────────┬───────────────────────────────┐
│ Currency │ Total Balance │ Channel Balance (Tradable) │ Wallet Balance (Not Tradable) │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ BTC │ 465.55099411 │ 0.08608198 │ 465.46491213 │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ DAI │ 6000 │ 1500 │ 4500 │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ ETH │ 34.99859449 │ 10 │ 24.99859449 │
├──────────┼───────────────┼────────────────────────────┼───────────────────────────────┤
│ USDT │ 2000 │ 0 │ 2000 │
└──────────┴───────────────┴────────────────────────────┴───────────────────────────────┘
➜ xud git:(feature/buy-sell-all-grpc) ✗ ./bin/xucli buy all BTC/DAI mkt
9 FAILED_PRECONDITION: DAI outbound balance of 150000000000 is not sufficient for order amount of 200000000000
How should we behave for such cases? I thought it was okay, also are you able to do same without all command, by specifying a quantity? |
Signed-off-by: rsercano <[email protected]>
rsercano
force-pushed
the
feature/buy-sell-all-grpc
branch
from
2ccf461
to
fedfa8f
Compare
We should place an order with 1500 DAI, filling the existing order partially. |
Sorry if I overlook something but how, in the orderbook the price is 100.000 DAI for 0.02 BTC (2000 DAI), when we try to buy it all with 6.000 DAI (our balance) even actually we have 1.500 channel balance, we can't fill it, we don't have enough funds, so it throws the error, also it comes from the underlying buy command, nothing I changed. |
|
Looks there was a misunderstanding what issue #1877 is supposed to do: it is not about buying
(that your balance allows). Todo:
The behaviour of the order type |
|
Sorry for misunderstanding, will resolve asap. |
…ature/buy-sell-all-grpc
|
Implemented below queries with tests;
But for last one:
Does it mean market really? Or was it a typo? @kilrau |
|
Looks like a typo. |
|
Typo - fixed. Please make sure my logic checks out though @raladev |
rsercano
force-pushed
the
feature/buy-sell-all-grpc
branch
from
0c72ad3
to
1217930
Compare
|
Should be fixed now, can you please check @raladev |
…ature/buy-sell-all-grpc
rsercano
force-pushed
the
feature/buy-sell-all-grpc
branch
from
1217930
to
a4e50b4
Compare
For now in following case u will place
|
|
Thanks for checking @raladev, but I'm a bit confused:
|
max flag for 100 sat error is correct for buy 0 |
rsercano
force-pushed
the
feature/buy-sell-all-grpc
branch
from
cf2377e
to
e2ff8c2
Compare
|
@raladev I fixed logic of market orders for max, could you please recheck. So waiting confirmation for this. p.s. I rebased from master. |
|
|
I added exact same test case for BTC/USDT, it's getting calculated as 0.08276396689944529, can't get the conclusion you're at @raladev EDIT: 8276396.689944529 can this be in satoshis? Because it seems correct to me, it's 1000 (USDT) / 12082.5528 (Order price) |
There was a problem hiding this comment.
Sorry I'm late to the party here. I think by its nature this is a complicated feature to implement. Here are a few high level thoughts:
-
I don't understand why we're concerned with the swap client type when determining the max quantity?
-
The current implementation is still making lots of checks of the order book, as I understand that's for satisfying market orders but I think the logic becomes very complex and unreliable at that point. It's impractical to calculate the max quantity for a market buy order up front, since we don't know which orders in the order book will successfully swap with us until we actually try them. It's certainly possible to do this (maybe with a matching procedure that allows us to specify quantity in terms of quote currency units) but I don't think we should be moving a whole bunch of logic into Service.ts to do it or make any sweeping changes as part of this PR, instead I think it makes sense to reduce the scope of this PR and only allow the all/max flag in certain cases.
-
I'm concerned that routing fees are going to make this feature unreliable in the real world, if we're really trying to use every last satoshi in our channel then we won't have any left over for routing fees.
|
Agree with @sangaman . Let's do the following:
|
rsercano
force-pushed
the
feature/buy-sell-all-grpc
branch
from
b57f63f
to
c42d58c
Compare
because connext currencies have different way for max amount calculation because of not-real max-buy bound - #1922 (comment) |
Gotcha. If we want to ignore the connext maximum capacity since it can be dynamically increased, for purposes of the code/logic, I think it's better to make a method like Also worth noting that in such cases the order will fail with an error due to insufficient collateral and will say to try again in a little while, I think that's fine but wanted to make sure we were all aware. |
|
not talking about routing fees, but i cant swap for full amount even with directly connected lnd node and i dont know why @sangaman
|
I couldn't get how creating |
rsercano
force-pushed
the
feature/buy-sell-all-grpc
branch
from
a3910eb
to
42308cd
Compare
@sangaman could you please clarify what to do for point 3? I'm not sure if it's okay to pull order quantity check to here. @raladev for point 2 I still need clarification of how much of decimals to be truncated @sangaman |
No you shouldn't need any if statements, you would just assign your Another option is to make connext return an effectively unlimited amount for its inbound/buy trading limits, but I'm not sure that's a good idea, especially since we will reject connext buy orders that exceed the inbound limit.
Do you mean point 3 re: fees? I don't have a great solution for it, it's not a concern with direct channels like we're doing here for tests but it will be any time there are hops involved.
We don't use fractional amounts for I am still unsure about the usefulness or reliability of this feature overall... |
For feature reliability I'm not sure about mainnet too if we talk about other fees etc. But for current implementation I guess I couldn't explain it well. Currently maxBuyable for a swap client completely depends on quote's swap client and even if I implement |
|
Let me know if you guys want to continue on this or not please. |
Usefulness is described in the issue. Reliability - agreed. This won't be reliable without probing the route and accounting for exact fees. Which I imagine will turn out not too trivial since pre-match probing isn't a flow we currently have.
I think this PR has been open on for too long and proved not to be the quick-win we initially thought it will be. Reviews have been slow too. I am putting this into draft state and we can pick it up some point in the future. Sry @michael1011 |
|
Todo's when this is being picked up:
|
attempts to resolve #1877