Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tradier: new broker and data_source skeleton code added along with in… #325

Merged
merged 1 commit into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lumibot/brokers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
from .broker import Broker
from .ccxt import Ccxt
from .interactive_brokers import InteractiveBrokers
from .tradier import Tradier
69 changes: 69 additions & 0 deletions lumibot/brokers/tradier.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
from lumibot.brokers import Broker
from lumibot.data_sources import TRADIER_LIVE_API_URL, TRADIER_PAPER_API_URL, TradierAPIError, TradierData
from lumibot.entities import Asset, Order


class Tradier(Broker):
"""
Broker that connects to Tradier API to place orders and retrieve data
"""
def __init__(self, account_id=None, api_token=None, paper=True, config=None, max_workers=20, connect_stream=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

category Code Design Improvements priority 6

The __init__ method is quite long and complex. It's a good practice to keep methods short and simple for better readability and maintainability. Consider breaking it down into smaller methods.

data_source=None):

if data_source is None:
data_source = TradierData(account_id=account_id, api_key=api_token, paper=paper, max_workers=max_workers)

super().__init__(name='Tradier', data_source=data_source, config=config, max_workers=max_workers,
connect_stream=connect_stream)

self._tradier_api_key = api_token
self._tradier_account_id = account_id
self._tradier_paper = paper
self._tradier_base_url = TRADIER_PAPER_API_URL if self._tradier_paper else TRADIER_LIVE_API_URL

try:
self.validate_credentials()
except TradierAPIError as e:
raise TradierAPIError("Invalid Tradier Credentials") from e

def validate_credentials(self):
pass
Comment on lines +29 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 9

The validate_credentials method is currently empty. It's important to implement this method to ensure that the provided API token and account ID are valid before making any API calls.

Suggested change
def validate_credentials(self):
pass
def validate_credentials(self):
response = requests.get(self._tradier_base_url + '/v1/user/profile', headers={'Authorization': 'Bearer ' + self._tradier_api_key})
if response.status_code != 200:
raise TradierAPIError('Invalid Tradier Credentials')


def cancel_order(self, order: Order):
pass
Comment on lines +32 to +33
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 8

The cancel_order method is currently empty. This method should be implemented to allow users to cancel orders.

Suggested change
def cancel_order(self, order: Order):
pass
def cancel_order(self, order: Order):
response = requests.delete(self._tradier_base_url + '/v1/accounts/' + self._tradier_account_id + '/orders/' + order.id, headers={'Authorization': 'Bearer ' + self._tradier_api_key})
if response.status_code != 200:
raise TradierAPIError('Failed to cancel order')


def _submit_order(self, order: Order):
pass
Comment on lines +35 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 8

The _submit_order method is currently empty. This method should be implemented to allow users to submit orders.

Suggested change
def _submit_order(self, order: Order):
pass
def _submit_order(self, order: Order):
data = {'class': order.order_class, 'symbol': order.symbol, 'side': order.side, 'quantity': order.quantity, 'type': order.order_type}
response = requests.post(self._tradier_base_url + '/v1/accounts/' + self._tradier_account_id + '/orders', headers={'Authorization': 'Bearer ' + self._tradier_api_key}, data=data)
if response.status_code != 201:
raise TradierAPIError('Failed to submit order')


def _get_balances_at_broker(self, quote_asset: Asset) -> float:
pass
Comment on lines +38 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 8

The _get_balances_at_broker method is currently empty. This method should be implemented to allow users to retrieve their account balance.

Suggested change
def _get_balances_at_broker(self, quote_asset: Asset) -> float:
pass
def _get_balances_at_broker(self, quote_asset: Asset) -> float:
response = requests.get(self._tradier_base_url + '/v1/accounts/' + self._tradier_account_id + '/balances', headers={'Authorization': 'Bearer ' + self._tradier_api_key})
if response.status_code != 200:
raise TradierAPIError('Failed to get account balance')
return float(response.json()['equity'])


def get_historical_account_value(self):
pass
Comment on lines +41 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 8

The get_historical_account_value method is currently empty. This method should be implemented to allow users to retrieve their historical account value.

Suggested change
def get_historical_account_value(self):
pass
def get_historical_account_value(self):
response = requests.get(self._tradier_base_url + '/v1/accounts/' + self._tradier_account_id + '/history', headers={'Authorization': 'Bearer ' + self._tradier_api_key})
if response.status_code != 200:
raise TradierAPIError('Failed to get historical account value')
return response.json()['history']


def _get_stream_object(self):
pass

def _register_stream_events(self):
pass

def _run_stream(self):
pass

def _parse_broker_position(self, broker_position, strategy, orders=None):
pass

def _pull_broker_position(self, asset: Asset):
pass

def _pull_broker_positions(self, strategy=None):
pass

