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

♻️ Capitalcom _fetch_account_data refactor #481

Merged
merged 4 commits into from
Jul 15, 2024
Merged

♻️ Capitalcom _fetch_account_data refactor #481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
348c5ea
⬆️ 🛠️(deps): update dependency sphinx to v7.4.0
renovate[bot] Jul 15, 2024
fc61ed1
♻️
mraniki Jul 15, 2024
619ef9d
♻️ Capitalcom _fetch_account_data refactor
mraniki Jul 15, 2024
541dd7a
⬆️ 🛠️(deps): update dependency sphinx to v7.4.3
renovate[bot] Jul 15, 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
33 changes: 26 additions & 7 deletions cefi/handler/capitalcom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,35 @@ def _fetch_account_data(self):
Returns:
None
"""
# try:
# if self.default_account:
# logger.debug("Default account: {}", self.default_account)
# self.switch_account(self.default_account)
# self.accounts_data = self.client.all_accounts()
# except Exception as e:
# logger.error("Error fetching account data: {}", e)
# return False
# logger.debug("Account data: {}", self.accounts_data)
# self.account_number = self.accounts_data["accounts"][0]["accountId"]
Comment on lines +86 to +95
Copy link

Choose a reason for hiding this comment

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

issue: Remove commented-out code

The commented-out code should be removed to keep the codebase clean and maintainable. If this code is no longer needed, it's better to delete it rather than leaving it commented out.

# logger.debug("Account number: {}", self.account_number)
# logger.debug("Session details: {}", self.client.get_sesion_details())
try:
self.accounts_data = self.client.all_accounts()
logger.debug("Account data: {}", self.accounts_data)
Copy link

Choose a reason for hiding this comment

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

🚨 issue (security): Potential sensitive data logging

Ensure that self.accounts_data does not contain sensitive information before logging it. Logging sensitive data can lead to security vulnerabilities.

if self.default_account:
logger.debug("Default account: {}", self.default_account)
self.switch_account(self.default_account)
self.accounts_data = self.client.all_accounts()
for account in self.accounts_data["accounts"]:
if account["accountId"] == self.default_account:
self.account_number = account["accountId"]
Comment on lines +103 to +105
Copy link

Choose a reason for hiding this comment

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

suggestion (performance): Inefficient loop for finding default account

Consider using a more efficient method to find the default account, such as a dictionary lookup, if possible. This will improve the performance, especially if the number of accounts is large.

Suggested change
for account in self.accounts_data["accounts"]:
if account["accountId"] == self.default_account:
self.account_number = account["accountId"]
account_dict = {account["accountId"]: account for account in self.accounts_data["accounts"]}
self.account_number = account_dict.get(self.default_account, self.accounts_data["accounts"][0])["accountId"]

else:
self.account_number = self.accounts_data["accounts"][0]["accountId"]

logger.debug("Account number: {}", self.account_number)
logger.debug("Session details: {}", self.client.get_sesion_details())
Copy link

Choose a reason for hiding this comment

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

issue (typo): Typo in method name

There is a typo in the method name get_sesion_details(). It should be get_session_details(). Ensure that the correct method name is used.

return True
except Exception as e:
logger.error("Error fetching account data: {}", e)
return False
logger.debug("Account data: {}", self.accounts_data)
self.account_number = self.accounts_data["accounts"][0]["accountId"]
logger.debug("Account number: {}", self.account_number)
logger.debug("Session details: {}", self.client.get_sesion_details())

def switch_account(self, account_number):
"""
Expand All @@ -106,7 +123,9 @@ def switch_account(self, account_number):
Returns:
None
"""
self.client.switch_account(self.default_account)
logger.debug("Switching to account {}", account_number)
switch = self.client.switch_account(self.default_account)
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Incorrect variable used in switch_account

The method switch_account should use the account_number parameter instead of self.default_account. This ensures that the correct account is switched to.

logger.debug("Switch: {}", switch)

async def get_quote(self, instrument):
"""
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ pytest-loguru = "^0.4.0"
optional = true

[tool.poetry.group.docs.dependencies]
sphinx = "7.3.7"
sphinx = "7.4.3"
pydata-sphinx-theme = "^0.15.0"
sphinx-hoverxref = "^1.3.0"
sphinx_copybutton = "0.5.2"
Expand Down
8 changes: 8 additions & 0 deletions tests/test_unit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@ async def test_submit_order_exception(CXTrader):
CXTrader.clients = []
await CXTrader.submit_order()

@pytest.mark.asyncio
async def test_modify_position(CXTrader, order):
result = await CXTrader.modify_position(order)
print(result)
assert result is not None
Comment on lines +113 to +116
Copy link

Choose a reason for hiding this comment

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

suggestion (testing): Consider adding a test for exception handling

It would be useful to add a test case that simulates an exception being raised during the modify_position method. This will ensure that your code handles exceptions gracefully and behaves as expected under error conditions.

Suggested change
async def test_modify_position(CXTrader, order):
result = await CXTrader.modify_position(order)
print(result)
assert result is not None
async def test_modify_position(CXTrader, order):
try:
result = await CXTrader.modify_position(order)
print(result)
assert result is not None
except Exception as e:
print(f"Exception occurred: {e}")
assert False


# assert ("binance" in result) or ("huobi" in result) or ("capital" in result)
# assert ("🔵" in result) or ("Error" in result)
Comment on lines +112 to +119
Copy link

Choose a reason for hiding this comment

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

suggestion (testing): Add more assertions to cover edge cases

The test test_modify_position currently only checks if the result is not None. It would be beneficial to add more assertions to cover different edge cases and expected outcomes. For example, you could check if the result contains specific keys or values that indicate a successful modification or an error.

Suggested change
@pytest.mark.asyncio
async def test_modify_position(CXTrader, order):
result = await CXTrader.modify_position(order)
print(result)
assert result is not None
# assert ("binance" in result) or ("huobi" in result) or ("capital" in result)
# assert ("🔵" in result) or ("Error" in result)
@pytest.mark.asyncio
async def test_modify_position(CXTrader, order):
result = await CXTrader.modify_position(order)
print(result)
assert result is not None
assert isinstance(result, dict)
assert "status" in result
assert result["status"] in ["success", "error"]
if result["status"] == "error":
assert "message" in result


# @pytest.mark.asyncio
# async def test_submit_limit_order(CXTrader, limit_order):
Expand Down