-
Notifications
You must be signed in to change notification settings - Fork 2
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
⬆️ 🤖 - You will be held hostage by a radical group #519
Conversation
Reviewer's Guide by SourceryThis pull request introduces new handler classes for different trading APIs and updates an existing one. The changes focus on expanding the supported trading platforms by adding skeleton implementations for FIX API and IG trading, as well as updating the cTrader handler with an additional library reference. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mraniki - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using abstract base classes or interfaces instead of empty implementations to define the structure for new handlers.
- Avoid committing large blocks of commented-out code. If these are placeholders, consider using TODO comments or issues to track future work.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
# from ._client import CexClient | ||
|
||
|
||
# class FixapiHandler(CexClient): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider implementing a base CexHandler class for common exchange handler functionality.
To reduce complexity and avoid duplication, consider implementing a base CexHandler
class that defines the common interface for different exchange handlers. Here's a suggested structure:
from abc import ABC, abstractmethod
class CexHandler(ABC):
def __init__(self, **kwargs):
super().__init__()
# Common initialization code
@abstractmethod
async def get_quote(self, instrument):
pass
@abstractmethod
async def get_account_balance(self):
pass
@abstractmethod
async def get_account_position(self):
pass
@abstractmethod
async def get_trading_asset_balance(self):
pass
@abstractmethod
async def execute_order(self, order_params):
pass
class FixapiHandler(CexHandler):
async def get_quote(self, instrument):
# Implement FIXAPI-specific logic
pass
# Implement other methods...
class IgHandler(CexHandler):
async def get_quote(self, instrument):
# Implement IG-specific logic
pass
# Implement other methods...
This approach:
- Reduces code duplication
- Enforces a consistent interface across handlers
- Clearly indicates which methods need to be implemented in subclasses
- Allows for easy addition of new exchange handlers in the future
Implement the common logic in the base class and override only what's necessary in the subclasses. This structure will make the codebase more maintainable and extensible.
# from ._client import CexClient | ||
|
||
|
||
# class IgHandler(CexClient): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (complexity): Consider implementing a base handler class to standardize the interface for all handlers.
While this appears to be a work in progress, the current structure introduces potential complexity through duplication. To improve maintainability and reduce redundancy, consider refactoring this and similar handlers (like fixapi.py
) to use a common base class. Here's a suggested approach:
- Create a base handler class:
from abc import ABC, abstractmethod
class BaseHandler(ABC):
def __init__(self, **kwargs):
super().__init__()
# Common initialization logic
@abstractmethod
async def get_quote(self, instrument):
pass
@abstractmethod
async def get_account_balance(self):
pass
@abstractmethod
async def get_account_position(self):
pass
@abstractmethod
async def get_trading_asset_balance(self):
pass
@abstractmethod
async def execute_order(self, order_params):
pass
# Implement any common methods here
- Refactor the
IgHandler
to inherit from this base class:
from ._base_handler import BaseHandler
from ._client import CexClient
class IgHandler(BaseHandler, CexClient):
def __init__(self, **kwargs):
super().__init__(**kwargs)
# IG-specific initialization
async def get_quote(self, instrument):
# IG-specific implementation
pass
# Implement other abstract methods...
This approach will:
- Reduce code duplication across handlers
- Clearly define the interface for all handlers
- Allow for easy addition of new handlers in the future
- Maintain flexibility for handler-specific implementations
As you continue development, implement the abstract methods in each specific handler. This structure will help manage complexity as your system grows.
Auto Release
Summary by Sourcery
Add new handlers for FIX protocol and IG client to support financial operations and update ctrader handler documentation with an additional library reference.
New Features:
Enhancements: