Emit Trade to Strategy

[werkkrew]

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

• What is the current behavior? (You can also link to an open issue here)
  Strategies are not aware of completed trades.

• What is the new behavior (if this is a feature change)?
  If you put an "onTrade(trade)" method in your strategy, when a trade occurs successfully on the exchange you can use that data to do things, such as update a stop-loss based on an actual trade value instead of using candle data to assume a trade actually executed at a given price.

• Other information:

[askmike]

can you remove this comment? Not using the full balance is an issue with a lot bigger scope than simply the execution logic.

[werkkrew]

Done

[askmike]

I like the idea, but I don't see how it would work: the trader (or paper trader) that is running and listening to advice from the strat might take a while to execute: it might take a minute before it was able to buy advice. Where this PR assumes straight after the advice is given by the strat the position is long.

I think we need to emit an event from a trading plugin (like the trader) that we can feed back into the strat. When we do this we need to keep the backtester in mind (avoiding race conditions between new candles and the position change from the trader).

[werkkrew]

I am using it in practice right now because I have another set of features I am working on that manages trades using websocket data, and I have a setting that will cancel a trade if it was not filled in a certain amount of time.

What I saw happening was that the baseTradingMethod code was assuming that since advice was given, it was successfully acted upon. What this change does is uses the result of an actual trade to set the current position so that duplicate advice can still be sent if it is duplicate advice in which the previous advice did not result in action.

In the case of the paper trader or the default way Gekko works right now, the setting is kind of irrelevant since Gekko will manage an order until it executes for an essentially unlimited amount of time once it receives advice.

Perhaps I should contain this particular change within the scope of my websocket changes only?

Also to add, unless I did something wrong in my code, it doesn't assume a position is held right after the advice, it only sets the position as being held once the result of the trade is emitted.

[werkkrew]

I removed the change we were discussing as it was out of scope for this PR. I will re-introduce that change in another PR if/when it makes sense.

[askmike]

What I saw happening was that the baseTradingMethod code was assuming that since advice was given, it was successfully acted upon.

baseTradingMethod is a bad name, but this is an abstract "strategy" class which forms the basis of a strategy, it is only there to provide extra functionality for strategies so they can execute (register indicators, propagate advice, etc). It is not supposed to assume anything about what happens with advice (eg. orders or trades).

The idea is that the tradingAdvisor (together with the baseTradingMethod) is responsible for making sure strategies get executed with all information they need (market data, indicators, etc), and that their advice signals get propagated so that the "trader" (real or paper) can do something with their results. Whether that happens or not does not matter to the tradingAdvisor (right now, but we can change that). It doesn't assume anything about trades, orders or positions.

---

You are right with that discussion being a bit out of scope. #1814 discusses how we can get to where you want to go :)

Though I am not understanding the current proposed changes:

• tradingAdvisor/tradingAdvisor.js is a glue plugin that creates a "strategy".
• tradingAdvisor/baseTradingMethod.js is a base strategy class from which user strategies get extended.

Your current PR seems to do:

Whenever the tradingAdvisor sees that the baseTradingMethod triggers something called trade it executes this.processTrade. this.processTrade executes the baseTradingMethod method called processTrade. When that gets executed it calls something which is called onTrade which gets initialised as an empty function.

a) I guess strategies can implement an onTrade method, which gets information about trades being performed.
b) who is actually calling processTrade of the tradingAdvisor? Right now it seems this will never get called?

[werkkrew]

I'll be honest, I am not 100% sure the process flow, but I tried numerous things to get emitted trades to be available by a method inside a strategy and this combination of things is what worked for me.

I am absolutely 100% open to any changes or improvements that can be made here, at the end of the day I just needed a reliable way to get emitted trade into a strategy I am working on and this works.

To answer a few of your questions:

• onTrade is initialized as an empty function as a filler for if the strategy does not include that function.
• tradingAdvisor actually recieves the emitted trade and relays that into baseTradingMethod which calls the method in the strategy itself. I found that even though simply having a processTrade method in the baseTradingMethod file should have worked without needing any modifications to tradingAdvisor, in my testing that was not the case.

For the whole thing to actually do anything, you need something like this in the user strategy file:

method.onTrade = function(trade) {
//do stuff
}

