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

[ansonphong]

Note: for support questions, please join our Discord server

• I'm submitting a ...
  [ ] bug report
  [X] feature request
  [ ] question about the decisions made in the repository

So this is an idea that @askmike came up with in this PR: #1834. I would like to surface it here so that we don't loose track of it, as it deserves it's own issue.

I propose to create something a bit more clear and subtle: a new class that is responsible for doing a single trade (let's call him broker or execution strategy or so), it needs to:

• Figure out if there is a limit on the size of the trade we can do.
• If we can't do any trade (low funds) don't do anything.
• If we can do a trade figure out what kind of trade we aim to do (sell X USD) and trigger an event (see [WIP] 0.6 new gekko events #1850).
• Try to buy sell according to current "limit order do not cross spread" strategy (explained here).
• only when fully completed figure out trade statistics (average execution rate, amount that was traded, etc).
• only after this upstream a trade event with the statistics.

The reason I want to pull this out is so that in the future we can have different execution strategies: the current one tries to get the best price - at the cost of execution speed (since it will always make orders it might take a long time before the trade is completed, especially when trying to catch a trend). Once it's split out we can easily create another one that will do a market order instead. Or one that tries to split the order out into multiple small ones (bigger traders need ways to take pressure of the market for example).

[Ali1]

Would be good if the trade processing code that is executed is separated from the attempt order code which may repeat a few times until the trade gets fully filled. Currently the trade method keeps executing itself if it's only partially filled.

There may be better terminology options for the full trade and each trade attempt that is made until fully filled:

• Trade and Attempt
• Trade and Order
• Execute Advice and Trade/Order/Trade Order
• Trade Action and Trade/Order/Trade Order
• Execute Trade and Trade Order/Order

The class name and method name should reflect the chosen terminology for clarity and consistency.

[askmike]

Great point.

Currently the trade method keeps executing itself if it's only partially filled.

This wasn't a problem before your PR since the whole balance was supposed to get swapped, but this is definitely limiting us right now in a lot of ways.

There may be better terminology options for the full trade and each trade attempt that is made until fully filled

Could you elaborate on what you mean with those names? I'm not following them.

[Ali1]

@askmike I have given examples of possible terminology pairs. To demonstrate what I mean, if the terminology option "Execute Advice and Trade" was chosen, then we would call the single action of conducting the trade "Executing Trade Advice" and the recurring action of making new orders until the fully filled "trades". The code would look something like this.

class Trade {
  method executeAdvice(what) {
    let filled=false;
    amount=getBalance();
    while(!filled){
      this.trade(what, amount);
    }
    console.log("Advice Execution was successful after 5 trades");
  }
  method trade(what, amount) {
    //code that creates order on exchange
  }
}

I'm leaning towards "Trade and Trade Orders" maybe? But I don't feel strongly:

class Trade {
  method trade(what) {
    let filled=false;
    amount=getBalance();
    while(!filled){
      this.tradeOrder(what, amount);
    }
    console.log("Trade was successful after 5 trade orders at the exchange");
  }
  method tradeOrder(what, amount) {
    //code that creates order on exchange
  }
}

[askmike]

Yes that sounds great, so to go over the terminology:

• advice is an advice coming from a strat to create trades to move a portfolio into a certain position.*
• "trade" is a a number of buy or sell orders that completely move into a position.
• "order" is a message we send to the exchange asking to create a (limit or market order)
• based on the type of strat we might need to create a new different orders, however they are all part of the same "trade".
• this way a "trade" will have one price (weighted average of all orders), one amount (sum of all orders) and one fee (weighter average of all orders).

*This is a basic advice that simple does one trade. I want to extend this logic to allow for advanced order types like stoploss-takeprofit where the advice can be something like:

• right now BUY @ 1000, then:
• if price < 950 SELL (stoploss)
• if price > 1050 SELL (take profit)

If you omit the last one you have a simple stoploss, without any predermined take profit (until the strat creates a new advice or so). In the example above one advice would consist of: 1 direct trade, 2 potential trades (only one of the 2 might ever get executed). And each of these 3 trades will have a number of orders, depending on the type of trade (market, limit, stick to BBO).

[ansonphong]

@askmike @Ali1 I'm actively working on this Trade class now. Agreed 100% on the terminology here as just stated.

I've been going with the Trade and Order terminology thus far, and separating out nested models for Trades (child of portfolio manager), and Orders (child of Trades) including local histories so you could fully trace out all the activity, and/or store it, log it as JSON, etc.

This is how I imagine it:

• PortfolioManager is limited to only managing the portfolio stats, balances and allowances. Instantiates Trades, and keeps track of Trade history
• Trade instantiates one or multiple Orders until trade is complete, and keeps track of Order history
• Order communicates with the exchange, and manages itself, types can include limit, stoplimit and market

I will look over all your notes and submit a PR after some initial testing to see what you think.

[ansonphong]

This is a hyper-simplified outline of the general class, so a new Trade class is instantiated by the portfolio manager, which then runs itself until it's finished, or until it's deinitialized by the portfolio manager.

class Trade {
  constructor(manager,settings){
    this.manager = manager // store a reference to the portfolio manager
    this.action = settings.action // BUY/SELL
    this.orders = [{},{}] // array of self-managing order objects
    this.averagePrice = 0
    this.averageSlippage = 0
    this.doTrade()
  }
  method doTrade() {
    // execute and track orders until trade is complete
    // different trade execution strategies can override this in a subclass
    this.orders.push(new Order({...}))
  }
  method deinit(callback){
    // cancel open orders and shut down the trading process
  }
}

There is also a currency allowance, including a net currency allowance which has the option to compound gains which is stored one level up in the portfolio manager.

Let me know if this type of structure makes sense from your perspective. As I'm settings it up, this should completely plug into the existing system without any modifications to any existing files except portfolioManager

[askmike]

@ansonphong sounds great, but please don't to much all at once. I am also already working on stuff like this:

There is also a currency allowance, including a net currency allowance which has the option to compound gains which is stored one level up in the portfolio manager.

Which will actually be managed above the portfolioManager (above a single Gekko even).

---

Please keep PRs simple and concise for now, else it's going to be extremely hard to make sure it works as intended and doesn't break in exchange X and Y. We can always have a few PRs for the full func.

---

Another thing: make sure you don't use any ES6 features unless they are supported by node v6. I am willing to upgrade the minimum required node version if needed, but this needs to go through it's own (PR) process so we can properly test everything.

[ansonphong]

@askmike Noted, although due to how interconnected all the aspects of this PR is, I find it difficult to see how it could be done incrementally since it's an entirely new structure to how orders are executed.

That said, obviously it's up to you what is committed to the Gekko core, and I don't want to be presumptuous or duplicate efforts.

I also appreciate the amount of robustness and testing required for this, I'm open to alternative ways to combine efforts.

[askmike]

@ansonphong we can start by one simple PR that pulls current "create a trade" behaviour (as is) out of the portfolio manager (using all the subclasses you described or not). After that we can do one PR where this trade class doesn't just get a buy/sell signal but also an amount (for now we can keep this in portfolio manager - later this comes from upstream to address situations where people have defined a trade limit but Gekko got restarted, etc.). After that we can extend the new class to address problems/bugs in current implementation such as:

• doesn't properly recalculate orders that were previously partially filled.
• recreates orders, even in case the BBO did not change.
• very slow in checking filled orders (needs to go below 1 second level) to be competitive.
• etc.

And we can also start adding new trade execution strategies (stoploss, etc. all the stuff you described before).

---

My main point is that I really want to avoid huge changes in a single PR to the codebase wherever possible, I think it's possible here since right now everything is inside a single class portfolio manager. It's going to be super complex to test and maintain if in one go we rewrite the whole thing, remember that it needs to work with all exchanges where Gekko can trade (10+ exchanges) as well as on a huge number of platforms (servers, windows, mac, rasppi, etc).

[askmike]

Let me know if you think this makes sense :)

…

On 24 Feb 2018 12:23, "phong" ***@***.***> wrote: @askmike <https://github.com/askmike> Noted, although due to how interconnected all the aspects of this PR is, I find it difficult to see how it could be done incrementally since it's an entirely new structure to how orders are executed. That said, obviously it's up to you what is committed to the Gekko core, and I don't want to be presumptuous or duplicate efforts. I also appreciate the amount of robustness and testing required for this, I'm open to alternative ways to combine efforts. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1942 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7MD01BuLE32I-oSPGphmWSJXl0mUztks5tX46ugaJpZM4SJnGL> .

[ansonphong]

@askmike Yes that makes sense, thanks for suggesting an initial pull request, we can do it as incrementally as possible. That seems like a clean first step.

I've been considering that there are many exchange files, I don't plan to modify any existing files except portfolioManager.js for this first PR for sure, and add two new files trade.js and order.js. Enhancements to the exchange classes can come later.

I'll work on getting it to work 'as-is' with the new implementation first, so that everything works normally in the same way, with a new structure, and we can kick the tires on that. This involves moving all of the order and trade execution logic into the new classes.

I'm running node 8.9.3 locally, it's been running fine on Windows and Ubuntu, haven't tested extensively on Mac yet and not at all on Rasppi. I'd suggest we start switching to full ES6 as it makes the class (and sub-class) definitions much less verbose, and the way it handles scopes for this is much more conductive to object oriented design patterns.

I'll let you know if any more questions. You can find me on the Gekko Discord as phong, would be great to chat to coordinate might help things move faster, up to you, GitHub also works.

[ansonphong]

To clarify the scope of changes, inside portfolioManager:

• trade method is being modified to instantiate the new Trade class
• some additional supporting methods and properties are being added
• the following methods are being changed / nested into the Trade class: buy, sell, noteOrder, cancelLastOrder, checkOrder, relayOrder
• Trade and Order classes are being added
• everything else is being left as-is

I might have left something out, though that's the general idea. Currency allowance, etc, can come with a following PR. I'm very tempted to do a more thorough refactor though I'll try and restrain it to the minimum restructure, and then take the next steps from there.

[Ali1]

@askmike I propose you accept #1834 first which seems to be working on my end, so that so we can see how it works with others, while @ansonphong can start working on a new PR separating the trade logic out of portfolioManager into a new class. This work should start after #1834 is accepted so the limit trade feature gets moved into the new class also and by the time he's done, we will already know that the features added by #1834 aren't causing issues out in the wild.

That would seem the most efficient way to do this.

[Ali1]

And I agree with @askmike , first PR would be great if it could just be moving the code out of portfolioManager. Thanks @ansonphong

[ansonphong]

I created an initial PR here: #1968

This PR is a minimal breakout of the Trade functionality into a separate class. It's far from ideal, and contains too many references to the parent class (this.manager), which is not best OO practices, although the intention of this PR is not to complete an object oriented refactor but incrementally move toward a new structure, which will include everything as discussed in this thread.

In order to minimize changes, I omitted an Order class object from this PR, and will introduce that in a future PR as part of this series.

All of the new code is generally formatted in ES6 class structure, although currently everything is a bit of a mash between ES5/6, I would like to push towards adopting ES6 moving forward, happy to have a discussion about that.

This has been tested buying and selling on the live market (Binance) and works as expected. Let me know if you find any issues with it.

@askmike What do you think about making a new branch for this series until it's fully refactored and tested, and then later we can merge into develop?

[ansonphong]

@askmike I updated the PR with the addition of a Portfolio class, it is thoroughly tested though not quite ready to merge yet, more here: #1968 (comment)

[askmike]

@ansonphong major thanks a lot for your early work, it was the main inspiration for Gekko Broker that has now been released!