Add vpin strategies

[Yebon56]

Added files:

• examples/vpin_strategy.py
• examples/vpindiff_strategy.py

[claude]

Pull Request Review: Add VPIN Strategies

I've reviewed the two new strategy files added in this PR. Here's my comprehensive feedback:

---

✅ Positive Aspects

1. Good Strategy Architecture: Both strategies properly extend the market-making logic with VPIN-based liquidity withdrawal, which is a valid trading approach
2. Consistent Patterns: Code follows similar patterns to the existing spread_strategy.py
3. Clear Documentation: Docstrings explain the VPIN logic and parameters well

---

🔴 Critical Issues

1. Missing Trade Ingestion (HIGH PRIORITY)

Location: Both files, VPIN logic section

Problem: The ingest_trade() method is defined but never called. VPIN calculation depends on processing actual trades, but there's no mechanism to feed trades into the buckets.

# Method exists but is never invoked:
def ingest_trade(self, price: float, size: float, is_buy: bool):
    """ Feed each trade into VPIN buckets. """
    # ...

Impact: The VPIN value will always return 0.0, making the strategy non-functional. The liquidity withdrawal logic will never trigger.

Fix Required: You need to:

• Subscribe to trade/fill data via WebSocket
• Call ingest_trade() for each trade received
• Classify trades as buyer/seller initiated (potentially using Lee-Ready algorithm or similar)

2. Import Issues

Location: vpindiff_strategy.py:18

Problem: Uses numpy without it being in project dependencies:

import numpy as np

But pyproject.toml doesn't list numpy as a dependency.

Fix: Add numpy to dependencies or use Python's built-in statistics module.

3. Unused Imports

Location: Both files

Problem:

from dr_manhattan.utils.logger import Colors  # Never used

Per CLAUDE.md rule #1: "The codebase should be focused, clean, and easy to understand" - remove unused imports.

---

⚠️ Design & Best Practices Issues

4. Environment Variable Violation

Location: Both files, main() function

Problem: Violates CLAUDE.md rule #4: "DO NOT place many variables in .env file. Place them in the code instead."

private_key = os.getenv('POLYMARKET_PRIVATE_KEY')
funder = os.getenv('POLYMARKET_FUNDER')
market_slug = os.getenv("MARKET_SLUG", "")

Recommendation:

• Keep sensitive credentials (private_key, funder) in .env ✅
• Hard-code or use CLI args for non-sensitive configs like default market_slug

5. Missing Order Tracking

Location: Both files

Problem: The base spread_strategy.py includes order fill tracking:

self.order_tracker = OrderTracker(verbose=True)

But the VPIN strategies omit this feature entirely. This removes valuable functionality for monitoring fills and debugging.

Recommendation: Add order tracking back, following the pattern from spread_strategy.py:151-166.

6. Inconsistent Error Handling

Location: Multiple locations in both files

Problem:

except:  # Bare except clause
    pass

This violates Python best practices and makes debugging difficult.

Fix: Use specific exceptions:

except Exception as e:
    logger.warning(f"Failed to cancel order: {e}")

7. Magic Numbers

Location: vpin_strategy.py:362, similar in vpindiff

if abs(order.price - our_bid) < 0.001:  # Magic number

Recommendation: Use self.tick_size or make it a configurable tolerance parameter.

---

🐛 Potential Bugs

8. Division by Zero Risk

Location: vpin_strategy.py:93

bucket_vpin = imbalance / max(self.current_vol, 1e-9)

Good defensive programming, but this scenario should never occur since the bucket only closes when current_vol >= bucket_volume. Consider adding an assertion or removing the guard if truly impossible.

9. Uninitialized Tick Size

Location: vpin_strategy.py:128

self.tick_size = self.exchange.get_tick_size(self.market)

If get_tick_size() returns None or the market has unexpected data, self.tick_size could be undefined. Add a fallback:

self.tick_size = self.exchange.get_tick_size(self.market) or 0.01