Also, based on the content of subscriptions.js it was my assumption that any other module containing a "processTrade" method would get executed any time a trade is emitted, which is why I put a processTrade method in tradingAdvisor.

[werkkrew]

@askmike Since this is a relatively small change, do you mind offering some advice or making the necessary changes to get it into an acceptable merge? I personally rely on this functionality quite a bit (getting completed trade details into my strategy) and if the approach I have taken to doing it is incorrect, I would love to see how it would be done better.

[askmike]

Hey! Apologies for the radio silence, was away from internet for a few days. I did look up the trade event in the meantime (offline) and I finally see what event you are hooking into. I love this function and will merge this as soon as I am behind my laptop again.

…

On 2 Feb 2018 11:30, "Bryan Chain" ***@***.***> wrote: @askmike <https://github.com/askmike> Since this is a relatively small change, do you mind offering some advice or making the necessary changes to get it into an acceptable merge? I personally rely on this functionality quite a bit (getting completed trade details into my strategy) and if the approach I have taken to doing it is incorrect, I would love to see how it would be done better. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1810 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7MD8SnYx1HtfiZVnNEP8c1DYcttyZtks5tQoFfgaJpZM4RxMOc> .

[werkkrew]

@askmike Thanks and no worries. I just have a small touch of OCD and leaving PR's open that will not get merged bothers me, I'd rather correct them so they can be merged or close them than leave them open.

That said, before you merge this, I have one other thing I wanted to add to this PR but have yet been unsuccessful in finding a graceful way to do it.

I want a way for the strategy, preferably in the init method, to get information about the starting portfolio. Right now the trade information contains the portfolio information so any time a trade happens we get updated portfolio information along with it. But, we can't get that data in the strategy until the first trade happens.

As is outlined in this issue: #1541

The order the plugins get initialized seems to prevent a strategy from getting the information early enough, even though when you look at the console the portfolio data is displayed very early in the process.

Ultimately having a simple way for the strategy init to get the portfolio.asset and portfolio.currency amounts would be all I am asking for and I would like it to be included in this PR if possible.

[askmike]

Thanks and no worries. I just have a small touch of OCD and leaving PR's open that will not get merged bothers me, I'd rather correct them so they can be merged or close them than leave them open.

I'll try to keep more communication open, this is definitely something I need to keep working on.

I want a way for the strategy, preferably in the init method, to get information about the starting portfolio. Right now the trade information contains the portfolio information so any time a trade happens we get updated portfolio information along with it. But, we can't get that data in the strategy until the first trade happens.

I've been talking about Gekko events for a while, but documentation was always poor and not many people had a clue what I was talking about (understandably), please check out this PR (it's not finished yet but that will be the way forward): #1850.

Specifically this page: https://github.com/askmike/gekko/blob/feature/sync-gekko-events/docs/internals/events.md

To answer your specific question, check this event: https://github.com/askmike/gekko/blob/feature/sync-gekko-events/docs/internals/events.md#portfolioupdate-event

Notes:

• all these events are exposed to all plugins, the tradingAdvisor is a plugin that wraps around the strategy, with some simple glue code all events there can be exposed to the strategy.
• the events currently listed on that page are not final yet, they can still change.

[askmike]

I'm merging this now, but won't be adding documentation about this new handle. Mainly because we don't know stability when backtesting.

[askmike]

this broke live all gekkos, see #1862.

[askmike]

Can you explain why you changed this? For now I am reverting to the old behaviour.

[werkkrew]

It didn't break ALL live Gekkos because it didnt break any of mine :) I changed it because the performance advisor for Binance had invalid dates coming back for trades. I believe someone submitted a PR for binance that likely fixes that issue in the correct place.

…

On Mon, Feb 5, 2018 at 9:10 AM Mike van Rossum ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plugins/trader/portfolioManager.js <#1810 (comment)>: > _.each(res.filter(o => !_.isUndefined(o) && o.amount), order => { - date = _.max([moment(order.date), date]); + date = _.max([moment(order.date).unix(), date]); Can you explain why you changed this? For now I am reverting to the old behaviour. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1810 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAzMtfcZ95oYcHflia2ks9a96ZUxGQQ6ks5tRwvFgaJpZM4RxMOc> .