-
Notifications
You must be signed in to change notification settings - Fork 15
Add jump strategy and dump hedge strategy #55
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
base: main
Are you sure you want to change the base?
Conversation
Pull Request Review: Add Jump Strategy and Dump Hedge StrategyOverviewThis PR adds two new trading strategies for prediction markets:
Code Quality & Best PracticesStrengths
Issues & Recommendations1. Violation of CLAUDE.md GuidelinesPer CLAUDE.md Rule 4: "Single Source of Truth: DO NOT place many variables in .env file. Place them in the code instead." Both strategies use
Recommendation: Remove 2. Code DuplicationThe Recommendation: Extract this to a shared utility module (e.g., 3. Error Handling - Silent FailuresMultiple locations have bare
Recommendation: Log errors or at minimum add a comment explaining why silent failure is acceptable. # Before
except Exception:
pass
# After
except Exception as e:
logger.debug(f"Failed to cancel orders during shutdown: {e}")4. Potential Logic Issue - Inventory Tracking in Test ModeIn
However, in test mode no orders are actually placed, so the inventory tracking becomes fictional. This could lead to confusing test results where the strategy thinks it has inventory but doesn't. Recommendation: Either skip inventory tracking in test mode or add clear warnings that test mode inventory is simulated. 5. Magic NumbersSeveral magic values could be constants:
Recommendation: Define as named constants at module level. ASK_HISTORY_MAXLEN = 50
MAX_MARKET_SEARCH_PAGES = 5Potential Bugs1. Race Condition in Dump Hedge StrategyLocation: while hist and (now - hist[0][0]) > self.dump_window_seconds:
hist.popleft()After this while loop clears old data, there's no guarantee Severity: Low (unlikely in single-threaded context) 2. Opposite Outcome Selection LogicLocation: opp = self.outcomes[0] if self.outcomes[1] == leg1 else self.outcomes[1]This assumes exactly 2 outcomes (checked at line 197), but if Severity: Low opp = next(o for o in self.outcomes if o != leg1)Performance Considerations1. Inefficient History CleanupBoth strategies have O(n) deque cleanup operations in tight loops:
With default Recommendation: Consider using a timestamp-based filter rather than repeatedly popping from the front. 2. Repeated Type ConversionsExcessive
Since these values are already cast in Recommendation: Remove redundant Security Concerns1. Order Execution Without ConfirmationWhen Recommendation: Add a confirmation prompt in live mode showing key parameters before starting. 2. Unbounded Position Growth (Jump Strategy)The jump strategy accumulates inventory up to Recommendation: Consider adding:
Test CoverageMajor Issue: No test files were added for either strategy. Recommendation: Add unit tests covering:
Example test structure: # tests/test_strategies.py
def test_dump_hedge_detect_dump():
# Test dump detection with various price movements
def test_jump_strategy_phase_transitions():
# Test ACCUMULATE -> DISTRIBUTE -> COOLDOWN flow
def test_strategies_respect_test_mode():
# Verify no real orders placed in test modeDocumentationMissing
SuggestionsConsider adding:
SummaryApproval Recommendation: The strategies are well-implemented with good structure, but have several issues that should be addressed: Must Fix (Blockers)
Should Fix (Important)
Nice to Have
Overall Assessment: Good code quality and clear implementation of interesting trading strategies. The main concerns are adherence to project conventions (CLAUDE.md), test coverage, and risk management in live mode. |
Strategies Overview
1. Step Jump Capture Strategy
Target markets
Logic
2. Dump Hedge Strategy
Target markets
(e.g. 15-minute up/down markets)
Logic
Reference