Trading Test Cases (WIP) #638

kilrau · 2018-11-05T16:50:31Z

Execute xucli buy 0.004 LTC/BTC 1.101

on these 10 default sell orders in the xud-simnet:

            "price": 1.14,
            "quantity": 0.003,

            "price": 1.14,
            "quantity": 0.003,

            "price": 1.13,
            "quantity": 0.0025,

            "price": 1.13,
            "quantity": 0.0025,
            
            "price": 1.12,
            "quantity": 0.002,

            "price": 1.12,
            "quantity": 0.002,

            "price": 1.11,
            "quantity": 0.0015,
            
            "price": 1.11,
            "quantity": 0.0015,
            
            "price": 1.1,
            "quantity": 0.001,
            
            "price": 1.1,
            "quantity": 0.001,

  "swapResultsList": [
    {
      "orderId": "214e7221-e0e2-11e8-b273-cf0c3bb20129",
      "localId": "d51cfde0-e0f0-11e8-a2c0-c5b61dce1c90",
      "pairId": "LTC/BTC",
      "quantity": 0.001,
      "rHash": "3011db4ec2ff8ab961f582d944193654aa209b58799dbc9945e3c9c2df8d84c5",
      "amountReceived": 100000,
      "amountSent": 110000,
      "peerPubKey": "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0",
      "role": 0
    }
  ],
  "remainingOrder": {
    "price": 1.101,
    "quantity": 0.003,
    "pairId": "LTC/BTC",
    "id": "d51cfde1-e0f0-11e8-a2c0-c5b61dce1c90",
    "peerPubKey": "",
    "localId": "d51cfde0-e0f0-11e8-a2c0-c5b61dce1c90",
    "createdAt": 1541418560702,
    "side": 0,
    "isOwnOrder": true
  }
}

BEFORE:

 lndbtc-lncli channelbalance
    "balance": "5000000",
lndltc-lncli channelbalance
    "balance": "5000000",

AFTER (as is):

lndbtc-lncli channelbalance
    "balance": "4669800",
lndltc-lncli channelbalance
    "balance": "5300000",

Swap result indicates 0.001 succeeded, 0.003 remaining which is what happened in reality but
Bug01: NOT the case when looking into the order book after the swap. Both matching orders for in total 0.002 BTC were removed (even though one swap definitely failed), and only 0.001 remaining order was added whereas it should 0.003.
Bug02: Shouldn't the lndbtc balance be 5000000−(1.101×300000)=4669700?
Bug03: The lndltc balance defifinitely should be 5100000, not 5300000

