askmike / gekko

A bitcoin trading bot written in node - https://gekko.wizb.it/
MIT License
10.06k stars 3.95k forks source link

Bitstamp - orders not monitored #197

Closed djmuk closed 10 years ago

djmuk commented 10 years ago

When an order is placed on bitstamp it is immediately flagged as successful even if it is outstanding.

There is a mismatch between the portfolio manager noteorder function (which just expects the order id to be passed) and the callback in the exchange/bitstamp.js code which passes

callback(err, result.id) 

so the noteorder function takes the value of err as the orderid which is usually null...

askmike commented 10 years ago

Ah this behaviour changed I think, now it's messed up for different exchanges. This document describes desired behaviour which has an err parameter. So I'll provide this from all exchanges and make sure the noteOrder handles it correctly. (Even though there is no possible error because exchanges are expected to retry for as long as it takes, I want this in for consistency).

djmuk commented 10 years ago

Mike while you are looking at this area: On Cexio I have had a couple of occasions where cancelling and replacing an order has failed and Gekko has crashed out with 'result.err' as undefined in this section of code

Cexio.js

Trader.prototype.checkOrder = function(order, callback) {
  var check = function(err, result) {

    if(err)
      callback(false, true);
    if(result.error)
      callback(false, true);

    var exists = false;
    _.forEach(result, function(entry) {
      if(entry.id === order) {
        exists = true; return;
      }
    });
    callback(err, !exists);
  };

I think there should be an 'else' clause so processing drops out if either 'err' is true/exists. I have put more logging in to try and catch the issue but I suspect the err value is set (due to a problem) and so the result value isn't set - causing a crash on the 2nd if statement...

askmike commented 10 years ago

Nice catch! Should be fixed now.

You could use an if/else clause for this, but my preferred approach is to return on error (to not get into a nesting hell).

djmuk commented 10 years ago

Yep - a return is more logical as we can't do any more...

Just had the error on my (unpatched) version so was going to post the details!

2014-02-13 12:32:09 (INFO): order 347837955 err { [Error: socket hang up] code: 'ECONNRESET' } result undefined
2014-02-13 12:32:09 (INFO): SELL was succesfull

/home/david/gekko/gekko/exchanges/cexio.js:162
    if(result.error)
             ^
TypeError: Cannot read property 'error' of undefined
    at check (/home/david/gekko/gekko/exchanges/cexio.js:162:14)
    at bound (/home/david/gekko/gekko/node_modules/lodash/dist/lodash.js:729:21)
    at ClientRequest.<anonymous> (/home/david/gekko/gekko/node_modules/cexio/cexio.js:71:5)
    at ClientRequest.EventEmitter.emit (events.js:95:17)
    at CleartextStream.socketCloseListener (http.js:1522:9)
    at CleartextStream.EventEmitter.emit (events.js:117:20)
    at tls.js:696:10
    at process._tickCallback (node.js:415:13)
askmike commented 10 years ago

So to clarify, this did not happen since my fix (which should prevent that from happening)?

djmuk commented 10 years ago

Still running older version but have manually applied patch (since then).