Added Robinhood broker class #79
Conversation
zipline/gens/brokers/rh/iex.py
Outdated
| if resp.status_code == 200: | ||
| return resp.json() | ||
| else: | ||
| print("Response error %d. Trying again..." % resp.status_code) |
There was a problem hiding this comment.
Please use logbook based logger instance throughout the code.
| @@ -0,0 +1,34 @@ | |||
| """ | |||
There was a problem hiding this comment.
Could you please move the content of this file to robinhood.py? It is not so big, if we remove the comments. The class names are self-explanatory, thus comments are not really needed in my view.
zipline/gens/brokers/rh/robinhood.py
Outdated
| headers = None | ||
| auth_token = None | ||
|
|
||
| logger = logging.getLogger('Robinhood') |
There was a problem hiding this comment.
Could you please move this out to the top level of the module? Additionally, please use logbook to instantiate - that's the logger used throughout zipline code.
zipline/gens/brokers/rh/robinhood.py
Outdated
| auth_token = None | ||
|
|
||
| logger = logging.getLogger('Robinhood') | ||
| logger.addHandler(logging.NullHandler()) |
There was a problem hiding this comment.
I'm not sure what's the purpose of this. Might be not needed.
zipline/gens/brokers/rh/robinhood.py
Outdated
| def login_prompt(self): #pragma: no cover | ||
| """Prompts user for username and password and calls login() """ | ||
|
|
||
| username = input("Username: ") |
There was a problem hiding this comment.
I'd really prefer to not to have any interaction.
If we have interaction it is so much harder to automatically start the app.
The login and password should be specified either via environment variables or application parameters.
There was a problem hiding this comment.
I'll make the user info come from a config file so no interaction is required
zipline/gens/brokers/rh/robinhood.py
Outdated
| req = self.session.post(self.endpoints['logout']) | ||
| req.raise_for_status() | ||
| except requests.exceptions.HTTPError as err_msg: | ||
| warnings.warn('Failed to log out ' + repr(err_msg)) |
zipline/gens/brokers/rh/robinhood.py
Outdated
| req.raise_for_status() | ||
| data = req.json() | ||
| except requests.exceptions.HTTPError: | ||
| raise RH_exception.InvalidTickerSymbol() |
There was a problem hiding this comment.
All HTTP errors will be treated as if we posted invalid symbol - which might not be the case. I'd suggest to do deeper inspection here. E.g. http 500, shall not raise InvalidTickerSymbol.
zipline/gens/brokers/rh/robinhood.py
Outdated
| return data | ||
|
|
||
|
|
||
| # We will keep for compatibility until next major release |
zipline/gens/brokers/rh/robinhood.py
Outdated
| raise Exception("Could not generate instrument object: %s " % res.headers) | ||
|
|
||
|
|
||
| def quote_data(self, stock=''): |
There was a problem hiding this comment.
Not sure if empty string as stock default makes any sense here. What does robinhood returns in such case?
Also, stock can be stocks as you expect comma delimited list. Shouldn't we call it stocks and mentioned it in the docs?
zipline/gens/brokers/rh/robinhood.py
Outdated
|
|
||
| #Prompt for stock if not entered | ||
| if not stock: #pragma: no cover | ||
| stock = input("Symbol: ") |
zipline/gens/brokers/rh/robinhood.py
Outdated
| """ | ||
|
|
||
| #Creates a tuple containing the information we want to retrieve | ||
| def append_stock(stock): |
There was a problem hiding this comment.
I don't understand the purpose of this function, documentation does not help either. Could you please rename it to have a more meaningful name?
zipline/gens/brokers/rh/robinhood.py
Outdated
|
|
||
|
|
||
| def get_quote(self, stock=''): | ||
| """Wrapper for quote_data """ |
zipline/gens/brokers/rh/robinhood.py
Outdated
| return self.get_quote_list(stock,'adjusted_previous_close') | ||
|
|
||
|
|
||
| def symbol(self, stock=''): |
There was a problem hiding this comment.
Same as above: not sure if it makes sense to pass an empty stock symbol
zipline/gens/brokers/rh/robinhood.py
Outdated
|
|
||
|
|
||
| ########################################################################### | ||
| # GET FUNDAMENTALS |
zipline/gens/brokers/rh/robinhood.py
Outdated
| ########################################################################### | ||
|
|
||
| def portfolios(self): | ||
| """Returns the user's portfolio data """ |
There was a problem hiding this comment.
In general: I'd rather not put doc-strings unless it is necessary. This comment does not help to understand what's returned. Please either remove it or make it meaningful.
zipline/gens/brokers/rh/robinhood.py
Outdated
| # PORTFOLIOS DATA | ||
| ########################################################################### | ||
|
|
||
| def portfolios(self): |
zipline/gens/brokers/rh/robinhood.py
Outdated
| return float(self.portfolios()['adjusted_equity_previous_close']) | ||
|
|
||
|
|
||
| def equity(self): |
zipline/gens/brokers/rh/robinhood.py
Outdated
| """Wrapper for portfolios | ||
|
|
||
| Returns: | ||
| (float): `extended_hours_market_value` value |
zipline/gens/brokers/rh/robinhood.py
Outdated
| try: | ||
| return float(self.portfolios()['extended_hours_market_value']) | ||
| except TypeError: | ||
| return None |
There was a problem hiding this comment.
Not sure if this is nice. At least we should log this failed attempt.
zipline/gens/brokers/rh/robinhood.py
Outdated
| """Place an order with Robinhood | ||
|
|
||
| Notes: | ||
| OMFG TEST THIS PLEASE! |
zipline/gens/brokers/rh/robinhood.py
Outdated
| Notes: | ||
| OMFG TEST THIS PLEASE! | ||
|
|
||
| Just realized this won't work since if type is LIMIT you need to use "price" and if |
zipline/gens/brokers/rh/robinhood.py
Outdated
| 'type': order.lower() | ||
| } | ||
|
|
||
| #data = 'account=%s&instrument=%s&price=%f&quantity=%d&side=%s&symbol=%s#&time_in_force=gfd&trigger=immediate&type=market' % ( |
zipline/gens/brokers/rh/robinhood.py
Outdated
| @@ -0,0 +1,1147 @@ | |||
| """Robinhood.py: a collection of utilities for working with Robinhood's Private API """ | |||
There was a problem hiding this comment.
Okay. I just realized that this code has been copy pasted from https://github.com/Jamonek/Robinhood/blob/master/Robinhood/Robinhood.py.
Please don't copy it here, but rather use it as external library.
If you have modifications in it we can create our own fork, but we should not compound libraries into zipline-live.
zipline/data/bundles/google.py
Outdated
| @@ -0,0 +1,133 @@ | |||
| import os | |||
There was a problem hiding this comment.
I don't think we should include this code as part of robinhood integration. If you need it back please post a new PR.
There was a problem hiding this comment.
I'll leave this out on the next PR
zipline/gens/brokers/rh/iex.py
Outdated
| @@ -0,0 +1,234 @@ | |||
| import requests | |||
There was a problem hiding this comment.
It is unclear to me why do we have IEX related code under robinhood. Are those two always used together?
There was a problem hiding this comment.
I decided to include IEX to pull in data since I couldn't get it using the Robinhood API. I couldn't get some of these things: last_trade_price, last_trade_size, last_trade_time, total_volume and vwap
I could leave it out, but I'll have an incomplete bar/tick.
| return timedelta(seconds=0) | ||
| # return self._rh.time_skew | ||
|
|
||
| def order(self, asset, amount, limit_price, stop_price, style): |
There was a problem hiding this comment.
This class uses the old API. We recently moved to use Blotter-Live, which changes some of the broker's call signatures. Could you please adopt your implementation too?
| self.account = account | ||
|
|
||
|
|
||
| class ROBINHOODBroker(Broker): |
There was a problem hiding this comment.
Most of this code is duplicate of IBBroker. We should consider moving up the shared code to Broker to avoid code duplication. Could you please check what's possible?
|
Thank your for your contribution. Unfortunately additional work needs to be done before we can merge it. I still haven't got through completely, will need at least one-more pass to better understand what's going on. Meanwhile: could you please address my comments, rebase your branch and fix the flake8 & testcase failures? Thank you! |
In order to make the Robinhood class I modified main.py to not need the broker-uri when Robinhood is selected. I added the 'rh' directory in brokers. This includes the modified Robinhood API and the modified pyEX API so they can be installed with zipline. I also added the google bundle downloader which is still a little buggy.
I've done a good amount of testing to see if all the functionality works, but I could use some help.