altangent / ccxws

WebSocket client for 38 cryptocurrency exchanges
MIT License
617 stars 186 forks source link

There are some bugs in the Kucoin exchange #196

Closed WoutervDijk closed 4 years ago

WoutervDijk commented 4 years ago

Exchange Kucoin Subscription type Level2orderbook Describe the bug The l2snapshot event doesn't emit the market, like all other exchanges do. This is a simple fix at line 399: this.emit("l2snapshot", snapshot); -> this.emit("l2snapshot", snapshot, market);

Second bug / feature is that with Kucoin or the code emits orderbook updates one by one? Simple test is to do the following: kucoin.on("l2update", (update, market) => { console.log("Handling response"); setTimeout(()=>console.log("After response"), 0); });` And then you often see multiple "handling response" without an after response after? It also seems weird if kucoin sends every orderbook update one by one? While documentation suggests multiple updates might come in one message? https://docs.kucoin.com/#level-2-market-data

WoutervDijk commented 4 years ago

Extra issue I would like to add is that in this Kucoin module, the "sequenceLast" property of L2update is inconsistent with other exchanges. For example it is called lastSequenceId in the Binance module. Maybe unify it?

bmancini55 commented 4 years ago

Thanks for submitting the issues.

We will get a fix together that address inclusion of market in the l2snapshot and add a lastSequenceId property to l2update to bring it inline with Binance.

For the l2updates it looks to be on Kucoin's end. The raw captured messages look to be single updates (shown below). I also ran a capture a good 15 minutes did not detect any events that had more than a single update.

If you have any other information and/or could reproduce an l2 update stream broadcasting more than a single update, that would be a help.

{
  "data": {
    "sequenceStart": "1594757119953",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [],
      "bids": [
        [
          "11841.7",
          "0.48421861",
          "1594757119953"
        ]
      ]
    },
    "sequenceEnd": "1594757119953"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119954",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [
        [
          "0",
          "0",
          "1594757119954"
        ]
      ],
      "bids": []
    },
    "sequenceEnd": "1594757119954"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119955",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [
        [
          "11860.7",
          "0.53421861",
          "1594757119955"
        ]
      ],
      "bids": []
    },
    "sequenceEnd": "1594757119955"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119956",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [],
      "bids": [
        [
          "0",
          "0",
          "1594757119956"
        ]
      ]
    },
    "sequenceEnd": "1594757119956"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119957",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [],
      "bids": [
        [
          "11847.3",
          "0.15572563",
          "1594757119957"
        ]
      ]
    },
    "sequenceEnd": "1594757119957"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119958",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [],
      "bids": [
        [
          "0",
          "0",
          "1594757119958"
        ]
      ]
    },
    "sequenceEnd": "1594757119958"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119959",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [],
      "bids": [
        [
          "11838.1",
          "1.09814917",
          "1594757119959"
        ]
      ]
    },
    "sequenceEnd": "1594757119959"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119960",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [
        [
          "0",
          "0",
          "1594757119960"
        ]
      ],
      "bids": []
    },
    "sequenceEnd": "1594757119960"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119961",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [
        [
          "0",
          "0",
          "1594757119961"
        ]
      ],
      "bids": []
    },
    "sequenceEnd": "1594757119961"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119962",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [
        [
          "0",
          "0",
          "1594757119962"
        ]
      ],
      "bids": []
    },
    "sequenceEnd": "1594757119962"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
{
  "data": {
    "sequenceStart": "1594757119963",
    "symbol": "BTC-USDT",
    "changes": {
      "asks": [
        [
          "11857.2",
          "0.34865543",
          "1594757119963"
        ]
      ],
      "bids": []
    },
    "sequenceEnd": "1594757119963"
  },
  "subject": "trade.l2update",
  "topic": "/market/level2:BTC-USDT",
  "type": "message"
}
WoutervDijk commented 4 years ago

Thank you for your fast response. Great to hear these two things will be addressed! I think you are right about Kucoin, which sucks a little. On the other hand, you get really fast data I guess. I might just throttle it myself.

bmancini55 commented 4 years ago

Yeah it's nice if you want to collect the complete delta of the book but it's certainly a lot of overhead.

Since the last time we looked at Kucoin, they've add L2 Snapshots (https://docs.kucoin.com/#level2-50-best-ask-bid-orders). We have another issue #172 to add L3 support. When we address that (plan to this week) we will add the l2snapshot support as well. This may be a better fit if you don't need the incremental steps between periodic snapshots.

WoutervDijk commented 4 years ago

Thank you, this might be much better, because of the lower overhead you talked about. Would be great to see that implemented 👍 . One question, when you do think you will release the fix?

bmancini55 commented 4 years ago

The plan is by end of this upcoming week. The Kucoin L3 support and a few other issues are on the remaining list before we cut this next huge release.

WoutervDijk commented 4 years ago

We have another issue #172 to add L3 support. When we address that (plan to this week) we will add the l2snapshot support as well. This may be a better fit if you don't need the incremental steps between periodic snapshots.

I am actually really interested in L3 support for kucoin. Since it is faster than L2 (I found out that the extra overhead of non throttling is negligible. Will you also add an example of how to implement an orderbook with L3 support?

bmancini55 commented 4 years ago

There is now a prototype L3 OB https://github.com/altangent/ccxws/blob/master/src/orderbooks/KucoinOrderBook.js

WoutervDijk commented 4 years ago

@bmancini55 I have tried your L3 prototype OB and I have also built one myself, but both of them lose quite a lot of messages it seems. I think at least every minute (so the sequenceId gets out of sync). To recreate the bug, just run your code and you should have an "out of sync, expected ${ob.sequenceId + 1}, got ${update.sequenceId}" error fast. const ccxws = require("ccxws"); const KucoinOrderBook = require("ccxws/src/orderbooks/KucoinOrderBook"); let market = {id: "BTC-USDT", base: "BTC", quote: "USDT"}; let updates = []; let ob; const client = new ccxws.Kucoin(); client.subscribeLevel3Updates(market); client.on("l3snapshot", snapshot => { ob = new KucoinOrderBook(snapshot, updates); }); client.on("l3update", update => { // enqueue updates until snapshot arrives if (!ob) { updates.push(update); return; } // validate the sequence and exit if we are out of sync if (ob.sequenceId + 1 !== update.sequenceId) { console.log(out of sync, expected ${ob.sequenceId + 1}, got ${update.sequenceId}); process.exit(1); } // apply update ob.update(update); }); ^^ The exact code I ran. The only thing I found about it as of right now, was someone who used python : https://github.com/Kucoin/kucoin-api-docs/issues/44#issuecomment-477105864

EDIT: Running on Node v14.7 on Windows 10

Really random guess, could be a fault in WS package and this could be the fix? https://github.com/websockets/ws/commit/e1349c047d7f1c120ca606364e35d5c4c627c599 Tried it myself didn't work unfortunately

Could you reopen the issue please or should I open a new issue?