Introduce Trade Class #1968

ansonphong · 2018-02-24T14:38:35Z

What kind of change does this PR introduce?
A new class that is responsible for doing a single trade.
Related to discussion in Issue A new class that is responsible for doing a single trade... #1942
What is the current behavior?
Currently all the trade and order functionality is mashed into the portfolio manager, making it impossible to deploy different trade and order execution strategies, as everything is hard-coded to one specific type of trade execution which is a single limit order for the full balance amount right across from the spread.
What is the new behavior (if this is a feature change)?
This PR strictly breaks out the trade specific functionality (as-is) into a separate Trade class, with minimal modification and no refactoring.
Other information:
This is an initial proposal for the Trade class. I have tested it on the live market and it works as expected without any errors so far. I'm curious to see others kick the tires and try to shoot holes in it and see how it works in different circumstances, and see if I missed anything.

This PR is far from ideal as a final implementation, and is part of an incremental series of PRs which will see a total refactor of the Portfolio Manager, this Trade class, and the introduction of a more robust Order model. Refer to the Issue #1942 for further discussion.

askmike · 2018-02-25T02:37:11Z

package-lock.json

@@ -94,6 +94,15 @@
        }
      }
    },
+    "JSONStream": {


do we need these changes?

No, any changes to the package-lock.json are based on local package preferences, and aren't required for this PR, I'll remove that.

askmike · 2018-02-25T02:38:30Z

plugins/trader/trade.js

@@ -0,0 +1,293 @@
+/*
+  @askmike


better to link to the discussion than copy snippets of it in a comment :)

Agreed, will update.

askmike · 2018-02-25T02:41:43Z

plugins/trader/trade.js

+  // the asset, if so BUY and keep track of the order
+  // (amount is in asset quantity)
+  buy(amount, price) {
+    let minimum = 0;


pretty sure we had a minimum, right?

Yes, that code is just directly pasted from the previous implementation, looks like the minimum is defined later on line 133 before the callback is executed.

askmike · 2018-02-25T02:58:46Z

plugins/trader/portfolioManager.js

-  // If unfilled, cancel and replace order with adjusted price
-  let cancelDelay = this.conf.orderUpdateDelay || 1;
-  setTimeout(this.checkOrder, util.minToMs(cancelDelay));
+  return this.currentTrade = new Trade(this,{action: what})


If we want to stick to OO: can you not pass the parent (portfolioManager) into the child (trade), these circular references are tightly coupled.

Yes, it's not ideal, let's fix this.

Currently there are multiple references to the manager inside the trade class, which would call for some restructuring. This is what we could do:

A new exchange object could be created inside the Trade class so it's not referencing it in the Manager, thus removing references to manager.exchange

We can pass in the current pair currency/asset handles into the Trade class on creation, and remove manager.asset and manager.currency

Currently the following methods are being called by the Trade class, how do you suggest we handle this?

this.manager.setTicker this.manager.setPortfolio this.manager.setFee this.manager.getMinimum this.manager.convertPortfolio

this.manager.convertPortfolio

This is currently only needed for emitting events upstream, the trade class doesn't need it.

Some other functions won't be needed anymore by the portfolioManager, like setFee and setTicker (we will need this one later again, for the portfolioValueChange event for example). All the other functions will be used differently anyway, so I propose the portfolioManager passes the exchange (this.exchange) to the tradeclass which reimplements what it needs, we can also drop in the portfolioManager what we don't need. What do you think?

@askmike OK that makes sense, yes let's keep a single Exchange instance, and move the methods into the Trade class. I'll make that happen, test it and update this PR.

askmike

See few comments, also: does the ES6 class syntax work nodev6? If it doesn't there is a node update process we need to go through.

ansonphong · 2018-02-27T10:51:48Z

Yes, Classes should work since Node V6.5, it also seems to support 97% of all ECMA 6 features listed:

http://kangax.github.io/compat-table/es6/

aapsoftware · 2018-02-28T22:26:19Z

what's the current status with this branch? is it usable or are there still changes to be made?

askmike · 2018-03-01T08:04:39Z

Yes, Classes should work since Node V6.5, it also seems to support 97% of all ECMA 6 features listed:

Gekko should work for Gekko v6.0, we can upgrade the minimum but as explained above this is a different process. I do want to move to use classes more so I'll look into any possible negative effects of requiring a higher version of nodejs.

ansonphong · 2018-03-01T14:03:50Z

@askmike Yes, I'm open to go through a Node upgrade process whatever is required, I think it's important to be progressive with adopting new standards.

…o class

ansonphong · 2018-03-19T15:57:23Z

@askmike I made the modifications that we discussed earlier in the code comments.

Since the "Portfolio" object and associated methods needed to be accessed by both the Trade class and the Portfolio Manager class, I broke it out into a "Portfolio" class / object which has a shared reference between both classes, which contains the portfolio data and ticker.

I tested this extensively in high frequency / high tick rate and high position reversal conditions on the live market and it works as expected.

This is not ready to merge yet until the logging and syntax is cleaned up, I'd like to make it more consistent. I'm curious to get your feedback on the last commit.

After this, the next thing I'd like to develop on this is a proper data model for Orders, which will help with having a perfect calculation of the status of a trade, and precisely measuring averages, slippage, etc, which will be needed for limiting position sizes, currency allowance, compounding gains, etc. Although that will require another pull request which may require modification to the exchange classes to collect additional data from orders such as how much was actually filled, order status, etc. We can discuss on another issue.

Let me know your thoughts on this last commit, it's ready to be live tested.

astralmedia · 2018-03-19T16:32:49Z

I am looking forward to this! Thanks everyone involved.

ansonphong · 2018-03-20T06:39:16Z

@askmike Also what is required to conduct a Node 6.5+ upgrade so we can get ES6 support? While we are at it why not update to Node 8? I've been running Node 8 on Windows and Linux without any issues thus far.

aapsoftware · 2018-03-20T07:13:50Z

@ansonphong node8 is diffcult to install on some raspberry pi versions. So you loose a little flexibility if you go to node 8 directly. Plus node 8 does not bring that much to the game to be worth making it a requirement.
Even with node 6 you have to be careful choosing your version; my raspberry pi runs node 6.2.1 and it the latest version you can install using yum

ansonphong · 2018-03-20T10:42:42Z

@aapsoftware How about Node V6.5, that supports ES6, is that manageable on Raspi?

ansonphong · 2018-03-20T10:48:39Z

plugins/trader/portfolioManager.js

+    exchange:this.exchange,
+    currency: this.conf.currency,
+    asset: this.conf.asset,
+    portfolio: this.portfolio,


I do not like all of these property values passed in, ideally it's just action, currency, asset, and then an amount which could have an undefined value which would then default to the system settings for currency allowance, keep asset, etc.

@askmike You know the architecture best, is there an currently established way by which we could store references to globally accessible objects / dependencies without resorting to a singleton?

ansonphong · 2018-03-30T03:04:39Z

@askmike Can you have a look at this when you have some time? I'd like to make a decisive move on this PR, as a lot of future work I'd like to do builds on this. Thanks!

askmike · 2018-03-30T03:41:19Z

Apologies for the delay. I am testing this now, If I don't find problems I will merge it today.

…

On Fri, 30 Mar 2018, 10:04 phong, ***@***.***> wrote: @askmike <https://github.com/askmike> Can you have a look at this when you have some time? I'd like to make a decisive move on this PR, as a lot of future work I'd like to do builds on this. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1968 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7MD7rH8J3kkCMFvnpo1iL5rZqqVNc7ks5tjaDLgaJpZM4SR6Y-> .

positonic · 2018-03-30T19:24:11Z

@ansonphong I'm interested in moving this forward and have availability, please let me know how to work with you in getting there faster...

@askmike I'm also eagerly awaiting this :)

ansonphong · 2018-03-31T01:26:47Z

Great thanks, @jamespfarrell you are welcome to test this PR, I'm only 99% confident that it works as expected, having others help testing is always helpful.

@askmike Check my comments above, I can do a final pass at it before merging.

minhnguyenvan95 · 2018-03-31T02:01:40Z

@ansonphong interesting

ansonphong · 2018-03-31T02:45:54Z

I made another push which basically just cleans up some of the debugging language and the comments a little bit, it should be ready to merge. I'm not content with the solution for passing the Portfolio object into the Trade, although it does work and makes a shared Portfolio object. Having a single object for the Portfolio itself is nice, although having a better way to access it via the core architecture would be ideal (IMHO), rather than having to pass it around.

That said, this PR simply breaks out the existing functionality into separate classes, and does not introduce any new functionality. The next work will require refactoring the methods to allow for an architecture which will allow for sub-classes of the Trade class which can carry out different trade strategies. For instance, alternate strategies could include layering multiple orders, placing market orders, etc, and pave the way for more sophisticated systems in Gekko which could allow for position sizing, etc. The refactoring can happen on another PR.

The very next step I'd like to take with it is to build an Order model which tracks all of the orders and their status, within a given Trade. This will require minimal updating of the Exchange class, which we can do on another PR. That will allow for a limited currency allowance to be traded with and then a ratio could be defined on how much profits to keep and how much to continue future trades to compound gains. This would allow Gekko users to basically run many instances of Gekko trading on the same exchange account on different pairs.

positonic · 2018-03-31T11:03:39Z

That said, this PR simply breaks out the existing functionality into separate classes, and does not introduce any new functionality. The next work will require refactoring the methods to allow for an architecture which will allow for sub-classes of the Trade class which can carry out different trade strategies. For instance, alternate strategies could include layering multiple orders, placing market orders, etc, and pave the way for more sophisticated systems in Gekko which could allow for position sizing, etc. The refactoring can happen on another PR.

If I understand you correctly, with this refactoring a Portfolio refers to an exchange portfolio, rather and an entire portfolio across many exchanges?

Also, if I understand this approach correctly you we will be able to apply many strategies on the market, but since this is operating through plugins on the GekkoStream, this will still only be able to act on 1 market at a time?

@ansonphong @askmike I would love to test the PR but can't get a copy of it besides making the change manually. I have done a lot of googling and tried a lot of commands, but can't seem to get a copy... Can you help? I'm super keen to test and further this issue...

I have tried:

git fetch origin pull/1968/ansonphong:trade-class
git fetch upstream pull/1968/head:trade-class

I have also run through this with no joyl.

and few other things...

@ansonphong are you working on a fork?

ansonphong · 2018-03-31T22:27:18Z

@jamespfarrell You can easily pull off my fork to test:
https://github.com/ansonphong/gekko/tree/trade-class

In your local developent Gekko, just go 👍
git pull [email protected]:ansonphong/gekko.git trade-class

Still will only be on one market at a time, though you will be able to run multiple instances.

askmike · 2018-04-01T18:28:50Z

This is long overdue, again apologies for the delay. This PR is perfect in that it only moves around code but doesn't change behaviour / introduces new concepts. Love to work on upgrades and will reply to your latest comment @ansonphong asap!

askmike · 2018-04-01T18:29:52Z

Before this can land in stable we do need to upgrade the minimal node version (as stated before), but as part of #1850 this will be necessary anyway. I will focus on that.

ansonphong · 2018-04-02T00:48:13Z

@askmike Sounds good, happy to get this merged. Are you open to eliminating all semicolons on line endings for ES6 code?

zawsx · 2018-04-02T11:01:40Z

eliminating all semicolons
Google say semicolons required - they are not the authority of course, but many people will follow their JavaScript Style Guide. The tendency is clear, lots of code I see has none, they're not needed.
I can see that the occasional horrible bug would be avoided if you litter up the code with superfluous symbols but not here, surely?

askmike · 2018-04-02T11:20:45Z

@askmike Are you open to eliminating all semicolons on line endings for ES6 code?

What do semicolons have to do with ES6? Did anything change? As far as I know they were optional and they still are optional. Most styleguides I know recommend using them though.

* subtract candle length from start time to calculate isPremature * Move startTimeMinusCandleSize inside realtime if check * return the indicator after adding it * develop trade class * make fixes after live market test * line spacing * rollback package-lock.json to develop branch * link to discussion: * move methods from portfolio manager into trade class and new portfolio class * cleanup logging, comments

* fix typo * fix server api changes * fix for Introduce Trade Class (#1968) breaking events emitter

chromadeployment and others added 8 commits January 26, 2018 15:38

subtract candle length from start time to calculate isPremature

964893e

Move startTimeMinusCandleSize inside realtime if check

0cac468

return the indicator after adding it

1fe8aff

Merge branch 'develop' of github.com:askmike/gekko into develop

681bd04

Merge branch 'develop' of github.com:askmike/gekko into develop

2cc8b2a

develop trade class

55b3fe5

make fixes after live market test

02c1d4d

line spacing

0568115

ansonphong mentioned this pull request Feb 24, 2018

A new class that is responsible for doing a single trade... #1942

Closed

ansonphong changed the title ~~Trade Class~~ Introduce Trade Class Feb 24, 2018

askmike reviewed Feb 25, 2018

View reviewed changes

askmike mentioned this pull request Feb 25, 2018

Multiple market orders on advice #1608

Closed

ansonphong added 2 commits February 27, 2018 04:01

rollback package-lock.json to develop branch

6e8e8ed

link to discussion:

56a131c

ansonphong mentioned this pull request Mar 1, 2018

Limit Trading Feature #1834

Closed

move methods from portfolio manager into trade class and new portfoli…

10f38be

…o class

ansonphong commented Mar 20, 2018

View reviewed changes

cleanup logging, comments

180e745

askmike merged commit 2627b47 into askmike:develop Apr 1, 2018

positonic mentioned this pull request Apr 3, 2018

Multiple Gekko streams #2079

Closed

H256 mentioned this pull request Apr 13, 2018

binance: event not emit after a trade occure #2086

Closed

hiyan mentioned this pull request May 15, 2018

fix for Introduce Trade Class (#1968) breaking events emitter #2181

Merged

askmike pushed a commit that referenced this pull request May 17, 2018

fix for Introduce Trade Class (#1968) breaking events emitter (#2181)

14fb0da

* fix typo * fix server api changes * fix for Introduce Trade Class (#1968) breaking events emitter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Trade Class #1968

Introduce Trade Class #1968

ansonphong commented Feb 24, 2018 •

edited

Loading

askmike Feb 25, 2018

ansonphong Feb 27, 2018

askmike Feb 25, 2018

ansonphong Feb 27, 2018

askmike Feb 25, 2018

ansonphong Feb 27, 2018

askmike Feb 25, 2018 •

edited

Loading

ansonphong Feb 27, 2018

askmike Mar 1, 2018

ansonphong Mar 3, 2018 •

edited

Loading

askmike left a comment

ansonphong commented Feb 27, 2018 •

edited

Loading

aapsoftware commented Feb 28, 2018

askmike commented Mar 1, 2018

ansonphong commented Mar 1, 2018

ansonphong commented Mar 19, 2018 •

edited

Loading

astralmedia commented Mar 19, 2018

ansonphong commented Mar 20, 2018

aapsoftware commented Mar 20, 2018

ansonphong commented Mar 20, 2018

ansonphong Mar 20, 2018

ansonphong commented Mar 30, 2018

askmike commented Mar 30, 2018 via email

positonic commented Mar 30, 2018

ansonphong commented Mar 31, 2018

minhnguyenvan95 commented Mar 31, 2018

ansonphong commented Mar 31, 2018 •

edited

Loading

positonic commented Mar 31, 2018 •

edited

Loading

ansonphong commented Mar 31, 2018

askmike commented Apr 1, 2018

askmike commented Apr 1, 2018

ansonphong commented Apr 2, 2018

zawsx commented Apr 2, 2018 •

edited

Loading

askmike commented Apr 2, 2018

Introduce Trade Class #1968

Introduce Trade Class #1968

Conversation

ansonphong commented Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askmike Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansonphong Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

askmike left a comment

Choose a reason for hiding this comment

ansonphong commented Feb 27, 2018 • edited Loading

aapsoftware commented Feb 28, 2018

askmike commented Mar 1, 2018

ansonphong commented Mar 1, 2018

ansonphong commented Mar 19, 2018 • edited Loading

astralmedia commented Mar 19, 2018

ansonphong commented Mar 20, 2018

aapsoftware commented Mar 20, 2018

ansonphong commented Mar 20, 2018

Choose a reason for hiding this comment

ansonphong commented Mar 30, 2018

askmike commented Mar 30, 2018 via email

positonic commented Mar 30, 2018

ansonphong commented Mar 31, 2018

minhnguyenvan95 commented Mar 31, 2018

ansonphong commented Mar 31, 2018 • edited Loading

positonic commented Mar 31, 2018 • edited Loading

ansonphong commented Mar 31, 2018

askmike commented Apr 1, 2018

askmike commented Apr 1, 2018

ansonphong commented Apr 2, 2018

zawsx commented Apr 2, 2018 • edited Loading

askmike commented Apr 2, 2018

ansonphong commented Feb 24, 2018 •

edited

Loading

askmike Feb 25, 2018 •

edited

Loading

ansonphong Mar 3, 2018 •

edited

Loading

ansonphong commented Feb 27, 2018 •

edited

Loading

ansonphong commented Mar 19, 2018 •

edited

Loading

ansonphong commented Mar 31, 2018 •

edited

Loading

positonic commented Mar 31, 2018 •

edited

Loading

zawsx commented Apr 2, 2018 •

edited

Loading