Skip to content
This repository has been archived by the owner on Feb 16, 2020. It is now read-only.

GDAX cancelOrder fix and misc other fixes #1808

Merged
merged 4 commits into from
Jan 29, 2018
Merged

Conversation

werkkrew
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Big Fix / Feature

  • What is the current behavior? (You can also link to an open issue here)
    Same as PR Binance cancelOrder Fix #1806 cancelOrder does not detect filled orders.

  • What is the new behavior (if this is a feature change)?
    Calls to cancelOrder that result in order not found error will presume the order was filled and return true to portfolioManager

Additional minor feature add is to have the ability to enable/disable sandbox using a config value instead of a hard coded boolean in the exchange file itself.

  • Other information:
    Also same as Binance cancelOrder Fix #1806 I am not 100% this approach is 100% as robust as it could/should be but it works as a stop-gap.

@askmike @cmroche

@askmike
Copy link
Owner

askmike commented Jan 29, 2018

ACK all your changes, would you mind reverting all the whitespace changes though? (2 space indents and the likes)? Thanks!

@werkkrew
Copy link
Contributor Author

Cleaned up the whitespace a good bit to clarify the changes introduced.

@askmike
Copy link
Owner

askmike commented Jan 29, 2018

👍 thanks!

@askmike askmike merged commit cb3fafe into askmike:develop Jan 29, 2018
Copy link
Contributor

@cmroche cmroche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small bug

var result = function(err, data) {
if(err) {
log.error('Error cancelling order:', err);
callback(true); // need to catch the specific error but usually an error on cancel means it was filled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't return here you will also call callback(false).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it was already merged can you implement this fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated my branch with the returns...

@werkkrew werkkrew mentioned this pull request Jan 30, 2018
This was referenced Mar 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants