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.

Sign up for GitHub

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

Jump to bottom

Adapt for docker compose and add ability to update fees #19

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Adapt for docker compose and add ability to update fees #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
03f9faf
added option to provide ghost_key and fetch the Bearer Token
Feb 26, 2024
dd939cd
handle optional env variables
Feb 27, 2024
725831c
fix variable usage
Feb 27, 2024
b44f539
add some logging
Feb 27, 2024
45658ef
Handle V80A symbol
Feb 27, 2024
c13b2d7
some adjustments
Feb 27, 2024
30c6c47
handle custom IBKR account name and trade fees
Mar 8, 2024
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
66 changes: 50 additions & 16 deletions SyncIBKR.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,12 @@


def get_cash_amount_from_flex(query):
print("Getting cash amount")
Copy link

Choose a reason for hiding this comment

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

suggestion (code_refinement): Consider removing or adjusting the debug print statement for production code to maintain a clean and professional output. If this information is crucial, consider implementing a logging framework with appropriate log levels.

Suggested change
print("Getting cash amount")
import logging
# Configure logging at the beginning of your script or application
logging.basicConfig(level=logging.INFO, filename='SyncIBKR.log', filemode='a', format='%(name)s - %(levelname)s - %(message)s')
# Replace the print statement with a logging call
logging.info("Getting cash amount")

cash = 0
try:
cash += query.FlexStatements[0].CashReport[0].endingCash
except Exception as e:
print(e)
try:
cash += query.FlexStatements[0].CashReport[0].endingCashPaxos
except Exception as e:
print(e)
print(e)
return cash


Expand Down Expand Up @@ -55,10 +52,14 @@ def get_diff(old_acts, new_acts):


class SyncIBKR:
IBKRCATEGORY = "9da3a8a7-4795-43e3-a6db-ccb914189737"
IBKRNAME = "Interactive Brokers"
IBKRCATEGORY = "66b22c82-a96c-4e4f-aaf2-64b4ca41dda2"

def __init__(self, ghost_host, ibkrtoken, ibkrquery, ghost_token, ghost_currency):
self.ghost_token = ghost_token
def __init__(self, ghost_host, ibkrtoken, ibkrquery, ghost_key, ghost_token, ghost_currency):
Copy link

Choose a reason for hiding this comment

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

issue (complexity): The addition of the create_ghost_token method and the conditional logic for token handling in the __init__ method significantly increases the complexity of the SyncIBKR class. This class now takes on additional responsibilities beyond its original scope, which can make it harder to maintain and understand. Specifically:

  1. The SyncIBKR class now handles both synchronization and token generation, which could be seen as a violation of the Single Responsibility Principle. Consider separating these concerns into different classes or modules to improve modularity and maintainability.

  2. The introduction of hardcoded symbol transformations directly in the business logic (e.g., .USD-PAXOS to USD, VUAA to .L, V80A to VNGA80.MI) makes the code less flexible and more difficult to maintain. It's generally a good practice to separate data transformation logic from business logic, possibly by using a configuration file or a dedicated transformation service.

  3. The new code introduces additional conditional logic and error handling paths without significantly improving the robustness of the error handling strategy. It's important to ensure that error handling is both comprehensive and easy to follow, which might not be the case with the added complexity.

  4. The overall length and complexity of the code have increased with these changes. While more lines of code don't necessarily mean higher complexity, in this case, the added responsibilities, conditional logic, and hardcoded values contribute to a more complex and less maintainable codebase.

Consider simplifying the initialization logic by encapsulating the token handling in a separate method or class. This could help reduce the complexity and improve the readability and maintainability of the code. Separating concerns more clearly would also make it easier to test individual components in isolation.

if ghost_token == "" and ghost_key != "":
self.ghost_token = self.create_ghost_token(ghost_host, ghost_key)
else:
self.ghost_token = ghost_token
self.ghost_host = ghost_host
self.ghost_currency = ghost_currency
self.ibkrtoken = ibkrtoken
Expand Down Expand Up @@ -87,6 +88,8 @@ def sync_ibkr(self):
symbol = trade.symbol.replace(".USD-PAXOS", "") + "USD"
elif "VUAA" in trade.symbol:
symbol = trade.symbol + ".L"
elif "V80A" in trade.symbol:
symbol = "VNGA80.MI"
Comment on lines +91 to +92
Copy link

Choose a reason for hiding this comment

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

suggestion (code_refinement): The handling of specific symbols (e.g., 'V80A') with hardcoded replacements ('VNGA80.MI') could lead to maintainability issues as more exceptions are added. Consider creating a mapping or a more flexible solution for symbol transformations.

Suggested change
elif "V80A" in trade.symbol:
symbol = "VNGA80.MI"
# Define a mapping for symbol transformations at the beginning of your script or function
symbol_mapping = {
"V80A": "VNGA80.MI",
# Add more mappings here as needed
}
# Then, use the mapping to transform symbols
if trade.symbol in symbol_mapping:
symbol = symbol_mapping[trade.symbol]

