PR Review: Add global market state management

[fabian1710]

Overview

The PR implements a global market state management system. Overall, the implementation is clean and well-structured, but there are a few suggestions for improvement.

state.py

Strengths:

• Good use of dataclasses for clean state management
• Clear method documentation
• Type hints are properly used
• Clean separation of concerns

Suggestions:

1. Consider adding validation for the update_price method (e.g., check if price > 0)
2. The any type in indicator methods could be more specific
3. Consider adding a method to clear/reset the state
4. Add type hints for the DataFrame columns to make the data structure clear

Suggested changes:

from typing import Dict, Optional, Union, List

# Add validation
def update_price(self, price: float) -> None:
    if price <= 0:
        raise ValueError("Price must be positive")
    self.current_price = price

# More specific types for indicators
IndicatorValue = Union[float, List[float], Dict[str, float]]
indicators: Dict[str, IndicatorValue] = field(default_factory=dict)

# Add reset method
def reset(self) -> None:
    """Reset the market state to default values."""
    self.symbol = ''
    self.timeframe = ''
    self.current_price = 0.0
    self.historical_data = None
    self.indicators.clear()
    self.last_updated = None

crew.py

Strengths:

• Good error handling
• Clear logging
• Well-organized methods

Suggestions:

1. Add retry logic for market data fetching
2. Consider adding a method to validate the symbol/timeframe before fetching
3. The convert_ohlcv_to_dataframe method might not exist in ccxt, consider implementing it
4. Add validation for the historical data before updating state

Suggested changes:

def _validate_market_config(self) -> None:
    """Validate market configuration."""
    markets = self.exchange.load_markets()
    if market_state.symbol not in markets:
        raise ValueError(f"Invalid symbol: {market_state.symbol}")
    
    valid_timeframes = self.exchange.timeframes
    if market_state.timeframe not in valid_timeframes:
        raise ValueError(f"Invalid timeframe: {market_state.timeframe}")

def _convert_ohlcv_to_dataframe(self, ohlcv: List) -> pd.DataFrame:
    """Convert OHLCV data to DataFrame."""
    return pd.DataFrame(
        ohlcv,
        columns=['timestamp', 'open', 'high', 'low', 'close', 'volume']
    )

main.py

Strengths:

• Clean CLI interface
• Good parameter documentation
• Logical organization

Suggestions:

1. Add basic logging configuration
2. Consider adding validation for timeframe format
3. Add signal handlers for clean shutdown
4. Consider adding a verbose mode

Suggested changes:

import signal
import sys

def setup_logging(verbose: bool = False) -> None:
    """Configure logging."""
    level = logging.DEBUG if verbose else logging.INFO
    logging.basicConfig(
        level=level,
        format='%(asctime)s - %(name)s - %(levelname)s - %(message)s'
    )

def signal_handler(signum, frame):
    """Handle shutdown signals."""
    log.info("Shutting down gracefully...")
    sys.exit(0)

def main():
    parser.add_argument('--verbose', action='store_true', help='Enable verbose logging')
    args = parser.parse_args()
    
    setup_logging(args.verbose)
    signal.signal(signal.SIGINT, signal_handler)
    signal.signal(signal.SIGTERM, signal_handler)

General Recommendations

1. Add unit tests for the new state management
2. Consider adding state persistence for recovery
3. Add documentation about the market state in README.md
4. Consider adding state validation hooks
5. Add proper thread safety if the state will be accessed from multiple threads

Overall, this is a solid foundation for market state management. The suggestions above would make it more robust and production-ready, but the current implementation is already functional and well-structured.