def _parse_broker_order(self, response, strategy_name, strategy_object=None):
pass

def _pull_broker_order(self, identifier):
pass

def _pull_broker_open_orders(self):
pass
7 changes: 7 additions & 0 deletions lumibot/data_sources/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,12 @@
from .exceptions import NoDataFound, UnavailabeTimestep
from .interactive_brokers_data import InteractiveBrokersData
from .pandas_data import PandasData
from .tradier_data import (
TRADIER_LIVE_API_URL,
TRADIER_PAPER_API_URL,
TRADIER_STREAM_API_URL,
TradierAPIError,
TradierData,
)
Comment on lines +9 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

category Code Design Improvements priority 4

The import statements for the Tradier API URLs (TRADIER_LIVE_API_URL, TRADIER_PAPER_API_URL, TRADIER_STREAM_API_URL) might not be necessary in this file. If these constants are only used in the TradierData class, they should be imported there instead. This would keep the __init__.py file cleaner and more focused on its main purpose of making the necessary classes and functions available for import from the lumibot.data_sources package.

Suggested change
from .tradier_data import (
TRADIER_LIVE_API_URL,
TRADIER_PAPER_API_URL,
TRADIER_STREAM_API_URL,
TradierAPIError,
TradierData,
)
from .tradier_data import (
TradierAPIError,
TradierData
)

Comment on lines +9 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

category Formatting Improvements priority 3

The import statements for the Tradier data source are not grouped together. According to PEP 8, related import statements should be grouped together and separated by a blank line from other import groups. This makes the code easier to read and understand.

Comment on lines +9 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

category Formatting Improvements priority 2

The import statements are not in alphabetical order. According to our company's coding standards, import statements should be sorted alphabetically to make it easier to find specific imports when the list gets long.

from .tradovate_data import TradovateData
from .yahoo_data import YahooData
35 changes: 35 additions & 0 deletions lumibot/data_sources/tradier_data.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
from .data_source import DataSource

TRADIER_LIVE_API_URL = "https://api.tradier.com/v1/"
TRADIER_PAPER_API_URL = "https://sandbox.tradier.com/v1/"
TRADIER_STREAM_API_URL = "https://stream.tradier.com/v1/" # Only valid Live, no Paper support


class TradierAPIError(Exception):
pass


class TradierData(DataSource):
MIN_TIMESTEP = "minute"
SOURCE = "Tradier"

def __init__(self, account_id, api_key, paper=True, max_workers=20):
super().__init__(api_key=api_key)
Comment on lines +16 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 9

The class 'TradierData' inherits from 'DataSource' but does not call the parent constructor with all the necessary arguments. The parent constructor requires 'name' and 'config' arguments which are not provided in the child class constructor.

Suggested change
def __init__(self, account_id, api_key, paper=True, max_workers=20):
super().__init__(api_key=api_key)
def __init__(self, account_id, api_key, paper=True, max_workers=20):
name = 'TradierData'
config = {'account_id': account_id, 'api_key': api_key, 'paper': paper, 'max_workers': max_workers}
super().__init__(name=name, config=config)

self._account_id = account_id
self._paper = paper
self._base_url = TRADIER_PAPER_API_URL if self._paper else TRADIER_LIVE_API_URL
self.max_workers = min(max_workers, 50)

def _pull_source_symbol_bars(self, asset, length, timestep=MIN_TIMESTEP, timeshift=None, quote=None, exchange=None,
include_after_hours=True):
pass

def _pull_source_bars(self, assets, length, timestep=MIN_TIMESTEP, timeshift=None, quote=None,
include_after_hours=True):
pass

def _parse_source_symbol_bars(self, response, asset, quote=None, length=None):
pass

def get_last_price(self, asset, quote=None, exchange=None):
pass
15 changes: 15 additions & 0 deletions tests/test_tradier.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from lumibot.brokers import Tradier
from lumibot.data_sources import TradierData


class TestTradierData:
def test_basics(self):
tdata = TradierData(account_id="1234", api_key="a1b2c3", paper=True)
assert tdata._account_id == "1234"


class TestTradierBroker:
def test_basics(self):
broker = Tradier(account_id="1234", api_token="a1b2c3", paper=True)
assert broker.name == "Tradier"
assert broker._tradier_account_id == "1234"
Comment on lines +5 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 7

The test cases in the test_tradier.py file are quite basic and do not cover all the methods in the Tradier broker and data source classes. It's important to have comprehensive tests to ensure all parts of the code are working as expected.

Comment on lines +5 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

category Bug Risk priority 6

The test cases are not checking for any potential errors or exceptions. It's important to include negative test cases to ensure the code can handle errors gracefully.

Comment on lines +8 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

category Code Design Improvements priority 5

The test cases are directly accessing private variables of the classes. This is against our coding standards. Instead, use getter methods or properties to access these variables.