round forex lot sizes to 0.01

[ward-taoshi]

round forex lot sizes to 0.01 multiples to match standard brokers

[github-actions]

🤖 Claude AI Code Review

Last reviewed on: 14:23:47

Summary

This PR modifies the parse_order_size method to round forex lot sizes to 0.01 multiples to comply with standard broker requirements. The changes include refactoring the order calculation logic and adding a use_floor parameter for rounding control.

✅ Strengths

• Clear intent: The PR addresses a real-world requirement for forex trading lot size precision
• Minimal scope: Changes are focused and don't introduce unnecessary modifications
• Backward compatibility: The use_floor parameter defaults to False, maintaining existing behavior for non-resized orders

⚠️ Concerns

Critical Issues

1. Logic error - Quantity field handling removed (Lines 394-397)

   • The original code handled three input scenarios: quantity, leverage, or value
   • The refactored code completely removes the if quantity is not None: branch
   • If a signal comes in with only quantity set, it will now be None throughout the calculation, resulting in incorrect results
   • Impact: This is a breaking change that could cause silent calculation errors

2. Floating point precision issues (Line 400)

   quantity = math.trunc(quantity / 0.01) * 0.01

   • Direct floating point division/multiplication by 0.01 can introduce precision errors
   • Example: round(0.07 / 0.01) * 0.01 might not equal exactly 0.07
   • Recommendation: Use integer arithmetic or Decimal for financial calculations

3. Missing validation (Line 400)

   • No check for quantity being None before the forex rounding logic
   • If signal["quantity"] is the only field set, quantity would be None at line 400, causing a TypeError

Design Issues

4. Inconsistent rounding behavior (Line 293)

   • use_floor=True is only applied when orders are resized due to limits
   • Normal orders use regular rounding, resized orders use floor rounding
   • This inconsistency could lead to unexpected behavior
   • Question: What's the business logic reasoning for this difference?

5. Missing test coverage

   • No test files included in the PR
   • Financial calculations require comprehensive unit tests
   • Edge cases like minimum lot sizes, very small quantities, and boundary conditions should be tested

💡 Suggestions

High Priority

1. Fix the quantity input handling:

   if quantity is not None:
       # Keep original quantity, just apply forex rounding if needed
       pass
   elif leverage is not None:
       quantity = (leverage * portfolio_value * usd_base_conversion) / trade_pair.lot_size
   elif value is not None:
       quantity = (value * usd_base_conversion) / trade_pair.lot_size
   
   # Apply forex rounding
   if trade_pair.is_forex:
       quantity = math.trunc(quantity / 0.01) * 0.01 if use_floor else round(quantity / 0.01) * 0.01

2. Use integer arithmetic for precision:

   if trade_pair.is_forex:
       # Convert to integer cents, round, then back to decimal
       quantity_cents = int(quantity * 100)
       if use_floor:
           quantity = (quantity_cents // 1) / 100
       else:
           quantity = round(quantity_cents) / 100

   Or better yet, use Python's Decimal module for financial calculations.

3. Add validation:

   if trade_pair.is_forex and quantity is not None:
       # Rounding logic here

Medium Priority

4. Clarify the use_floor parameter:

   • Add documentation explaining when and why floor vs round should be used
   • Consider renaming to something more descriptive like force_downsize or use_conservative_rounding

5. Add comprehensive tests:

   def test_parse_order_size_forex_rounding():
       # Test with quantity input
       # Test with leverage input
       # Test with value input
       # Test floor vs round behavior
       # Test edge cases (0.001, 0.005, 0.009, etc.)

6. Consider lot size minimums:

   • What happens if rounding results in 0.0 quantity?
   • Should there be a minimum order size check?

Low Priority

7. Update docstring (Line 382):

   """
   Parses an order signal and calculates leverage, value, and quantity.
   
   Args:
       signal: Dict with one of 'leverage', 'value', or 'quantity'
       usd_base_conversion: Price conversion to USD
       trade_pair: TradePair object with lot_size and is_forex
       portfolio_value: Total portfolio value
       use_floor: If True, floors forex quantities; if False, rounds (default: False)
   
   Returns:
       Tuple of (quantity, leverage, value)
   """

8. Consider extracting forex rounding logic:

   @staticmethod
   def round_forex_quantity(quantity: float, use_floor: bool = False) -> float:
       """Rounds quantity to 0.01 precision for forex compliance."""
       return math.trunc(quantity / 0.01) * 0.01 if use_floor else round(quantity / 0.01) * 0.01

🔒 Security Notes

• Financial calculation accuracy: The floating point precision issue could result in incorrect trade sizes, potentially leading to financial loss
• Input validation: Ensure trade_pair.is_forex is properly validated and cannot be manipulated
• No direct security vulnerabilities identified, but financial correctness is critical

🎯 Verdict

❌ NEEDS REVISION - The removal of the quantity input handling is a critical bug that must be fixed before merging. Additionally, floating point precision issues should be addressed for financial calculations.

Recommended Actions:

1. Restore the quantity input handling logic
2. Add comprehensive unit tests
3. Consider using Decimal for financial precision
4. Clarify the use_floor parameter usage and document it