if trade.buySell == BuySell.BUY:
buysell = "BUY"
elif trade.buySell == BuySell.SELL:
Expand All @@ -101,7 +104,7 @@ def sync_ibkr(self):
"currency": trade.currency,
"dataSource": "YAHOO",
"date": iso_format,
"fee": float(0),
"fee": abs(float(self.get_fee_for_trade(trade.tradeID,query.FlexStatements[0].UnbundledCommissionDetails))),
"quantity": abs(float(trade.quantity)),
"symbol": symbol.replace(" ", "-"),
"type": buysell,
Expand All @@ -113,18 +116,46 @@ def sync_ibkr(self):
print("Nothing new to sync")
else:
self.import_act(diff)

def get_fee_for_trade(self, trade_id, commission_details):
for commission_detail in commission_details:
if commission_detail.tradeID == trade_id:
return commission_detail.totalCommission
return 0

def create_ghost_token(self, ghost_host, ghost_key):
print("No bearer token provided, fetching one")
token = {
'accessToken': ghost_key
}

url = f"{ghost_host}/api/v1/auth/anonymous"

payload = json.dumps(token)
headers = {
'Content-Type': 'application/json'
}
try:
response = requests.request("POST", url, headers=headers, data=payload)
except Exception as e:
print(e)
return ""
if response.status_code == 201:
print("Bearer token fetched")
return response.json()["authToken"]
print("Failed fetching bearer token")
return ""

def set_cash_to_account(self, account_id, cash):
if cash == 0:
if cash <= 1:
Copy link

Choose a reason for hiding this comment

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

question (code_clarification): Changing the condition to 'cash <= 1' for setting cash to an account might not be intuitive. Consider adding a comment to explain the rationale behind using '1' as the threshold, especially if it's related to avoiding setting negligible cash amounts.

Suggested change
if cash <= 1:
# Check if the cash value is 1 or less. This threshold is used to avoid setting negligible cash amounts to the account.
# This might be particularly relevant in scenarios where only significant cash amounts should trigger certain actions.
if cash <= 1:

print("No cash set, no cash retrieved")
return False
account = {
"accountType": "SECURITIES",
"balance": float(cash),
"id": account_id,
"currency": self.ghost_currency,
"isExcluded": False,
"name": "IBKR",
"name": self.IBKRNAME,
"platformId": self.IBKRCATEGORY
}

Expand Down Expand Up @@ -206,13 +237,13 @@ def addAct(self, act):
return response.status_code == 201

def create_ibkr_account(self):
print("Creating IBKR account")
account = {
"accountType": "SECURITIES",
"balance": 0,
"currency": self.ghost_currency,
"isExcluded": False,
"name": "IBKR",
"platformId": "9da3a8a7-4795-43e3-a6db-ccb914189737"
"name": self.IBKRNAME,
"platformId": self.IBKRCATEGORY
}

url = f"{self.ghost_host}/api/v1/account"
Expand All @@ -228,11 +259,13 @@ def create_ibkr_account(self):
print(e)
return ""
if response.status_code == 201:
print("IBKR account: " + response.json()["id"])
Copy link

Choose a reason for hiding this comment

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

🚨 suggestion (security): Logging the account ID upon creation is useful for tracking and debugging. However, ensure that this does not pose a security risk by inadvertently exposing sensitive information in logs.

Suggested change
print("IBKR account: " + response.json()["id"])
import logging
# It's a good practice to configure logging at the beginning of your script or application
logging.basicConfig(level=logging.INFO)
# Replace the print statement with a logging call
# Ensure to use appropriate logging level. INFO is used here as an example.
logging.info("IBKR account: %s", response.json()["id"])

return response.json()["id"]
print("Failed creating ")
return ""

def get_account(self):
print("Finding IBKR account")
url = f"{self.ghost_host}/api/v1/account"

payload = {}
Expand All @@ -252,7 +285,8 @@ def get_account(self):
def create_or_get_IBKR_accountId(self):
accounts = self.get_account()
for account in accounts:
if account["name"] == "IBKR":
if account["name"] == self.IBKRNAME:
print("IBKR account: ", account["id"])
return account["id"]
return self.create_ibkr_account()

Expand Down
5 changes: 3 additions & 2 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@

GETALLACTS = "GETALLACTS"

ghost_tokens = os.environ.get('GHOST_TOKEN').split(",")
ghost_keys = os.environ.get("GHOST_KEY", "").split(",")
ghost_tokens = os.environ.get('GHOST_TOKEN', "").split(",")
ibkr_tokens = os.environ.get("IBKR_TOKEN").split(",")
ibkr_queries = os.environ.get("IBKR_QUERY").split(",")
ghost_hosts = os.environ.get("GHOST_HOST", "https://ghostfol.io").split(",")
Expand All @@ -16,7 +17,7 @@

if __name__ == '__main__':
for i in range(len(operations)):
ghost = SyncIBKR(ghost_hosts[i], ibkr_tokens[i], ibkr_queries[i], ghost_tokens[i], ghost_currency[i])
ghost = SyncIBKR(ghost_hosts[i], ibkr_tokens[i], ibkr_queries[i], ghost_keys[i], ghost_tokens[i], ghost_currency[i])
if operations[i] == SYNCIBKR:
print("Starting sync")
ghost.sync_ibkr()
Expand Down