AFTER (#612 how it should be):

lndbtc-lncli channelbalance
    "balance": "4669700",
lndltc-lncli channelbalance
    "balance": "5100000",

The text was updated successfully, but these errors were encountered:

moshababo · 2018-11-06T12:31:58Z

Swap result indicates 0.001 succeeded, 0.003 remaining which is what happened in reality but
Bug01: NOT the case when looking into the order book after the swap. Both matching orders for in total 0.002 BTC were removed (even though one swap definitely failed), and only 0.001 remaining order was added whereas it should 0.003.

Both matching sell orders to be removed is fine is one of them failed in a swap, while the other succeed. is that the case?
You're saying that your remainingOrder quantity doesn't match what's added in the order book? I'll try to reproduce that.

Bug02: Shouldn't the lndbtc balance be 5000000−(1.101×300000)=4669700?

Mind that the price is taken from the maker order, not the taker. So it's 1.1 and not 1.101.
The swap deal amounts looks fine:

    "amountReceived": 100000, (quantity * 100000000)
    "amountSent": 110000, (quantity * price * 100000000)

so you were supposed to send 110000 satoshis (lndbtc), and recieve 100000 litoshi (lndltc). you said you were expecting to pay 5000000−(1.101×300000)=4669700 satoshis, so I guess you're referring to a quantity of 0.003, and not 0.001. If so, you could have expect 5000000−(1.1×300000)=4670000, but you got 4669800, so you paid extra 200 satoshis. @offerm

Bug03: The lndltc balance defifinitely should be 5100000, not 5300000

It makes sense to be 5300000, because the lndbtc balances implies that you executed 0.003 quantity, and not 0.001.

kilrau · 2018-11-06T15:21:44Z

Swap result indicates 0.001 succeeded, 0.003 remaining which is what happened in reality but
Bug01: NOT the case when looking into the order book after the swap. Both matching orders for in total 0.002 BTC were removed (even though one swap definitely failed), and only 0.001 remaining order was added whereas it should 0.003.

Both matching sell orders to be removed is fine is one of them failed in a swap, while the other succeed. is that the case?

You're saying that your remainingOrder quantity doesn't match what's added in the order book? I'll try to reproduce that.

Yes I am pretty sure only one swap succeeded, the other failed. Reason: only one server was capable of swaping at the time of my test and see swapResult above. And yes, removing both sell orders in this case is correct, sorry if that reads differently. What is NOT correct: 0.003 remaining order should be added to the order book, whereas only 0.001 get added. Very likely related to Bug03.

Bug02: Shouldn't the lndbtc balance be 5000000−(1.101×300000)=4669700?

Mind that the price is taken from the maker order, not the taker. So it's 1.1 and not 1.101.
The swap deal amounts looks fine:

Damn, yes of course. Ignore.

Bug03: The lndltc balance defifinitely should be 5100000, not 5300000

It makes sense to be 5300000, because the lndbtc balances implies that you executed 0.003 quantity, and not 0.001.

I am almost 100% sure I only executed 0.001 quantity since only one server in our test cloud with 0.001 quantity was actually swap enabled.

offerm · 2018-11-06T15:23:43Z

BUG1 - no problem here. 2 orders removed, one for execution and one for failed execution. Remaining order is 0.003 as it should.

The wrong balances are a result of 3 swaps and not one.

The taker initiates a swap for 0.001 and create a remaining order of 0.003. The remaining order is sent to the peer (maker) where it is match with the same order that is being swapped (there is no check for hold in matching engine). as a result the maker starts a swap of 0.001 and creates a remaining order of 0.002 which is sent to the taker and matched again.

I would start solving this by properly using and checking the hold. Amount should be held when we accept the swap but also when we suggest the swap. Matching engine should check it.

kilrau · 2018-11-06T15:26:23Z

Remaining order is 0.003 as it should.

It says remaining order 0.003 in the SwapResult which is correct, but actually only adds 0.001 to the order book (run getOrders after swap). That's at least how it was for me. Please reproduce.

offerm · 2018-11-06T15:27:21Z

again, the first match added 0.003 but there are another 2 swaps of 0.001 each.
As a result, you left with 0.001.

kilrau · 2018-11-06T15:35:22Z

Ok, gotcha. @offerm Sound's like #637 should solve this?

offerm · 2018-11-06T15:36:14Z

There were 2 swaps at print 1.1 and one swap at 1.101. This is why the calculation does not work out.

offerm · 2018-11-06T15:38:10Z

@kilrau #637 will not solve it completely.

Holding is missing when initiating the swap deal.
matching engine does not check the amount held when matching.
Amounts should be presented as part of order book

kilrau · 2018-11-06T15:58:08Z

Ok: #637 + #640 should theoretically close this.

sangaman · 2018-11-06T16:10:40Z

We may have to discuss this on the call today but there's no concept of putting orders on hold during the taker matching process.

kilrau · 2018-11-07T10:08:10Z

We may have to discuss this on the call today but there's no concept of putting orders on hold during the taker matching process.

We will never put orders on-hold since this is incompatible with how order book exchanges currently work (at least in nomatching mode). No orders get put on hold, amounts in the payment channels get put on hold (and released if failed). Related discussion here. But I see it's getting confusing here, I'll draw an EPC to hopefully have a better overview. Will post it here first.

offerm · 2018-11-07T12:14:34Z

OK. Need more time on this one to better understand the root cause.

It might be the fact the the matching engine is not using a hold when matching.

On the other hand, as I understand, the matching engine should never match a peer order with and existing own order, is this correct @moshababo ?

Regardless, I will dive into it and find the reason

sangaman · 2018-11-07T14:48:25Z

On the other hand, as I understand, the matching engine should never match a peer order with and existing own order, is this correct @moshababo ?

This is correct. I rechecked the code and I'm not seeing how this could happen.

offerm · 2018-11-07T15:37:15Z

So, here is what going on in #638:

The maker (nodeA) created 5 buy orders and 5 sell orders

The taker (nodeB) is sending xucli sell 0.004 LTC/BTC 0.895 aiming at the maker's buy 0.001 LTC/BTC 0.9 (part of the 10 orders).

There is a match for 0.001 (swap) and a reminder order of 0.003 is created on NodeB and added to order book.

The reminder order is sent to node A and added to the order book

The swap (#1) is over and the maker order (nodeA) is removed from the book.

Now, a new player steps in, the trading bot which issue a replace for the traded order - buy 0.001 LTC/BTC 0.9. This new order starts a swap where nodeA is now the taker and node B is the maker.
0.001 is held on the order in node B.

Swap (#2) is working fine. Quantity in node B is reduced to 0.002.

The bot order is removed on Node A.

Now, the bot issue another order buy 0.001 LTC/BTC 0.9 which is matched with the peer order and swap staring.
0.001 is held, making total held to be 0.002.

Swap (#3) is working fine. Quantity in node B is reduced to 0.001.

The bot order is removed on Node A.

Now, the bot issue another order buy 0.001 LTC/BTC 0.9 which is matched with the peer order and swap staring.

This time, there is no availability at node B (quantity is 0.001 buy 0.002 is hed).
swap fails with OrderUnavilable

As a result, on node B there is an own order of 0.001 (can't be traded). 3 swaps done.

Action items: #626, #649

kilrau · 2018-11-08T12:36:50Z

Understood. Thanks for the comprehensive summary! As I see some small changes on #626 then merge, release and we should be able to test this again @offerm @sangaman

offerm · 2018-11-09T10:28:42Z

So, after #626 installed, the command is working it should. It is still very confusing to the user:

the user see a swap of 0.001 at 1.1 and remaining order of 0.003 at 1.101
checking getorders will not show the reminder order (since it will trade agains the bot new orders)
after trading the 4 0.001 orders, the bot will issue another sell order so from the user point of view nothing changed.

getorder should be imported:

should be sorted by price
should include the time of the to order so it will be easy to see that this is a new order.

We should consider to change the bot to be more dynamic with new orders.

When checking streamorders you get this:

Order removed: {"quantity":0.001,"pairId":"LTC/BTC","orderId":"68ab65c1-e406-11e8-954b-61efc8b39816","localId":"","isOwnOrder":false}
Order added: {"price":1.101,"quantity":0.003,"pairId":"LTC/BTC","id":"69f0a511-e408-11e8-b089-ed5c3daa7fe5","peerPubKey":"","localId":"69f0a510-e408-11e8-b089-ed5c3daa7fe5","createdAt":1541758542305,"side":0,"isOwnOrder":true}
Order removed: {"quantity":0.001,"pairId":"LTC/BTC","orderId":"69f0a511-e408-11e8-b089-ed5c3daa7fe5","localId":"","isOwnOrder":false}
Order swapped: {"orderId":"69f0a511-e408-11e8-b089-ed5c3daa7fe5","localId":"69f0a510-e408-11e8-b089-ed5c3daa7fe5","pairId":"LTC/BTC","quantity":0.001,"rHash":"7669c2c87ff3dacf4dc3bfab83f7bb74a4ab4987aef53d45d757f58648c4e32e","amountReceived":100000,"amountSent":110100,"peerPubKey":"03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0","role":1}
Order removed: {"quantity":0.001,"pairId":"LTC/BTC","orderId":"69f0a511-e408-11e8-b089-ed5c3daa7fe5","localId":"","isOwnOrder":false}
Order swapped: {"orderId":"69f0a511-e408-11e8-b089-ed5c3daa7fe5","localId":"69f0a510-e408-11e8-b089-ed5c3daa7fe5","pairId":"LTC/BTC","quantity":0.001,"rHash":"8a0329cb516812a1e2ba0279e6173b0c7171e8b25be1cd7349827fb750fa2578","amountReceived":100000,"amountSent":110100,"peerPubKey":"03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0","role":1}
Order added: {"price":1.1,"quantity":0.001,"pairId":"LTC/BTC","id":"6c436eb1-e408-11e8-954b-61efc8b39816","peerPubKey":"03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0","localId":"","createdAt":1541758547775,"side":1,"isOwnOrder":false}
Order removed: {"quantity":0.001,"pairId":"LTC/BTC","orderId":"69f0a511-e408-11e8-b089-ed5c3daa7fe5","localId":"","isOwnOrder":false}
Order swapped: {"orderId":"69f0a511-e408-11e8-b089-ed5c3daa7fe5","localId":"69f0a510-e408-11e8-b089-ed5c3daa7fe5","pairId":"LTC/BTC","quantity":0.001,"rHash":"2dbdae4acfce55019fd3114e056cd52c403803a4c1acf33cca70db24dc3382c9","amountReceived":100000,"amountSent":110100,"peerPubKey":"03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0","role":1}

Easy to see :

we are missing one swap even (should have 4)
same order is removed 3 times.

So, still a bug but not top priority any more.

sangaman · 2018-11-10T05:37:31Z

We have an issue to sort the getorders table by price. As for displaying the time of the order, my feeling is it's not important information to a user who's just trying to see what orders are out there, but you can see createdAt for each order in the json output. Does that work for you?

It's not unusual for the same order to be "removed" multiple times, since each time could just be removing only part of the order. I know it's a bit confusing terminology, but it was tricky to get right and it's what we went with. Maybe the cli output can just be tweaked to say "Order quantity removed" - but either way at the end of the day I don't think this command will be used by end users.

I'm confused as to why we'd only show 3 swaps when we should have 4, are the steps to reproduce just xucli sell 0.004 LTC/BTC 0.895 on one node and buy 0.001 LTC/BTC 0.9 4 times on another?

offerm · 2018-11-11T06:44:23Z

@sangaman createdAt is not a fine solution. I explained the motivation above. I want to make sure that this is not the same trade as I saw before (and no, I don't want to remember the ID....).
The problem with createdAtis its format. If it was a string datetime format it could be a solution. I also think it should get into the table as it gives information on how static are the orders.

order quantity removed has a different semantic form order removed. We should reflect that.

To reproduce setup a simnet machine against the cloud servers and use xucli sell 0.004 LTC/BTC 0.895.

@sangaman
A side note - I think you underestimate the value of the CLI commands. I agree with you that in a production system the CLI will not be heavily used. However, there is a long way until XUD is in production. The CLI commands are important as:

They are easy to use for "playing with " and "getting know " the system
They explain what can be done with RPC calls. If you see an interesting option on the CLI you can be sure that it is supported by one or more RPC calls.
They show how thinks can be done
They ensure that the RPC layer has full support for 3rd party application. For example, it today we don't show the trade price on swapResultsList it might be that we don't have this element in the RPC level.
They help us to decide where things should be implemented (CLI side ot server side). For example, sorting the orderbook by price - should it be done at the CLI level or at the server-rpc level?
They can be used by developers in environments that do not support gRPC, bash for example. Note that our simnet is currently using LND's CLI and BTCD/LTCD CLI.

With all the above, I think we should, again, take an example from LND and make sure we have a very strong an solid CLI interface that can be used for the above points.

kilrau · 2018-11-11T10:52:35Z

We all agree with you on the importance of the CLI (!) @offerm and let's continue open issues for improving it!

Maybe let's start with "change the bot to be more dynamic with new orders" in a way, that once an order is completely filled, the bot issues a new order e.g. 1% above/below the previous order. This will clear things a lot to not mistake it with the "same order". Can you do that?

sangaman · 2018-11-11T23:04:14Z

The problem with createdAtis its format. If it was a string datetime format it could be a solution.

It would be quite easy to add a createdAtDate field or something to the JSON output which is the toLocaleTimeString of createdAt, if that works for you I can open an issue. FWIW the cli for lnd is printing all time stamps as epochs as far as I know.

offerm · 2018-11-12T06:36:55Z

No need to change the RPC layer for it. showing the data as date string has to do with presentation layer. It should be done on the CLI level.

…

On Mon, Nov 12, 2018 at 1:04 AM Daniel McNally ***@***.***> wrote: The problem with createdAtis its format. If it was a string datetime format it could be a solution. It would be quite easy to add a createdAtDate field or something to the JSON output which is the toLocaleTimeString of createdAt, if that works for you I can open an issue. FWIW the cli for lnd is printing all time stamps as epochs as far as I know. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#638 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJQ0cl0QwhzZimZeJjS015klxnO4yKc2ks5uuKzvgaJpZM4YO1qg> .

sangaman · 2018-11-12T12:33:20Z

Yes I didn't say to change the RPC layer, the cli can just add that field to the JSON output that it prints to console.

offerm · 2018-11-17T13:32:09Z

Is there a way to see everything happened to an order? getOrder?

kilrau added the P1 top priority label Nov 5, 2018

kilrau added this to the 1.0.0-alpha.3 milestone Nov 5, 2018

kilrau assigned offerm, moshababo and kilrau Nov 5, 2018

kilrau mentioned this issue Nov 5, 2018

Trading issue #612

Closed

kilrau modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 6, 2018

kilrau added the swaps label Nov 6, 2018

kilrau modified the milestones: 1.0.0-alpha.4, 1.0.0-alpha.5 Nov 16, 2018

kilrau changed the title ~~Test Case for #612~~ Trading Test Cases (WIP) Nov 18, 2018

offerm mentioned this issue Nov 18, 2018

New CLI - ListOrderHistory #689

Closed

kilrau modified the milestones: 1.0.0-alpha.5, 1.0.0-alpha.6 Dec 5, 2018

kilrau closed this as completed Dec 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trading Test Cases (WIP) #638

Trading Test Cases (WIP) #638

kilrau commented Nov 5, 2018 •

edited

Loading

moshababo commented Nov 6, 2018

kilrau commented Nov 6, 2018 •

edited

Loading

offerm commented Nov 6, 2018

kilrau commented Nov 6, 2018

offerm commented Nov 6, 2018

kilrau commented Nov 6, 2018

offerm commented Nov 6, 2018

offerm commented Nov 6, 2018

kilrau commented Nov 6, 2018 •

edited

Loading

sangaman commented Nov 6, 2018

kilrau commented Nov 7, 2018 •

edited

Loading

offerm commented Nov 7, 2018

sangaman commented Nov 7, 2018

offerm commented Nov 7, 2018

kilrau commented Nov 8, 2018

offerm commented Nov 9, 2018

sangaman commented Nov 10, 2018

offerm commented Nov 11, 2018

kilrau commented Nov 11, 2018

sangaman commented Nov 11, 2018

offerm commented Nov 12, 2018 via email

sangaman commented Nov 12, 2018

offerm commented Nov 17, 2018

Trading Test Cases (WIP) #638

Trading Test Cases (WIP) #638

Comments

kilrau commented Nov 5, 2018 • edited Loading

moshababo commented Nov 6, 2018

kilrau commented Nov 6, 2018 • edited Loading

offerm commented Nov 6, 2018

kilrau commented Nov 6, 2018

offerm commented Nov 6, 2018

kilrau commented Nov 6, 2018

offerm commented Nov 6, 2018

offerm commented Nov 6, 2018

kilrau commented Nov 6, 2018 • edited Loading

sangaman commented Nov 6, 2018

kilrau commented Nov 7, 2018 • edited Loading

offerm commented Nov 7, 2018

sangaman commented Nov 7, 2018

offerm commented Nov 7, 2018

kilrau commented Nov 8, 2018

offerm commented Nov 9, 2018

sangaman commented Nov 10, 2018

offerm commented Nov 11, 2018

kilrau commented Nov 11, 2018

sangaman commented Nov 11, 2018

offerm commented Nov 12, 2018 via email

sangaman commented Nov 12, 2018

offerm commented Nov 17, 2018

kilrau commented Nov 5, 2018 •

edited

Loading

kilrau commented Nov 6, 2018 •

edited

Loading

kilrau commented Nov 6, 2018 •

edited

Loading

kilrau commented Nov 7, 2018 •

edited

Loading