Tucsky / SignificantTrades

better than 1 min chart
https://aggr.trade
GNU General Public License v3.0
621 stars 211 forks source link

OKex Trades not being grouped #47

Closed joyser closed 5 years ago

joyser commented 5 years ago

It was mentioned in issue #42 that the grouping was disabled for OKex trades for some reason? Now that we have V3 of the API can we enable it? Because as it stands, none of the OKex trades are being grouped.

For example, the following trades with the same timestamp are not being grouped, and wouldn't pass a 1,500 threshold even though the sell is larger than 1,500.

{"trade_id":"3110651246903329","side":"sell","price":"311.55","qty":"1215","timestamp":"2019-07-04T00:39:31.311Z"},{"trade_id":"3110651246903327","side":"sell","price":"311.561","qty":"40","timestamp":"2019-07-04T00:39:31.311Z"},{"trade_id":"3110651246903325","side":"sell","price":"311.588","qty":"252","timestamp":"2019-07-04T00:39:31.311Z"},{"trade_id":"3110651246903323","side":"sell","price":"311.602","qty":"380","timestamp":"2019-07-04T00:39:31.311Z"},

joyser commented 5 years ago

Are trades grouped continuously or only when they are received within the same packet from the web socket? Because OKex seems to send a new packet for each trade, even if they are for the same millisecond.

joyser commented 5 years ago

If the problem is due to the above, I suggest we could have a "holding" stack of trades in the OKex.js class. Every trade received goes into the holding stack, until either a) The next trade received has a different timestamp (release the held trades and add the new trade to the stack, or b) 10 milliseconds elapses without a trade received. When either condition is true, the held trades are pushed to the emit().

What do you think?

Tucsky commented 5 years ago

only when they are received within the same packet

this

Have you tried this setting ? image This basically do what you want, it hold the trades until total amount for thoses trades reach the threshold, or skip them if {trade timestamp} - {stack timestamp} > 10ms (the value I set above)

Down side of this is that it does not trigger a sound because atm, the trades that have been stacked might already have been heard (single bip), that feature is done in the tradelist component.

10ms sounds fast enough for that logic to be made in the low parts of the app tho, im considering adding that to the "groupTrades" function.

joyser commented 5 years ago

This is very interesting. I have two thoughts on it which might not be accurate, but maybe you can clarify:

1) It seems in the trades list component that the stack is released as soon as the threshold is reached? So if I have a string of trades like 500 - 990 - 500 - 900 and the threshold is 1000, then the first two trades will trigger the stack release, and the second two trades will release another stack even though they might all be part of the same order?

2) In the lag aggregation, it doesn't account for whether the timestamp of the order is the same or different? Therefore a stop loss could be triggered, which sends two trades within milliseconds of each other, but they shouldn't be aggregated, ideally.

joyser commented 5 years ago

10ms sounds fast enough for that logic to be made in the low parts of the app tho, im considering adding that to the "groupTrades" function.

I thought 10ms would be fast enough to get it, but testing using the aggregation lag, OKex is only getting grouped when I set it to 500ms or more ! Not sure if this is due to the websocket or some delays in the higher level of the code.

Tucsky commented 5 years ago

Not sure if this is due to the websocket or some delays in the higher level of the code.

That might be a bug, just checked and the app does {current time} - {stack timestamp} > 10ms instead of {trade timestamp} - {stack timestamp} > 10ms

Tucsky commented 5 years ago

It seems in the trades list component that the stack is released as soon as the threshold is reached

yes it is

Tucsky commented 5 years ago

I changed the timestamp comparison using the trade timestamp let me know if thats better

joyser commented 5 years ago

I think it should be {current time} - {stack timestamp} > 10ms || {trade timestamp} != {stack timestamp}?

We're trying to account for the lag in the trades received client side, after all, not the lag in the matching engine.

Edit: Updated the conditional statement (forgot it was to trigger the delete)

joyser commented 5 years ago

We're trying to account for the lag in the trades received client side, after all, not the lag in the matching engine.

It depends if this is the goal of the aggregation lag though? If not, then better to implement the solution in the okex.js I think.

Did you build the aggregationLag just to fix the OKex issue? or is it used to solve something with the other exchanges?

joyser commented 5 years ago

Maybe something like this

I haven't been able to test it but hopefully you can see what I'm trying to do.

Edit: Fixed a bug in my code and updated link.