10. VPIN Threshold Logic

Location: vpindiff_strategy.py:132-134

if len(self.delta_history) < self.delta_window:
    return float("inf")  # threshold disabled until enough data

Returning inf means the liquidity withdrawal will never trigger during the warm-up period. This could expose the strategy to risk during initial buckets when VPIN data is limited. Consider a more conservative default (e.g., lower threshold or don't trade until ready).

---

🔒 Security Concerns

11. No Input Validation

Problem: No validation on user inputs (market_slug, order sizes, etc.)

Risk: Could lead to unexpected behavior or errors with malformed inputs

Recommendation: Add basic validation:

if not market_slug:
    logger.error("Market slug is required")
    return 1
if order_size <= 0 or max_position <= 0:
    logger.error("Order size and max position must be positive")
    return 1

---

📊 Performance Considerations

12. Inefficient List Operations

Location: Both files, bucket management

self.buckets.pop(0)  # O(n) operation
self.delta_history.pop(0)  # O(n) operation

Issue: pop(0) on a list is O(n) as it requires shifting all elements.

Optimization: Use collections.deque with maxlen:

from collections import deque
self.buckets = deque(maxlen=self.bucket_count)
self.delta_history = deque(maxlen=self.delta_window)

Then you can just append() and old items are automatically removed.

---

🧪 Test Coverage

13. No Tests Provided

Problem: No unit tests for VPIN calculation logic, percentile computation, or liquidity withdrawal triggers.

Recommendation: Add tests for:

• VPIN bucket filling and calculation
• ΔVPIN percentile threshold computation
• Edge cases (empty buckets, insufficient data)
• Trade ingestion logic (once implemented)

Example test structure:

def test_vpin_calculation():
    strategy = VPINStrategy(...)
    strategy.ingest_trade(0.5, 50, is_buy=True)
    strategy.ingest_trade(0.5, 50, is_buy=False)
    assert strategy.get_vpin() == 0.0  # Balanced

---

📝 Code Quality Issues

14. Duplicated Code

Problem: 90%+ code duplication between the two strategies and with spread_strategy.py.

Recommendation: Refactor using inheritance:

class VPINStrategy(SpreadStrategy):
    def __init__(self, *args, bucket_volume=100.0, vpin_threshold=0.8, **kwargs):
        super().__init__(*args, **kwargs)
        # VPIN-specific initialization
        
    def should_withdraw_liquidity(self) -> bool:
        """Override point for liquidity withdrawal logic"""
        vpin = self.get_vpin()
        return vpin >= self.vpin_threshold
        
    def place_orders(self):
        if self.should_withdraw_liquidity():
            self.cancel_all_orders()
            return
        super().place_orders()

This would reduce the PR from 808 additions to ~100-150 lines.

15. Logging Inconsistencies

Problem: Mixed logging styles:

• vpin_strategy.py:209 uses f-strings
• vpindiff_strategy.py:210 uses f-strings with warning vs info

Recommendation: Be consistent with log levels:

• INFO: Normal operations
• WARNING: Liquidity withdrawal, risk conditions
• ERROR: Failures

---

🎯 Recommendations Summary

Must Fix Before Merge:

1. ✅ Implement trade ingestion or the strategies won't work
2. ✅ Add numpy to dependencies or replace with stdlib
3. ✅ Remove unused imports

Should Fix:
4. Refactor to use inheritance (DRY principle)
5. Use deque for better performance
6. Add order tracking back
7. Fix bare except clauses
8. Add input validation

Nice to Have:
9. Add unit tests
10. Document trade classification methodology
11. Add example usage in docstrings

---

🔧 Action Items

1. Critical: Implement WebSocket trade subscription and feed into ingest_trade()
2. Update pyproject.toml to include numpy
3. Consider refactoring to reduce duplication
4. Add tests to validate VPIN calculations
5. Run the strategies in a test environment to verify functionality

---

Overall Assessment: The strategies show good understanding of VPIN concepts, but the implementation has a critical flaw (trade ingestion not connected) that makes them non-functional in their current state. With the fixes above, these would be valuable additions to the codebase.

Would you like me to help implement any of these fixes? I'd be happy to provide specific code examples.

[claude]

Code Review - PR #8: Add VPIN Strategies

Overview

This PR adds 4 new trading strategy examples totaling ~2,363 lines of code:

• binary_arb_strategy.py (589 lines) - Binary arbitrage detection
• endgame_sweep_strategy.py (711 lines) - Endgame sweep market making
• vpin_strategy.py (656 lines) - VPIN-based liquidity withdrawal
• vpindiff_strategy.py (407 lines) - VPIN derivative percentile strategy

✅ Strengths

1. Well-structured examples - Each strategy is self-contained with clear documentation
2. Consistent patterns - Follows established codebase patterns from spread_strategy.py
3. Binary market optimization - Smart inference of NO prices from YES orderbook to reduce WebSocket bandwidth
4. Defensive error handling - Try/except blocks protect against API failures
5. Test mode support - --mode=test allows dry-run without placing orders

🔴 Critical Issues

1. Security: Environment Variables in .env (violates CLAUDE.md #4)

Location: All 4 files
Issue: Files use .env for POLYMARKET_PRIVATE_KEY and POLYMARKET_FUNDER

Per CLAUDE.md:

4. Single Source of Truth: DO NOT place many variables in .env file. Place them in the code instead.

Risk:

• Private keys in .env files can be accidentally committed to version control
• Goes against the project's explicit guidance

Recommendation:

• Use direct environment variable access or config module
• Add .env to .gitignore if not already present
• Consider using a secrets management approach that aligns with project conventions

2. Missing Input Validation

binary_arb_strategy.py:230-255 - _poll_trades_loop

ts_int = int(ts)  # No validation that ts exists or is valid

endgame_sweep_strategy.py:563-583 - Missing validation on market metadata timestamps

Risk: Runtime crashes with malformed API responses

Fix: Add null checks and type validation before conversions

3. Hardcoded Magic Numbers

Multiple instances across all files:

• binary_arb_strategy.py:45: ENDGAME_TIME_HOURS = 3.0
• binary_arb_strategy.py:46-47: ENDGAME_PRICE_TRIGGER = 0.97, ENDGAME_SPREAD_MAX = 0.003
• vpin_strategy.py:54: bucket_volume=100.0

Issue: Values should be configurable parameters, not class constants

Recommendation: Move to constructor parameters with these as defaults

4. Race Conditions in Threading

vpin_strategy.py:210-256 - _poll_trades_loop

self.seen_trades.add(key)  # Accessed from multiple threads without locks
self.flow_buy_count += 1   # Not thread-safe
self.ingest_trade(...)     # Modifies shared state

Risk: Data corruption with concurrent access from main thread and trade polling thread

Fix: Use threading.Lock() to protect shared state or use thread-safe data structures

5. Resource Leaks

All files - WebSocket cleanup in finally blocks uses join with timeout but doesn't verify cleanup:

finally:
    self.ws.stop()
    if self.ws_thread:
        self.ws_thread.join(timeout=5)  # What if thread doesn't stop?

Risk: Orphaned threads/connections

Fix: Check thread.is_alive() after join and log warnings

⚠️ Major Issues

6. Missing numpy Dependency

vpindiff_strategy.py:18, 124, 136

import numpy as np
return np.mean(self.buckets)
return np.percentile(self.delta_history, self.percentile_cutoff)

Issue: numpy not listed in pyproject.toml dependencies

Fix: Add numpy>=1.24.0 to dependencies or implement these calculations without numpy

7. Poor Error Messages

vpin_strategy.py:264-265

if not self.market:
    logger.error("Failed to fetch market")  # No details about why

Recommendation: Include exception details to aid debugging

8. Inconsistent Mode Handling

• binary_arb_strategy.py: Checks self.live_mode before every order
• vpindiff_strategy.py: No mode support at all (missing --mode flag)

Fix: Standardize mode handling across all strategies

9. Excessive Logging / Code Duplication

vpin_strategy.py:407-561 - 150+ lines of duplicated order placement logic from spread_strategy.py

Issue: Violates DRY principle, makes maintenance harder

Recommendation: Extract common market-making logic to base class or shared utility

🟡 Performance Considerations

10. Inefficient Data Structures

binary_arb_strategy.py:232

self.seen_trades = set()  # Grows unbounded

Issue: Memory leak - set never cleared, will grow indefinitely in long-running processes

Fix: Use collections.deque with maxlen or implement periodic cleanup

11. Polling Instead of Streaming

vpin_strategy.py:210-256 - Polls trades API every 2 seconds

Issue: Less efficient than WebSocket for trade data if available

Recommendation: Document why polling is used or migrate to WebSocket trades if supported

12. String Operations in Hot Path

vpin_strategy.py:278-280

for outcome, price in self.market.prices.items():
    ps = f"{price:.4f}"  # String formatting in loop
    if '.' in ps and len(ps.split('.')[1].rstrip('0')) == 3:

Fix: Perform tick size inference once during initialization, not in trading loop

🔵 Code Quality Issues

13. Inconsistent Code Style

vpin_strategy.py:267: self.token_ids = self.market.metadata.get('clobTokenIds', []) (single quotes)
binary_arb_strategy.py:230: self.token_ids = self.market.metadata.get("clobTokenIds", []) (double quotes)

Fix: Run uv run black examples/ to standardize formatting per pyproject.toml:30-32

14. Dead Code / Comments

binary_arb_strategy.py:73-84 - Complex helper function with minimal documentation
endgame_sweep_strategy.py:15-20 - Incomplete docstring

15. Missing Type Hints

Many functions lack return type hints:

def fetch_market(self):  # Should be -> bool
def get_positions(self):  # Should be -> Dict[str, float]

🟢 Test Coverage

16. No Tests

Issue: No test files added for 2,363 lines of new code

Recommendation: Add tests for:

• Market data parsing edge cases
• VPIN calculation correctness
• Binary price inference accuracy
• Error handling paths

📋 Checklist Summary

Category	Status	Notes
Code Quality	⚠️	Needs formatting, type hints, DRY improvements
Potential Bugs	🔴	Race conditions, validation gaps, resource leaks
Performance	🟡	Memory leaks, inefficient patterns
Security	🔴	.env usage violates project rules, input validation gaps
Test Coverage	🔴	No tests added

🎯 Recommendations (Priority Order)

Must Fix Before Merge:

1. ✅ Add numpy to dependencies or remove numpy usage
2. ✅ Fix thread safety in vpin_strategy.py with locks
3. ✅ Remove .env dependency per CLAUDE.md fix: polymarket impl #4
4. ✅ Add input validation for API responses
5. ✅ Fix unbounded memory growth in seen_trades

Should Fix:

6. Add --mode support to vpindiff_strategy.py
7. Extract common market-making logic to reduce duplication
8. Run uv run black examples/ and uv run ruff check examples/
9. Add type hints

Nice to Have:

10. Add unit tests
11. Improve error messages with context
12. Document why polling is used vs WebSocket for trades
13. Make hardcoded constants configurable

🚀 Testing Performed

Per CLAUDE.md #5 ("Run and Debug yourself PROACTIVELY"), please confirm:

• Scripts run without errors in test mode
• All dependencies are available
• WebSocket connections establish correctly
• Error paths are tested

Overall Assessment

The code demonstrates good understanding of the domain and follows existing patterns well. However, it has critical issues around thread safety, dependency management, and project conventions that must be addressed before merging. The strategies appear functionally sound but need hardening for production use.

Recommendation: Request changes before merge.

[claude]

Pull Request Review - PR #8: Add VPIN Strategies

Summary

This PR adds three sophisticated trading strategy examples to the dr-manhattan library:

1. Binary Arbitrage Strategy (binary_arb_strategy.py) - Detects and exploits price inefficiencies
2. Endgame Sweep Strategy (endgame_sweep_strategy.py) - Hybrid market maker with endgame positioning
3. VPIN Strategy (vpin_strategy.py) - Market maker with Volume-Synchronized Probability of Informed Trading

Code Quality & Best Practices

Strengths

• Well-structured code with clear class organization and separation of concerns
• Comprehensive documentation with detailed docstrings explaining usage and strategy logic
• Good error handling throughout with try-except blocks
• Thread-safe implementation using locks in vpin_strategy.py (line 118: self.trade_lock)
• Flexible mode system (live/test) for safe testing before real trading

Issues & Recommendations

1. Configuration Management (CLAUDE.md Violation)

Severity: Medium

The CLAUDE.md guideline states: "Single Source of Truth: DO NOT place many variables in .env file. Place them in the code instead."

However, all three files heavily rely on environment variables:

• binary_arb_strategy.py:509-510 - POLYMARKET_PRIVATE_KEY, POLYMARKET_FUNDER
• endgame_sweep_strategy.py:668-669 - Same
• vpin_strategy.py:635-636 - Same

Recommendation: Consider using a config class with reasonable defaults:

class Config:
    PRIVATE_KEY = os.getenv("POLYMARKET_PRIVATE_KEY", "")
    FUNDER = os.getenv("POLYMARKET_FUNDER", "")
    CACHE_TTL = 2.0
    VERBOSE = True

2. Thread Safety Issues

Severity: High (vpin_strategy.py only)

vpin_strategy.py:223-278 - The _poll_trades_loop method has potential race conditions:

• Lines 246-248: Checking seen_trades_set without holding lock
• Lines 256-258: Checking last_trade_ts without holding lock
• These checks should be inside the with self.trade_lock: block

Fix:

# Consolidate all state checks under a single lock acquisition
with self.trade_lock:
    if key in self.seen_trades_set or ts_int < self.last_trade_ts:
        continue
    new_rows.append((ts_int, key, row))

3. Hardcoded Configuration Values

Severity: Low-Medium

Multiple configuration values are hardcoded in class bodies:

• binary_arb_strategy.py:521 - ClobClient URL hardcoded
• endgame_sweep_strategy.py:65-72 - Strategy thresholds as class constants
• vpin_strategy.py:83 - seen_trades_max = 5000 hardcoded

Recommendation: Make these configurable via constructor parameters or a config system.

4. Input Validation

Severity: Medium

Missing validation for user inputs:

• binary_arb_strategy.py:195-207 - No validation for negative/zero values in order_size, check_interval, etc.
• endgame_sweep_strategy.py:74-89 - Same issues
• vpin_strategy.py:50-64 - Same issues

Recommendation: Add parameter validation in __init__:

if order_size <= 0:
    raise ValueError("order_size must be positive")
if check_interval <= 0:
    raise ValueError("check_interval must be positive")

5. Error Messages Could Be More Informative

Severity: Low

• binary_arb_strategy.py:175-177 - Silent exception swallowing in scan_binary_underround_candidates
• vpin_strategy.py:274 - Generic "Trade poll failed" message without specifics

Recommendation: Log the exception type or use more specific error messages.

Security Concerns

1. Credential Exposure

Severity: High

All three files check for missing credentials but don't validate their format:

• binary_arb_strategy.py:509-514
• No validation that keys are properly formatted before passing to exchange

Recommendation: Add basic validation:

if not private_key or not private_key.startswith("0x"):
    logger.error("POLYMARKET_PRIVATE_KEY must start with 0x")

2. Command Injection (Low Risk)

Severity: Low

The docstrings include example commands with user-provided slugs:

• binary_arb_strategy.py:11 - uv run python examples/binary_arb_alert.py <market_slug_or_url>

While the normalize_slug function provides basic sanitization, consider adding explicit warnings about slug validation.

Performance Considerations

1. Inefficient Polling

Severity: Medium

vpin_strategy.py:223-277 polls the trades endpoint every 2 seconds (configurable):

• This could be inefficient for high-frequency trading
• REST API may have rate limits

Recommendation: Consider using WebSocket for trade data if available, or implement exponential backoff.

2. Memory Management

Severity: Low

vpin_strategy.py:84 - Uses deque(maxlen=5000) which is good, but:

• Also maintains a separate seen_trades_set that grows unbounded
• Line 267: Manual eviction helps but could still leak memory over very long runs

Recommendation: Consider periodic cleanup or use a LRU cache.

3. WebSocket Thread Management

Severity: Medium

All three files create daemon threads:

• binary_arb_strategy.py:295 - daemon=True
• If main thread crashes, daemon threads may not clean up properly

Recommendation: Use context managers or ensure proper shutdown in finally blocks.

Test Coverage

Severity: Medium

No test files were added with these strategies. Given the financial nature of this code:

Recommendation:

• Add unit tests for core logic (VPIN calculation, arbitrage detection, endgame signal)
• Add integration tests with mocked exchange responses
• Add property-based tests for price/size calculations

Example test structure:

def test_vpin_calculation():
    # Test bucket filling logic
    # Test VPIN threshold triggers
    pass

def test_arbitrage_detection():
    # Test edge cases (yes+no = 1.0, yes+no < 1.0, etc.)
    pass

Potential Bugs

1. Race Condition in has_all_data

Severity: Medium

binary_arb_strategy.py:421-430 - _has_all_data() method accesses orderbook_manager without synchronization:

• Could return inconsistent results if orderbook updates mid-check

2. Float Comparison Issues

Severity: Low

Multiple locations use direct float comparison:

• binary_arb_strategy.py:331 - if not (0.0 < yes_ask < 1.0)
• Should use epsilon-based comparison for production code

3. Missing Null Checks

Severity: Medium

endgame_sweep_strategy.py:628-633 - Accesses orderbook without checking if get returns data:

• Line 628: ob = self.orderbook_manager.get(self.token_ids[0])
• Line 629: if ob: - good
• But then line 629: iterates without checking if bids/asks exist

Documentation

Strengths

• Excellent inline documentation
• Clear usage examples in docstrings
• Good explanation of strategy logic

Improvements Needed

• Add examples of expected output
• Document rate limits and API constraints
• Add troubleshooting section for common errors

Alignment with CLAUDE.md Guidelines

1. ✅ Focused and clean - Code is well-organized
2. ⚠️ No new documents - Good, only code files added
3. ✅ UV usage - Properly uses uv run in examples
4. ❌ Single Source of Truth - Heavy reliance on .env violates guideline fix: polymarket impl #4
5. ❌ Run and Debug proactively - No evidence of testing/debugging (no test files)

Overall Assessment

Quality: Good (7/10)
Security: Moderate concerns (6/10)
Test Coverage: Poor (2/10)
Documentation: Good (8/10)

Recommendations Summary

Must Fix (Before Merge)

1. Fix thread safety issues in vpin_strategy.py (race conditions)
2. Add input validation for all constructor parameters
3. Add credential format validation

Should Fix (High Priority)

1. Reduce reliance on environment variables per CLAUDE.md
2. Add basic unit tests for core logic
3. Improve error messages and logging

Nice to Have

1. Add WebSocket support for trade data
2. Implement better memory management
3. Add comprehensive integration tests
4. Document API rate limits and constraints

---

Verdict: This is solid work with well-implemented trading strategies. The code demonstrates good understanding of async programming, threading, and market microstructure. However, thread safety issues and lack of tests are concerning for production use. Recommend addressing critical issues before merge.